Electrolysis (e10s) support for requestAutocomplete

RESOLVED FIXED in mozilla34

Status

()

Toolkit
Form Manager
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks: 1 bug)

Trunk
mozilla34
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +

Firefox Tracking Flags

(e10s+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
For requestAutocomplete multi-process support, most processing can probably be done in the parent process. The portion that should reside in the content process is probably only related to the actual form handling.
Does this really need to be a separate bug? The expectation for all new features is that they should support E10S as part of their initial design / implementation, and not push off E10S as a future followup.
(Assignee)

Comment 2

4 years ago
(In reply to Justin Dolske [:Dolske] from comment #1)
> Does this really need to be a separate bug? The expectation for all new
> features is that they should support E10S as part of their initial design /
> implementation, and not push off E10S as a future followup.

The reason this bug is here is that we have included support for Electrolysis as part of the initial design :-) We're still discussing the details, but we do have an idea about the effort estimate regardless of how exactly we'll do this, and we chose to track that in this meta-bug, at least for the moment.

The actual implementation will most likely not be in this bug - but will be worked on as part of the rest of the development. We'll take care of keeping this in mind in the effort estimates for other bugs. I've added the "meta" keyword to this bug to clarify.
Keywords: meta
(Assignee)

Updated

4 years ago
Depends on: 1023862
(Assignee)

Comment 3

3 years ago
The initial e10s work ended up consisting of the testing framework (bug 1023862) and the actual message passing (that can be placed in this bug).

Work on future requestAutocomplete bugs, especially the concurrency handling in bug 1020464 and bug 1020459, will keep into account e10s from the start, as they will be developed on top of this framework.
Iteration: --- → 33.3
Points: --- → 5
Keywords: meta
Whiteboard: p=13
(Assignee)

Comment 4

3 years ago
Created attachment 8453152 [details] [diff] [review]
Preliminary patch

This will need some more code comments to explain what is going on, but the code should be mostly final. This uses a file layout similar to Form History.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #8453152 - Flags: feedback?(MattN+bmo)
Added to Iteration 33.3
QA Whiteboard: [qa?]
(Assignee)

Updated

3 years ago
QA Whiteboard: [qa?] → [qa-]
Attachment #8453152 - Flags: feedback?(MattN+bmo) → feedback+

Updated

3 years ago
Flags: firefox-backlog+

Updated

3 years ago
Iteration: 33.3 → 34.1
tracking-e10s: --- → +
Removed from Iteration 34.1 based on GMC feddback.
Status: ASSIGNED → NEW
Iteration: 34.1 → ---
(Assignee)

Comment 7

3 years ago
Created attachment 8463948 [details] [diff] [review]
The patch
Attachment #8453152 - Attachment is obsolete: true
Attachment #8463948 - Flags: review?(MattN+bmo)
Comment on attachment 8463948 [details] [diff] [review]
The patch

Review of attachment 8463948 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments

::: toolkit/components/formautofill/FormAutofillContentService.js
@@ +100,5 @@
> +                                  .getInterface(Ci.nsIDocShell)
> +                                  .sameTypeRootTreeItem
> +                                  .QueryInterface(Ci.nsIDocShell);
> +    let frameMM = rootDocShell.QueryInterface(Ci.nsIInterfaceRequestor)
> +                              .getInterface(Ci.nsIContentFrameMessageManager);

How does this work with sub-frames? Do they have their own message manager? I just want to ensure we don't accidentally confuse subframes and top level frames when sending/receiving messages.

::: toolkit/components/formautofill/formautofill.manifest
@@ +1,5 @@
>  component {ed9c2c3c-3f86-4ae5-8e31-10f71b0f19e6} FormAutofillContentService.js
>  contract @mozilla.org/formautofill/content-service;1 {ed9c2c3c-3f86-4ae5-8e31-10f71b0f19e6}
> +component {51c95b3d-7431-467b-8d50-383f158ce9e5} FormAutofillStartup.js
> +contract @mozilla.org/formautofill/startup;1 {51c95b3d-7431-467b-8d50-383f158ce9e5}
> +category profile-after-change FormAutofillStartup @mozilla.org/formautofill/startup;1

I'm not sure we should be initializing and loading this code on channels where the feature isn't shipping as there will some memory and execution overhead. Perhaps some of the code should be Nightly-only for now?
Attachment #8463948 - Flags: review?(MattN+bmo) → review+
Status: NEW → ASSIGNED

Updated

3 years ago
Iteration: --- → 34.1
(Assignee)

Comment 9

3 years ago
Created attachment 8464816 [details] [diff] [review]
Updated patch

(In reply to Matthew N. [:MattN] from comment #8)
> How does this work with sub-frames? Do they have their own message manager?

No, if I understand correctly, message managers are only present at the level of the <browser> element. They are called "browser message managers" on the parent side and "frame message managers" on the child side. The global message manager in the parent process listens and broadcasts to the browser message managers, which are the only ones that really communicate with the child process.

http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIMessageManager.idl

> I just want to ensure we don't accidentally confuse subframes and top level
> frames when sending/receiving messages.

We shouldn't have issues here, though as noted there's no reentrancy check anyways for now.

> I'm not sure we should be initializing and loading this code on channels
> where the feature isn't shipping as there will some memory and execution
> overhead. Perhaps some of the code should be Nightly-only for now?

I've limited the startup loading and the tests that depend on it to Nightly.
Attachment #8463948 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/9efe325b6dc0
https://hg.mozilla.org/mozilla-central/rev/9efe325b6dc0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34

Updated

3 years ago
Blocks: 1160565
You need to log in before you can comment on or make changes to this bug.