Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections. r=seanlee,steveck
authorRay Lin <ralin@mozilla.com>
Tue, 21 Nov 2017 12:26:10 +0800
changeset 394690 4acb1f17f25243557d4505a1d75c9a77c31aad9c
parent 394689 0f109cab61c99bd33d95154f8626be0c8c6dbb22
child 394691 6cab14274c26792857e75267b6e64c86dffaa21f
push id97969
push userbtara@mozilla.com
push dateMon, 04 Dec 2017 20:48:53 +0000
treeherdermozilla-inbound@b580f65bdb21 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersseanlee, steveck
bugs1415073
milestone59.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 1415073 - Refactor records structure of form autofill submission to adapt multiple sections. r=seanlee,steveck MozReview-Commit-ID: Fs2hgA7H5GX
browser/extensions/formautofill/FormAutofillContent.jsm
browser/extensions/formautofill/FormAutofillHandler.jsm
browser/extensions/formautofill/FormAutofillParent.jsm
--- a/browser/extensions/formautofill/FormAutofillContent.jsm
+++ b/browser/extensions/formautofill/FormAutofillContent.jsm
@@ -411,17 +411,17 @@ var FormAutofillContent = {
 
     let handler = this._formsDetails.get(formElement);
     if (!handler) {
       this.log.debug("Form element could not map to an existing handler");
       return true;
     }
 
     let records = handler.createRecords();
-    if (!Object.keys(records).length) {
+    if (!Object.values(records).some(typeRecords => typeRecords.length)) {
       return true;
     }
 
     this._onFormSubmit(records, domWin, handler.timeStartedFillingMS);
     return true;
   },
 
   receiveMessage({name, data}) {
--- a/browser/extensions/formautofill/FormAutofillHandler.jsm
+++ b/browser/extensions/formautofill/FormAutofillHandler.jsm
@@ -981,17 +981,32 @@ class FormAutofillHandler {
           }
           input.removeEventListener("input", this);
         }
         this.timeStartedFillingMS = Date.now();
         break;
     }
   }
 
+  /**
+   * Collect the filled sections within submitted form and convert all the valid
+   * field data into multiple records.
+   *
+   * @returns {Object} records
+   *          {Array.<Object>} records.address
+   *          {Array.<Object>} records.creditCard
+   */
   createRecords() {
-    // TODO [Bug 1415073] `FormAutofillHandler.createRecords` should traverse
-    // all sections and aggregate the records into one result.
-    if (this.sections.length > 0) {
-      return this.sections[0].createRecords();
+    const records = {
+      address: [],
+      creditCard: [],
+    };
+
+    for (const section of this.sections) {
+      const secRecords = section.createRecords();
+      for (const [type, record] of Object.entries(secRecords)) {
+        records[type].push(record);
+      }
     }
-    return null;
+    log.debug("Create records:", records);
+    return records;
   }
 }
--- a/browser/extensions/formautofill/FormAutofillParent.jsm
+++ b/browser/extensions/formautofill/FormAutofillParent.jsm
@@ -378,29 +378,31 @@ FormAutofillParent.prototype = {
     });
 
     Services.ppmm.broadcastAsyncMessage("FormAutofill:savedFieldNames",
                                         Services.ppmm.initialProcessData.autofillSavedFieldNames);
     this._updateStatus();
   },
 
   _onAddressSubmit(address, target, timeStartedFillingMS) {
+    let showDoorhanger = null;
     if (address.guid) {
       // Avoid updating the fields that users don't modify.
       let originalAddress = this.profileStorage.addresses.get(address.guid);
       for (let field in address.record) {
         if (address.untouchedFields.includes(field) && originalAddress[field]) {
           address.record[field] = originalAddress[field];
         }
       }
 
       if (!this.profileStorage.addresses.mergeIfPossible(address.guid, address.record, true)) {
         this._recordFormFillingTime("address", "autofill-update", timeStartedFillingMS);
 
-        FormAutofillDoorhanger.show(target, "updateAddress").then((state) => {
+        showDoorhanger = async () => {
+          const state = await FormAutofillDoorhanger.show(target, "updateAddress");
           let changedGUIDs = this.profileStorage.addresses.mergeToStorage(address.record, true);
           switch (state) {
             case "create":
               if (!changedGUIDs.length) {
                 changedGUIDs.push(this.profileStorage.addresses.add(address.record));
               }
               break;
             case "update":
@@ -408,52 +410,54 @@ FormAutofillParent.prototype = {
                 this.profileStorage.addresses.update(address.guid, address.record, true);
                 changedGUIDs.push(address.guid);
               } else {
                 this.profileStorage.addresses.remove(address.guid);
               }
               break;
           }
           changedGUIDs.forEach(guid => this.profileStorage.addresses.notifyUsed(guid));
-        });
+        };
         // Address should be updated
         Services.telemetry.scalarAdd("formautofill.addresses.fill_type_autofill_update", 1);
-        return;
+      } else {
+        this._recordFormFillingTime("address", "autofill", timeStartedFillingMS);
+        this.profileStorage.addresses.notifyUsed(address.guid);
+        // Address is merged successfully
+        Services.telemetry.scalarAdd("formautofill.addresses.fill_type_autofill", 1);
       }
-      this._recordFormFillingTime("address", "autofill", timeStartedFillingMS);
-      this.profileStorage.addresses.notifyUsed(address.guid);
-      // Address is merged successfully
-      Services.telemetry.scalarAdd("formautofill.addresses.fill_type_autofill", 1);
     } else {
       let changedGUIDs = this.profileStorage.addresses.mergeToStorage(address.record);
       if (!changedGUIDs.length) {
         changedGUIDs.push(this.profileStorage.addresses.add(address.record));
       }
       changedGUIDs.forEach(guid => this.profileStorage.addresses.notifyUsed(guid));
       this._recordFormFillingTime("address", "manual", timeStartedFillingMS);
 
       // Show first time use doorhanger
       if (FormAutofillUtils.isAutofillAddressesFirstTimeUse) {
         Services.prefs.setBoolPref(FormAutofillUtils.ADDRESSES_FIRST_TIME_USE_PREF, false);
-        FormAutofillDoorhanger.show(target, "firstTimeUse").then((state) => {
+        showDoorhanger = async () => {
+          const state = await FormAutofillDoorhanger.show(target, "firstTimeUse");
           if (state !== "open-pref") {
             return;
           }
 
           target.ownerGlobal.openPreferences("panePrivacy",
                                              {origin: "autofillDoorhanger"});
-        });
+        };
       } else {
         // We want to exclude the first time form filling.
         Services.telemetry.scalarAdd("formautofill.addresses.fill_type_manual", 1);
       }
     }
+    return showDoorhanger;
   },
 
-  async _onCreditCardSubmit(creditCard, target, timeStartedFillingMS) {
+  _onCreditCardSubmit(creditCard, target, timeStartedFillingMS) {
     // Updates the used status for shield/heartbeat to recognize users who have
     // used Credit Card Autofill.
     let setUsedStatus = status => {
       if (FormAutofillUtils.AutofillCreditCardsUsedStatus < status) {
         Services.prefs.setIntPref(FormAutofillUtils.CREDITCARDS_USED_STATUS_PREF, status);
       }
     };
 
@@ -480,17 +484,17 @@ FormAutofillParent.prototype = {
         recordUnchanged &= untouched;
       }
 
       if (recordUnchanged) {
         this.profileStorage.creditCards.notifyUsed(creditCard.guid);
         // Add probe to record credit card autofill(without modification).
         Services.telemetry.scalarAdd("formautofill.creditCards.fill_type_autofill", 1);
         this._recordFormFillingTime("creditCard", "autofill", timeStartedFillingMS);
-        return;
+        return false;
       }
       // Add the probe to record credit card autofill with modification.
       Services.telemetry.scalarAdd("formautofill.creditCards.fill_type_autofill_modified", 1);
       this._recordFormFillingTime("creditCard", "autofill-update", timeStartedFillingMS);
     } else {
       // Indicate that the user neither sees the doorhanger nor uses Autofill
       // but somehow has a duplicate record in the storage. Will be reset to 2
       // if the doorhanger actually shows below.
@@ -500,65 +504,89 @@ FormAutofillParent.prototype = {
       Services.telemetry.scalarAdd("formautofill.creditCards.fill_type_manual", 1);
       this._recordFormFillingTime("creditCard", "manual", timeStartedFillingMS);
     }
 
     // Early return if it's a duplicate data
     let dupGuid = this.profileStorage.creditCards.getDuplicateGuid(creditCard.record);
     if (dupGuid) {
       this.profileStorage.creditCards.notifyUsed(dupGuid);
-      return;
+      return false;
     }
 
     // Indicate that the user has seen the doorhanger.
     setUsedStatus(2);
 
-    let state = await FormAutofillDoorhanger.show(target, creditCard.guid ? "updateCreditCard" : "addCreditCard");
-    if (state == "cancel") {
-      return;
-    }
+    return async () => {
+      // Suppress the pending doorhanger from showing up if user disabled credit card in previous doorhanger.
+      if (!FormAutofillUtils.isAutofillCreditCardsEnabled) {
+        return;
+      }
+
+      const state = await FormAutofillDoorhanger.show(target, creditCard.guid ? "updateCreditCard" : "addCreditCard");
+      if (state == "cancel") {
+        return;
+      }
+
+      if (state == "disable") {
+        Services.prefs.setBoolPref("extensions.formautofill.creditCards.enabled", false);
+        return;
+      }
+
+      // TODO: "MasterPassword.ensureLoggedIn" can be removed after the storage
+      // APIs are refactored to be async functions (bug 1399367).
+      if (!await MasterPassword.ensureLoggedIn()) {
+        log.warn("User canceled master password entry");
+        return;
+      }
 
-    if (state == "disable") {
-      Services.prefs.setBoolPref("extensions.formautofill.creditCards.enabled", false);
-      return;
-    }
+      let changedGUIDs = [];
+      if (creditCard.guid) {
+        if (state == "update") {
+          this.profileStorage.creditCards.update(creditCard.guid, creditCard.record, true);
+          changedGUIDs.push(creditCard.guid);
+        } else if ("create") {
+          changedGUIDs.push(this.profileStorage.creditCards.add(creditCard.record));
+        }
+      } else {
+        changedGUIDs.push(...this.profileStorage.creditCards.mergeToStorage(creditCard.record));
+        if (!changedGUIDs.length) {
+          changedGUIDs.push(this.profileStorage.creditCards.add(creditCard.record));
+        }
+      }
+      changedGUIDs.forEach(guid => this.profileStorage.creditCards.notifyUsed(guid));
+    };
+  },
 
-    // TODO: "MasterPassword.ensureLoggedIn" can be removed after the storage
-    // APIs are refactored to be async functions (bug 1399367).
-    if (!await MasterPassword.ensureLoggedIn()) {
-      log.warn("User canceled master password entry");
-      return;
+  async _onFormSubmit(data, target) {
+    let {profile: {address, creditCard}, timeStartedFillingMS} = data;
+
+    // Don't record filling time if any type of records has more than one section being
+    // populated. We've been recording the filling time, so the other cases that aren't
+    // recorded on the same basis should be out of the data samples. E.g. Filling time of
+    // populating one profile is different from populating two sections, therefore, we
+    // shouldn't record the later to regress the representation of existing statistics.
+    if (address.length > 1 || creditCard.length > 1) {
+      timeStartedFillingMS = null;
     }
 
-    let changedGUIDs = [];
-    if (creditCard.guid) {
-      if (state == "update") {
-        this.profileStorage.creditCards.update(creditCard.guid, creditCard.record, true);
-        changedGUIDs.push(creditCard.guid);
-      } else if ("create") {
-        changedGUIDs.push(this.profileStorage.creditCards.add(creditCard.record));
-      }
-    } else {
-      changedGUIDs.push(...this.profileStorage.creditCards.mergeToStorage(creditCard.record));
-      if (!changedGUIDs.length) {
-        changedGUIDs.push(this.profileStorage.creditCards.add(creditCard.record));
+    // Transmit the telemetry immediately in the meantime form submitted, and handle these pending
+    // doorhangers at a later.
+    await Promise.all([
+      address.map(addrRecord => this._onAddressSubmit(addrRecord, target, timeStartedFillingMS)),
+      creditCard.map(ccRecord => this._onCreditCardSubmit(ccRecord, target, timeStartedFillingMS)),
+    ].map(pendingDoorhangers => {
+      return pendingDoorhangers.filter(pendingDoorhanger => !!pendingDoorhanger &&
+                                                            typeof pendingDoorhanger == "function");
+    }).map(pendingDoorhangers => new Promise(async resolve => {
+      for (const showDoorhanger of pendingDoorhangers) {
+        await showDoorhanger();
       }
-    }
-    changedGUIDs.forEach(guid => this.profileStorage.creditCards.notifyUsed(guid));
-  },
-
-  _onFormSubmit(data, target) {
-    let {profile: {address, creditCard}, timeStartedFillingMS} = data;
-
-    if (address) {
-      this._onAddressSubmit(address, target, timeStartedFillingMS);
-    }
-    if (creditCard) {
-      this._onCreditCardSubmit(creditCard, target, timeStartedFillingMS);
-    }
+      resolve();
+    })));
   },
   /**
    * Set the probes for the filling time with specific filling type and form type.
    *
    * @private
    * @param  {string} formType
    *         3 type of form (address/creditcard/address-creditcard).
    * @param  {string} fillingType