737050 - Add support for the WEBAPP intent

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Fennec XUL has support for a WEBAPP intent which allows for special handling of WebApps. In this case, we would pick an existing tab running the webapp over creting a new one.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

WebApps are not blocking Fx13

Comment 2

[Richard Newman [:rnewman]]

And presumably (for parity with native apps) showing way less chrome, a custom icon, etc., rite? Or am I misunderstanding the scope?

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #2)
> And presumably (for parity with native apps) showing way less chrome, a
> custom icon, etc., rite? Or am I misunderstanding the scope?

Chromeless is a different bug

Comment 4

[Ragavan S [:rags]]

(In reply to Mark Finkle (:mfinkle) from comment #0)
> Fennec XUL has support for a WEBAPP intent which allows for special handling
> of WebApps. In this case, we would pick an existing tab running the webapp
> over creting a new one.

Would we load webapps in a tab or a window? Based on comment #3, sounds like chromeless is a different bug. What's that bug?

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Ragavan S [:rags] from comment #4)
> Would we load webapps in a tab or a window? Based on comment #3, sounds like
> chromeless is a different bug. What's that bug?

it is yet to be filed as far as I know

Comment 7

[JP Rosevear [:jpr]]

Re-nomming to see if soft blocker.

Comment 8

[Jennifer Arguello :ticachica]

Is this bug a dup of https://bugzilla.mozilla.org/show_bug.cgi?id=738547? Because it is blocking the bug 741626 which is the use case for Phase 2 or Firefox 15. It might be better to make this a blocker of 738547. Thoughts?

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

This patch adds special "open url" behavior for WEBAPP intents (defined by org.mozilla.gecko). It has to touch a few parts of the code. Here is a summary:
* Fennec handles launches via intents using the Gecko commandline handler. This patch adds support for adding a new flag to that system. The flag could be nothing, "-webapp", or "-bookmark". I add static helper methods to make this easier to use.
* If Fennec is not running the commandline is passed to Gecko directly, so the patch adds code to do that.
* Once the flag gets to the Gecko commandline handler, we check for the flag and do one of two things: Pass the flag (pinned) as a window.argument for the initial launch, or Pass a new nsIBrowserDOMWindow.OPEN_SWITCHTAB flag so the BrowserDOMWindow code knows to try to switch to an open tab instead of making a new tab.
* We use the session store to save the "appOrigin" (like the desktop patch and the about:apps code) and use that to find existing tabs.
* We use "pinned" to signify that tab is special and should act like an "app tab" (desktop uses the same terminology so I stole it here)

Tapping Brad for the Java and Event bits

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Gavin for the nsIBrowserDOMWindow bits

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Brian for the BrowserCLH bits

Comment 12

[Brian Nicholson (:bnicholson)]

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

BrowserCLH bits look fine

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

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

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

::: mobile/android/chrome/content/browser.js
@@ +265,1 @@
>          gScreenHeight = window.arguments[3];

presumably this should be window.arguments[4], but as I say later on, let's just put pinned at the end of the existing argument list so it doesn't change the order of existing args

::: mobile/android/components/BrowserCLH.js
@@ +28,5 @@
>    heightInt.data = "height" in aArgs ? aArgs.height : 1;
>  
>    argsArray.AppendElement(urlString, false);
>    argsArray.AppendElement(restoreSessionBool, false);
> +  argsArray.AppendElement(pinnedBool, false);

nit, put the pinned arg at the end of the list so it doesn't reorder the existing arguments

::: widget/android/nsAppShell.cpp
@@ +418,5 @@
>              break;
>  
> +        char *flag = ToNewUTF8String(curEvent->CharactersExtra());
> +        if (!flag)
> +            flag = ToNewUTF8String(NS_LITERAL_STRING(""));

drop this

@@ +426,4 @@
>              "dummyappname",
> +            "-url",
> +            uri,
> +            flag

flag ? flag : ""

@@ +432,4 @@
>          if (NS_SUCCEEDED(rv))
>              cmdline->Run();
>          nsMemory::Free(uri);
> +        nsMemory::Free(flag);

if (flag)
  nsMemory::Free(flag);

Comment 14

[:Gavin Sharp [email: [e-mail]]]

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

We talked about adding OPEN_ALLOW_SWITCHING as an aContext flag instead of a different aWhere value, but that ended up being stymied by the fact that people currently check aContext's exact value (e.g. == Ci.nsIBrowserDOMWindow.OPEN_EXTERNAL), rather than treating it as a bit field, and changing all the implementations of nsIBrowserDOMWindow would be a pain. So let's just go with this, even if it may not be quite as elegant...

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

with brad's requested changes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ce1f809b765

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/2ce1f809b765

Comment 17

[Aaron Train [:aaronmt]]

Verified Fixed, tested via:

Samsung Galaxy Nexus (Android 4.0.4)
Mozilla/5.0 (Android; Mobile; rv:14.0) Gecko/14.0 Firefox/14.0a1