813736 - Enable and use the installPackage API on Android

Status: VERIFIED FIXED

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

Description

[Wesley Johnston (:wesj)]

installPackage is currently turned off in Webapp.js for Android:

http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#236

We should turn it on, and use it. Using it will require looking at the manifest returned to us when we're asked to install (I think here):

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6675

for the isPackage property (http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.js#236). If its true, use the package API.

Comment 2

[James Hugman [:jhugman] [@jhugman]]

Attachment: Review request for first patch to enable installPackage on Android.

Installation of packaged app, using the shortcut on the homescreen.
Tapping on this shortcut launches the app correctly.
Missing: detection/generation of correct icon.
Missing: Apps show up in the apps page only intermittently.

Comment 3

[Jason Smith [:jsmith]]

Comment on attachment 715543 [details] [diff] [review]
Review request for first patch to enable installPackage on Android.

This is making some changes to DOM apps - this needs fabrice's review as well.

Also - can we get a try build with this patch? Someone from QA can take a quick look to see if this hooks up okay.

Comment 4

[Jason Smith [:jsmith]]

We also should really get some unit tests with this patch if it's possible.

Comment 5

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

Comment on attachment 715543 [details] [diff] [review]
Review request for first patch to enable installPackage on Android.

Flipping with Wes. I will be distracted for the next week and I don't want to be the blocker for making progress on this.

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 715543 [details] [diff] [review]
Review request for first patch to enable installPackage on Android.

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

My review only applies to Webapps.js and Webapps.jsm. I'd like to see fixed versions. Also, remove all the extra debug() you added in this file.

::: dom/apps/src/Webapps.js
@@ +264,5 @@
>  
>    QueryInterface: XPCOMUtils.generateQI([Ci.mozIDOMApplicationRegistry,
>  #ifdef MOZ_B2G
>                                           Ci.mozIDOMApplicationRegistry2,
> +#elifdef ANDROID

This should be MOZ_WIDGET_ANDROID. ANDROID is actually true for both b2g and fennec on android, but I'd rather user two ifdefs anyway.

@@ +274,5 @@
>                                      contractID: "@mozilla.org/webapps;1",
>                                      interfaces: [Ci.mozIDOMApplicationRegistry,
>  #ifdef MOZ_B2G
>                                                   Ci.mozIDOMApplicationRegistry2,
> +#elifdef ANDROID

ditto

::: dom/apps/src/Webapps.jsm
@@ +22,5 @@
>  Cu.import("resource://gre/modules/AppDownloadManager.jsm");
>  
>  function debug(aMsg) {
>    //dump("-*-*- Webapps.jsm : " + aMsg + "\n");
> +  Services.console.logStringMessage(aMsg);

Please remote that.

@@ +1743,5 @@
>      }
>      aData.mm.sendAsyncMessage("Webapps:Install:Return:KO", aData);
>    },
>  
> +  confirmInstall: function(aData, aFromSync, aProfileDir, aOfflineCacheObserver, zipDownloadSuccessCallback) {

nit: aZipDownloadSuccessCallback
Also, try to not cross 80 columns.

@@ +1904,5 @@
>                                                      app: app,
>                                                      manifest: aManifest });
> +          if (zipDownloadSuccessCallback) {
> +            zipDownloadSuccessCallback();
> +          }                                                    

Nit: trailing whitespace.

@@ +2354,5 @@
> +      }
> +    } else {
> +      // deviceStorage isn't available, so use FileUtils to check find the size of available storage.
> +      let dir = FileUtils.getDir(DIRECTORY_NAME, ["webapps"], true, true);
> +      checkDownloadSize(dir.diskSpaceAvailable);

diskSpaceAvailable is not implemented on every platforms, and will throw when failing. See for instance https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/downloads.js#449

@@ +2355,5 @@
> +    } else {
> +      // deviceStorage isn't available, so use FileUtils to check find the size of available storage.
> +      let dir = FileUtils.getDir(DIRECTORY_NAME, ["webapps"], true, true);
> +      checkDownloadSize(dir.diskSpaceAvailable);
> +    }             

Nit: trailing whitespace.

Comment 7

[Mozilla RelEng Bot]

Try run for 93c6a663344f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=93c6a663344f
Results (out of 38 total builds):
    success: 36
    warnings: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-93c6a663344f

Comment 8

[Jason Smith [:jsmith]]

My notes from testing out your build are below.

Device: Galaxy Nexus
OS: Android 4.1

Sanity test for installing and launching a web packaged app (10 KB)

	* [Good] Install prompt showed up, allowed me to install the packaged app
	* [Improvement Needed] No install confirmation UI seen
	* [Good] Icon on Android homescreen
	* [Good] App launched to the correct launch_path
	* [Improvement Needed] Flash of the splashscreen can sometimes be seen (not even sure if the splashscreen is needed for packaged apps?)
	* [Good] Icon on about:apps page
	* [Improvement Needed] Permission prompt shows the packaged app origin (app://) which should *never* be exposed to a user (it's not end-user understandable) - should be using app name in this case
	* [Good] Add to homescreen from about:apps page did add the shortcut
	* [Improvement Needed] No notification when adding the icon to the homescreen
	* [Good] App successfully uninstalled when no app icon was on the homescreen

Sanity test for installing a privileged app off of a non-trusted site

	* [Improvement Needed] Download fails as expected, but we leave behind the broken image app icon on about:apps. We did make sure to not add the app icon to the Android homescreen, however.

Sanity test for installing a large web packaged app (> 1 MB)

	* [Good] App successfully installed with the correct icons on screen
	* [Improvement Needed] The lack of the download UI definitely made this confusing to not understand the progress of the app download
	* [Improvement Needed] Uninstalling the packaged app from about:apps isn't removing it from the Android homescreen

Comment 9

[James Hugman [:jhugman] [@jhugman]]

Attachment: Cleanup following feedback from fabrice.

Also icons now showing on homescreen and app page.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 717174 [details] [diff] [review]
Cleanup following feedback from fabrice.

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

This looks really good to me. Its also big enough that I'd be fine getting this in (if fabrice is ok with the Webapps.jsm stuff), and then doing follow ups in separate bugs.

::: mobile/android/base/WebAppAllocator.java
@@ +80,5 @@
>          return -1;
>      }
>  
> +	public synchronized void allocateAppWithIcon(final String app, final int index,
> +			final Bitmap aIcon) {

Lets name things something more like "updateApp". Also, make sure eclipse is using spaces-for-tabs and has the right number here... It looks kinda large.

@@ +89,5 @@
> +
> +					try {
> +						color = BitmapUtils.getDominantColor(aIcon);
> +					} catch (Exception e) {
> +						e.printStackTrace();

We usually log this using Log.e(LOGTAG, "Something", e); so that they're easy to grep for.

@@ +92,5 @@
> +					} catch (Exception e) {
> +						e.printStackTrace();
> +					}
> +					mPrefs.edit()
> +							.putString(appKey(index), app)

Keep the first method on the same line. i.e.

mPrefs.edit().putString(...)
             .putInt(...)

::: mobile/android/chrome/content/browser.js
@@ +7179,5 @@
> +        let self = this;
> +        DOMApplicationRegistry.confirmInstall(aData, false, file, null, 
> +          function (manifest) {
> +            // the manifest argument is the manifest from within the zip file,
> +            // TODO so now would be a good time to ask about permissions. 

Since we actually prompt at runtime for permissions, I don't think we need to do it at all here.

@@ +7182,5 @@
> +            // the manifest argument is the manifest from within the zip file,
> +            // TODO so now would be a good time to ask about permissions. 
> +            self.makeBase64Icon(self.getBiggestIcon(manifest.icons, Services.io.newURI(aData.app.origin, null, null)),
> +              function(scaledIcon, fullsizeIcon) {
> +                // if java returned a profile path to us, try to use it to pre-populate the app cache

We've already done this in confirmInstall, so no need to do it again.

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 715543 [details] [diff] [review]
Review request for first patch to enable installPackage on Android.

Clearing these since the patch is obsolete.

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 717174 [details] [diff] [review]
Cleanup following feedback from fabrice.

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

I should I caught that in the first review, but you also need to update FreeSpaceWatcher.jsm for your situation where device storage is not available.

::: dom/apps/src/Webapps.jsm
@@ +1740,5 @@
>      aData.mm.sendAsyncMessage("Webapps:Install:Return:KO", aData);
>    },
>  
> +  confirmInstall: function(aData, aFromSync, aProfileDir,
> +                      aOfflineCacheObserver, aZipDownloadSuccessCallback) {

nit: align parameters aData and aOfflineCacheObserver

@@ +2345,5 @@
> +        let freeBytes = e.target.result;
> +        checkDownloadSize(freeBytes);
> +      }
> +    } else {
> +      // deviceStorage isn't available, so use FileUtils to check find the size of available storage.

Nit: I'm not sure that "to check find the size" is proper english.

@@ +2351,5 @@
> +      try {
> +        checkDownloadSize(dir.diskSpaceAvailable);
> +      } catch(ex) {
> +        // if we disk space information isn't available, we'll end up here.
> +        // We should either proceed anyway, otherwise devices that support neither 

nit: trailing whitespace, and start sentences with a capital If.

::: mobile/android/base/GeckoApp.java
@@ +908,5 @@
> +            	String name = message.getString("name");
> +            	String manifestURL = message.getString("manifestURL");
> +            	String iconURL = message.getString("iconURL");
> +            	String origin = message.getString("origin");
> +            	

Nit: trailing whitespace

@@ +924,5 @@
> +            	String name = message.getString("name");
> +            	String manifestURL = message.getString("manifestURL");
> +            	String iconURL = message.getString("iconURL");
> +            	String origin = message.getString("origin");
> +            	

ditto

::: mobile/android/base/GeckoAppShell.java
@@ +853,3 @@
>          return profile.getDir();
>      }
> +    

nit: trailing whitespace

@@ +929,5 @@
>                  intent.putExtra(Intent.EXTRA_SHORTCUT_ICON, getLauncherIcon(aIcon, aType));
>  
>                  // Do not allow duplicate items
>                  intent.putExtra("duplicate", false);
> +                

ditto

::: mobile/android/base/WebAppAllocator.java
@@ +104,5 @@
> +					.putInt(iconKey(index), 0)
> +					.commit();
> +		}
> +	}
> +    

nit: trailing whitespace

::: mobile/android/chrome/content/browser.js
@@ +7174,5 @@
> +        // write them into the app profile
> +        let defaultPrefsFile = file.clone();
> +        defaultPrefsFile.append(this.DEFAULT_PREFS_FILENAME);
> +        this.writeDefaultPrefs(defaultPrefsFile, prefs);
> +        

nit: trailing whitespace

@@ +7176,5 @@
> +        defaultPrefsFile.append(this.DEFAULT_PREFS_FILENAME);
> +        this.writeDefaultPrefs(defaultPrefsFile, prefs);
> +        
> +        let self = this;
> +        DOMApplicationRegistry.confirmInstall(aData, false, file, null, 

ditto

@@ +7179,5 @@
> +        let self = this;
> +        DOMApplicationRegistry.confirmInstall(aData, false, file, null, 
> +          function (manifest) {
> +            // the manifest argument is the manifest from within the zip file,
> +            // TODO so now would be a good time to ask about permissions. 

ditto

Comment 13

[James Hugman [:jhugman] [@jhugman]]

Attachment: installPackage + FreeSpaceWatcher

Review request for patch following feedback from fabrice. Includes FreeSpaceWatcher.

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 725449 [details] [diff] [review]
installPackage + FreeSpaceWatcher

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

::: dom/apps/src/FreeSpaceWatcher.jsm
@@ +67,5 @@
> +          } else {
> +            // deviceStorage isn't available, so use the profile's
> +            // nsIFile to find the size of available storage.
> +            let dir = Services.dirsvc.get("ProfD", Ci.nsIFile);
> +            checkFreeSpace(dir.diskSpaceAvailable);

This part should be similar to the one in Webapps.jsm, where you have:

let dir = FileUtils.getDir(DIRECTORY_NAME, ["webapps"], true, true);
try {
  checkFreeSpace(dir.diskSpaceAvailable);
} catch(ex) {
 // diskSpaceAvailable failed...
}

Or let me know why it needs to be different.

::: dom/apps/src/Webapps.js
@@ +20,5 @@
>  
>  function convertAppsArray(aApps, aWindow) {
>    let apps = Cu.createArrayIn(aWindow);
>    for (let i = 0; i < aApps.length; i++) {
> +    let app = aApps[i]

revert this change

Comment 15

[James Hugman [:jhugman] [@jhugman]]

Attachment: Changed the method of getting a directory in FreeSpaceWatcher

Changed to using FileUtils as per request.

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 726863 [details] [diff] [review]
Changed the method of getting a directory in FreeSpaceWatcher

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

r=me with nit fixed.

::: dom/apps/src/FreeSpaceWatcher.jsm
@@ +69,5 @@
> +            // deviceStorage isn't available, so use the webappsDir instead.
> +            // This needs to be moved from a hardcoded string to DIRECTORY_NAME
> +            // in AppsUtils. See bug 852685.
> +            let dir = FileUtils.getDir("webappsDir", ["webapps"], true, true);
> +            checkFreeSpace(dir.diskSpaceAvailable);

you still need to catch exceptions here, as in Webapps.jsm and probably set the status to "free" in case of failure (the download will fail later when we actually run out of space).

Comment 17

[James Hugman [:jhugman] [@jhugman]]

Attachment: Amended minor issues following r+ from fabrice.

This patch is a rollup patch for checkin. It contains the minor corrections detailed by fabrice in his r+.

Comment 18

[Wesley Johnston (:wesj)]

In! Nice! I removed the whitespace changes James. I also added a small fix for our new ThreadingUtils in WebAppAllocator.

https://hg.mozilla.org/integration/mozilla-inbound/rev/eadf6b0ea8de

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eadf6b0ea8de

Comment 20

[Alex Keybl [:akeybl]]

We received feedback that this functionality isn't currently/easily testable on the Marketplace or elsewhere. Will relnote when that changes.

Comment 21

[Daniel Veditz [:dveditz]]

This bug was landed during the Firefox 22 cycle. Is the feature ready to be released in 5 weeks or do we need to worry about turning it off/backing it out? This is the bug number we got during a security review, are there additional bugs/code associated with this feature?

Comment 22

[Jason Smith [:jsmith]]

(In reply to Daniel Veditz [:dveditz] from comment #21)
> This bug was landed during the Firefox 22 cycle. Is the feature ready to be
> released in 5 weeks or do we need to worry about turning it off/backing it
> out? This is the bug number we got during a security review, are there
> additional bugs/code associated with this feature?

Nope, it's not ready for release. We currently do not publicly expose UI here on Beta or higher, so like other apps features on Android, I think the plan would likely be to leave this preffed on but not publicly advertise the feature. We could pref it off if someone feels strongly for some particular reason, however.

Additional bugs I'd look at are basically any bug highlighted on this wiki page - https://wiki.mozilla.org/Mobile/Triage/WebRT.