Bug 1135451 - fillForm() cleanup part A: remove unused return type, kill E10S unfriendly fillForm from nsILoginManager, kill passwordmgr-found-form notification, largely a backout of bug 439365. r=MattN draft
authorJustin Dolske <dolske@mozilla.com>
Fri, 27 Feb 2015 14:55:08 -0800
changeset 246549 fc3988d05880b1f09a8f2af2667998fb63a7fa98
parent 246548 3d3904aa4379a1d40931dcb3edca895d68a0d9fb
child 246550 152a3f5ec21312278360a06b1e2ab22e5d026ae8
push id858
push userjdolske@mozilla.com
push dateFri, 27 Feb 2015 23:40:23 +0000
reviewersMattN
bugs1135451, 439365
milestone39.0a1
Bug 1135451 - fillForm() cleanup part A: remove unused return type, kill E10S unfriendly fillForm from nsILoginManager, kill passwordmgr-found-form notification, largely a backout of bug 439365. r=MattN
toolkit/components/passwordmgr/LoginManagerContent.jsm
toolkit/components/passwordmgr/nsILoginManager.idl
toolkit/components/passwordmgr/test/mochitest.ini
toolkit/components/passwordmgr/test/test_basic_form_2.html
toolkit/components/passwordmgr/test/test_basic_form_observer_autofillForms.html
--- a/toolkit/components/passwordmgr/LoginManagerContent.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ -568,25 +568,25 @@ var LoginManagerContent = {
                                       newPasswordField: mockPassword,
                                       oldPasswordField: mockOldPassword },
                                     { openerWin: opener });
   },
 
   /*
    * _fillform
    *
-   * Fill the form with the provided login information.
-   * The logins are returned so they can be reused for
-   * optimization. Success of action is also returned in format
-   * [success, foundLogins].
+   * Attempt to find the username and password fields in a form, and fill them
+   * in using the provided logins.
    *
    * - autofillForm denotes if we should fill the form in automatically
+   * - clobberPassword controls if an existing password value can be
+   *     overwritten
    * - userTriggered is an indication of whether this filling was triggered by
    *     the user
-   * - foundLogins is an array of nsILoginInfo
+   * - foundLogins is an array of nsILoginInfo that could be used for the form
    */
   _fillForm : function (form, autofillForm, clobberPassword,
                         userTriggered, foundLogins) {
     let ignoreAutocomplete = true;
     const AUTOFILL_RESULT = {
       FILLED: 0,
       NO_PASSWORD_FIELD: 1,
       PASSWORD_DISABLED_READONLY: 2,
@@ -608,38 +608,38 @@ var LoginManagerContent = {
       const autofillResultHist = Services.telemetry.getHistogramById("PWMGR_FORM_AUTOFILL_RESULT");
       autofillResultHist.add(result);
     }
 
     // Nothing to do if we have no matching logins available.
     if (foundLogins.length == 0) {
       // We don't log() here since this is a very common case.
       recordAutofillResult(AUTOFILL_RESULT.NO_SAVED_LOGINS);
-      return [false, foundLogins];
+      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] =
         this._getFormFields(form, false);
 
     // 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);
-      return [false, foundLogins];
+      return;
     }
 
     // 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);
-      return [false, foundLogins];
+      return;
     }
 
     // Discard logins which have username/password values that don't
     // fit into the fields (as specified by the maxlength attribute).
     // The user couldn't enter these values anyway, and it helps
     // with sites that have an extra PIN to be entered (bug 391514)
     var maxUsernameLen = Number.MAX_VALUE;
     var maxPasswordLen = Number.MAX_VALUE;
@@ -657,17 +657,17 @@ var LoginManagerContent = {
         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);
-      return [false, foundLogins];
+      return;
     }
 
     // The reason we didn't end up filling the form, if any.  We include
     // this in the formInfo object we send with the passwordmgr-found-logins
     // notification.  See the _notifyFoundLogins docs for possible values.
     var didntFillReason = null;
 
     // Attach autocomplete stuff to the username field, if we have
@@ -678,17 +678,17 @@ var LoginManagerContent = {
 
     // Don't clobber an existing password.
     if (passwordField.value && !clobberPassword) {
       didntFillReason = "existingPassword";
       this._notifyFoundLogins(didntFillReason, usernameField,
                               passwordField, foundLogins, null);
       log("form not filled, the password field was already filled");
       recordAutofillResult(AUTOFILL_RESULT.EXISTING_PASSWORD);
-      return [false, foundLogins];
+      return;
     }
 
     // If the form has an autocomplete=off attribute in play, don't
     // fill in the login automatically. We check this after attaching
     // the autocomplete stuff to the username field, so the user can
     // still manually select a login to be filled in.
     var isFormDisabled = false;
     if (!ignoreAutocomplete &&
@@ -765,26 +765,20 @@ var LoginManagerContent = {
           usernameField.setUserInput(selectedLogin.username);
         }
       }
       if (passwordField.value != selectedLogin.password) {
         passwordField.setUserInput(selectedLogin.password);
       }
       didFillForm = true;
     } else if (selectedLogin && !autofillForm) {
-      // For when autofillForm is false, but we still have the information
-      // to fill a form, we notify observers.
       didntFillReason = "noAutofillForms";
-      Services.obs.notifyObservers(form, "passwordmgr-found-form", didntFillReason);
       log("autofillForms=false but form can be filled; notified observers");
     } else if (selectedLogin && isFormDisabled) {
-      // For when autocomplete is off, but we still have the information
-      // to fill a form, we notify observers.
       didntFillReason = "autocompleteOff";
-      Services.obs.notifyObservers(form, "passwordmgr-found-form", didntFillReason);
       log("autocomplete=off but form can be filled; notified observers");
     }
 
     this._notifyFoundLogins(didntFillReason, usernameField, passwordField,
                             foundLogins, selectedLogin);
 
     if (didFillForm) {
       recordAutofillResult(AUTOFILL_RESULT.FILLED);
@@ -802,18 +796,16 @@ var LoginManagerContent = {
           autofillResult = AUTOFILL_RESULT.NO_AUTOFILL_FORMS;
           break;
         case "autocompleteOff":
           autofillResult = AUTOFILL_RESULT.AUTOCOMPLETE_OFF;
           break;
       }
       recordAutofillResult(autofillResult);
     }
-
-    return [didFillForm, foundLogins];
   },
 
 
   /**
    * Notify observers about an attempt to fill a form that resulted in some
    * saved logins being found for the form.
    *
    * This does not get called if the login manager attempts to fill a form
--- a/toolkit/components/passwordmgr/nsILoginManager.idl
+++ b/toolkit/components/passwordmgr/nsILoginManager.idl
@@ -8,17 +8,17 @@
 interface nsIURI;
 interface nsILoginInfo;
 interface nsIAutoCompleteResult;
 interface nsIFormAutoCompleteObserver;
 interface nsIDOMHTMLInputElement;
 interface nsIDOMHTMLFormElement;
 interface nsIPropertyBag;
 
-[scriptable, uuid(f0c5ca21-db71-4b32-993e-ab63054cc6f5)]
+[scriptable, uuid(38c7f6af-7df9-49c7-b558-2776b24e6cc1)]
 interface nsILoginManager : nsISupports {
   /**
    * This promise is resolved when initialization is complete, and is rejected
    * in case initialization failed.  This includes the initial loading of the
    * login data as well as any migration from previous versions.
    *
    * Calling any method of nsILoginManager before this promise is resolved
    * might trigger the synchronous initialization fallback.
@@ -211,26 +211,16 @@ interface nsILoginManager : nsISupports 
    *       probably be callback registered through the FFC.
    */
   void autoCompleteSearchAsync(in AString aSearchString,
                                in nsIAutoCompleteResult aPreviousResult,
                                in nsIDOMHTMLInputElement aElement,
                                in nsIFormAutoCompleteObserver aListener);
 
   /**
-   * Fill a form with login information if we have it. This method will fill
-   * aForm regardless of the signon.autofillForms preference.
-   *
-   * @param aForm
-   *        The form to fill
-   * @return Promise that is resolved with whether or not the form was filled.
-   */
-  jsval fillForm(in nsIDOMHTMLFormElement aForm);
-
-  /**
    * Search for logins in the login manager. An array is always returned;
    * if there are no logins the array is empty.
    *
    * @param count
    *        The number of elements in the array. JS callers can simply use
    *        the array's .length property, and supply an dummy object for
    *        this out param. For example: |searchLogins({}, matchData)|
    * @param matchData
--- a/toolkit/components/passwordmgr/test/mochitest.ini
+++ b/toolkit/components/passwordmgr/test/mochitest.ini
@@ -26,26 +26,24 @@ support-files =
   subtst_privbrowsing_4.html
   subtst_prompt_async.html
   auth2/authenticate.sjs
 
 [test_basic_form.html]
 [test_basic_form_0pw.html]
 [test_basic_form_1pw.html]
 [test_basic_form_1pw_2.html]
-[test_basic_form_2.html]
 [test_basic_form_2pw_1.html]
 [test_basic_form_2pw_2.html]
 [test_basic_form_3pw_1.html]
 [test_basic_form_autocomplete.html]
 skip-if = toolkit == 'android'
 [test_case_differences.html]
 skip-if = toolkit == 'android'
 [test_basic_form_html5.html]
-[test_basic_form_observer_autofillForms.html]
 [test_basic_form_observer_foundLogins.html]
 [test_basic_form_pwevent.html]
 [test_basic_form_pwonly.html]
 [test_bug_221634.html]
 # This test doesn't pass because we can't ensure a cross-platform event that
 # occurs between DOMContentLoaded and Pageload
 skip-if = true
 [test_bug_242956.html]
deleted file mode 100644
--- a/toolkit/components/passwordmgr/test/test_basic_form_2.html
+++ /dev/null
@@ -1,64 +0,0 @@
-<!DOCTYPE HTML>
-<html>
-<head>
-  <title>Test for Login Manager</title>
-  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>  
-  <script type="text/javascript" src="pwmgr_common.js"></script>
-  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
-</head>
-<body>
-Login Manager test: simple form fill with autofillForms disabled
-<script>
-commonInit();
-SimpleTest.waitForExplicitFinish();
-
-var pwmgr = SpecialPowers.Cc["@mozilla.org/login-manager;1"]
-                         .getService(SpecialPowers.Ci.nsILoginManager);
-</script>
-
-<p id="display"></p>
-
-<div id="content" style="display: block">
-
-  <form id="form1" action="formtest.js">
-    <p>This is form 1.</p>
-    <input  type="text"       name="uname">
-    <input  type="password"   name="pword">
-
-    <button type="submit">Submit</button>
-    <button type="reset"> Reset </button>
-  </form>
-
-</div>
-
-<pre id="test">
-<script class="testbody" type="text/javascript">
-
-/** Test for Login Manager: simple form fill with autofillForms disabled **/
-
-function startTest(){
-  // Ensure the form is empty at start
-  is($_(1, "uname").value, "", "Checking for blank username");
-  is($_(1, "pword").value, "", "Checking for blank password");
-
-  // Call the public method, check return value
-  pwmgr.fillForm(document.getElementById("form1"))
-       .then(function(result) {
-         is(result, true, "Checking return value of fillForm");
-
-         // Check that the form was filled
-         is($_(1, "uname").value, "testuser", "Checking for filled username");
-         is($_(1, "pword").value, "testpass", "Checking for filled password");
-
-         SimpleTest.finish();
-       });
-}
-// Assume that the pref starts out true, so set to false
-SpecialPowers.pushPrefEnv({"set":[["signon.autofillForms", false]]}, setup);
-function setup() {
- window.addEventListener("runTests", startTest);
-}
-</script>
-</pre>
-</body>
-</html>
deleted file mode 100644
--- a/toolkit/components/passwordmgr/test/test_basic_form_observer_autofillForms.html
+++ /dev/null
@@ -1,104 +0,0 @@
-<!DOCTYPE HTML>
-<html>
-<head>
-  <title>Test for Login Manager</title>
-  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>  
-  <script type="text/javascript" src="pwmgr_common.js"></script>
-  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
-</head>
-<body>
-Login Manager test: simple form with autofillForms disabled and notifying observers
-<script>
-commonInit(true);
-SimpleTest.waitForExplicitFinish();
-
-const Cc = SpecialPowers.Cc;
-const Ci = SpecialPowers.Ci;
-var TestObserver;
-// Assume that the pref starts out true, so set to false
-SpecialPowers.pushPrefEnv({"set":[["signon.autofillForms", false]]}, function() {
-  TestObserver = {
-  receivedNotificationFoundForm : false,
-  receivedNotificationFoundLogins : false,
-  dataFoundForm : "",
-  dataFoundLogins : null,
-  observe : function (subject, topic, data) {
-    var pwmgr = Cc["@mozilla.org/login-manager;1"].
-                getService(Ci.nsILoginManager);
-    if (topic == "passwordmgr-found-form") {
-      info("got passwordmgr-found-form");
-      this.receivedNotificationFoundForm = true;
-      this.dataFoundForm = data;
-      // Now fill the form
-      pwmgr.fillForm(subject)
-           .then(window.startTest)
-           .then(null, function(e) { alert(e); });
-    } else if (topic == "passwordmgr-found-logins") {
-      info("got passwordmgr-found-logins");
-
-      // We only care about the first notification (the second comes from our
-      // own call to pwmgr.fillForm.
-      if (this.receivedNotificationFoundLogins)
-        return;
-      this.receivedNotificationFoundLogins = true;
-      this.dataFoundLogins = subject.QueryInterface(Ci.nsIPropertyBag2);
-    }
-  }
-};
-
-// Add the observer
-SpecialPowers.addObserver(TestObserver, "passwordmgr-found-form", false);
-SpecialPowers.addObserver(TestObserver, "passwordmgr-found-logins", false);
-});
-</script>
-
-<p id="display"></p>
-
-<div id="content" style="display: block">
-
-  <form id="form1" action="formtest.js">
-    <p>This is form 1.</p>
-    <input  type="text"       name="uname">
-    <input  type="password"   name="pword">
-
-    <button type="submit">Submit</button>
-    <button type="reset"> Reset </button>
-  </form>
-
-</div>
-
-<pre id="test">
-<script class="testbody" type="text/javascript">
-
-/** Test for Login Manager: simple form with autofillForms disabled and notifying observers **/
-function startTest() {
-  // Test that found-form observer is notified & got correct data
-  is(TestObserver.receivedNotificationFoundForm, true, "Checking found-form observer was notified");
-  is(TestObserver.dataFoundForm, "noAutofillForms", "Checking found-form observer got correct data");
-
-  // Check that form1 was filled
-  is($_(1, "uname").value, "testuser", "Checking for filled username");
-  is($_(1, "pword").value, "testpass", "Checking for filled password");
-
-  // Test that found-logins observer is notified & got correct data
-  is(TestObserver.receivedNotificationFoundLogins, true, "Checking found-logins observer was notified");
-  is(TestObserver.dataFoundLogins.get("didntFillReason"), "noAutofillForms", "Checking didntFillReason is noAutofillForms");
-  is(SpecialPowers.unwrap(TestObserver.dataFoundLogins.get("usernameField")), $_(1, "uname"), "Checking username field is correct");
-  is(SpecialPowers.unwrap(TestObserver.dataFoundLogins.get("passwordField")), $_(1, "pword"), "Checking password field is correct");
-  is(TestObserver.dataFoundLogins.get("foundLogins").constructor.name, "Array", "Checking foundLogins is array");
-  is(TestObserver.dataFoundLogins.get("foundLogins").length, 1, "Checking foundLogins contains one login");
-  ok(TestObserver.dataFoundLogins.get("selectedLogin").QueryInterface(Ci.nsILoginInfo), "Checking selectedLogin is nsILoginInfo");
-  ok(TestObserver.dataFoundLogins.get("selectedLogin").equals(TestObserver.dataFoundLogins.get("foundLogins")[0]),
-     "Checking selectedLogin is found login");
-  // Remove the observer
-  SpecialPowers.removeObserver(TestObserver, "passwordmgr-found-form");
-  SpecialPowers.removeObserver(TestObserver, "passwordmgr-found-logins");
-
-  SimpleTest.finish();
-}
-
-
-</script>
-</pre>
-</body>
-</html>