Closed Bug 1432485 Opened 3 years ago Closed 3 years ago

Add NavigationListener.onNewSession() to handle new windows

Categories

(GeckoView :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: snorp, Assigned: snorp)

References

Details

Attachments

(1 file)

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.
Summary: Change ContentDelegate.onLoadUri() to return a GeckoSession → Change NavigationListener.onLoadUri() to return a GeckoSession
Assignee: nobody → snorp
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.
Whoops, see comment #1
Flags: needinfo?(nchen)
Flags: needinfo?(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.
Flags: needinfo?(esawin)
(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.
I think we floated the idea of a callback too, e.g.

> void onNewSession(GeckoSession session, String url, TargetWindow where, NewSessionCallback callback);
Flags: needinfo?(nchen)
(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.
Summary: Change NavigationListener.onLoadUri() to return a GeckoSession → Add NavigationListener.onNewSession() to handle new windows
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()?
Attachment #8952542 - Flags: review?(esawin) → review+
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 (...)`
Attachment #8952542 - Flags: review?(nchen) → review+
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 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 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.
(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.
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d39e77de7c54
Add GeckoSession.NavigationListener.onNewSession r=jchen,esawin
https://hg.mozilla.org/mozilla-central/rev/d39e77de7c54
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1440592
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 60 → mozilla60
You need to log in before you can comment on or make changes to this bug.