Open Bug 1364602 Opened 9 years ago Updated 3 years ago

Include autofill profile emails in <input type=email> form manager results

Categories

(Toolkit :: Form Manager, enhancement, P3)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

People

(Reporter: MattN, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1364602 - switch form-history "totalScore" computation from SQL to js.
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1364602 - include autofill results with FormHistory.
59 bytes, text/x-review-board-request
Details
Bug 1364602 - test including autofill values with form history.
59 bytes, text/x-review-board-request
Details
Currently there are two sources of data for autocomplete suggestions that don't get Form Autofill: 1) Form history based on the id/name attribute value[1] 2) @list/<datalist> specified by the author[2] With Form Autofill enabled we will have user-curated profiles of more relevant data than random emails previous used from #1. We should include deduped emails from the autofill profile storage in the list of results with an appropriate frecency score mixed in with form history results. We shouldn't allow deleting these form autofill results though. For now we can just do this for type=email but eventually we could share the heuristic results with form history. Import autofill profile storage like so: > XPCOMUtils.defineLazyModuleGetter(this, "profileStorage", > "resource://formautofill/ProfileStorage.jsm"); Example: https://dxr.mozilla.org/mozilla-central/rev/d8762cb967423618ff0a488f14745f60964e5c49/browser/extensions/formautofill/FormAutofillParent.jsm#42-43,228 [1] https://dxr.mozilla.org/mozilla-central/rev/8a7d0b15595f9916123848ca906f29c62d4914c9/toolkit/components/satchel/nsFormFillController.cpp#777-778 [2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/satchel/nsInputListAutoComplete.js
Attachment #8871959 - Flags: review?(MattN+bmo)
Edit #4 in "add unit & integration test." fixes errors thrown by eslint in `test_autofill.js`.
Comment on attachment 8871373 [details] Bug 1364602 - switch form-history "totalScore" computation from SQL to js. https://reviewboard.mozilla.org/r/142850/#review154294 Hi Jonathan, This patch is switching the calculations from SQL to JS but part of the point of that change is to unify the code. When I said to make a "helper", I meant one that would be shared/used by more than one place. Since the existing sorting is happening in nsFormAutoComplete (in the content process), can we keep all the sorting there so the parent just returns the results (now with raw frecency info) and the child just sorts it? ::: toolkit/components/satchel/FormHistory.jsm:1016 (Diff revision 3) > + let boundaryCalc = 0; > + // for each word, calculate word boundary weights > + for (let token of searchTokens) { > + if (entryLowerCase.startsWith(token)) { > + ++boundaryCalc; > + } > + if (entryLowerCase.includes(` ${token}`)) { > + ++boundaryCalc; > + } > + } > + boundaryCalc *= params.boundaryWeight; > + > + // now add more weight if we have a traditional prefix match and > + // multiply boundary bonuses by boundary weight > + if (entryLowerCase.startsWith(searchString)) { > + boundaryCalc += params.prefixWeight; > + } > + let boundaryBonuses = Math.max(1, boundaryCalc); This is duplicating `_calculateScore` from nsFormAutoComplete. I had mentioned before that we should use that existing code. I don't think it makes sense to have two calculation methods that have the same purpose. ::: toolkit/components/satchel/FormHistory.jsm:1072 (Diff revision 3) > * The score is then divided by the bucket size and we round the result so that entries > * with a very similar frecency are bucketed together with an alphabetical sort. This is > * to reduce the amount of moving around by entries while typing. > */ > > - let query = "/* do not warn (bug 496471): can't use an index */ " + > + let query = `/* do not warn (bug 496471): can't use an index */ Since you're removing the `ORDER BY` you should be able to remove this comment but double-check that the warning doesn't re-appear ::: toolkit/components/satchel/FormHistory.jsm:1095 (Diff revision 3) > + if (!aCallbacks || !aCallbacks.handleResult) { > + return; > + } Do you know any valid reason for us to hit this return? And why can't we do this check at the top of method? ::: toolkit/components/satchel/FormHistoryStartup.js:1 (Diff revision 3) > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components; It would be great if you added a comment stating that this component runs in the parent process.
Attachment #8871373 - Flags: review?(MattN+bmo)
Comment on attachment 8871959 [details] Bug 1364602 - include autofill results with FormHistory. https://reviewboard.mozilla.org/r/143482/#review154298 ::: toolkit/components/satchel/FormHistory.jsm:1149 (Diff revision 5) > totalScore: Math.round(frecency * boundaryBonuses), > + valueMetadata, > }); Should this have been in part 1? Perhaps it wasn't needed with your approach but I think it would be to do the sorting in the content process.
Comment on attachment 8871373 [details] Bug 1364602 - switch form-history "totalScore" computation from SQL to js. https://reviewboard.mozilla.org/r/142850/#review154294 > Do you know any valid reason for us to hit this return? And why can't we do this check at the top of method? I put this early return because, previously, this check was done for every item in `aResultSet`, which is not necessary because the state of `aCallbacks` is independent from `aResultSet`. Therefore, I put it outside of that loop, though, I reckon it would be even better to put it at the top of the method, I'll fix that.
Comment on attachment 8871373 [details] Bug 1364602 - switch form-history "totalScore" computation from SQL to js. https://reviewboard.mozilla.org/r/142850/#review159962 Thanks ::: commit-message-8f4d2:1 (Diff revision 6) > +Bug 1364602 - switch totalScore computation from SQL to js. r=mattn Nit: After reading this commit message most people would have no idea what area of the code it's about. Maybe add "form history" before the word "totalScore" ::: toolkit/components/satchel/FormHistory.jsm:991 (Diff revision 6) > updateFormHistoryWrite(aChanges, aCallbacks); > } > }, > > + /** > + * helper to compute frecency of an entry Nit: Start this with a capital ::: toolkit/components/satchel/FormHistory.jsm:995 (Diff revision 6) > + * @param {Object} metaData > + * meta-data of the entry > + * @param {number} metaData.firstUsed > + * timestamp of the first use of the entry > + * @param {number} metaData.lastUsed > + * timestamp of the last use of the entry > + * @param {number} metaData.timesUsed Nit: "metadata" is one word so there shouldn't be a capital "D" ::: toolkit/components/satchel/FormHistory.jsm:1006 (Diff revision 6) > + * @param {number} metaData.timesUsed > + * how many times the entry has been used > + * @returns {Object} > + * frecency and boundaryBonuses > + */ > + getEntryFrecency(params, {firstUsed, lastUsed, timesUsed}) { Nit: How about `computeFrecency`? ::: toolkit/components/satchel/FormHistory.jsm:1007 (Diff revision 6) > + * how many times the entry has been used > + * @returns {Object} > + * frecency and boundaryBonuses > + */ > + getEntryFrecency(params, {firstUsed, lastUsed, timesUsed}) { > + // first, compute frecency This comment seems unnecessary since that's all we do here ::: toolkit/components/satchel/FormHistory.jsm:1008 (Diff revision 6) > + * @returns {Object} > + * frecency and boundaryBonuses > + */ > + getEntryFrecency(params, {firstUsed, lastUsed, timesUsed}) { > + // first, compute frecency > + let expiredEntryBool = (firstUsed < params.expiryDate) ? 1 : 0; Nit: no need for parentheses ::: toolkit/components/satchel/FormHistory.jsm:1042 (Diff revision 6) > /* Three factors in the frecency calculation for an entry (in order of use in calculation): > * 1) average number of times used - items used more are ranked higher > * 2) how recently it was last used - items used recently are ranked higher > * 3) additional weight for aged entries surviving expiry - these entries are relevant > * since they have been used multiple times over a large time span so rank them higher > * The score is then divided by the bucket size and we round the result so that entries > * with a very similar frecency are bucketed together with an alphabetical sort. This is > * to reduce the amount of moving around by entries while typing. > */ This comment should be moved to the JSDoc of getEntryFrecency ::: toolkit/components/satchel/FormHistory.jsm:1064 (Diff revision 6) > let escapedToken = stmt.escapeStringForLIKE(searchTokens[i], "/"); > - stmt.params["tokenBegin" + i] = escapedToken + "%"; > + stmt.params[`tokenContains${i}`] = `%${escapedToken}%`; > - stmt.params["tokenBoundary" + i] = "% " + escapedToken + "%"; Note that the changes to backticks and searchTokenCount (x2) made this patch harder to review because it mades lines change that didn't need to. it wasn't really necessary in this commit but it's fine to leave them now that I took the time to review them. ::: toolkit/components/satchel/FormHistory.jsm:1077 (Diff revision 6) > let pending = stmt.executeAsync({ > - handleResult(aResultSet) { > + handleResult: aResultSet => { > for (let row = aResultSet.getNextRow(); row; row = aResultSet.getNextRow()) { > let value = row.getResultByName("value"); > - let frecency = row.getResultByName("frecency"); > - let entry = { > + let lowerValue = value.toLowerCase(); > + let valueMetadata = { Nit: `metadata` is good enough ::: toolkit/components/satchel/FormHistory.jsm:1086 (Diff revision 6) > text: value, > - textLowerCase: value.toLowerCase(), > + textLowerCase: lowerValue, > frecency, Revert this line since we don't need a variable for one use ::: toolkit/components/satchel/nsFormAutoComplete.js:372 (Diff revision 6) > > - let searchTokens = searchString.split(/\s+/); > // We have a list of results for a shorter search string, so just Why move this? Seems like it should be reverted ::: toolkit/components/satchel/nsFormAutoComplete.js:530 (Diff revision 6) > if (entry.textLowerCase.startsWith(token)) { > - boundaryCalc++; > + ++boundaryCalc; > } > if (entry.textLowerCase.includes(" " + token)) { > - boundaryCalc++; > + ++boundaryCalc; > } > } > - boundaryCalc = boundaryCalc * this._boundaryWeight; > + boundaryCalc *= this._boundaryWeight; > // now add more weight if we have a traditional prefix match and > // multiply boundary bonuses by boundary weight > if (entry.textLowerCase.startsWith(aSearchString)) { > boundaryCalc += this._prefixWeight; > } > - entry.totalScore = Math.round(entry.frecency * Math.max(1, boundaryCalc)); > + let boundaryBonuses = Math.max(1, boundaryCalc); > - }, > > + entry.totalScore = Math.round(entry.frecency * boundaryBonuses); To make reviewing easier please try to keep unrelated changes to a minimum. It also makes it harder for someone to read the commit after landing and understand why some lines changed in a specific commit. I would prefer if these changes were reverted.
Attachment #8871373 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8871959 [details] Bug 1364602 - include autofill results with FormHistory. https://reviewboard.mozilla.org/r/143482/#review159994 I still need to review dedupeAutofillAndHistory and nsFormAutoComplete.js ::: commit-message-b7f17:1 (Diff revision 8) > +Bug 1364602 - inject autofill results with FormHistory. r=mattn Nit: s/inject/Include/ ::: toolkit/components/satchel/FormHistory.jsm:1007 (Diff revision 8) > + * field on which you want to filter on (e.g. "email") > + * @returns {Array.<Object>} > + * clones of all the filtered profiles > + */ > + getAutoFillProfiles(searchString, fieldName = "email") { > + if (!profileStorage) { Is there a reason why this helper isn't in FormHistoryStartup.js since it's not related to Form History storage. ::: toolkit/components/satchel/FormHistory.jsm:1011 (Diff revision 8) > + * @param {string} fieldName > + * field on which you want to filter on (e.g. "email") > + * @returns {Array.<Object>} > + * clones of all the filtered profiles > + */ > + getAutoFillProfiles(searchString, fieldName = "email") { I'm not sure it makes sense to have a default argument value in the long term so I would remove it. ::: toolkit/components/satchel/FormHistory.jsm:1011 (Diff revision 8) > + getAutoFillProfiles(searchString, fieldName = "email") { > + if (!profileStorage) { > + return []; IMO fieldName should be the first argument since it should be required whereas having non searchString could mean to give all matching values ::: toolkit/components/satchel/FormHistoryStartup.js:121 (Diff revision 8) > this.pendingQuery = null; > if (aReason) { > return; > } > > + // XXX condition to remove eventually Rather than an XXX comment (which to me indicates more of a bug, which IMO this is not) you can write a a comment saying: // For now we only query autofill storage for @type=email ::: toolkit/components/satchel/FormHistoryStartup.js:123 (Diff revision 8) > + let autofillProfiles = FormHistory > + .getAutoFillProfiles(searchString) > + .map(profile => { > + let value = profile[params.fieldtype]; > + let lowerValue = value.toLowerCase(); > + let valueMetadata = { > + firstUsed: profile.timeCreated * 1000, > + lastUsed: profile.timeLastUsed * 1000, > + timesUsed: profile.timesUsed, > + }; > + > + return { > + text: value, > + textLowerCase: lowerValue, > + valueMetadata, > + }; > + }); IMO this should move into the `getAutoFillProfiles` helper (which should be renamed getAutofillValues with a lowercase "f"). Basically I don't think it makes sense for this complexity to be inline here. ::: toolkit/components/satchel/nsFormAutoComplete.js:258 (Diff revision 8) > } > - dump("FormAutoComplete: " + message + "\n"); > Services.console.logStringMessage("FormAutoComplete: " + message); Please revert this
Attachment #8871959 - Flags: review?(MattN+bmo)
Comment on attachment 8874614 [details] Bug 1364602 - test including autofill values with form history. https://reviewboard.mozilla.org/r/145952/#review160000 ::: commit-message-c2f98:1 (Diff revision 7) > +Bug 1364602 - add unit & integration test. r=mattn Nit: "Test including autofill values with form history" ::: toolkit/components/satchel/FormHistory.jsm:1002 (Diff revision 7) > + getProfileStorage() { > + profileStorage.initialize(); > + return profileStorage; > + }, > + For the same reason as I said in part 2, I don't think it makes sense for this to be part of FormHistory.jsm which is about Form History storage. ::: toolkit/components/satchel/test/unit/head_satchel.js:49 (Diff revision 7) > var dirSvc = Cc["@mozilla.org/file/directory_service;1"] > .getService(Ci.nsIProperties); > > // Send the profile-after-change notification to the form history component to ensure > // that it has been initialized. > -var formHistoryStartup = Cc["@mozilla.org/satchel/form-history-startup;1"] > +var formHistoryStartupService = Cc["@mozilla.org/satchel/form-history-startup;1"]; This isn't the service reference so should probably be renamed
Comment on attachment 8871373 [details] Bug 1364602 - switch form-history "totalScore" computation from SQL to js. https://reviewboard.mozilla.org/r/142850/#review159962 > Why move this? Seems like it should be reverted Oh, sorry, I didn't notice this was getting moved out of the `if` to above it so it can be used in the `else`.
Comment on attachment 8871959 [details] Bug 1364602 - include autofill results with FormHistory. https://reviewboard.mozilla.org/r/143482/#review160320 There are still some more things to fix here… not too big though. ::: toolkit/components/satchel/FormHistoryStartup.js:138 (Diff revision 10) > + // remove unnecessary information at this point, and sort the results > + for (let result of results) { > + result.frecency = FormHistory.computeFrecency(params, result.metadata); > + delete result.metadata; > + } The comment talks about sorting but I don't see sorting happening here ::: toolkit/components/satchel/FormHistoryStartup.js:168 (Diff revision 10) > } > } > }, > + > + /** > + * dedupe autofill & FormHistory results Nit: Capital "D" on "Dedupe" ::: toolkit/components/satchel/FormHistoryStartup.js:184 (Diff revision 10) > + for (let result of formhistoryResults) { > + dedupes.set(result.textLowerCase, result); > + } > + > + for (let result of autofillResults) { > + let deduped = dedupes.get(result.textLowerCase); s/deduped/existing/ ::: toolkit/components/satchel/FormHistoryStartup.js:192 (Diff revision 10) > + if (!deduped) { > + dedupes.set(result.textLowerCase, result); > + continue; > + } > + > + // collision! I think a comment like: > We have a collision so appropriately merge the metadata for accurate frecency calculations. ::: toolkit/components/satchel/FormHistoryStartup.js:216 (Diff revision 10) > + * @param {string} fieldName > + * field on which you want to filter on (e.g. "email") field type to be filtered and returned ::: toolkit/components/satchel/FormHistoryStartup.js:218 (Diff revision 10) > + * @param {string} searchString > + * according field must start with searchString This description should be reworded ::: toolkit/components/satchel/FormHistoryStartup.js:220 (Diff revision 10) > + * @returns {Array.<Object>} > + * clones of all the filtered profiles > + */ > + getAutofillProfiles(fieldName, searchString = "") { I think you forgot to rename the method and update the description and @returns since it should return profiles values, not whole profiles ::: toolkit/components/satchel/FormHistoryStartup.js:252 (Diff revision 10) > + getProfileStorage() { > + profileStorage.initialize(); How do you ensure initialize is called for actual users, not in automated tests? ::: toolkit/components/satchel/FormHistoryStartup.js:252 (Diff revision 10) > + getProfileStorage() { > + profileStorage.initialize(); We're trying to move away from the `profile` name and I think in this code it would be clearer to talk about it as "autofill" storage so `getAutofillStorage` ::: toolkit/components/satchel/FormHistoryStartup.js:252 (Diff revision 10) > + getProfileStorage() { > + profileStorage.initialize(); > + return profileStorage; > + }, Normally this should be a lazy getter so we don't cal initialize every time it's accessed but you're not even using this method in this patch so I don't know if that's fine… ::: toolkit/components/satchel/nsFormAutoComplete.js:263 (Diff revision 10) > /* > * autoCompleteSearchAsync I thought you converted these to JSDoc and deleted the redundant method name already but I guess not in this file… Don't do that in this commit though. ::: toolkit/components/satchel/nsFormAutoComplete.js:266 (Diff revision 10) > }, > > /* > * autoCompleteSearchAsync > * > * aInputName -- |name| attribute from the form input being autocompleted. While you're here can you update this line to reflect reality since it's the @id if @name is empty ::: toolkit/components/satchel/nsFormAutoComplete.js:442 (Diff revision 10) > - this.getAutoCompleteValues(client, aInputName, searchString, processEntry); > + // if aField is not given, fill in the props so `getAutoCompleteValues` doesn't throw an error > + if (!aField) { > + aField = { > + name: aInputName || "", > + id: aInputName || "", > + }; > + } With my later suggestion this can be removed. ::: toolkit/components/satchel/nsFormAutoComplete.js:496 (Diff revision 10) > * > * client - a FormHistoryClient instance to perform the search with > - * fieldName - fieldname field within form history (the form input name) > + * aField - the form input > * searchString - string to search for I think you missed the comment on my last review to use JSDoc syntax to describe the types that aField can be ::: toolkit/components/satchel/nsFormAutoComplete.js:509 (Diff revision 10) > - getAutoCompleteValues(client, fieldName, searchString, callback) { > + getAutoCompleteValues(client, aField, searchString, callback) { > let params = { > agedWeight: this._agedWeight, > bucketSize: this._bucketSize, > expiryDate: 1000 * (Date.now() - this._expireDays * 24 * 60 * 60 * 1000), > - fieldname: fieldName, > + fieldname: aField.name || aField.id, I don't think we should duplicate this logic from nsFormFillController.cpp's GetSearchParam so maybe we should keep two separate arguments `fieldName` and `aField` and then define that aField can be null like for `autoCompleteSearchAsync` and use a ternary to null-check it before assigning to `fieldtype`?
Attachment #8871959 - Flags: review?(MattN+bmo)
Priority: -- → P3
Comment on attachment 8871959 [details] Bug 1364602 - include autofill results with FormHistory. Pretty sure this will need rebasing and isn't something I'll be able to work on any time soon. Luke/Cindy, do you think this is something that should get finished by the autofill team? I still think it's very useful e.g. for email newsletter signups.
Flags: needinfo?(lchang)
Flags: needinfo?(chsiang)
Attachment #8871959 - Flags: review?(MattN+bmo)
Attachment #8874614 - Flags: review?(MattN+bmo)
Yeah, it's useful indeed and is already a backlog of bug 1338485. I think the autofill team can take this bug if it's not in a hurry, say, as soon as we get V2 & V3 features done. Cindy, what do you think?
Flags: needinfo?(lchang)
Unflag this and revisit when there are resources to work on form autofill
Flags: needinfo?(chsiang)
Assignee: jonathan.guillotte.blouin → nobody
Mentor: mozilla+bmo
Status: ASSIGNED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: