Closed Bug 1021172 Opened 5 years ago Closed 5 years ago

Electrolysis (e10s) support for requestAutocomplete

Categories

(Toolkit :: Form Manager, defect)

defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
mozilla34
Iteration:
34.1
Tracking Status
e10s + ---

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
(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
Depends on: 1023862
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
Attached patch Preliminary patch (obsolete) — Splinter Review
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?]
QA Whiteboard: [qa?] → [qa-]
Attachment #8453152 - Flags: feedback?(MattN+bmo) → feedback+
Flags: firefox-backlog+
Iteration: 33.3 → 34.1
Removed from Iteration 34.1 based on GMC feddback.
Status: ASSIGNED → NEW
Iteration: 34.1 → ---
Attached patch The patch (obsolete) — Splinter Review
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+
Iteration: --- → 34.1
Attached patch Updated patchSplinter Review
(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
https://hg.mozilla.org/mozilla-central/rev/9efe325b6dc0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Blocks: 1160565
You need to log in before you can comment on or make changes to this bug.