Bug 1330111 - Replace recordAutofillResult with a method-scoped variable and .add in `finally`. r=johannh
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Fri, 03 Feb 2017 01:55:40 -0800
changeset 341258 ad413db36e46c53a68ec85cc7fb6c3f2c1a51daa
parent 341257 bfe0bd22fa4263e7cc658dd35b8336859a70cb7f
child 341259 c0ca0a0795bdc4cee897de0744d43e4799724d63
push id86670
push usermozilla@noorenberghe.ca
push dateWed, 08 Feb 2017 02:20:06 +0000
treeherdermozilla-inbound@7538950ebcc9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjohannh
bugs1330111
milestone54.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1330111 - Replace recordAutofillResult with a method-scoped variable and .add in `finally`. r=johannh This improves readability (no local function) and allows code in `finally` to use the autofillResult. MozReview-Commit-ID: 77t1ML7uw6G
toolkit/components/passwordmgr/LoginManagerContent.jsm
--- a/toolkit/components/passwordmgr/LoginManagerContent.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ -917,47 +917,40 @@ var LoginManagerContent = {
    * @param {Set} recipes that could be used to affect how the form is filled
    * @param {Object} [options = {}] is a list of options for this method.
             - [inputElement] is an optional target input element we want to fill
    */
   _fillForm(form, autofillForm, clobberUsername, clobberPassword,
                         userTriggered, foundLogins, recipes, {inputElement} = {}) {
     log("_fillForm", form.elements);
     let ignoreAutocomplete = true;
+    // Will be set to one of AUTOFILL_RESULT in the `try` block.
+    let autofillResult = -1;
     const AUTOFILL_RESULT = {
       FILLED: 0,
       NO_PASSWORD_FIELD: 1,
       PASSWORD_DISABLED_READONLY: 2,
       NO_LOGINS_FIT: 3,
       NO_SAVED_LOGINS: 4,
       EXISTING_PASSWORD: 5,
       EXISTING_USERNAME: 6,
       MULTIPLE_LOGINS: 7,
       NO_AUTOFILL_FORMS: 8,
       AUTOCOMPLETE_OFF: 9,
       INSECURE: 10,
     };
 
-    function recordAutofillResult(result) {
-      if (userTriggered) {
-        // Ignore fills as a result of user action.
-        return;
-      }
-      const autofillResultHist = Services.telemetry.getHistogramById("PWMGR_FORM_AUTOFILL_RESULT");
-      autofillResultHist.add(result);
-    }
-
     try {
       // Nothing to do if we have no matching logins available,
       // and there isn't a need to show the insecure form warning.
       if (foundLogins.length == 0 &&
           (InsecurePasswordUtils.isFormSecure(form) ||
           !LoginHelper.showInsecureFieldWarning)) {
         // We don't log() here since this is a very common case.
-        recordAutofillResult(AUTOFILL_RESULT.NO_SAVED_LOGINS);
+        autofillResult = AUTOFILL_RESULT.NO_SAVED_LOGINS;
         return;
       }
 
       // Heuristically determine what the user/pass fields are
       // We do this before checking to see if logins are stored,
       // so that the user isn't prompted for a master password
       // without need.
       var [usernameField, passwordField, ignored] =
@@ -977,26 +970,26 @@ var LoginManagerContent = {
         } else {
           throw new Error("Unexpected input element type.");
         }
       }
 
       // Need a valid password field to do anything.
       if (passwordField == null) {
         log("not filling form, no password field found");
-        recordAutofillResult(AUTOFILL_RESULT.NO_PASSWORD_FIELD);
+        autofillResult = AUTOFILL_RESULT.NO_PASSWORD_FIELD;
         return;
       }
 
       this._formFillService.markAsLoginManagerField(passwordField);
 
       // If the password field is disabled or read-only, there's nothing to do.
       if (passwordField.disabled || passwordField.readOnly) {
         log("not filling form, password field disabled or read-only");
-        recordAutofillResult(AUTOFILL_RESULT.PASSWORD_DISABLED_READONLY);
+        autofillResult = AUTOFILL_RESULT.PASSWORD_DISABLED_READONLY;
         return;
       }
 
       // Attach autocomplete stuff to the username field, if we have
       // one. This is normally used to select from multiple accounts,
       // but even with one account we should refill if the user edits.
       // We would also need this attached to show the insecure login
       // warning, regardless of saved login.
@@ -1004,25 +997,25 @@ var LoginManagerContent = {
         this._formFillService.markAsLoginManagerField(usernameField);
       }
 
       // Nothing to do if we have no matching logins available.
       // Only insecure pages reach this block and logs the same
       // telemetry flag.
       if (foundLogins.length == 0) {
         // We don't log() here since this is a very common case.
-        recordAutofillResult(AUTOFILL_RESULT.NO_SAVED_LOGINS);
+        autofillResult = AUTOFILL_RESULT.NO_SAVED_LOGINS;
         return;
       }
 
       // Prevent autofilling insecure forms.
       if (!userTriggered && !LoginHelper.insecureAutofill &&
           !InsecurePasswordUtils.isFormSecure(form)) {
         log("not filling form since it's insecure");
-        recordAutofillResult(AUTOFILL_RESULT.INSECURE);
+        autofillResult = AUTOFILL_RESULT.INSECURE;
         return;
       }
 
       var isAutocompleteOff = false;
       if (this._isAutocompleteDisabled(form) ||
           this._isAutocompleteDisabled(usernameField) ||
           this._isAutocompleteDisabled(passwordField)) {
         isAutocompleteOff = true;
@@ -1047,41 +1040,41 @@ var LoginManagerContent = {
         if (!fit)
           log("Ignored", l.username, "login: won't fit");
 
         return fit;
       }, this);
 
       if (logins.length == 0) {
         log("form not filled, none of the logins fit in the field");
-        recordAutofillResult(AUTOFILL_RESULT.NO_LOGINS_FIT);
+        autofillResult = AUTOFILL_RESULT.NO_LOGINS_FIT;
         return;
       }
 
       // Don't clobber an existing password.
       if (passwordField.value && !clobberPassword) {
         log("form not filled, the password field was already filled");
-        recordAutofillResult(AUTOFILL_RESULT.EXISTING_PASSWORD);
+        autofillResult = AUTOFILL_RESULT.EXISTING_PASSWORD;
         return;
       }
 
       // Select a login to use for filling in the form.
       var selectedLogin;
       if (!clobberUsername && usernameField && (usernameField.value ||
                                                 usernameField.disabled ||
                                                 usernameField.readOnly)) {
         // If username was specified in the field, it's disabled or it's readOnly, only fill in the
         // password if we find a matching login.
         var username = usernameField.value.toLowerCase();
 
         let matchingLogins = logins.filter(l =>
                                            l.username.toLowerCase() == username);
         if (matchingLogins.length == 0) {
           log("Password not filled. None of the stored logins match the username already present.");
-          recordAutofillResult(AUTOFILL_RESULT.EXISTING_USERNAME);
+          autofillResult = AUTOFILL_RESULT.EXISTING_USERNAME;
           return;
         }
 
         // If there are multiple, and one matches case, use it
         for (let l of matchingLogins) {
           if (l.username == usernameField.value) {
             selectedLogin = l;
           }
@@ -1100,34 +1093,34 @@ var LoginManagerContent = {
         let matchingLogins;
         if (usernameField)
           matchingLogins = logins.filter(l => l.username);
         else
           matchingLogins = logins.filter(l => !l.username);
 
         if (matchingLogins.length != 1) {
           log("Multiple logins for form, so not filling any.");
-          recordAutofillResult(AUTOFILL_RESULT.MULTIPLE_LOGINS);
+          autofillResult = AUTOFILL_RESULT.MULTIPLE_LOGINS;
           return;
         }
 
         selectedLogin = matchingLogins[0];
       }
 
       // We will always have a selectedLogin at this point.
 
       if (!autofillForm) {
         log("autofillForms=false but form can be filled");
-        recordAutofillResult(AUTOFILL_RESULT.NO_AUTOFILL_FORMS);
+        autofillResult = AUTOFILL_RESULT.NO_AUTOFILL_FORMS;
         return;
       }
 
       if (isAutocompleteOff && !ignoreAutocomplete) {
         log("Not filling the login because we're respecting autocomplete=off");
-        recordAutofillResult(AUTOFILL_RESULT.AUTOCOMPLETE_OFF);
+        autofillResult = AUTOFILL_RESULT.AUTOCOMPLETE_OFF;
         return;
       }
 
       // Fill the form
 
       if (usernameField) {
       // Don't modify the username field if it's disabled or readOnly so we preserve its case.
         let disabledOrReadOnly = usernameField.disabled || usernameField.readOnly;
@@ -1143,22 +1136,32 @@ var LoginManagerContent = {
           usernameField.setUserInput(selectedLogin.username);
         }
       }
       if (passwordField.value != selectedLogin.password) {
         passwordField.setUserInput(selectedLogin.password);
       }
 
       log("_fillForm succeeded");
-      recordAutofillResult(AUTOFILL_RESULT.FILLED);
+      autofillResult = AUTOFILL_RESULT.FILLED;
       let doc = form.ownerDocument;
       let win = doc.defaultView;
       let messageManager = messageManagerFromWindow(win);
       messageManager.sendAsyncMessage("LoginStats:LoginFillSuccessful");
     } finally {
+      if (autofillResult == -1) {
+        // eslint-disable-next-line no-unsafe-finally
+        throw new Error("_fillForm: autofillResult must be specified");
+      }
+
+      if (!userTriggered) {
+        // Ignore fills as a result of user action for this probe.
+        Services.telemetry.getHistogramById("PWMGR_FORM_AUTOFILL_RESULT").add(autofillResult);
+      }
+
       Services.obs.notifyObservers(form.rootElement, "passwordmgr-processed-form", null);
     }
   },
 
   /**
    * Verify if a field is a valid login form field and
    * returns some information about it's FormLike.
    *