741621 - Mozilla Marketplace activity should open for marketplace URLs

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), enhancement, P1)

Description

[:Harald Kirschner :digitarald]

Clicking an app link in any phone app (like marketplace.mozilla.org/*) should prompt the user to open in the "Mozilla Marketplace". (using action.VIEW, category.DEFAULT and category.BROWSABLE).

Needs specs for Marketplace URLs that need to be included.

Comment 1

[Jason Smith [:jsmith]]

If I understand this feature correctly, an example of this feature in real use would be something like:

1. I start my facebook native app
2. Upon startup, I click the "Get the app on mozilla marketplace!"
3. Fennec Native starts up, takes me to the mozilla marketplace facebook app install page

Comment 2

[:Harald Kirschner :digitarald]

Also any links to Marketplace app detail pages in Twitter/Facebook/LinkedIn/whatever (native app or web site), emails, SMS, etc. … All links that hook into the Android Intent system.

Comment 3

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

For what I gather, we want Firefox Mobile to look at normal external URLs and see if they are destined for the marketplace (but only the Mozilla marketplace) and if so, launch the URL like it's a webapp.

Since the URL is the marketplace, the app _is_ a webapp, but it was never launched as a webapp (not using the WEBAPP intent).

This only works if the user taps the link and picks "Firefox" from the list of browsers, if they have more than one browser installed.

Comment 4

[Jason Smith [:jsmith]]

Nominating for k9o, this falls under this user story - https://wiki.mozilla.org/Kilimanjaro/ProductDraft#Mozilla_will_deliver_a_compelling_device.2Fsystem_un-boxing_experience

Comment 5

[Sheila Mooney]

Ragavan, we need someone to make a decision on what we are going to do here.

Comment 6

[Jennifer Arguello :ticachica]

Dependent on the Marketplace being it's own application. But Firefox could still maybe receive this call and open up the right page within fennec. 

At minimum, the Android manifest can be modified so that if a URL with marketplace will open up in the right page in the Marketplace, no matter what browser it came from.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Sheila Mooney from comment #5)
> Ragavan, we need someone to make a decision on what we are going to do here.

Flagging productwanted - Need Ragavan to make a decision on what to do with marketplace on Android.

Comment 8

[Jason Smith [:jsmith]]

Per the dependent bug, I'm won't fixing this bug on the same basis. Please reopen if there's a firm decision on what to do here.

Comment 9

[Jason Smith [:jsmith]]

Reopening for re-evaluation.

Comment 10

[Erin Lancaster [:elan]]

Ragavan would like to target support for this in Q4.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Erin Lancaster [:elancaster] from comment #10)
> Ragavan would like to target support for this in Q4.

Should we still block on this for the 1st release then? Are we able to ship without this or bug 738545 complete?

Putting this into the re-triage queue.

Comment 12

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

Summary of this bug:
* Add entries to AndroidManifest.xml so an intent is fired when a "marketplace.mozilla.org" URL is executed by any application
* Add code to startup the Marketplace webapp (pre-installed in bug 778913) with the URL

If Fennec has not been able to pre-install the Marketplace app by the time this happens, we should try to install it on demand.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch

This works pretty well. It creates a real Marketplace activity in Android. When other apps have links pointing to marketplace url's, users can opt to open them in the marketplace app. The Activity checks to see if the marketplace webapp is installed. If it isn't, it installs it and launches it (right now that CAN show a telemetry prompt if its first run...). If the web is installed, it just launches it.

For links within Fennec this handles them using our old friend HelperApps. We've turned that off for most things, but for just marketplace urls I turned it back on. When you visit a market url, you'll see an "Open in the Marketplace" doorhanger appear with a "Remember for this site" checkbox as well.

There are some weird issues here with the Marketplace activity. Some are problems with the Webapps Allocator that I tried to hack around here. Wondering if vlad can give me any pointers on the right way to do that?

I've also seen strange cases where Fennec seems to get in a very bad state and crash. I think maybe because I started the Marketplace activity first, didn't initialize something correctly, and left Fennec in a strange state..... Still looking at that stuff....

Comment 15

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This seems to work well for me with the patches in bug 781061 and 781060. Time to start review I think.

Comment 16

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

Comment on attachment 651025 [details] [diff] [review]
Patch v1

># HG changeset patch
># Parent 7a689b7833cd4366fc815725747cbfbaa9c175e0
>
>diff --git a/mobile/android/base/AndroidManifest.xml.in b/mobile/android/base/AndroidManifest.xml.in
>--- a/mobile/android/base/AndroidManifest.xml.in
>+++ b/mobile/android/base/AndroidManifest.xml.in
>@@ -52,32 +52,53 @@
>                  android:name="org.mozilla.gecko.GeckoApplication"
> 		 android:hardwareAccelerated="true"
> #if MOZILLA_OFFICIAL
> 		 android:debuggable="false">
> #else
> 		 android:debuggable="true">
> #endif
> 
>+        <activity android:name=".Marketplace"

.MarketplaceApp

And move this after the "App" activity. We might need App to be the first listed activity for the testing framework to function correctly.

>+                  android:label="Mozilla Marketplace"
>+                  android:configChanges="keyboard|keyboardHidden|mcc|mnc|orientation|screenSize"
>+                  android:windowSoftInputMode="stateUnspecified|adjustResize"
>+                  android:icon="@drawable/marketplace"
>+                  android:launchMode="singleTask"
>+                  android:theme="@style/Gecko.NoActionBar"
>+                  android:noHistory="true">

Someday we might want to find a way to share these settings for App, WebApp and MarkeyplaceApp to keep them in sync

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java

>+    public static final String MARKETPLACE_HOST     = "marketplace.mozilla.org";
>+    public static final String MARKETPLACE_URI      = "https://marketplace.mozilla.org";

Move to MarketplaceApp.java

>diff --git a/mobile/android/base/Marketplace.java.in b/mobile/android/base/Marketplace.java.in

Rename to MarketplaceApp.java

>+    private void handleMarketplaceLink(final String url) {

>+        // otherwise just launch the webapp with this url
>+        } else {
>+            Intent webappIntent = GeckoAppShell.getWebAppIntent(index, url);

Move the comment into the else block

>diff --git a/mobile/android/base/WebAppAllocator.java b/mobile/android/base/WebAppAllocator.java

>     public static synchronized WebAppAllocator getInstance(Context cx) {

>-        if (cx != sContext)
>-            throw new RuntimeException("Tried to get WebAppAllocator instance for different context than it was created for");
>+        if (cx != sContext) {
>+            sInstance = null;
>+            sContext = (GeckoApp) cx;
>+            sInstance = new WebAppAllocator(cx);
>+        }

What are these changes doing?

>diff --git a/mobile/android/chrome/content/HelperApps.js b/mobile/android/chrome/content/HelperApps.js

>+  showDoorhanger: function showDoorhanger(aUri, aCallback) {
>+    try {
>+    let permValue = Services.perms.testPermission(aUri, "helperapps");

I am not in love with using "helperapps" for the permission key :)
What do you think of "native-intent"? Does that kinda make sense, since we are delegating to the registered native android intents?

>+    if (permValue != Services.perms.UNKNOWN_ACTION) {
>+      if (permValue == Services.perms.ALLOW_ACTION) {
>+        if (aCallback) aCallback(aUri);

I like putting aCallback(aUri) on a separate line

>+        if (aCallback) aCallback(aUri);
>+        else this.openUriInApp(aUri);

Same

>+    let options = { checkbox: Strings.browser.GetStringFromName("helperapps.dontAskAgain") };
>+    NativeWindow.doorhanger.show(message, "helperapps-open", buttons, BrowserApp.selectedTab.id, options);
>+    } catch(ex) {

try/catch indenting is off

>+      console.log("state change 5 " + ex);

Remove this? Or is it valuable?

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+    // This is where we might check for helper apps.
>+    // For now it is special cased to only check for the marketplace urls
>+    if (WebappsUI.isMarketplace(aLocationURI)) {
>+      // the marketplace app may not actually be installed, so instead we use a custom
>+      // callback that will install and launch it for us if necessary
>+      HelperApps.showDoorhanger(aLocationURI, function() {
>+        WebappsUI.installAndLaunchMarketplace(aLocationURI.spec);
>+        if (aRequest)
>+          aRequest.cancel(Cr.NS_OK);
>+      });
>+    }

I am tempted to move this code into a single function in WebappsUI, but this is OK for now.

>+  installAndLaunchMarketplace: function installAndLaunchMarketplace(aLaunchUrl) {

>+      let launchFun = (function() {
>+          if (aLaunchUrl)
>+            WebappsUI.openURL(aLaunchUrl || WebappsUI.MARKETPLACE.URI.prePath, WebappsUI.MARKETPLACE.URI.prePath);

indented too much

Comment 17

[Wesley Johnston (:wesj)]

Attachment: Patch v2

OK. Cleaned up the nits. Added a comment for the WebAppAllocator bits. Basically, we check in there if the context that we're given equals the context that created the singleton.

Since MarketplaceApp (which is different than the marketplace webapp) is in the SAME process as BrowserApp, this isn't true and it fails. This fixes that by just getting a new allocator instance.

Comment 18

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

Comment on attachment 652236 [details] [diff] [review]
Patch v2


>diff --git a/mobile/android/base/WebAppAllocator.java b/mobile/android/base/WebAppAllocator.java

>-        if (cx != sContext)
>-            throw new RuntimeException("Tried to get WebAppAllocator instance for different context than it was created for");
>+        // The marketplaceApp will run in this same process, but has a different context
>+        // Rather than just failing, we want to create a new Allocator instead
>+        if (cx != sContext) {
>+            sInstance = null;
>+            sContext = (GeckoApp) cx;
>+            sInstance = new WebAppAllocator(cx);
>+        }

Marketplace App is run in the same process as Firefox? Is that only for situations where we launch it from a URL intent?


>diff --git a/mobile/android/chrome/content/WebAppRT.js b/mobile/android/chrome/content/WebAppRT.js

>+      // prevent offering to use helper apps for things that this app handles
>+      // i.e. don't show the "Open in market?" popup when we're showing the market app
>+      let uri = Services.io.newURI(url, null, null);
>+      Services.perms.add(uri, "helperapps", Ci.nsIPermissionManager.DENY_ACTION);

You missed this one: helperapps -> native-intent

Comment 19

[Wesley Johnston (:wesj)]

> Marketplace App is run in the same process as Firefox? Is that only for
> situations where we launch it from a URL intent?
URL intent seems kinda vague to me. MarketplaceApp.java will run in the same process as Fennec. All it does is install and launch the webapp.

WebApp<0-99>.java which is pointing to marketplace.mozilla.org will run in its own process.

Comment 20

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c5961842578e

Comment 21

[Wesley Johnston (:wesj)]

backout for test failures that aren't really failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f27460e427cc

Comment 22

[Wesley Johnston (:wesj)]

I think that this is just throwing for about url's which have no host. The exceptions show up in the wrong spot and make it look like tests failed. Pushed a fix to try:

https://tbpl.mozilla.org/?tree=Try&rev=4910e94d1963

Comment 23

[Wesley Johnston (:wesj)]

Attachment: Patch

Try run all green. We're throwing for uri.host on about urls. Fixes the errors by try-catching them. Alternatively, I guess I could check the scheme. What would you rather mark?

Comment 24

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

Comment on attachment 654054 [details] [diff] [review]
Patch

This approach is fine for now

Comment 25

[Wesley Johnston (:wesj)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/2e06b3299a5e

Comment 26

[Wesley Johnston (:wesj)]

Comment on attachment 652236 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New stuff
User impact if declined: Market app doesn't install
Testing completed (on m-c, etc.): Landed on mc today
Risk to taking this patch (and alternatives if risky): Medium risky. Few changes to real fennec unless you're going to the market which is unlikely for average joe users.
String or UUID changes made by this patch: Some new strings! "Open this in the marketplace app?" "Open" "Ignore"

Comment 27

[Ed Morley [:emorley]]

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

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

We're past code-freeze on mozilla-central and this patch has string changes which we don't take for Aurora.  Is it possible to do a version of this without strings for Aurora and get the strings in 6 weeks?

Comment 29

[Wesley Johnston (:wesj)]

Not really. We might be able to remove the helper apps bits, or perhaps try to get by using other strings for it (we have several don't ask again for this site strings around and can use the prompt service's "Yes" and "No" labels perhaps?) But we have to name the market something. Maybe Mozilla Marketplace can get by without being localized?

Comment 30

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

Lukas, this is one the few "blockers" we have for WebApps, which we really want to get in for Fx16. We can look at doing something with the strings, like using strings from other places, but we don't want to do that if needed.

"Mozilla Marketplace" should probably not be translated, but we should get Product to state one way or the other.

Wes, how hard would it be to remove the helper apps part?

Comment 31

[Alex Keybl [:akeybl]]

Moving the l10n conversation for approval to email.

Comment 32

[Alex Keybl [:akeybl]]

Comment on attachment 652236 [details] [diff] [review]
Patch v2

[Triage Comment]
Per Erin and the marketplace folks, there's no need to perform this uplift since most Marketplace testing will be occurring in FF17 as opposed to FF16.