972320 - [User Story] Applications scope

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Runtime, defect)

Description

[Ben Francis [:benfrancis]]

Currently if you click a link to an external web site inside an app it will open in the current app window.

It would be nice if these links opened in a browser window instead of inside the app so that you don't get stranded on a web site with no browser chrome and your visit is recorded in browsing history.

My proposal is to open any link that doesn't match the origin of the app in a browser window, or the app window of another app corresponding to the origin of that URL.

Maybe all we need to do is fire a mozbrowseropen event instead of just navigate to the URL when the user clicks an external URL and and let the system app decide which window to load the URL in.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: app-uri-space.patch

This patch forces every links out of origin for the current app to be opened in a "_blank" target, which is properly handled by the mozbrowser API and gaia to end up in the browser app.

Boris, I'm not sure whether to go with this patch, or to create something similar to browserChrome3->OnBeforeLinkTraversal() but with the app id information. In any case this would be implemented by the apps service so it's very similar complexity (i.e pretty simple). Let me know what you prefer!

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8377908 [details] [diff] [review]
app-uri-space.patch

This seems like it would disallow apps having random web content showing in them, or at least links in that web content working to change what web content is showing.  Is that the goal?

The overall approach feels reasonable, I guess; just not sure whether we should be restricting this to the root docshell of an app or something...

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Won't this break the ability for apps to use facebook login?

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Jonas Sicking (:sicking) from comment #3)
> Won't this break the ability for apps to use facebook login?

not sure, but in any case it seems we need something more complex based on what you and pauljt have been discussing.

Comment 5

[Ben Francis [:benfrancis]]

See also bug 996039 which would implement the inverse - opening links to app URLs from outside an app inside the app.

Comment 6

[Ben Francis [:benfrancis]]

(In reply to Jonas Sicking (:sicking) from comment #3)
> Won't this break the ability for apps to use facebook login?

As discussed on the mailing list this kind of use case could use window.open with the dialog feature to create an overlay rather than open outside the app.

Comment 7

[[:fabrice] Fabrice Desré]

We will implement the proposal at https://groups.google.com/forum/#!topic/mozilla.dev.webapi/yqLffXgokuI

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: wip

Posting my current WIP since I believe it's enough for Ben to make progress on the gaia parts.

This implements support for the `scope` and `open-in-app` properties in manifests, and navigation within the same context.
When the decision is to load the url in an app context, gaia now receives a mozbrowseropenwindow event with the following properties:
details.isApp is true
details.name is the manifest url of the app.

If several apps can open a url, I reused the activity picker for now. We just don't display any title, and also ignore "cancel" actions, defaulting to the first app in the list. I'll let ux people decide what exactly we want there.

Comment 9

[[:fabrice] Fabrice Desré]

Attachment: patch

Jonas, can you take a look? We're not doing anything special for "dialog" windows yet, but maybe that could go in a follow up?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8478661 [details] [diff] [review]
patch

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

Still looking, but submitting what I have.

::: docshell/base/nsDocShell.cpp
@@ +6335,5 @@
> +  nsString target = NS_LITERAL_STRING("");
> +  if (aLoadInfo) {
> +    aLoadInfo->GetTarget(getter_Copies(target));
> +  }
> +  rv = appsService->NavigateAppTo(appId, aURI, target, callback);

Hmm.. I had expected it to be the responsibility of the system app to choose which app to open a URL in. If this is simpler then I think this is totally fine too.

@@ +6339,5 @@
> +  rv = appsService->NavigateAppTo(appId, aURI, target, callback);
> +  if (NS_SUCCEEDED(rv) && aChannel) {
> +    // Stop the current load, but don't trigger an error page.
> +    nsCOMPtr<nsIRequest> request(do_QueryInterface(aChannel));
> +    request->Cancel(NS_OK);

This is a problem though. But maybe not one we need to address right now.

The problem is that if if a request is made with POST data, then calling NavigateAppTo(..., url, ...) and then cancelling the channel will drop that POST data.

Also, will we end up sending out the request to the network twice? It would be good to avoid that.

@@ +12745,5 @@
>    NS_IMETHOD Run() {
> +    // If we are not a top window and there is no target, don't delegate the
> +    // load to an app.
> +    bool isTopLevel;
> +    nsresult rv = mHandler->GetIsBrowserOrApp(&isTopLevel);

Won't this return true even if we're inside a nested <iframe mozbrowser>? Isn't the goal here to only have the "real" toplevel <iframe mozbrowser> to get the new behavior?

@@ +12821,5 @@
> +      mTargetSpec = NS_LITERAL_STRING("_blank");
> +      break;
> +    case nsIAppNavigationCallback::OPEN_IN_APP:
> +      // Build a special target name as _app_$ManifestURL.
> +      mTargetSpec = NS_LITERAL_STRING("_app_");

Ugh :( Can webpages use this to target links at specific apps?

Comment 11

[[:fabrice] Fabrice Desré]

(In reply to Jonas Sicking (:sicking) from comment #10)
> Comment on attachment 8478661 [details] [diff] [review]
> patch
> 
> Review of attachment 8478661 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Still looking, but submitting what I have.
> 
> ::: docshell/base/nsDocShell.cpp
> @@ +6335,5 @@
> > +  nsString target = NS_LITERAL_STRING("");
> > +  if (aLoadInfo) {
> > +    aLoadInfo->GetTarget(getter_Copies(target));
> > +  }
> > +  rv = appsService->NavigateAppTo(appId, aURI, target, callback);
> 
> Hmm.. I had expected it to be the responsibility of the system app to choose
> which app to open a URL in. If this is simpler then I think this is totally
> fine too.

Yep, I'm not sure this would have changed anything really, mostly because we still need to be able to not open in an app or new frame. And this meant more plumbing.

> @@ +6339,5 @@
> > +  rv = appsService->NavigateAppTo(appId, aURI, target, callback);
> > +  if (NS_SUCCEEDED(rv) && aChannel) {
> > +    // Stop the current load, but don't trigger an error page.
> > +    nsCOMPtr<nsIRequest> request(do_QueryInterface(aChannel));
> > +    request->Cancel(NS_OK);
> 
> This is a problem though. But maybe not one we need to address right now.
> 
> The problem is that if if a request is made with POST data, then calling
> NavigateAppTo(..., url, ...) and then cancelling the channel will drop that
> POST data.

I didn't know we could carry around POST data while doing redirects.

> Also, will we end up sending out the request to the network twice? It would
> be good to avoid that.

I need to double check, but the channel is the newly created channel for the redirect, *before* we call asyncOpen and go to the network. So I believe we are fine with cancelling it there.

> @@ +12745,5 @@
> >    NS_IMETHOD Run() {
> > +    // If we are not a top window and there is no target, don't delegate the
> > +    // load to an app.
> > +    bool isTopLevel;
> > +    nsresult rv = mHandler->GetIsBrowserOrApp(&isTopLevel);
> 
> Won't this return true even if we're inside a nested <iframe mozbrowser>?
> Isn't the goal here to only have the "real" toplevel <iframe mozbrowser> to
> get the new behavior?

Likely, I had not tested with nested <iframe mozbrowser>, only with nested <iframes> in a <iframe mozbrowser>

> @@ +12821,5 @@
> > +      mTargetSpec = NS_LITERAL_STRING("_blank");
> > +      break;
> > +    case nsIAppNavigationCallback::OPEN_IN_APP:
> > +      // Build a special target name as _app_$ManifestURL.
> > +      mTargetSpec = NS_LITERAL_STRING("_app_");
> 
> Ugh :( Can webpages use this to target links at specific apps?

Yes, I need to change that. Not sure yet how though.

Comment 12

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Boris & Olli,

I'm requesting your feedback on the changes I made in this wip.

What we want to achieve is to let gaia load urls in a context that may be different depending on the app (or not) the load comes from, and what the url to load is. Apps can define which URLs they can open in their scope. See https://groups.google.com/forum/#!topic/mozilla.dev.webapi/yqLffXgokuI for the full write up.

The decision of where to load a URL for a given app is implemented on the AppsService by the NavigateAppTo() method that calls back with a flag to let the docshell do the appropriate load. In some cases, we want to open the url in a frame that belongs to an app, but is not the current app. The previous wip was using a hack where the target was _app_$ManifestURL, but that's meant that any link could also use that. Instead, in this patch I'm adding a `manifestURL` parameter that bubbles up to BrowserElementParent.cpp that can then tweak the event that is sent to gaia, and gaia reuses an existing frame or creates a new one as needed. Just adding this parameter touches a lot of interfaces/callers, so let me know if you think of a better way to do that. thanks!

Comment 13

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

So we send manifest url from the child to parent. Can we trust that being valid or why does it not matter?

But I can't really comment the patch much since I don't know what DoAppRedirectIfNeeded is for, and
bug 1044490 doesn't really explain that too well (and it didn't add any tests as examples).

Comment 14

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

(+    nsCOMPtr<nsIAppsService> appsService =
+      do_GetService(APPS_SERVICE_CONTRACTID);
+    NS_ASSERTION(appsService, "No AppsService available");
+    nsCOMPtr<nsIURI> redirect;
+    rv = appsService->GetRedirect(appId, aURI, getter_AddRefs(redirect));
is the main part I don't understand from bug 1044490 )

Comment 15

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

Comment on attachment 8481726 [details] [diff] [review]
patch v2

Looks like bz reviewed all the app redirect, so better if he gives feedback.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8481726 [details] [diff] [review]
patch v2

Sorry this took so long; I've been trying, and failing, to understand what the goal is here.

But either this is really overengineered or I'm just confused.  Why, for example, do we have to pass manifest stuff to OpenNoNavigate?

Which cases are we actually trying to handle?  Apparently not form submission?  But yes link clicks and location sets?  Or something else?  There are changes to window.open which don't make much sense to me...

Can you please clearly describe what the goal is here, or link to such a description?  If it's to intercept all navigations as per https://groups.google.com/forum/#!topic/mozilla.dev.webapi/yqLffXgokuI then why don't we just check in LoadInternal if we're in an app and if so go ahead and ask whether it's OK to proceed before continuing with LOAD_INTERNAL?  That shouldn't need app manifest stuff sprinkled all over the codebase...

Comment 17

[BMO Automation]

Firefox OS is not being worked on