1432485 - Add NavigationListener.onNewSession() to handle new windows

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

This would allow you to return the GeckoSession you want the load to occur in, or null to block the load altogether.

Right now we have no way to open a new window (session) while preserving things like window.opener. I'm seeing problems with this while trying to run mochitest under GV.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

I took a look at this today, since it's blocking us from running mochitest/reftests completely.

My first thought was as comment #0 says, just make onLoadUri() return a GeckoSession. But we actually have two different usages of onLoadUri(). The first is as a predicate for whether or not the URI should be loaded at all, and the second is to decide in which <browser> the load should occur. In the second case we simply return the browser for the current GeckoView if the predicate returns false (which means it was not handled by the app). I think we actually need to split this second case out into something like "GeckoSession onNewSession(String url)". Jim, Eugen, opinions?

The other thing we could do is speculatively create a new GeckoSession automatically using the same settings as the current one. That would look like "void onNewSession(GeckoSession session)". The app would then close the passed GeckoSession if it didn't want to do anything with it, but most likely the load would have already started at that point. I kind of like this API better if we could solve that issue.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Whoops, see comment #1

Comment 3

[Eugen Sawin [:esawin]]

To clarify, you want extend the NavigationListener by

GeckoSession onNewSession(GeckoSession session, String url, TargetWindow where)

which has to return the session the url should be loaded in and potentially the the current session?

This could be OK, but we need to provide some sensible default behavior for it since some apps will probably not be interested in handling the loading at all.

For the speculative callback, I find it odd to expose optimizations like this in the API. Instead we could provide a GeckoSession factory which would return a pre-cooked session in such a case.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Eugen Sawin [:esawin] from comment #3)
> To clarify, you want extend the NavigationListener by
> 
> GeckoSession onNewSession(GeckoSession session, String url, TargetWindow
> where)
> 
> which has to return the session the url should be loaded in and potentially
> the the current session?
>

I think for cases where Gecko calls this, it usually expects a new window. We'll need to investigate whether that can work or not.
 
> This could be OK, but we need to provide some sensible default behavior for
> it since some apps will probably not be interested in handling the loading
> at all.

I think if you don't implement the delegate at all, it would be the same as implementing onNewSession() and returning null, which means the load is blocked entirely.

> 
> For the speculative callback, I find it odd to expose optimizations like
> this in the API. Instead we could provide a GeckoSession factory which would
> return a pre-cooked session in such a case.

Yeah, thinking about it more I don't really like the speculative thing for several reasons.

Comment 5

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

I think we floated the idea of a callback too, e.g.

> void onNewSession(GeckoSession session, String url, TargetWindow where, NewSessionCallback callback);

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jim Chen [:jchen] [:darchons] from comment #5)
> I think we floated the idea of a callback too, e.g.
> 
> > void onNewSession(GeckoSession session, String url, TargetWindow where, NewSessionCallback callback);

Yeah, the patch I have now uses something like this, where NewSessionCallback is

public class Response<T> {
    void respond(T t);
    void abort();
}

I'm having some problems with the TabGroup for new sessions, but once I figure that out I'll get it up for discussion.

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1432485 - Add GeckoSession.NavigationListener.onNewSession

This allows apps to decide which GeckoSession should handle a load that
will be in a new window (e.g., window.open()).

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

Comment 8

[Eugen Sawin [:esawin]]

Comment on attachment 8952542 [details]
Bug 1432485 - Add GeckoSession.NavigationListener.onNewSession

https://reviewboard.mozilla.org/r/221770/#review227826

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java:57
(Diff revision 1)
>          final String intentArgs = getIntent().getStringExtra("args");
>  
>          if (BuildConfig.DEBUG) {
>              // In debug builds, we want to load JavaScript resources fresh with each build.
>              geckoArgs = "-purgecaches";
> +

Empty line.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java
(Diff revision 1)
>  
>          @Override
>          public boolean onLoadUri(final GeckoSession session, final String uri,
>                                   final TargetWindow where) {
> -            Log.d(LOGTAG, "onLoadUri=" + uri +
> -                          " where=" + where);

Since it's currently our testing app, I would leave the logs in.

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:101
(Diff revision 1)
>      Services.tm.spinEventLoopUntil(() => handled !== undefined);
>  
>      return handled;
>    }
>  
> +  waitAndSetOpener(sessionId, opener) {

aSessionId, aOpener

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:103
(Diff revision 1)
>      return handled;
>    }
>  
> +  waitAndSetOpener(sessionId, opener) {
> +    if (!sessionId) {
> +      return Promise.resolve(null);

Shouldn't that be a .reject()?

Comment 9

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

Comment on attachment 8952542 [details]
Bug 1432485 - Add GeckoSession.NavigationListener.onNewSession

https://reviewboard.mozilla.org/r/221770/#review227970

::: commit-message-2c000:4
(Diff revision 1)
> +Bug 1432485 - Add GeckoSession.NavigationListener.onNewSession r=jchen,esawin
> +
> +This allows apps to decide which GeckoSession should handle a load that
> +will be in a new window (e.g., window.open()).

`window.open()` has some additional features like opening in a specific named window. It'd be nice to look into that in a followup.

::: mobile/android/chrome/geckoview/geckoview.js:12
(Diff revision 1)
>  ChromeUtils.import("resource://gre/modules/AppConstants.jsm");
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  ChromeUtils.defineModuleGetter(this, "EventDispatcher",
>    "resource://gre/modules/Messaging.jsm");
> +ChromeUtils.defineModuleGetter(this, "Services",

I think we should start using `XPCOMUtils.defineLazyModuleGetters` with two or more modules.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java:147
(Diff revision 1)
> +                    final String uri = message.getString("uri");
> +                    listener.onNewSession(GeckoSession.this, uri,
> +                        new Response<GeckoSession>() {
> +                            @Override
> +                            public void respond(GeckoSession session) {
> +                                if (session.isOpen() && session.isReady()) {

`session` can be null

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java:147
(Diff revision 1)
> +                    final String uri = message.getString("uri");
> +                    listener.onNewSession(GeckoSession.this, uri,
> +                        new Response<GeckoSession>() {
> +                            @Override
> +                            public void respond(GeckoSession session) {
> +                                if (session.isOpen() && session.isReady()) {

`isReady()` can race with the Gecko thread, but we can address that in a follow-up.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java:148
(Diff revision 1)
> +                    listener.onNewSession(GeckoSession.this, uri,
> +                        new Response<GeckoSession>() {
> +                            @Override
> +                            public void respond(GeckoSession session) {
> +                                if (session.isOpen() && session.isReady()) {
> +                                    throw new IllegalArgumentException("Must use a new GeckoSession instance");

I think this is worth revisiting in a follow-up. It seems logical to me to allow an existing GeckoSession, and there are ways we can potentially implement that (e.g. creating a new `<browser>`).

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java:454
(Diff revision 1)
>  
>      public GeckoSession() {
>          this(/* settings */ null);
>      }
>  
> +    public GeckoSession(GeckoSession otherSession) {

I don't think this gets us much benefit. "Copy of GeckoSession" is not very well defined; right now the only thing it copies is settings, but a consumer may as well expect copying of the current page, page history, etc. The other existing constructor does everything this does, and is much more explicit that the only thing copied is the set of settings.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java:1420
(Diff revision 1)
>          * @param where The target window.
>          *
> -        * @return True if the URI loading has been handled, false if Gecko
> -        *         should handle the loading.
> +        * @return Whether or not the load was handled. Returning false will allow Gecko
> +        *         to continue the load as normal.
>          */
>          boolean onLoadUri(GeckoSession session, String uri, TargetWindow where);

We should make `onLoadUri` use `Response<Boolean>` eventually in a follow-up; there's no reason not to.

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:168
(Diff revision 1)
>            " aWhere=" + aWhere +
>            " aFlags=" + aFlags);
>  
> -    let handled = this.handleLoadUri(aUri, aOpener, aWhere, aFlags,
> +    const browser = this.handleNewSession(aUri, aOpener, aWhere, aFlags,
> -                                     aTriggeringPrincipal);
> +                                          aTriggeringPrincipal);
> -    if (!handled &&
> +    if (!browser) {

`return browser && browser.contentWindow` instead of `if (...)`

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8952542 [details]
Bug 1432485 - Add GeckoSession.NavigationListener.onNewSession

https://reviewboard.mozilla.org/r/221770/#review227826

> Shouldn't that be a .reject()?

I think reject() should be treated the same as throwing an exception. 'null' is an allowed value, so we don't want to throw an exception.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8952542 [details]
Bug 1432485 - Add GeckoSession.NavigationListener.onNewSession

https://reviewboard.mozilla.org/r/221770/#review227970

> `window.open()` has some additional features like opening in a specific named window. It'd be nice to look into that in a followup.

That should work. Gecko takes care of that before asking to open a new window.

> I think we should start using `XPCOMUtils.defineLazyModuleGetters` with two or more modules.

Good point, I'll fix that.

> `session` can be null

Indeed!

> `isReady()` can race with the Gecko thread, but we can address that in a follow-up.

Can you describe the race? I think I can drop isOpen() here, since that's implied by isReady().

> I think this is worth revisiting in a follow-up. It seems logical to me to allow an existing GeckoSession, and there are ways we can potentially implement that (e.g. creating a new `<browser>`).

The assumption we have so far is that GeckoSession relates 1:1 with a single <browser>, so that would be a large change if we could swap out the underlying <browser> later. Worth thinking about.

We should at least be able to use an existing GeckoSession where the <browser> has the same opener as the new one. I'll file a followup.

> I don't think this gets us much benefit. "Copy of GeckoSession" is not very well defined; right now the only thing it copies is settings, but a consumer may as well expect copying of the current page, page history, etc. The other existing constructor does everything this does, and is much more explicit that the only thing copied is the set of settings.

Yeah, good points.

> We should make `onLoadUri` use `Response<Boolean>` eventually in a follow-up; there's no reason not to.

I agree, most (all?) of the delegate methods that return stuff should be async. I don't like Response<> though. I'd rather return a Promise-like thing from the delegate method.

Comment 12

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

Comment on attachment 8952542 [details]
Bug 1432485 - Add GeckoSession.NavigationListener.onNewSession

https://reviewboard.mozilla.org/r/221770/#review227970

> Can you describe the race? I think I can drop isOpen() here, since that's implied by isReady().

One scenario is when after the UI thread runs `newSession.openWindow(...)` and the `isReady()` check, but before the UI thread has had a chance to respond back the session ID. During that time, I think the Gecko thread can load the new session and finish loading, before it receives the session ID, at which point it is too late to call `presetOpenerWindow()`. It's pretty unlikely though.

> I agree, most (all?) of the delegate methods that return stuff should be async. I don't like Response<> though. I'd rather return a Promise-like thing from the delegate method.

IMHO I like `Response<>` because it's simple and it doesn't force the consumer into any particular async/Promise framework, which we'd be doing if the consumer is to return a Promise.

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jim Chen [:jchen] [:darchons] from comment #12)
> Comment on attachment 8952542 [details]
> Bug 1432485 - Add GeckoSession.NavigationListener.onNewSession
> 
> https://reviewboard.mozilla.org/r/221770/#review227970
> 
> > Can you describe the race? I think I can drop isOpen() here, since that's implied by isReady().
> 
> One scenario is when after the UI thread runs `newSession.openWindow(...)`
> and the `isReady()` check, but before the UI thread has had a chance to
> respond back the session ID. During that time, I think the Gecko thread can
> load the new session and finish loading, before it receives the session ID,
> at which point it is too late to call `presetOpenerWindow()`. It's pretty
> unlikely though.

Hmm, indeed. I think we need a timeout on the waitAndSetOpener(), so at least we can exit the inner loop in that case.

> 
> > I agree, most (all?) of the delegate methods that return stuff should be async. I don't like Response<> though. I'd rather return a Promise-like thing from the delegate method.
> 
> IMHO I like `Response<>` because it's simple and it doesn't force the
> consumer into any particular async/Promise framework, which we'd be doing if
> the consumer is to return a Promise.

That's true, but I feel like if the lib we choose is lightweight enough it shouldn't be a burden. The current thing is indeed very simple, and I don't think we'll ever really need chaining, etc, like you'd get from a Promise. Let's chat about it next week.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d39e77de7c54
Add GeckoSession.NavigationListener.onNewSession r=jchen,esawin

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d39e77de7c54