Closed Bug 1294502 Opened 9 years ago Closed 8 years ago

Combine e10s and non-e10s nsFormAutoComplete implementations

Categories

(Toolkit :: Form Manager, enhancement, P3)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
e10s + ---
firefox51 --- verified

People

(Reporter: mconley, Assigned: mconley)

References

Details

Attachments

(4 files)

Bug 1294502 - Combine e10s and non-e10s nsFormAutoComplete implementations.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1294502 - Move AutoCompletePopup implementation for content to browser-content.js so that non-e10s can use it in a later patch.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1294502 - Make content in non-e10s use the same nsIFormAutoComplete as e10s.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1294502 - Use the same AutoCompletePopup logic for e10s and non-e10s.
58 bytes, text/x-review-board-request
MattN
: review+
Details
Inside nsFormAutoComplete.js, we've got two nsFormAutoComplete implementations - one that runs in the content process, and one in the parent. The content process one messages the parent, which then messages the parent implementation, and the results of that are then sent back down to the child. It's all a bit ham-fisted. I think I know how we can combine these, and just have a single nsFormAutoComplete.js implementation that'll be process agnostic. I think we can mostly keep the parent process implementation, but have it send messages to get FormHistory results from FormHistoryStartup.js. This should work even in the parent process case. In the event that we're doing form history look-ups on the search container (which is kind of a special parent-only case), we won't be able to resolve to an nsIContentFrameMessageManager, and we should fall back to the Services.cpmm, which FormHistoryStartup.js can also respond to.
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Priority: -- → P3
Comment on attachment 8780611 [details] Bug 1294502 - Combine e10s and non-e10s nsFormAutoComplete implementations. https://reviewboard.mozilla.org/r/71294/#review70008 ::: toolkit/components/satchel/nsFormAutoComplete.js:32 (Diff revision 2) > + * It is assumed that nsFormAutoComplete will only ever use > + * one instance at a time, and will not attempt to perform more > + * than one search request with the same instance. However, "more than one search request…with the same instance" Should `…` be "at a time" or do you mean that one needs to make a new isntance for each search? ::: toolkit/components/satchel/nsFormAutoComplete.js:144 (Diff revision 2) > + if (!this.callback) { > + // How did this happen? > + return; Cu.reportError so we can catch this? ::: toolkit/components/satchel/nsFormAutoComplete.js:147 (Diff revision 2) > + } > + this.callback(results); > + }, Is it intentional that we don't remvoe the message listener here? Should we also null out this.callback?
Comment on attachment 8780611 [details] Bug 1294502 - Combine e10s and non-e10s nsFormAutoComplete implementations. https://reviewboard.mozilla.org/r/71294/#review70012
Attachment #8780611 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8780612 [details] Bug 1294502 - Move AutoCompletePopup implementation for content to browser-content.js so that non-e10s can use it in a later patch. https://reviewboard.mozilla.org/r/71296/#review70014
Attachment #8780612 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8780613 [details] Bug 1294502 - Make content in non-e10s use the same nsIFormAutoComplete as e10s. https://reviewboard.mozilla.org/r/71298/#review70016
Attachment #8780613 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8781134 [details] Bug 1294502 - Use the same AutoCompletePopup logic for e10s and non-e10s. https://reviewboard.mozilla.org/r/71632/#review70018 Everything looks good so far but I'm still looking at AutoCompletePopup.jsm since there's a lot going on there. ::: toolkit/components/satchel/AutoCompletePopup.jsm:109 (Diff revision 1) > - _showPopup: function(results) { > - AutoCompleteE10SView.clearResults(); > + // This function is used by the login manager, which uses a single message > + // to fill in the autocomplete results. See Is this comment still correct in that it's only used by login manager?
Comment on attachment 8781134 [details] Bug 1294502 - Use the same AutoCompletePopup logic for e10s and non-e10s. https://reviewboard.mozilla.org/r/71632/#review70026 Since I see the satchel mochitests don't run on Android I just did a quick sanity check and things seem to work there. Awesome job and thanks for doing this cleanup!
Attachment #8781134 - Flags: review?(MattN+bmo) → review+
(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #9) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad5b5abaef47 I wasn't able to add tests to your push (maybe due to permissions) so I triggered another try run in mozreview.
Comment on attachment 8780611 [details] Bug 1294502 - Combine e10s and non-e10s nsFormAutoComplete implementations. https://reviewboard.mozilla.org/r/71294/#review70008 > "more than one search request…with the same instance" > > Should `…` be "at a time" or do you mean that one needs to make a new isntance for each search? You're right, it should be "at a time". The FormHistoryClient can be re-used, so long as previous requests are cancelled. > Is it intentional that we don't remvoe the message listener here? > > Should we also null out this.callback? Oh, good call! I forgot to remove the message listener in this case.
Comment on attachment 8781134 [details] Bug 1294502 - Use the same AutoCompletePopup logic for e10s and non-e10s. https://reviewboard.mozilla.org/r/71632/#review70018 > Is this comment still correct in that it's only used by login manager? Not technically correct, no. I've modified it to: ```JavaScript // Along with being called internally by the receiveMessage handler, // this function is also called directly by the login manager, which // uses a single message to fill in the autocomplete results. See // "RemoteLogins:autoCompleteLogins". ``` Since it does seem a little odd that there are multiple callers.
Comment on attachment 8781134 [details] Bug 1294502 - Use the same AutoCompletePopup logic for e10s and non-e10s. https://reviewboard.mozilla.org/r/71632/#review70268 Note that I updated this patch to fix test_searchSuggestions.js xpcshell test. The test needed to make sure that FormHistoryStartup was first initialized in order to properly respond to its nsFormAutoComplete requests.
Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eaed9150655a Combine e10s and non-e10s nsFormAutoComplete implementations. r=MattN https://hg.mozilla.org/integration/autoland/rev/3ab902adf2ce Move AutoCompletePopup implementation for content to browser-content.js so that non-e10s can use it in a later patch. r=MattN https://hg.mozilla.org/integration/autoland/rev/2557ea45b8e5 Make content in non-e10s use the same nsIFormAutoComplete as e10s. r=MattN https://hg.mozilla.org/integration/autoland/rev/f2ea401ab10c Use the same AutoCompletePopup logic for e10s and non-e10s. r=MattN
Depends on: 1296338
Severity: normal → enhancement
tracking-e10s: --- → +
Blocks: 1296638
Depends on: 1298204
Depends on: 1300919
Depends on: 1305050
I can confirm the change did not affect the user experience behavior of the auto-fill feature. I tested using Fx 51.0b14 both e10s + and e10s - . Cheers!
Status: RESOLVED → VERIFIED
status-firefox51: fixed → verified
Depends on: 1374887
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: