Closed Bug 1458327 Opened Last year Closed Last year

Improve GeckoView JS module management

Categories

(GeckoView :: General, enhancement, P2)

All
Android
enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(5 files)

Right now the module manager in geckoview.js does not do much. We should improve it by letting it handle module-management events.
Comment on attachment 8972450 [details]
Bug 1458327 - 1. Pass init-data instead of settings to Window;

https://reviewboard.mozilla.org/r/241054/#review246998

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSessionSettings.java:169
(Diff revision 1)
>              return mBundle.getString(key.name);
>          }
>      }
>  
>      /* package */ GeckoBundle asBundle() {
> -        return mBundle;
> +        return new GeckoBundle(mBundle);

The idea for asBundle was to avoid the copy. The instancing should be either done at the call site or we have to rename this to toBundle().
Comment on attachment 8972451 [details]
Bug 1458327 - 2. Let ModuleManager manage more aspects of modules;

https://reviewboard.mozilla.org/r/241056/#review247014

It's not clear to me how moving properties out of GVModule to ModuleInfo helps us, can you please explain this on a specific case?
Comment on attachment 8972452 [details]
Bug 1458327 - 3. Make GeckoSessionHandler and GeckoSessionSettings work with init-data;

https://reviewboard.mozilla.org/r/241058/#review247016

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSessionHandler.java:93
(Diff revision 1)
>          }
>  
> -        final GeckoBundle msg = new GeckoBundle(1);
> +        final GeckoBundle msg = new GeckoBundle(2);
>          msg.putString("module", mModuleName);
> -        eventDispatcher.dispatch(ready ? "GeckoView:Register"
> -                                       : "GeckoView:Unregister", msg);
> +        msg.putBoolean("enabled", ready);
> +        session.getEventDispatcher().dispatch("GeckoView:EnableModule", msg);

:EnableModule suggests enabled state to me. How about :ModuleState instead?
Comment on attachment 8972452 [details]
Bug 1458327 - 3. Make GeckoSessionHandler and GeckoSessionSettings work with init-data;

https://reviewboard.mozilla.org/r/241058/#review247018

r+ with nits.
Attachment #8972452 - Flags: review?(esawin) → review+
Comment on attachment 8972451 [details]
Bug 1458327 - 2. Let ModuleManager manage more aspects of modules;

https://reviewboard.mozilla.org/r/241056/#review247014

Do you mean properties like "messageManager" and "settings"? I want to have one source of truth for these, which will be the ModuleManager in geckoview.js. I wanted one source of truth for per-module properties like "enabled" as well, inside geckoview.js; so those go inside ModuleInfo.

Having module properties inside geckoview.js lets us perform some optimizations down the road, such as not loading a module (and therefore GVModule) on startup if it's not enabled. Module loading (fetching/parsing/running each jsm) is showing up on startup profiles, so delay-loading is something we want down the road.
Comment on attachment 8972453 [details]
Bug 1458327 - 4. Always register some events / frame scripts;

https://reviewboard.mozilla.org/r/241060/#review247030

::: mobile/android/modules/geckoview/GeckoViewContentModule.jsm:70
(Diff revision 1)
>      );
>  
>      this.onInit();
> +
> +    this.messageManager.sendAsyncMessage(
> +      "GeckoView:ContentRegistered", { module: this.moduleName });

This change seems out-of-context in this patch.
Attachment #8972453 - Flags: review?(esawin) → review+
Comment on attachment 8972451 [details]
Bug 1458327 - 2. Let ModuleManager manage more aspects of modules;

https://reviewboard.mozilla.org/r/241056/#review247074

Please add some ModuleInfo comments.

::: mobile/android/chrome/geckoview/geckoview.js:18
(Diff revision 1)
>  });
>  
>  XPCOMUtils.defineLazyGetter(this, "WindowEventDispatcher",
>    () => EventDispatcher.for(window));
>  
> +class ModuleInfo {

Please comment on the purpose and responsibilities of the class.

::: mobile/android/chrome/geckoview/geckoview.js:134
(Diff revision 1)
>  
> -  remove: function(aType) {
> -    this.modules.delete(aType);
> +  forEach(aCallback) {
> +    this._modules.forEach(aCallback, this);
>    },
>  
> -  forEach: function(aCallback) {
> +  _onSettingsUpdate() {

What about _updateSettings(aSettings) instead?
Attachment #8972451 - Flags: review?(esawin) → review+
Comment on attachment 8972450 [details]
Bug 1458327 - 1. Pass init-data instead of settings to Window;

https://reviewboard.mozilla.org/r/241054/#review247108

r+ with nits because it helps with remote runtime support.
Attachment #8972450 - Flags: review?(esawin) → review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a9d7817a640
1. Pass init-data instead of settings to Window; r=esawin
https://hg.mozilla.org/integration/autoland/rev/f37d7d18d8e7
2. Let ModuleManager manage more aspects of modules; r=esawin
https://hg.mozilla.org/integration/autoland/rev/5ab8f1a1eb57
3. Make GeckoSessionHandler and GeckoSessionSettings work with init-data; r=esawin
https://hg.mozilla.org/integration/autoland/rev/3647f62ed6b6
4. Always register some events / frame scripts; r=esawin
https://hg.mozilla.org/integration/autoland/rev/3e1d4faf82ad
5. Fix some comments; r=jchen
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 61 → mozilla61
You need to log in before you can comment on or make changes to this bug.