1021172 - Electrolysis (e10s) support for requestAutocomplete

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[:Paolo Amadini]

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.

Comment 1

[Justin Dolske [:Dolske]]

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.

Comment 2

[:Paolo Amadini]

(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.

Comment 3

[:Paolo Amadini]

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.

Comment 4

[:Paolo Amadini]

Attachment: 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.

Comment 5

[Marco Mucci [:MarcoM]]

Added to Iteration 33.3

Comment 6

[Marco Mucci [:MarcoM]]

Removed from Iteration 34.1 based on GMC feddback.

Comment 7

[:Paolo Amadini]

Attachment: The patch

Comment 8

[Matthew N. [:MattN]]

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?

Comment 9

[:Paolo Amadini]

Attachment: 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.

Comment 10

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/9efe325b6dc0

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9efe325b6dc0