1396065 - Enable general GeckoView URI load delegation

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

[Eugen Sawin [:esawin]]

In bug 1377580 we've allowed delegation of external link loading in GeckoView.

In this bug we want to enable general URI load delegation in GeckoView to allow for the GeckoView app to override the default behavior of Gecko when handling top-level loads.

This can be useful, e.g., when handling out-of-scope links in progressive web apps, in which case we want to fire a custom tab intent instead of loading the page in the app's window.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1396065-1.0-Add-nsILoadURIDelegate-to-handle-loa.patch

We've looked for a less-intrusive way to delegate loads without changing Gecko behavior and adding a XPCOM module seems like it could be a sensible approach for this.

With this module, we could also simplify the openURI/createContentWindow split mechanics introduced in bug 1377580 and bug 1394520, although the split itself might still be a good idea since it clears up the openURI semantics a bit.

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1396065-2.0-Override-nsILoadURIDelegate-Handle-i.patch

Comment 3

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1396065-3.0-Only-handle-external-links-in-the-na.patch

Comment 4

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

Comment on attachment 8903751 [details] [diff] [review]
0001-Bug-1396065-1.0-Add-nsILoadURIDelegate-to-handle-loa.patch

Review of attachment 8903751 [details] [diff] [review]:
-----------------------------------------------------------------

Needs a bit of refinement IMHO

::: docshell/base/nsDocShell.cpp
@@ +10089,5 @@
> +  if (aURI && mLoadURIDelegate) {
> +    // Dispatch the load request to the delegate, e.g., to allow for
> +    // GeckoView apps to handle the load event outside of Gecko.
> +    bool handled = false;
> +    int where = aWindowTarget.LowerCaseEqualsLiteral("_blank")

This is wrong, as the target can be any string. "_blank" is just a special one to always use a new window. Later on in InternalLoad() you can see where we try to locate the target docshell, etc. It seems like maybe you want to do the check around here and just not worry about the target string at all: https://dxr.mozilla.org/mozilla-central/rev/13d241d08912be31884f9d0d0e805b25343d6c0a/docshell/base/nsDocShell.cpp#10259

::: xpcom/base/nsILoadURIDelegate.idl
@@ +14,5 @@
> +[scriptable, uuid(78e42d37-a34c-4d96-b901-25385669aba4)]
> +interface nsILoadURIDelegate : nsISupports
> +{
> +    boolean
> +    handle(in nsIURI aURI, in short aWhere, in long aFlags,

This should be a more descriptive name. Also, even if we are doing the "return true if you handled this" in the GeckoView delegate, I don't think it's a very common pattern in Gecko. I'd rather this just be canLoad() or something.

::: xpcom/base/nsLoadURIDelegate.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef nsLoadURIDelegate_h__

You should only need this and the accompanying cpp if you're going to actually implement the interface, which you don't (at least in this patch).

Comment 5

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

Comment on attachment 8903752 [details] [diff] [review]
0002-Bug-1396065-2.0-Override-nsILoadURIDelegate-Handle-i.patch

Review of attachment 8903752 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm with couple problems

::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm
@@ +47,5 @@
>  
> +  // nsILoadURIDelegate.
> +  handle(aUri, aWhere, aFlags, aTriggeringPrincipal) {
> +    debug("handle " + aUri + " " + aWhere + " " + aFlags + " " + aTriggeringPrincipal);
> +    if (this.loadingUri) {

You're trying to avoid calling the delegate for loads that initiated itself, right? You should be able to use the load type in nsDocShell::InternalLoad() to check for this and just not call the delegate in that case. Your approach could cause us to miss load attempts that occur while the page is loading.

@@ +51,5 @@
> +    if (this.loadingUri) {
> +      return false;
> +    }
> +    try {
> +      return !this.createContentWindow(aUri, null, aWhere, aFlags,

should probably refactor things such that this and createContentWindow call a common thing rather than call createContentWindow from here.

Comment 6

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1396065-1.1-Add-nsILoadURIDelegate-to-handle-loa.patch

(In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> This is wrong, as the target can be any string. "_blank" is just a special
> one to always use a new window. Later on in InternalLoad() you can see where
> we try to locate the target docshell, etc. It seems like maybe you want to
> do the check around here and just not worry about the target string at all:
> https://dxr.mozilla.org/mozilla-central/rev/
> 13d241d08912be31884f9d0d0e805b25343d6c0a/docshell/base/nsDocShell.cpp#10259

The target docshell for current-window-requests is set before that and I'm now using it in combination with the target window string to determine the where arguments.

> This should be a more descriptive name. Also, even if we are doing the
> "return true if you handled this" in the GeckoView delegate, I don't think
> it's a very common pattern in Gecko. I'd rather this just be canLoad() or
> something.

I've changed the behavior to the error-based return pattern that is used more commonly. Naming it anything other than load*/open* would be misleading since it's not a side-effect free check.

> You should only need this and the accompanying cpp if you're going to
> actually implement the interface, which you don't (at least in this patch).

I've removed it.

Comment 7

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1396065-2.1-Override-nsILoadURIDelegate-Handle-i.patch

(In reply to James Willcox (:snorp) ([e-mail]) from comment #5)
> You're trying to avoid calling the delegate for loads that initiated itself,
> right? You should be able to use the load type in nsDocShell::InternalLoad()
> to check for this and just not call the delegate in that case. Your approach
> could cause us to miss load attempts that occur while the page is loading.

Removed it, it's not longer required.

> should probably refactor things such that this and createContentWindow call
> a common thing rather than call createContentWindow from here.

Added handleLoadUri as the generic load function.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8904345 [details] [diff] [review]
0001-Bug-1396065-1.1-Add-nsILoadURIDelegate-to-handle-loa.patch

>+  if (aURI && mLoadURIDelegate) {
>+    // Dispatch the load request to the delegate, e.g., to allow for
>+    // GeckoView apps to handle the load event outside of Gecko.
>+    const int where = (aWindowTarget.IsEmpty() || targetDocShell)
>+                      ? nsIBrowserDOMWindow::OPEN_CURRENTWINDOW
>+                      : nsIBrowserDOMWindow::OPEN_NEW;
This is confusing. If there is targetDocShell, it probably means to not open in current window, but in another, already opened window
(or iframe). OPEN_DEFAULTWINDOW might make a bit more sense than OPEN_CURRENTWINDOW
With that, r+

Comment 9

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1396065-1.2-Add-nsILoadURIDelegate-to-handle-loa.patch

(In reply to Olli Pettay [:smaug] from comment #8)
> This is confusing. If there is targetDocShell, it probably means to not open
> in current window, but in another, already opened window
> (or iframe). OPEN_DEFAULTWINDOW might make a bit more sense than
> OPEN_CURRENTWINDOW

I've extended the condition for the load delegation to make it clearer.
At the point of delegation, targetDocShell is either null or refers to the current docshell.
Should targetDocShell refer to an existing docshell other than the current one, then the InternalLoad call will be transfered to it, so we can handle that case then.

As for setting OPEN_DEFAULTWINDOW, this is something we would like to avoid since Gecko defaults don't matter when delegating up to GeckoView. We have plans to remove TargetWindow.DEFAULT to account for that.

Comment 10

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

Comment on attachment 8904548 [details] [diff] [review]
0001-Bug-1396065-1.2-Add-nsILoadURIDelegate-to-handle-loa.patch

Review of attachment 8904548 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsIDocShell.idl
@@ +463,5 @@
>     * <browser> or nsWebBrowser for their root docshell.
>     */
>    attribute nsISecureBrowserUI securityUI;
>  
> +  attribute nsILoadURIDelegate loadURIDelegate;

Need to add a comment about what this does

Comment 11

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1396065-1.3-Add-nsILoadURIDelegate-to-handle-loa.patch

Added nsIDocShell.idl comment.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47b2e91b1060
[1.3] Add nsILoadURIDelegate to handle load delegation to the window (GeckoView). r=smaug,snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/83e6e35ab72b
[2.1] Override nsILoadURIDelegate::Handle in GeckoViewNavigation to allow for app specific URI loading handling. r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/8886840becc8
[3.0] Only handle external links in the navigation listener in the example app. r=snorp

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/47b2e91b1060
https://hg.mozilla.org/mozilla-central/rev/83e6e35ab72b
https://hg.mozilla.org/mozilla-central/rev/8886840becc8