1453537 - Add "init browser" phase in GeckoView startup

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P2)

Description

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

Some modules perform tasks between creating the browser element and binding the browser element to the window. We should add a separate onInitBrowser call for that.

Comment 1

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

Attachment: Bug 1453537 - 1. Use Map for GV modules;

Map generally works better than object as key-value stores.

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

Comment 2

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

Attachment: Bug 1453537 - 2. Name GV module overrides more consistently;

init(), onSettingsUpdate(), register(), and unregister() in
GeckoView(Content)Module.jsm are all expected to be overridden. Make
their naming consistent by always using the "on" prefix, similar to
our Java callback naming in GeckoSession.

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

Comment 3

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

Attachment: Bug 1453537 - 3. Add a "onInitBrowser" call for GV module;

Several modules could use an "init browser" phase, where the browser or
the environment has to be initialized before the browser element is
bound to the window.

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

Comment 4

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

Attachment: Bug 1453537 - 4. Implement GeckoViewNavigation.onInitBrowser;

Initialize browserDOMWindow and any pending "geckoview-window-created"
observers in onInitBrowser, before the browser is bound to the window.

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

Comment 5

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

Attachment: Bug 1453537 - 5. Set browser remote attribute during onInitBrowser;

Set the browse remote attribute in onInitBrowser so we only set it once
and don't have to remove and re-add the browser to the window.

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

Comment 6

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

Attachment: Bug 1453537 - 6. Clean up Settings JS modules;

Clean up the Settings JS modules a little. Don't keep extra state
variables if we don't have to.

Set displayMode inside the proper frame script rather than through a
data URI. Because we never unregister frame scripts, the data URIs can
start accumulating if display mode is changed several times.

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

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8967232 [details]
Bug 1453537 - 2. Name GV module overrides more consistently;

https://reviewboard.mozilla.org/r/235928/#review241664


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: mobile/android/modules/geckoview/GeckoViewContentModule.jsm:69
(Diff revision 1)
> -  unregister() {}
> +
> +  // Override to detect settings change. Access settings via this.settings.
>    onSettingsUpdate() {}
> +
> +  // Override to enable module after setting a Java delegate.
> +  register {}

Error: Parsing error: unexpected token { [eslint]

Comment 8

[Eugen Sawin [:esawin]]

Comment on attachment 8967231 [details]
Bug 1453537 - 1. Use Map for GV modules;

https://reviewboard.mozilla.org/r/235926/#review241748

Comment 9

[Eugen Sawin [:esawin]]

Comment on attachment 8967232 [details]
Bug 1453537 - 2. Name GV module overrides more consistently;

https://reviewboard.mozilla.org/r/235928/#review241750

::: mobile/android/chrome/geckoview/GeckoViewContentSettings.js:13
(Diff revision 1)
>  // Handles GeckoView content settings including:
>  // * tracking protection
>  // * desktop mode
>  class GeckoViewContentSettings extends GeckoViewContentModule {
> -  init() {
> +  onInit() {
>      debug `init`;

Please fix debug logs ("onInit", "onEnable", ...).

::: mobile/android/chrome/geckoview/GeckoViewNavigationContent.js:16
(Diff revision 1)
>    LoadURIDelegate: "resource://gre/modules/LoadURIDelegate.jsm",
>  });
>  
>  // Implements nsILoadURIDelegate.
>  class GeckoViewNavigationContent extends GeckoViewContentModule {
> -  register() {
> +  onEnable() {

We should document the register and enable mechanics more explicitly.

::: mobile/android/modules/geckoview/GeckoViewModule.jsm:79
(Diff revision 1)
>  
> -  register() {}
> +  _unregister() {
> +    if (!this.isRegistered) {
> +      return;
> +    }
> +    this._eventProxy.unregisterListener();

I think we might want to remove automatic unregistering so that the module has to call it. Some modules may register listeners in onInit, which would break things.

Comment 10

[Eugen Sawin [:esawin]]

Comment on attachment 8967233 [details]
Bug 1453537 - 3. Add a "onInitBrowser" call for GV module;

https://reviewboard.mozilla.org/r/235930/#review241752

Comment 11

[Eugen Sawin [:esawin]]

Comment on attachment 8967234 [details]
Bug 1453537 - 4. Implement GeckoViewNavigation.onInitBrowser;

https://reviewboard.mozilla.org/r/235932/#review241756

Comment 12

[Eugen Sawin [:esawin]]

Comment on attachment 8967235 [details]
Bug 1453537 - 5. Set browser remote attribute during onInitBrowser;

https://reviewboard.mozilla.org/r/235934/#review241758

That's a nice change!

Comment 13

[Eugen Sawin [:esawin]]

Comment on attachment 8967236 [details]
Bug 1453537 - 6. Clean up Settings JS modules;

https://reviewboard.mozilla.org/r/235936/#review241760

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm:46
(Diff revision 1)
>    onInit() {
> -    this._isSafeBrowsingInit = false;
> +    this._useTrackingProtection = false;
>      this._useDesktopMode = false;
> -    this._displayMode = Ci.nsIDocShell.DISPLAY_MODE_BROWSER;
>  
> -    this.messageManager.loadFrameScript(
> +    this.registerContent(

We can't use it here with automatic unregistering, see previous patch's comment.

::: mobile/android/modules/geckoview/GeckoViewSettings.jsm:68
(Diff revision 1)
> -      SafeBrowsing.init();
> -      this._isSafeBrowsingInit = true;
> -    }
> +  }
> +
> +  set useTrackingProtection(aUse) {
> +    aUse && SafeBrowsing;

Looks a bit odd.

Comment 14

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

Comment on attachment 8967236 [details]
Bug 1453537 - 6. Clean up Settings JS modules;

https://reviewboard.mozilla.org/r/235936/#review241760

> We can't use it here with automatic unregistering, see previous patch's comment.

`registerContent` shouldn't be affected by automatic unregistering. But I think I'll go ahead and remove automatic unregistering anyways.

Comment 15

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

Attachment: Bug 1453537 - 2b. Don't unregister listeners automatically on disable;

Only unregister listeners explicitly. Also clean up the EventProxy code
a little.

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

Comment 16

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

Comment on attachment 8967231 [details]
Bug 1453537 - 1. Use Map for GV modules;

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

Comment 17

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

Comment on attachment 8967232 [details]
Bug 1453537 - 2. Name GV module overrides more consistently;

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

Comment 18

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

Comment on attachment 8967233 [details]
Bug 1453537 - 3. Add a "onInitBrowser" call for GV module;

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

Comment 19

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

Comment on attachment 8967234 [details]
Bug 1453537 - 4. Implement GeckoViewNavigation.onInitBrowser;

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

Comment 20

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

Comment on attachment 8967235 [details]
Bug 1453537 - 5. Set browser remote attribute during onInitBrowser;

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

Comment 21

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

Comment on attachment 8967236 [details]
Bug 1453537 - 6. Clean up Settings JS modules;

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

Comment 22

[Eugen Sawin [:esawin]]

Comment on attachment 8967513 [details]
Bug 1453537 - 2b. Don't unregister listeners automatically on disable;

https://reviewboard.mozilla.org/r/236192/#review241966

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a91a03cf5528
1. Use Map for GV modules; r=esawin
https://hg.mozilla.org/integration/autoland/rev/8ebfd97f230b
2. Name GV module overrides more consistently; r=esawin
https://hg.mozilla.org/integration/autoland/rev/e8b47563c732
2b. Don't unregister listeners automatically on disable; r=esawin
https://hg.mozilla.org/integration/autoland/rev/d6da57f4b96d
3. Add a "onInitBrowser" call for GV module; r=esawin
https://hg.mozilla.org/integration/autoland/rev/0b95977592df
4. Implement GeckoViewNavigation.onInitBrowser; r=esawin
https://hg.mozilla.org/integration/autoland/rev/095e2adce50b
5. Set browser remote attribute during onInitBrowser; r=esawin
https://hg.mozilla.org/integration/autoland/rev/61be6eea7441
6. Clean up Settings JS modules; r=esawin

Comment 24

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/a91a03cf5528
https://hg.mozilla.org/mozilla-central/rev/8ebfd97f230b
https://hg.mozilla.org/mozilla-central/rev/e8b47563c732
https://hg.mozilla.org/mozilla-central/rev/d6da57f4b96d
https://hg.mozilla.org/mozilla-central/rev/0b95977592df
https://hg.mozilla.org/mozilla-central/rev/095e2adce50b
https://hg.mozilla.org/mozilla-central/rev/61be6eea7441