Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections. r=steveck, seanlee draft
authorRay Lin <ralin@mozilla.com>
Tue, 21 Nov 2017 12:26:10 +0800
changeset 705711 7aaeaa7cdca76e6e6c83177a0b5d98ba6d0ad72b
parent 705710 1b64e25f072f64e6d30b1dd7923c16e9793a7734
child 705712 d3d602508986e2fe05e1690b27ffb27c47941fb6
push id91557
push userbmo:ralin@mozilla.com
push dateThu, 30 Nov 2017 15:31:27 +0000
reviewerssteveck, seanlee
bugs1415073
milestone59.0a1
Bug 1415073 - Refactor records structure of form autofill submission to adapt multiple sections. r=steveck, seanlee 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, {mozSystemGroup: true});
         }
         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
@@ -356,29 +356,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":
@@ -386,52 +388,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);
       }
     };
 
@@ -458,17 +462,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.
@@ -478,65 +482,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