1460047 - Support delay-loading of modules on startup

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P3)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Delay load some modules that are not used until they're enabled.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1460047 - 1. Assign module resource to phases;

Assign a module's .jsm resource to either the init phase or the enable
phase, so that it's possible to delay loading the module resource until
it is enabled.

Review commit: https://reviewboard.mozilla.org/r/242474/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242474/

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1460047 - 2. Add frame script support to ModuleManager;

Make ModuleManager capable of loading frame scripts. This consolidates
more logic from GeckoViewModule into ModuleManager.

Review commit: https://reviewboard.mozilla.org/r/242476/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242476/

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1460047 - 3. Make module resource optional;

Some modules such as Scroll and SelectionAction only require a frame
script and not a resource. Make the module resource optional so we can
remove the redundant resources for these modules.

Review commit: https://reviewboard.mozilla.org/r/242478/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/242478/

Comment 4

[Eugen Sawin [:esawin]]

Comment on attachment 8974177 [details]
Bug 1460047 - 1. Assign module resource to phases;

https://reviewboard.mozilla.org/r/242474/#review248576

That's nice! Let's make sure to track the perf win with this landing.

::: mobile/android/chrome/geckoview/geckoview.js:136
(Diff revision 1)
>   * modules. It is responsible for loading the module JSM file if necessary,
>   * and it acts as the intermediary between ModuleManager and the module
>   * object that extends GeckoViewModule.
>   */
>  class ModuleInfo {
> -  constructor({enabled, manager, name, resource}) {
> +  constructor({manager, name, enabled, onInit, onEnable}) {

Please add comments.

::: mobile/android/chrome/geckoview/geckoview.js:154
(Diff revision 1)
>    onInit() {
>      this._impl.onInit();
>      this.enabled = this._enabledOnInit;
>    }
>  
> +  _loadPhase(aPhase) {

Please add comments.

Comment 5

[Eugen Sawin [:esawin]]

Comment on attachment 8974178 [details]
Bug 1460047 - 2. Add frame script support to ModuleManager;

https://reviewboard.mozilla.org/r/242476/#review248600

We used to have modules and content modules, which were loaded as frame scripts, with a 1:1 (or 1:0) relationship - the naming scheme was consistent about that.
With this patch we rename content modules to frame scripts, which is more generic and removes the GV-specific relationship.

I'm not sure I like that, is there a reason for that?

Comment 6

[Eugen Sawin [:esawin]]

Comment on attachment 8974179 [details]
Bug 1460047 - 3. Make module resource optional;

https://reviewboard.mozilla.org/r/242478/#review248620

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8974178 [details]
Bug 1460047 - 2. Add frame script support to ModuleManager;

https://reviewboard.mozilla.org/r/242476/#review248600

Not really. I can change to content. Do you think we should use "frameScript" as the name of the property that specifis the frame script URL to load?

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8974177 [details]
Bug 1460047 - 1. Assign module resource to phases;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242474/diff/1-2/

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8974178 [details]
Bug 1460047 - 2. Add frame script support to ModuleManager;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242476/diff/1-2/

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8974179 [details]
Bug 1460047 - 3. Make module resource optional;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/242478/diff/1-2/

Comment 11

[Eugen Sawin [:esawin]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #7)
> Not really. I can change to content. Do you think we should use
> "frameScript" as the name of the property that specifis the frame script URL
> to load?

I think "frameScript" makes sense in this context, so does "resource".

Comment 12

[Eugen Sawin [:esawin]]

Comment on attachment 8974178 [details]
Bug 1460047 - 2. Add frame script support to ModuleManager;

https://reviewboard.mozilla.org/r/242476/#review249574

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da7d2ac6100f
1. Assign module resource to phases; r=esawin
https://hg.mozilla.org/integration/autoland/rev/91e14caab7a8
2. Add frame script support to ModuleManager; r=esawin
https://hg.mozilla.org/integration/autoland/rev/699cb2e4fdc1
3. Make module resource optional; r=esawin

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/da7d2ac6100f
https://hg.mozilla.org/mozilla-central/rev/91e14caab7a8
https://hg.mozilla.org/mozilla-central/rev/699cb2e4fdc1