Bug 1330111 - Replace recordAutofillResult with a method-scoped variable and .add in `finally`. r=johannh draft
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Fri, 03 Feb 2017 01:55:40 -0800
changeset 478921 46e234139b5fd969fa2fca4966afea02bf0f3b5d
parent 478920 25c0e4cd01e288e4b97829f2ab2c6f6eae42a36a
child 478922 829c2dfbc279b4093368c3c4dc27219bdc6ee612
push id44090
push usermozilla@noorenberghe.ca
push dateSat, 04 Feb 2017 01:25:53 +0000
reviewersjohannh
bugs1330111
milestone54.0a1
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
@@ -915,47 +915,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] =
@@ -975,26 +968,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.
@@ -1002,25 +995,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;
@@ -1045,41 +1038,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;
           }
@@ -1098,34 +1091,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;
@@ -1141,22 +1134,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.
    *