819037 - navigator.mozApps.getSelf does not include .result

Status: RESOLVED FIXED

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

Description

[:Harald Kirschner :digitarald]

getSelf success event should include a .result object when called from the app. Works on FxOS, doesn't work on Android.

Comment 1

[[:fabrice] Fabrice Desré]

What's happening is that at https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#1619 we check if we are really running the app, and not just browsing to the same origin. If not, we bail out.

To fix that on android, you need to ensure that you run the app with the correct appId. On fxOS this is done by adding the mozapps=$MANIFEST_URL on the iframe that is used to run the app.

Comment 2

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

(In reply to Fabrice Desré [:fabrice] from comment #1)

> To fix that on android, you need to ensure that you run the app with the
> correct appId. On fxOS this is done by adding the mozapps=$MANIFEST_URL on
> the iframe that is used to run the app.

I assume this would work just as well on a <browser> ?

Comment 3

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

This works! Testing with:
http://dl.dropbox.com/u/72157/Apps/testApp.html

(the test in browser.js apparently uses the wrong docshell and fails which confused me for a bit. Need to retest with the simpler docshell.setAppId() stuff again).... Need more work to store or get the manifest somehow at startup, but we're getting there.

Also, setting dom.mozBrowserFramesEnabled = true in prefs results in us trying to use the B2G Prompt service and falling on our face whenever Services.prompt is called. Lesson learned.

Comment 4

[Jason Smith [:jsmith]]

How exactly are we able to install an app off of dropbox? That seems....strange?

I thought that wasn't allowed?

Comment 5

[Wesley Johnston (:wesj)]

Heh. AFAIK, as long as the manifest and launch url are on the same domain I'm fine.

On the topic of the bug, my plan is to alter webapp shortcuts so that they pass us the manifest url rather than the launch path. We can then use that to see if an app is installed with the manifest, get its manifest, and launch with the launch path there. If the app isn't installed we can show some sort of error? Or try to/offer to "install" the app.

I feel like thats much safer than our current setup which makes it pretty simple to launch any url with webapps "permissions" (right now that's basically nothing). And it makes things like this a bit easier to implement at the expense of a startup hit...

This could also break our current shortcuts. I can add a fallback that will launch with a url if we're passed it instead of a manifest, or we can just deprecate things. Any opinions there?

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This uses a manifest url on the app intent rather than the launchPath. That means we have to check the database at startup which kinda sucks, but is likely more secure. I'd like eventually also get rid of our origin -> uniqueURL stuff so that its easy to have multiple apps per origin (manifestURL becomes the unique identifier), but that's a bit of unrelated work.

For users of old apps, things should continue to work, but I remove whatever shortcut they already had and create a new "correct" one. It will likely not be in the exact same place, and it will be created even if they have deleted it and are launching the app through about:apps. I expect these people will be few and far between.

If we can't find a webapp with this url as its launch path or as its manifest url, I send the url back to Android and shutdown the app.

Also has some cleanup of uniqueURI -> origin to make the code a little clearer.

Comment 8

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

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

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

>+Cu.import("resource://gre/modules/commonjs/promise/core.js");

might be heavyweight for this, but let's try it

>+  findManifestUrlFor: function(aUrl) {
>+    var deferred = Promise.defer();

var -> let

>+    request.onsuccess = function() {
>+      var apps = request.result;

var -> let

>+        // First see if this url matches any manifests we have registered
>+        // If so, get the launchUrl from the manifest and we'll launch with that
>+        //let app = DOMApplicationRegistry.getAppByManifestURL(aUrl);

is this commented line needed? if not, let's remove

>+        // Otherwise, see if the apps launch path is this url
>+        if (manifest.fullLaunchPath() == aUrl) {
>+          // remove the old shortuct
>+          sendMessageToJava({
>+            gecko: {
>+              type: "Shortcut:Remove",
>+              title: manifest.name,
>+              url: manifest.fullLaunchPath(),
>+              origin: app.origin,
>+              shortcutType: "webapp"
>+            }
>+          });
>+          WebappsUI.createShortcut(manifest.name, manifest.fullLaunchPath(),
>+                                   WebappsUI.getBiggestIcon(manifest.icons, app.origin), "webapp");

let's remove this code. we can follow the same path next time and not need to remove shortcuts. we can remove this path altogether at some point in the future.

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

>     if (pinned) {
>-      WebAppRT.init(updated, url);
>+      WebAppRT.init(updated, url).then(function(aUrl) {
>+        BrowserApp.addTab(aUrl);
>+      }, function() {
>+        console.log("Requested webapp " + url + " is not installed");

remove the log

>+        let uri = Services.io.newURI(url, null, null);

should we check the uri for null?

>+        Cc["@mozilla.org/uriloader/external-protocol-service;1"].
>+          getService(Ci.nsIExternalProtocolService).
>+          getProtocolHandlerInfo(uri.scheme).
>+          launchWithURI(uri);

all on one line


>       case "WebApps:InstallMarketplace":
>-        this.installAndLaunchMarketplace(data.url);
>+        console.log("Install Marketplace is deprecated");
>         break;

let's just remove this case altogether?

>   MARKETPLACE: {
>       MANIFEST: "https://marketplace.mozilla.org/manifest.webapp",

can the MARKETPLACE code be removed too?

Comment 9

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e7b32ff9fde

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e7b32ff9fde