I noticed this in Custom Tabs, but PWA has the same problem.

Comment on attachment 8882712 [details] Bug 1377580 - Fix target=_blank somewhat for GeckoView https://reviewboard.mozilla.org/r/153796/#review159144 We need to look into the validity of the assertion in [1], which prevents this from working with e10s-disabled geckoview_example. Also, this is broken for e10s-enabled GeckoView apps, e.g. it asserts in [2]. r+ because it enables opening target=_blank links in PWAs and custom tabs, let's handle the remaining issues in a follow-up bug. [1] https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#14585 [2] https://searchfox.org/mozilla-central/source/dom/ipc/ContentParent.cpp#4723 ::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:21 (Diff revision 1) > > var dump = Cu.import("resource://gre/modules/AndroidLog.jsm", {}) > .AndroidLog.d.bind(null, "ViewNavigation"); > > function debug(aMsg) { > - // dump(aMsg); > + // dump(aMsg + '\n'); Please use " for consistency. ::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm:87 (Diff revision 1) > debug("receiveMessage " + aMsg.name); > } > > // nsIBrowserDOMWindow::openURI implementation. > openURI(aUri, aOpener, aWhere, aFlags) { > - debug("openURI: aUri.spec=" + aUri.spec); > + debug('openURI: ' + aUri); " for consistency.

Currently, nsIBrowserDOMWindow::openURI is used in two cases, to get the content window target and to actually open a given URI. The former case is differentiated by calling openURI with a null-URI. GeckoView has a direct 1:1 relationship to a content window, which means that it can not return anything reasonable for an openURI call with aWhere being OPEN_NEWWINDOW, OPEN_NEWTAB or OPEN_SWITCHTAB. One way to handle this case would be to delegate such requests to the application holding the GeckoView so that it can act appropriately (e.g., ignore the flag and open in the same GeckoView or create a new GeckoView maybe in a new tab). To enable this, we would need to extend the API to replace the openURI-null-URI case (e.g., nsIBrowserDOMWindow::getContentWindow) so that the URI is provided in such a call, too, which is required for the delegation of the request to the GeckoView application. We also have to make sure that we properly abort the URI opening mechanics in the case where nsIBrowserDOMWindow::getContentWindow returns an invalid window and that we handle the e10s case accordingly. Would this be a reasonable approach and would such an extension of nsIBrowserDOMWindow be acceptable?

I don't think I quite understand what is being asked here. Shouldn't the geckoview implementation of nsIBrowserDOMWindow just do whatever is needed internally when those not-currently-supported (OPEN_*) flags are used? Why would nsIBrowserDOMWindow need to be changed?

GeckoView has no concept of tabs or windows (besides its own), which is why we can't (synchronously) return anything meaningful in the noted openURI cases. What we can do is delegate the request to the application level, in that case we need to pass the URI (e.g., at [1]). We could special-case the behavior for GeckoView or we could split the openURI(null, ...) case into a dedicated call (e.g., nsIBrowserDOMWindow::getContentWindow) to support the case cross-platform. For OPEN_NEWWINDOW, OPEN_NEWTAB and OPEN_SWITCHTAB, GeckoView would always return null in getContentWindow to abort the URI-loading on the Gecko side while providing the URI and the rest of the arguments to its application which can handle it appropriately. In the case of progressive web apps and Google custom tabs, we could fire an Android intent if the URL is not in the app's scope/domain in that case. [1] https://searchfox.org/mozilla-central/source/xpfe/appshell/nsContentTreeOwner.cpp#934

In short, extending nsIBrowserDOMWindow would help because for GeckoView we always need to provide the URI when calling openURI to allow for up-delegation, however that would break some existing use cases where we call openURI with a null-URI to get the content window without actually loading the URI. Extending the API for this existing case would solve this issue.

So you propose splitting openURI to two methods, openURI which always gets non-null uri, and getContentWindow? That should be ok, since the current openURI behavior is hackish. (why is the method called openURI when it doesn't always open an URI)

(In reply to Olli Pettay [:smaug] from comment #7) > So you propose splitting openURI to two methods, openURI which always gets > non-null uri, and getContentWindow? That should be ok, since the current > openURI behavior is hackish. (why is the method called openURI when it > doesn't always open an URI) That's the idea. I'll clean up the patch and flag you for review. First, we have to get it to run for the non-e10s case, which would be the default for Chrome custom tabs and PWAs for the time being (through Fennec).

Add getContentWindow API, redirect openURI(null,...) calls and fix comments consistency. Disregarding the final implementation on the GeckoView side, I think splitting openURI would be an improvement.

Comment on attachment 8893543 [details] [diff] [review] 0001-Bug-1377580-1.0-Extend-nsIBrowserDOMWindow-to-suppor.patch I think browser.js should have a helper method which is like the current openURI, and openURI would become something which checks that aURI is non-null. getContentWindow would then also call the helper method using null as uri.. Want to fix the typo privlege. privilege

Addressed comments and renamed getContentWindow to createContentWindow as it does more accurately reflect the implementation.

This allows GeckoView to forward external link requests to its app (non-e10s only for now). For external link requests, both createContentWindow and openURI will forward the request to the app and abort window creation/URI loading on the Gecko side.

Comment on attachment 8895362 [details] [diff] [review] 0001-Bug-1377580-1.1-Extend-nsIBrowserDOMWindow-to-suppor.patch >--- a/browser/base/content/browser.js >+++ b/browser/base/content/browser.js >@@ -5264,7 +5264,20 @@ nsBrowserAccess.prototype = { > return browser; > }, > >+ createContentWindow(aURI, aOpener, aWhere, aFlags) { >+ return this.getContentWindow(null, aOpener, aWhere, aFlags); >+ }, >+ > openURI(aURI, aOpener, aWhere, aFlags, aTriggeringPrincipal) { >+ if (!aURI) { >+ Cu.reportError("openURI should only be called with a valid URI"); >+ throw Cr.NS_ERROR_FAILURE; >+ } >+ return this.getContentWindow(aURI, aOpener, aWhere, aFlags, >+ aTriggeringPrincipal); >+ }, >+ >+ getContentWindow(aURI, aOpener, aWhere, aFlags, aTriggeringPrincipal) { Could you call getContentWindow something else. It is confusing that it may load something. Perhaps getContentWindowOrOpenURI. (I know that is horribly long, but the current openURI is just odd beast) >+++ b/toolkit/components/windowwatcher/nsWindowWatcher.cpp >@@ -871,7 +871,7 @@ nsWindowWatcher::OpenWindowInternal(mozIDOMWindowProxy* aParent, > uriToLoad, name, features, aForceNoOpener, > &windowIsNew, getter_AddRefs(newWindow)); > >- if (NS_SUCCEEDED(rv)) { >+ if (NS_SUCCEEDED(rv) && newWindow) { > GetWindowTreeItem(newWindow, getter_AddRefs(newDocShellItem)); > if (windowIsNew && newDocShellItem) { > // Make sure to stop any loads happening in this window that the I should have asked this before, but why this change. Could you drop this change. With that, r+

Comment on attachment 8895365 [details] [diff] [review] 0002-Bug-1377580-2.0-Add-support-for-external-URI-loading.patch Review of attachment 8895365 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/modules/geckoview/GeckoViewNavigation.jsm @@ +80,5 @@ > + return this.browser.contentWindow; > + } > + > + let message = { > + type: "GeckoView:LoadUriExternal", GeckoView:LoadUriExternal seems like a message that GeckoView would send to Gecko, not really the other way around. Maybe GeckoView:OnLoadUri instead? Or LoadUriRequested?

Comment on attachment 8895366 [details] [diff] [review] 0003-Bug-1377580-3.0-Implement-external-URI-loading-for-c.patch Review of attachment 8895366 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java @@ +539,5 @@ > + view.loadUri(uri); > + } else { > + final Intent intent = new Intent(Intent.ACTION_VIEW); > + intent.setData(uri); > + startActivity(intent); We should probably explicitly launch whatever Fennec we're a part of.

Comment on attachment 8895367 [details] [diff] [review] 0004-Bug-1377580-4.0-Implement-external-URI-loading-for-s.patch Review of attachment 8895367 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/java/org/mozilla/gecko/webapps/WebAppActivity.java @@ +270,5 @@ > + view.loadUri(uri); > + } else { > + final Intent intent = new Intent(Intent.ACTION_VIEW); > + intent.setData(Uri.parse(uri)); > + startActivity(intent); We want to use a custom tab here, but this is better than nothing for now.

Addressed comments.

Renamed event name to GeckoView:OnLoadUri.

Rebased on new event name. (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #18) > We should probably explicitly launch whatever Fennec we're a part of. I think firing the standard intent is the more user-friendly way. Chrome custom tabs offer opening in Fennec if that's the default browser. Let's address this in a follow up bug, same as the PWA implementation details.

Rebased on new event name. (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #19) > We want to use a custom tab here, but this is better than nothing for now. Going to address this in more detail in bug 1389236.

Rebased on new event name.

Pushed by esawin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/216dc8344ce0 [1.2] Extend nsIBrowserDOMWindow to support content window creation without URI loading. r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/315ef1c38167 [2.1] Add support for external URI loading in GeckoView. r=snorp https://hg.mozilla.org/integration/mozilla-inbound/rev/bc20d7133078 [3.1] Implement external URI loading for custom tabs. r=snorp https://hg.mozilla.org/integration/mozilla-inbound/rev/22735f5c52cf [4.1] Implement external URI loading for standalone progressive web apps. r=snorp https://hg.mozilla.org/integration/mozilla-inbound/rev/2ef6679f5c0e [5.1] Implement external URI loading in the GeckoView example app. r=snorp

https://hg.mozilla.org/mozilla-central/rev/216dc8344ce0 https://hg.mozilla.org/mozilla-central/rev/315ef1c38167 https://hg.mozilla.org/mozilla-central/rev/bc20d7133078 https://hg.mozilla.org/mozilla-central/rev/22735f5c52cf https://hg.mozilla.org/mozilla-central/rev/2ef6679f5c0e

I'm porting this to TB and SM and looking at https://hg.mozilla.org/mozilla-central/rev/216dc8344ce0#l1.12 + createContentWindow(aURI, aOpener, aWhere, aFlags) { you forgot the principal which the function is meant to have: https://hg.mozilla.org/mozilla-central/rev/216dc8344ce0#l2.90 + mozIDOMWindowProxy + createContentWindow(in nsIURI aURI, in mozIDOMWindowProxy aOpener, + in short aWhere, in long aFlags, + in nsIPrincipal aTriggeringPrincipal);

uh, how did that happen. I'll let esawin to fix.

Thanks, that was on oversight or failed rebase (although it's not critical since it would always be a null-principal). Since this has already landed on m-c, I'll land the hotfix in bug 1390469.