Mozilla Marketplace activity should open for marketplace URLs

VERIFIED FIXED in Firefox 17

Status

()

enhancement
P1
normal
VERIFIED FIXED
7 years ago
3 years ago

People

(Reporter: Harald, Assigned: wesj)

Tracking

unspecified
Firefox 17
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:+, firefox16 affected, firefox17 verified)

Details

(Whiteboard: [blocking-webrtandroid1+])

Attachments

(2 attachments, 2 obsolete attachments)

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.
Blocks: 738546
Whiteboard: [WebRT:Phase1]
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
Whiteboard: [WebRT:Phase1] → [Phase1]
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.
Whiteboard: [Phase1]
No longer blocks: 738546
Whiteboard: [Phase1]
Whiteboard: [Phase1] → [Phase1] [marketplace-beta?]
Whiteboard: [Phase1] [marketplace-beta?] → [Phase1]
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.
blocking-kilimanjaro: --- → ?
Ragavan, we need someone to make a decision on what we are going to do here.
Severity: normal → enhancement
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.
blocking-kilimanjaro: ? → +
Depends on: 738545
Depends on: k9o-webrt
(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.
Keywords: productwanted
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: productwanted
Resolution: --- → WONTFIX
Whiteboard: [Phase1]
Reopening for re-evaluation.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee: nobody → ragavan.bugs
Keywords: productwanted
Priority: -- → P1
Whiteboard: [blocking-webrtandroid1+]
Ragavan would like to target support for this in Q4.
(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.
No longer blocks: Blocking-FFA-WebRT1+
Priority: P1 → --
Whiteboard: [blocking-webrtandroid1+] → [blocking-webrtandroid1?]
Whiteboard: [blocking-webrtandroid1?] → [blocking-webrtandroid1+]
Assignee: ragavan.bugs → nobody
Keywords: productwanted
No longer depends on: 738545
Priority: -- → P1
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.
Posted patch Patch (obsolete) — Splinter Review
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....
Assignee: nobody → wjohnston
Attachment #648879 - Flags: feedback?(mfinkle)
Attachment #648879 - Flags: feedback?(vladimir)
Duplicate of this bug: 778913
Posted patch Patch v1 (obsolete) — Splinter Review
This seems to work well for me with the patches in bug 781061 and 781060. Time to start review I think.
Attachment #648879 - Attachment is obsolete: true
Attachment #648879 - Flags: feedback?(vladimir)
Attachment #648879 - Flags: feedback?(mark.finkle)
Attachment #651025 - Flags: review?(mark.finkle)
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
Attachment #651025 - Flags: review?(mark.finkle) → review-
Posted patch Patch v2Splinter Review
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.
Attachment #651025 - Attachment is obsolete: true
Attachment #652236 - Flags: review?(mark.finkle)
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
Attachment #652236 - Flags: review?(mark.finkle) → review+
> 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.
backout for test failures that aren't really failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f27460e427cc
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
Posted patch PatchSplinter Review
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?
Attachment #654054 - Flags: review?(mark.finkle)
Comment on attachment 654054 [details] [diff] [review]
Patch

This approach is fine for now
Attachment #654054 - Flags: review?(mark.finkle) → review+
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"
Attachment #652236 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/2e06b3299a5e
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Status: RESOLVED → VERIFIED
Flags: in-moztrap?(aaron.train)
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?
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?
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?
Depends on: 785496
Moving the l10n conversation for approval to email.
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.
Attachment #652236 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Flags: in-moztrap?(aaron.train)
You need to log in before you can comment on or make changes to this bug.