Combine e10s and non-e10s nsFormAutoComplete implementations

VERIFIED FIXED in Firefox 51

Status

()

Toolkit
Form Manager
P3
enhancement
VERIFIED FIXED
a year ago
2 months ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox51 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

a year ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a952e05e0fdd
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Priority: -- → P3
(Assignee)

Comment 9

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad5b5abaef47
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.
(Assignee)

Comment 17

a year ago
mozreview-review-reply
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.
(Assignee)

Comment 18

a year ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

a year ago
mozreview-review
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

a year ago
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
(Assignee)

Updated

a year ago
Depends on: 1296338

Updated

a year ago
Severity: normal → enhancement
tracking-e10s: --- → +

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eaed9150655a
https://hg.mozilla.org/mozilla-central/rev/3ab902adf2ce
https://hg.mozilla.org/mozilla-central/rev/2557ea45b8e5
https://hg.mozilla.org/mozilla-central/rev/f2ea401ab10c
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Updated

a year ago
Blocks: 1296638
Depends on: 1298204
Depends on: 1300919

Updated

11 months ago
Blocks: 1300547
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.