934756 - implement synthetic APK install flow

Status: RESOLVED FIXED

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

Description

[Myk Melez [:myk] [@mykmelez]]

We should implement the synthetic APK install flow, which is something like:

* web page calls mozApps.install()
* webapp registry tells fennec to install the app
* fennec downloads apk
* fennec advises/guides user to check "unknown sources" as needed
* fennec triggers native installer
* user installs app
* fennec tells registry to register app
* registry registers app and notifies web page that app was registered

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: apks-install-run-wip.diff

Here's a work-in-progress patch implementing the install flows for synthetic APKs on Android.  It also implements the launch flow (in combination with code in the synthetic APK template).


Mark: I'd like your feedback on the overall strategy and structure, especially for the Java code and the WebappManager module.

Fabrice: I'd like to hear your thoughts on the way we integrate the platform-specific functionality with the cross-platform registry.


Note that WebappManager.jsm serves two purposes: to isolate platform-specific code from the cross-platform code in Webapps.jsm (as a precursor to the proper glue we'll implement in bug 910473), and to isolate webapp-specific code from the miscellaneous code in browser.js.


Here's a basic outline of the install flow:

Part One, from Web Page to Native Android Installer:
* Web Page calls mozApps.install()
* DOMApplicationRegistry notifies webapps-download-apk
* WebappManager._downloadApk downloads APK, sends WebApps:InstallApk to Java
* GeckoApp.handleMessage calls GeckoAppShell.installApk()
* GeckoAppShell.installApk starts ACTION_VIEW intent
* Native Android Installer prompts user to install APK

Part Two, from Native Android Installer back to Web Page:
* Native Android Installer installs APK, starts ACTION_PACKAGE_ADDED intent
* InstallListener.onReceive calls InstallHelper.startInstall(), which calls InstallHelper.install()
* InstallHelper.install sends Webapps:AutoInstall to Gecko
* WebappManager._autoInstall calls DOMApplicationRegistry.doInstall[Package](), which calls confirmInstall(), which broadcasts Webapps:Install:Return:OK
* WebappsRegistry.receiveMessage calls DOMRequest.fireSuccess()
* Web Page receives "success"

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Attachment: apks-install-run-wip-2.diff

This patch is identical to the previous one except that it also includes the implementation of the uninstall flow.  So it contains a complete install/launch/uninstall implementation.

I'm still looking for the same feedback described in comment 1.

Comment 3

[[:fabrice] Fabrice Desré]

Comment on attachment 8342003 [details] [diff] [review]
apks-install-run-wip.diff

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

Myk, I just looked at the *.jsm. I have on overall question:
We download the apk and after that, we download the package right? If so, we do security checks after we got the package, which is strange. But I may be totally wrong there ;)

::: dom/apps/src/AppsUtils.jsm
@@ +70,5 @@
>        csp: aApp.csp,
>        installOrigin: aApp.installOrigin,
>        origin: aApp.origin,
> +#ifdef MOZ_ANDROID_SYNTHAPKS
> +      packageName: aApp.packageName,

nit: maybe apkName to not be confused with the "standard" packages?

::: dom/apps/src/Webapps.jsm
@@ +2339,5 @@
>        if (aInstallSuccessCallback) {
>          aInstallSuccessCallback(app.manifest);
>        }
>      }
> +#ifdef MOZ_ANDROID_SYNTHAPKS

hm, this code is used on b2g for preinstalled operator apps also...

::: mobile/android/modules/WebappManager.jsm
@@ +91,5 @@
> +         }
> +      } catch (e) {
> +        dump("Error in fetch - " + e);
> +      }
> +    });

I think I would rewrite that with xhr in chunked mode and OS.File.

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 8342534 [details] [diff] [review]
apks-install-run-wip-2.diff

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

I don't have more to say compared to the previous one.

Comment 5

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

Comment on attachment 8342534 [details] [diff] [review]
apks-install-run-wip-2.diff

>diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js

>+pref("dom.mozApps.apkGeneratorEndpoint", "http://107.22.148.17:8080/application.apk");

I see dom.mozApps using camelCasePrefs and underscore_style_prefs. At least this isn't a 3rd style :)

>diff --git a/mobile/android/base/AndroidManifest.xml.in b/mobile/android/base/AndroidManifest.xml.in

>+        <receiver android:name="org.mozilla.gecko.webapp.UninstallListener" >

Why no MOZ_ANDROID_SYNTHAPKS? Intentional? I'm OK with not using one if that was your intent.

>+        </receiver>
>+
> 
> #include ../services/manifests/AnnouncementsAndroidManifest_services.xml.in

nit: No need for the extra blank line

>diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
>             } else if (event.equals("WebApps:Install")) {
>+                // TODO delete this message. It's not used.

>             } else if (event.equals("WebApps:PreInstall")) {
>+                // is any of this needed now?

File followups for this potential removals

>             } else if (event.equals("WebApps:Uninstall")) {
>                 String origin = message.getString("origin");
>-                GeckoAppShell.uninstallWebApp(origin);
>+                String packageName = message.optString("packageName");
>+                GeckoAppShell.uninstallWebApp(packageName);

"origin" is unused now

>+            } else if (event.equals("WebApps:InstallApk")) {
>+                GeckoAppShell.installApk(this, message.getString("filePath"), message.getString("data"));

Feel free to skip the local string variables in the other message handlers too

>+            } else if (event.equals("WebApps:GetTempFilePath")) {
>+                mCurrentResponse = GeckoAppShell.getTempFilePath(this, message.getString("fileName"));

Let's try to keep this call out of Java. We should be able to get the TmpD and DownloadDir from JS.

>+                //perform webapp uninstalls as appropiate
>+                org.mozilla.gecko.webapp.UninstallListener.initUninstallPackageScan(sAppContext);

Can you import your way to a shorter line? And can this be passed the Application Context instead of the Activity context? In fact, could we move this init to GeckoApplication ?

>+
>+        // TODO Consider moving install related things into InstallHelper

Good idea

>         } else if (action != null && action.startsWith(ACTION_WEBAPP_PREFIX)) {
>+            // TODO What does this branch actually supposed to do.
>+            // It is currently never called.

Another potential followup

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

>     public static void removeShortcut(final String aTitle, final String aURI, final String aUniqueURI, final String aType) {

>                 if (aType.equalsIgnoreCase(SHORTCUT_TYPE_WEBAPP)) {

We might want to remove the entire SHORTCUT_TYPE_WEBAPP block if that is something we don't intend to support anymore.

>+    public static String getTempFilePath(Context context, String fileName) {

I think this impl could be moved to JS

>+    public static void installApk(final Activity context, String filePath, String data) {
>+
>+        // This is the data that mozApps.install sent to Webapps.jsm

Remove the blank line

>+        sActivityHelper.startIntentForActivity(context, intent,
>+                new ActivityResultHandler() {

Indented too much

>diff --git a/mobile/android/base/WebAppManifestFragment.xml.frag b/mobile/android/base/WebAppManifestFragment.xml.frag
>diff --git a/mobile/android/base/webapp/ApkResources.java b/mobile/android/base/webapp/ApkResources.java

>+public class ApkResources {
>+
>+    private static final String LOGTAG = "Gecko ApkResources";
>+
>+    private final String mPackageName;
>+
>+    private final ApplicationInfo mInfo;
>+
>+    private final Context mContext;

You could drop the double spacing here. And remove the leading blank line.

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

>+public class InstallHelper implements GeckoEventListener {
>+
>+    private static final String LOGTAG = "GeckoInstallHelper";
>+
>+    private static final String[] INSTALL_EVENT_NAMES = new String[] {"WebApps:PostInstall"};
>+
>+    private final Context mContext;
>+
>+    private final InstallCallback mCallback;
>+
>+    private final ApkResources mApkResources;

Same

>+    public File copyApplicationZipFile() throws IOException {

>+        return destPath;
>+
>+    }

Remove blank line

>+    private JSONObject getInstallMessageFromPackage(JSONObject installMessage) throws JSONException {
>+
>+        assert installMessage != null;

Remove leading blank line

>+    public void handleMessage(String event, JSONObject message) {
>+
>+        for (String eventName : INSTALL_EVENT_NAMES) {

Same

>+    }
>+
>+}

Same

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

>+    public void onReceive(Context context, Intent intent) {

>+        String manifestUrl = apkResources.getManifestUrl();
>+        if(TextUtils.isEmpty(manifestUrl)) {

if (

>+        context.unregisterReceiver(this);
>+
>+
>+    }

Remove blank lines

>+    public boolean isCorrectManifest(String manifestUrl) {

>+            URL registered = new URL(mManifestUrl),
>+                observed = new URL(manifestUrl);

I might be old school but dropping the comma list and using "URL .." for both would be more readable IMO

>+    }
>+
>+
>+}

Remove blank lines

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

>+    public void onReceive(Context context, Intent intent) {

>+        if(installedPackages.contains(packageName)) {

if (

>+        Set<String> allInstalledPackages = new HashSet<String>();
>+
>+
>+
>+        for (ApplicationInfo packageInfo : packages) {

One blank line is enough

>+        for(String packageName : fennecPackages) {
>+            if(!allInstalledPackages.contains(packageName)) {

for (
if (

>+        if(uninstalledPackages.size() > 0) {

if (

>+                for(String packageName : uninstalledPackages) {

for (

>+        }
>+
>+    }
>+}

Remove blank line

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

>+    }
>+
>+
>+}

Same

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

>+Cu.import("resource://gre/modules/WebappManager.jsm");

I'd like to see if we could delay load this

>+    Services.obs.addObserver(this, "Webapps:LaunchFromJava", false);

I'm wondering if we could move this to WebAppRT.js and use the notification to delay load WebAppRT. Also, dropping the "Java" part.

>diff --git a/mobile/android/modules/WebappManager.jsm b/mobile/android/modules/WebappManager.jsm

>+function sendMessageToJava(aMessage) {
>+  return Services.androidBridge.handleGeckoMessage(JSON.stringify(aMessage));
>+}
>+this.WebappManager = {

Add a blank line

>+  _downloadApk: function(aMsg) {
>+
>+    let manifestUrl = aMsg.app.manifestURL;

Remove blank line

>+    let generatorUrl = NetUtil.newURI(GENERATOR_URL_BASE).
>+                       QueryInterface(Ci.nsIURL);

No need to wrap

>+    let filePath = sendMessageToJava({
>+      type: "WebApps:GetTempFilePath",
>+      fileName: manifestUrl.replace(/[^a-zA-Z0-9]/gi, "")
>+    });

We should be able to do this right here

>+  _autoInstall: function(aData) {

>+    let data = JSON.parse(aData),
>+        origin = Services.io.newURI(data.manifestUrl, null, null).prePath;

Grrr. Separate declaration please

>+
>+
>+    let message = data.request || {

Remove the extra blank line

>+    message.mm = mm;

Deliciously sneaky! I love it :)

>+    }
>+
>+  },

Remove the blank line

>+  _autoUninstall: function(aData) {

>+    for(let app in installedPackages.apps) { 

for (

>+      if(data.packages.indexOf(installedPackages.apps[app].packageName) > -1) {

if (

>+        });
>+
>+      }
>+    }
>+
>+  }
>+
>+};

Remove the blank lines

I tried to focus on the Fennec parts. This looks like a good approach. For final reviews we should get WesJ and BrianN to help out too.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

(In reply to Mark Finkle (:mfinkle) from comment #5)

> >+pref("dom.mozApps.apkGeneratorEndpoint", "http://107.22.148.17:8080/application.apk");
> 
> I see dom.mozApps using camelCasePrefs and underscore_style_prefs. At least
> this isn't a 3rd style :)

Heh, ok, I changed this to browser.webapps.apkFactoryUrl.

* dom.mozApps -> browser.webapps because this supports a Fennec feature
  that is implemented in platform-specific browser code.
* apkGenerator -> apkFactory because we're calling it a factory these days
  <https://wiki.mozilla.org/User:Clouserw/APKFactory>.
* Endpoint -> Url because it's conventional
  (cf. browser.snippets.*Url, although other prefs use all-caps "URL").

https://github.com/mykmelez/gecko-dev/commit/afb2f86


> >+        <receiver android:name="org.mozilla.gecko.webapp.UninstallListener" >
> 
> Why no MOZ_ANDROID_SYNTHAPKS? Intentional? I'm OK with not using one if that
> was your intent.

Erm, no, unintentional.  Corrected by moving it into the existing #ifdef block.

https://github.com/mykmelez/gecko-dev/commit/b45d3db


> >+        </receiver>
> >+
> > 
> > #include ../services/manifests/AnnouncementsAndroidManifest_services.xml.in
> 
> nit: No need for the extra blank line

Fixed.

https://github.com/mykmelez/gecko-dev/commit/b45d3db


> >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> >             } else if (event.equals("WebApps:Install")) {
> >+                // TODO delete this message. It's not used.
> 
> >             } else if (event.equals("WebApps:PreInstall")) {
> >+                // is any of this needed now?
> 
> File followups for this potential removals

I removed WebApps:Install and determined that WebApps:PreInstall is still needed when synthetic APKs are disabled, so I left it in.


> >             } else if (event.equals("WebApps:Uninstall")) {
> >                 String origin = message.getString("origin");
> >-                GeckoAppShell.uninstallWebApp(origin);
> >+                String packageName = message.optString("packageName");
> >+                GeckoAppShell.uninstallWebApp(packageName);
> 
> "origin" is unused now

Fixed (and changed optString to getString for packageName, which is required).

https://github.com/mykmelez/gecko-dev/commit/6d0a5b5


> >+            } else if (event.equals("WebApps:InstallApk")) {
> >+                GeckoAppShell.installApk(this, message.getString("filePath"), message.getString("data"));
> 
> Feel free to skip the local string variables in the other message handlers
> too

Fixed for WebApps:Uninstall.

https://github.com/mykmelez/gecko-dev/commit/5dedcc6

But I left the existing instances alone to avoid more changes in this busy area of code.


> >+            } else if (event.equals("WebApps:GetTempFilePath")) {
> >+                mCurrentResponse = GeckoAppShell.getTempFilePath(this, message.getString("fileName"));
> 
> Let's try to keep this call out of Java. We should be able to get the TmpD
> and DownloadDir from JS.

Indeed, we can get the download dir from nsIDownloadManager.defaultDownloadsDirectory.

(We can also get TmpP, but we wouldn't be able to install an APK from it, since it's something like /data/data/org.mozilla.fennec/app_tmpdir, to which the Android installer doesn't have access.)

https://github.com/mykmelez/gecko-dev/commit/903e63c


> >+                //perform webapp uninstalls as appropiate
> >+                org.mozilla.gecko.webapp.UninstallListener.initUninstallPackageScan(sAppContext);
> 
> Can you import your way to a shorter line? And can this be passed the
> Application Context instead of the Activity context?

We can do both these things!

https://github.com/mykmelez/gecko-dev/commit/0107b63


> In fact, could we move this init to GeckoApplication ?

We could, but then wouldn't it block startup?  We do it in the background to avoid that.


> >+
> >+        // TODO Consider moving install related things into InstallHelper
> 
> Good idea

Indeed, but I haven't made any changes here, since it isn't clear exactly how to do so.


> >         } else if (action != null && action.startsWith(ACTION_WEBAPP_PREFIX)) {
> >+            // TODO What does this branch actually supposed to do.
> >+            // It is currently never called.
> 
> Another potential followup

Indeed.  I left this one alone for now.


> >     public static void removeShortcut(final String aTitle, final String aURI, final String aUniqueURI, final String aType) {
> 
> >                 if (aType.equalsIgnoreCase(SHORTCUT_TYPE_WEBAPP)) {
> 
> We might want to remove the entire SHORTCUT_TYPE_WEBAPP block if that is
> something we don't intend to support anymore.

It isn't, but let's keep it around until we enable and ship synthetic APKs.


> >+    public static String getTempFilePath(Context context, String fileName) {
> 
> I think this impl could be moved to JS

Yup, done, as described above.


> >+    public static void installApk(final Activity context, String filePath, String data) {
> >+
> >+        // This is the data that mozApps.install sent to Webapps.jsm
> 
> Remove the blank line

https://github.com/mykmelez/gecko-dev/commit/fd4e045


> >+        sActivityHelper.startIntentForActivity(context, intent,
> >+                new ActivityResultHandler() {
> 
> Indented too much

Too much, eh?  I agree!  Although the only other caller <http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1011> begs to differ. ;-)

https://github.com/mykmelez/gecko-dev/commit/ea8a05c


> You could drop the double spacing here. And remove the leading blank line.
> Same
> Remove blank line
> Remove leading blank line
> Same
> Same
> if (
> Remove blank lines

Done!

https://github.com/mykmelez/gecko-dev/commit/7fcb8fe


> >+    public boolean isCorrectManifest(String manifestUrl) {
> 
> >+            URL registered = new URL(mManifestUrl),
> >+                observed = new URL(manifestUrl);
> 
> I might be old school but dropping the comma list and using "URL .." for
> both would be more readable IMO

You are old school!  But so am I, and I agree.  Fixed.  (I don't have the commit URL for this.)


> Remove blank lines
> One blank line is enough
> for (
> if (
> if (
> for (
> Remove blank line
> Same

Done!

https://github.com/mykmelez/gecko-dev/commit/7fcb8fe


> >+Cu.import("resource://gre/modules/WebappManager.jsm");
> 
> I'd like to see if we could delay load this

We can delay-load it by moving webapp notification observations from WebappManager to BrowserApp (provided browser.js is loaded only once, which appears to be the case).

https://github.com/mykmelez/gecko-dev/commit/50a16ce


> >+    Services.obs.addObserver(this, "Webapps:LaunchFromJava", false);
> 
> I'm wondering if we could move this to WebAppRT.js and use the notification
> to delay load WebAppRT. Also, dropping the "Java" part.

We can't move much code to WebAppRT.js, because the observer calls _launchWebAppFromJava, which is mostly just a call to WebAppRT.init with a callback function that references BrowserApp.  But we can delay-load WebAppRT, and I've made that change.

As for "dropping the 'Java' part," I'm unsure what you mean, but perhaps you mean to drop "FromJava" from the name of this notification?  That seems reasonable, although note that WebappsRegistry (in Webapps.js) sends Webapps:Launch to DOMApplicationRegistry (in Webapps.jsm) via the CPMM, which then notifies webapps-launch; so it might be confusing for WebAppImpl to broadcast Webapps:Launch to BrowserApp.

Thus I've renamed the event to Webapps:Load.  That's more accurate anyway, since the webapp has already been launched by the time WebAppImpl broadcasts the event, and the real purpose of the event is to make BrowserApp *load* the launched app.

https://github.com/mykmelez/gecko-dev/commit/758456a


> >diff --git a/mobile/android/modules/WebappManager.jsm b/mobile/android/modules/WebappManager.jsm
> Add a blank line
> Remove blank line
> No need to wrap
> Grrr. Separate declaration please
> Remove the extra blank line
> Remove the blank line
> for (
> if (
> Remove the blank lines

https://github.com/mykmelez/gecko-dev/commit/2405965

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to Fabrice Desré [:fabrice] from comment #3)
> Myk, I just looked at the *.jsm. I have on overall question:
> We download the apk and after that, we download the package right? If so, we
> do security checks after we got the package, which is strange. But I may be
> totally wrong there ;)

The package actually comes with the APK, so we download it at the same time.  Which means we do indeed do the security checks after we download the APK.  But we still do it before we use the package!


> ::: dom/apps/src/AppsUtils.jsm
> @@ +70,5 @@
> >        csp: aApp.csp,
> >        installOrigin: aApp.installOrigin,
> >        origin: aApp.origin,
> > +#ifdef MOZ_ANDROID_SYNTHAPKS
> > +      packageName: aApp.packageName,
> 
> nit: maybe apkName to not be confused with the "standard" packages?

It's a good point, and we do need to be careful to distinguish the APK package from the webapp package.  On the other hand, packageName is the Android-standard descriptor for an APK identifier, and it permeates our code (not just the webapps-related code; Fennec uses it all over).

Perhaps we should call the property apkName in app records in the webapp registry though; unsure.


> ::: dom/apps/src/Webapps.jsm
> @@ +2339,5 @@
> >        if (aInstallSuccessCallback) {
> >          aInstallSuccessCallback(app.manifest);
> >        }
> >      }
> > +#ifdef MOZ_ANDROID_SYNTHAPKS
> 
> hm, this code is used on b2g for preinstalled operator apps also...

Good catch, I narrowed the scope of the ifdef and reinstated the code that was removed (except for the declaration of origPath, which isn't actually used anywhere).

https://github.com/mykmelez/gecko-dev/commit/fa22a2d


> ::: mobile/android/modules/WebappManager.jsm
> @@ +91,5 @@
> > +         }
> > +      } catch (e) {
> > +        dump("Error in fetch - " + e);
> > +      }
> > +    });
> 
> I think I would rewrite that with xhr in chunked mode and OS.File.

That makes sense, although I'm concerned about the dearth of consumers of chunked XHR.  Besides a couple tests, only Shumway uses it, and it only uses it to load files into memory.  There is no existing code writing chunked XHR to disk.

In any case, I've updated WebappManager to do it!  I put the implementation in a worker, as my reading of the OS.File code suggests that I would have to buffer the incoming chunks manually in order to write them to the file asynchronously on the main thread.

https://github.com/mykmelez/gecko-dev/commit/6573bbf
https://github.com/mykmelez/gecko-dev/commit/eba7178

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: the install/launch/uninstall flow

Here's an updated patch that is ready for review.  In addition to fixing the issues identified in feedback (comment 6 and comment 7), I made a number of other fixes for code correctness and readability.

And I extended the flag MOZ_ANDROID_SYNTHAPKS to cover all code being changed, so it's possible to disable the changes (f.e. pending additional work, or while waiting for an APK Factory server to get stood up, or on a release branch if we decide to let it ride the trains longer).

The flag is mostly used at compile time to preprocess files, including WebAppAllocator.java and WebAppImpl.java, which are almost completely different between the old and new implementations.  But some Java files with minor changes use it to determine behavior at runtime via AppConstants.

(And I'm not very happy with preprocessing WebAppAllocator.java and WebAppImpl.java, which seems clunky; so I'm hoping one of y'all'll clue me in to a better alternative.)


Fabrice: can you review the Webapps.jsm parts of the patch?  I did a bit more refactoring, but the changes are similar to the ones you looked at before.

WesJ: I'm hoping you can be the primary reviewer for the bulk of the changes, which modify/extend Fennec code.

BrianN: mfinkle suggested you as an alternate reviewer, but too many reviewers spoils the pot, so I'm requesting feedback from you.


Additional Notes:

* The patch includes some extraneous whitespace fixes made by an aggressive
  editor at some point.  I can easily exclude those if you'd prefer.

* The patch also includes some new @override annotations that Martyn
  recommends as good Java code hygiene, although they too are extraneous
  and can be stripped if you want.

* The patch disables the feature by default; to enable it,
  set `ac_add_options --enable-android-synthapks`.

* JS Engine regression bug 950540 means a debug build will currently crash
  loading marketplace.firefox.com, so build optimized to test the patch.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8348617 [details] [diff] [review]
patch v1: the install/launch/uninstall flow

Mark says BrianN is away, so he'll give feedback on the parts of the patch that he was going to have BrianN look at.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 8348617 [details] [diff] [review]
patch v1: the install/launch/uninstall flow

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

I just did a quick read through to sorta understand what was going on. The move to .in files for some stuff in Java surprised me a bit. We've been working to do NO preprocessing in Java if possible. So much so that, if its impossible to just have every (changed) method check what's available, I'd rather have two different classes.

Removing the whitespace and @Override changes would clean up a chunk of this if you want (and shouldn't be too hard with the qcrecord mercurial extension. I'll try to give it another read through tonight now that I have a better understanding of the flow.

Also, I see a lot of comments above in the bug that I still need to read through :)

::: mobile/android/base/GeckoAppShell.java
@@ +707,5 @@
>      static void scheduleRestart() {
>          gRestartScheduled = true;
>      }
>  
> +    //#ifdef MOZ_ANDROID_SYNTHAPKS

Whats up with these commented out ifdefs? Do we need them?

@@ +764,3 @@
>      }
>  
> +    //#ifndef MOZ_ANDROID_SYNTHAPKS

And this

@@ +2752,5 @@
> +        try {
> +            argsObj = new JSONObject(data);
> +            manifestUrl = argsObj.getJSONObject("app").getString("manifestURL");
> +        } catch (JSONException e) {
> +            Log.e(LOGTAG, "Cannot register APK install listener", e);

You probably want to bail if you can't get any of that info?

@@ +2777,5 @@
> +                // Now deal with if the user pressed cancel.
> +                // IllegalArgumentException happens because resultCode is RESULT_CANCELED
> +                // when the user presses the Done button in the install confirmation dialog,
> +                // even though the install has been successful (and InstallListener already
> +                // unregistered the receiver).

Move this second part of the comment down by/in the catch

::: mobile/android/base/GeckoThread.java
@@ +126,5 @@
>          return resourcePath;
>      }
>  
>      private String getTypeFromAction(String action) {
> +        Log.i(LOGTAG, "Intent action = " + action);

Remove

::: mobile/android/base/WebAppAllocator.java.in
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko;
> +
> +#ifdef MOZ_ANDROID_SYNTHAPKS

Hmm. We should just move this file. But we're really trying to kill all pre-processed java files. Do we need this?

::: mobile/android/base/webapp/InstallHelper.java
@@ +70,5 @@
> +    protected void handleException(Exception e) {
> +        if (mCallback != null) {
> +            mCallback.installErrored(this, e);
> +        } else {
> +            Log.e(LOGTAG, "mozApps.install failed. Most likely a filesystem problem.", e);

I'd drop the "guess" at what's wrong. The exception should provide better info.

@@ +94,5 @@
> +                message.putOpt("zipFilePath", "file://" + zipFilePath);
> +            }
> +
> +        } catch (JSONException e) {
> +            // NOP

Do you want to keep going here?

@@ +127,5 @@
> +            throw e;
> +        } finally {
> +            close(in);
> +            close(out);
> +        }

I would pull this logic out and put it in a utils/FileUtils.java class (I've been trying to get a version of that landed for awhile now...) i.e. something like:

boolean FileUtils.copy(File src, File dest)

@@ +153,5 @@
> +        // find the app launcher's launcher icon
> +        Bitmap bitmap = null;
> +        Drawable d = mApkResources.loadAppIcon();
> +        if (d instanceof BitmapDrawable) {
> +            bitmap = ((BitmapDrawable) d).getBitmap();

We should be able to draw any drawable onto a canvas, right?

@@ +157,5 @@
> +            bitmap = ((BitmapDrawable) d).getBitmap();
> +        } else {
> +            Log.e(LOGTAG, "AppIcon is not a bitmap");
> +            return;
> +        }

insert a blank line between these sections

@@ +161,5 @@
> +        }
> +        int color = -1;
> +        try {
> +            color = BitmapUtils.getDominantColor(bitmap);
> +        } catch (Exception e) {

Does this method throw? Lets just bail :)

@@ +177,5 @@
> +
> +        installMessage.putOpt("packageName", mApkResources.getPackageName());
> +        installMessage.putOpt("manifestUrl", mApkResources.getManifestUrl());
> +        installMessage.putOpt("title", mApkResources.getAppName());
> +        installMessage.putOpt("manifest", new JSONObject(mApkResources.getManifest(mContext)));

Are all these things really optional?

::: mobile/android/base/webapp/UninstallListener.java
@@ +84,5 @@
> +            try {               
> +                for (String packageName : uninstalledPackages) {
> +                    packageNames.put(packageName);
> +                }
> +                message.put("packages", packageNames);   

Trailing whitespace in here.

::: mobile/android/chrome/content/browser.js
@@ +699,2 @@
>      WebappsUI.uninit();
> +#endif

Why do we only do this for synthetic apks now?

@@ +7038,1 @@
>  var WebappsUI = {

Oh, this entire thing is synth only now? Why?

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: addresses wesj's review comments

(In reply to Wesley Johnston (:wesj) from comment #10)

> I just did a quick read through to sorta understand what was going on. The
> move to .in files for some stuff in Java surprised me a bit. We've been
> working to do NO preprocessing in Java if possible. So much so that, if its
> impossible to just have every (changed) method check what's available, I'd
> rather have two different classes.

I don't like preprocessing Java either.  And I was able to do runtime checking in a bunch of places where we made minor changes.  But WebAppAllocator and WebAppImpl are effectively rewritten, and their method names and signatures are different, so it's hard to implement runtime checking in them.

Nevertheless, two different classes sounds like a better idea than preprocessing more Java.  Done!

https://github.com/mykmelez/gecko-dev/commit/7729280fbf02dc8add2c8d70bc9ad2aad5808e25

I named the new classes WebAppAllocator2 and WebAppImpl2, and I put them into the new webapp/ subdirectory.  I suppose we could give them the same names as their old implementations but reference them via their fully-qualified names.  Let me know what you'd prefer.


> Removing the whitespace and @Override changes would clean up a chunk of this
> if you want (and shouldn't be too hard with the qcrecord mercurial
> extension. I'll try to give it another read through tonight now that I have
> a better understanding of the flow.

Fixed!  I actually prefer to let sleeping trailing whitespace lie, since fixing it makes the changelog lie; but @override could be a different story.  In any case, if those annotations are a good idea, then it's easy enough to reintroduce them in a separate patch.

https://github.com/mykmelez/gecko-dev/commit/2ce10a2dc8505e16313ce7c687e6c2d76d3b6f1f
https://github.com/mykmelez/gecko-dev/commit/cd517c06dcbbc23b04db1496fe25b59e35684356


> > +    //#ifdef MOZ_ANDROID_SYNTHAPKS
> 
> Whats up with these commented out ifdefs? Do we need them?
…
> > +    //#ifndef MOZ_ANDROID_SYNTHAPKS
> 
> And this

We don't need them.  They're just markers to distinguish between the versions of postInstallWebApp used by the old and new implementations (and to identify preInstallWebApp and getWebAppIntent as being used only by the old implementation).  I made the comments verbose, prosaic, and clearer.

https://github.com/mykmelez/gecko-dev/commit/159d01f4094928ffa0788fcedebd2dd8deae6962


> @@ +2752,5 @@
> > +        try {
> > +            argsObj = new JSONObject(data);
> > +            manifestUrl = argsObj.getJSONObject("app").getString("manifestURL");
> > +        } catch (JSONException e) {
> > +            Log.e(LOGTAG, "Cannot register APK install listener", e);
> 
> You probably want to bail if you can't get any of that info?

Indeed, given that the next thing we do is instantiate an InstallListener, which asserts manifestUrl != null.  Fixed.

https://github.com/mykmelez/gecko-dev/commit/5d29e03894273ff134a0a7db40985a9a4765c04a


> @@ +2777,5 @@
> > +                // Now deal with if the user pressed cancel.
> > +                // IllegalArgumentException happens because resultCode is RESULT_CANCELED
> > +                // when the user presses the Done button in the install confirmation dialog,
> > +                // even though the install has been successful (and InstallListener already
> > +                // unregistered the receiver).
> 
> Move this second part of the comment down by/in the catch

You bet.

https://github.com/mykmelez/gecko-dev/commit/f74f1089b49536ad2ff5767b20fb739e66d2d9c5


> ::: mobile/android/base/GeckoThread.java
> @@ +126,5 @@
> >          return resourcePath;
> >      }
> >  
> >      private String getTypeFromAction(String action) {
> > +        Log.i(LOGTAG, "Intent action = " + action);
> 
> Remove

Done.

https://github.com/mykmelez/gecko-dev/commit/9ac84b6f82140c17baa2e818c09d48a4d0174a69


> ::: mobile/android/base/WebAppAllocator.java.in
> @@ +4,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +package org.mozilla.gecko;
> > +
> > +#ifdef MOZ_ANDROID_SYNTHAPKS
> 
> Hmm. We should just move this file. But we're really trying to kill all
> pre-processed java files. Do we need this?

Per my comment above, I moved the old implementation back to WebAppAllocator.java and put the new implementation in webapps/WebAppAllocator2.java.


> ::: mobile/android/base/webapp/InstallHelper.java
> @@ +70,5 @@
> > +    protected void handleException(Exception e) {
> > +        if (mCallback != null) {
> > +            mCallback.installErrored(this, e);
> > +        } else {
> > +            Log.e(LOGTAG, "mozApps.install failed. Most likely a filesystem problem.", e);
> 
> I'd drop the "guess" at what's wrong. The exception should provide better
> info.

Sure. :-)

https://github.com/mykmelez/gecko-dev/commit/9ee1dd9ed1c85b4c0826117fd0b7a8c28fd125b6


> @@ +94,5 @@
> > +                message.putOpt("zipFilePath", "file://" + zipFilePath);
> > +            }
> > +
> > +        } catch (JSONException e) {
> > +            // NOP
> 
> Do you want to keep going here?

NOPe, I don't think so, as it looks like all operations on the JSON message are necessary.

https://github.com/mykmelez/gecko-dev/commit/99f3c2d5e59a67cdf644a0b0c9c0fad807a40130


> @@ +127,5 @@
> > +            throw e;
> > +        } finally {
> > +            close(in);
> > +            close(out);
> > +        }
> 
> I would pull this logic out and put it in a utils/FileUtils.java class (I've
> been trying to get a version of that landed for awhile now...) i.e.
> something like:
> 
> boolean FileUtils.copy(File src, File dest)

Seems reasonable.  I haven't done this yet but will tackle it in the next iteration (I'm assuming there will be one!).


> @@ +153,5 @@
> > +        // find the app launcher's launcher icon
> > +        Bitmap bitmap = null;
> > +        Drawable d = mApkResources.loadAppIcon();
> > +        if (d instanceof BitmapDrawable) {
> > +            bitmap = ((BitmapDrawable) d).getBitmap();
> 
> We should be able to draw any drawable onto a canvas, right?

Why yes, we should!

https://github.com/mykmelez/gecko-dev/commit/8a153fa3d0ba0390bb1cc87762c8267b2718ec02


> @@ +157,5 @@
> > +            bitmap = ((BitmapDrawable) d).getBitmap();
> > +        } else {
> > +            Log.e(LOGTAG, "AppIcon is not a bitmap");
> > +            return;
> > +        }
> 
> insert a blank line between these sections

This is no longer needed per the next comment.


> @@ +161,5 @@
> > +        }
> > +        int color = -1;
> > +        try {
> > +            color = BitmapUtils.getDominantColor(bitmap);
> > +        } catch (Exception e) {
> 
> Does this method throw? Lets just bail :)

Not only does it never throw, it always returns a valid color!  Which means we can simplify this implementation significantly (down to four lines, so no need to insert a blank line between sections anymore).

https://github.com/mykmelez/gecko-dev/commit/be0ab963695a000cdd0767550723b999e3d67d60


> @@ +177,5 @@
> > +
> > +        installMessage.putOpt("packageName", mApkResources.getPackageName());
> > +        installMessage.putOpt("manifestUrl", mApkResources.getManifestUrl());
> > +        installMessage.putOpt("title", mApkResources.getAppName());
> > +        installMessage.putOpt("manifest", new JSONObject(mApkResources.getManifest(mContext)));
> 
> Are all these things really optional?

Hmm, no, I don't think so; changed to *put*.

https://github.com/mykmelez/gecko-dev/commit/feba44e4de5906265835d2be9c61854da3419256


> ::: mobile/android/base/webapp/UninstallListener.java
> @@ +84,5 @@
> > +            try {               
> > +                for (String packageName : uninstalledPackages) {
> > +                    packageNames.put(packageName);
> > +                }
> > +                message.put("packages", packageNames);   
> 
> Trailing whitespace in here.

Ugh, indeed, the patch introduced seven lines with trailing whitespace!  How funny that Git warns you when applying a patch that contains trailing whitespace but doesn't warn you when generating same.  All instances fixed.

https://github.com/mykmelez/gecko-dev/commit/a55f606ded96bcdf74796ff9262e80be6c678f4d


> ::: mobile/android/chrome/content/browser.js
> @@ +699,2 @@
> >      WebappsUI.uninit();
> > +#endif
> 
> Why do we only do this for synthetic apks now?
> 
> @@ +7038,1 @@
> >  var WebappsUI = {
> 
> Oh, this entire thing is synth only now? Why?

The work that WebappsUI used to do is now done by the WebappManager module (WebappManager.jsm), so we can lazy-load it, so it's easier to hack, and because it doesn't need access to the chrome browser window (since it no longer creates a shortcut, which is the only thing for which the old implementation needs the window).


Here's an updated patch for your further review!

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 8350560 [details] [diff] [review]
patch v2: addresses wesj's review comments

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

Nothing major to complain about. mfinkle might want to look at the WebAppAllocator2/WappImpl2 and comment on those. I'd prefer if we could hide the versioning behind an interface and just have "getAllocator()" return the right implementation.

::: dom/apps/src/Webapps.jsm
@@ +2064,5 @@
> +        this.confirmInstall(aData);
> +      } else {
> +        Services.obs.notifyObservers(aMm, "webapps-ask-install",
> +                                     JSON.stringify(aData));
> +      }

I think I would prefer (long term?) that this whole NativeApp install experience was made into a component that could do something like check for a native install method, and fallback to a non-native one if that failed.

::: mobile/android/base/GeckoApp.java
@@ +634,5 @@
>                  final String type = message.getString("shortcutType");
>                  GeckoAppShell.removeShortcut(title, url, origin, type);
> +            } else if (AppConstants.MOZ_ANDROID_SYNTHAPKS && event.equals("WebApps:InstallApk")) {
> +                GeckoAppShell.installApk(this, message.getString("filePath"), message.getString("data"));
> +            } else if (!AppConstants.MOZ_ANDROID_SYNTHAPKS && event.equals("WebApps:PreInstall")) {

I'd like to move all of this code to a WebAppsHelper in your webapps package. We can do that separately though if you'd like :) Can you file a bug?

@@ +1911,5 @@
>                                              Tabs.LOADURL_USER_ENTERED |
>                                              Tabs.LOADURL_EXTERNAL);
>          } else if (action != null && action.startsWith(ACTION_WEBAPP_PREFIX)) {
> +            // TODO Figure out what this branch is actually supposed to do,
> +            // as it doesn't appear to ever get called.

We can probably remove this. It was supposed to support everything.me, and I've heard their name coming up lately though... I guess leaving it in for now is best.

::: mobile/android/base/GeckoAppShell.java
@@ +720,2 @@
>      public static void postInstallWebApp(String aTitle, String aURI, String aOrigin, String aIconURL, String aOriginalOrigin) {
> +        WebAppAllocator allocator = WebAppAllocator.getInstance(getContext());

I also love to move all this logic into the webapp package as well. GeckoAppShell is my greatest enemy that I helped create.

@@ +728,5 @@
> +    // The new implementation of postInstallWebApp.  Used by MOZ_ANDROID_SYNTHAPKS.
> +    public static void postInstallWebApp(String aPackageName, String aOrigin) {
> +        WebAppAllocator2 allocator = WebAppAllocator2.getInstance(getContext());
> +        allocator.begin();
> +        int index = allocator.allocatePackage(aPackageName);

It might be nice to name thing something like findOrAllocatePackage to make it clear that it handle's already allocated package names nicely. We do that in sqlite sometimes too. i.e. updateOrInsert

@@ +874,5 @@
>              @Override
>              public void run() {
> +                int index;
> +                if (AppConstants.MOZ_ANDROID_SYNTHAPKS) {
> +                    index = WebAppAllocator2.getInstance(getContext()).releaseIndexForApp(uniqueURI);

I wonder if we could make this prettier with an interface. WebAppAllocator.getInstance() could return either implementation based on the SYNCTH_APK pref.

::: mobile/android/base/WebAppManifestFragment.xml.frag.in
@@ +8,5 @@
> +                  android:launchMode="singleTop"
> +                  android:exported="true"/>
> +#else
> +                  android:launchMode="singleTask"
> +                  android:taskAffinity="org.mozilla.gecko.WEBAPP@APPNUM@"

This looks wrong. No end tag is this is in synth apks?

::: mobile/android/base/webapp/ApkResources.java
@@ +20,5 @@
> +import android.os.Environment;
> +import android.util.Log;
> +
> +public class ApkResources {
> +    private static final String LOGTAG = "Gecko ApkResources";

We usually don't put spaces in logtags.

@@ +52,5 @@
> +    public String getMiniManifest(Context context) {
> +        return readResource(context, "mini");
> +    }
> +
> +

Remove the extra space.

@@ +58,5 @@
> +        return metadata().getString("manifestUrl");
> +    }
> +
> +    public boolean isPackaged() {
> +        return "packaged".equals(metadata().getString("webapp", null));

Reuse getWebAppType here.

@@ +86,5 @@
> +    public Uri getAppIconUri() {
> +        return Uri.parse("android.resource://" + mPackageName + "/" + info().icon);
> +    }
> +
> +    public Drawable loadAppIcon() {

getAppIcon

::: mobile/android/base/webapp/InstallHelper.java
@@ +84,5 @@
> +        // we can change the profile to be in the app's area here
> +        GeckoProfile profile = GeckoProfile.get(mContext, profileName);
> +
> +        try {
> +            message = getInstallMessageFromPackage(message);

Since getInstallMessageFromPackage asserts if message is null, I don't think it really needs to return?

@@ +90,5 @@
> +
> +            if (mApkResources.isPackaged()) {
> +                File zipFile = copyApplicationZipFile();
> +                String zipFilePath = zipFile.toString();
> +                message.putOpt("zipFilePath", "file://" + zipFilePath);

Uri.fromFile(zipFile)?

@@ +149,5 @@
> +            GeckoAppShell.registerEventListener(eventName, this);
> +        }
> +    }
> +
> +    private void calculateColor() {

Its so important this be done on a backgound thread, I'd be happy if we had an assert about it.

@@ +156,5 @@
> +        Bitmap bitmap = drawableToBitmap(mApkResources.loadAppIcon());
> +        slots.updateColor(index, BitmapUtils.getDominantColor(bitmap));
> +    }
> +
> +    public static Bitmap drawableToBitmap(Drawable drawable) {

I would put this in our BitmapUtils class.

@@ +174,5 @@
> +
> +        return bitmap;
> +    }
> +
> +    private JSONObject getInstallMessageFromPackage(JSONObject installMessage) throws JSONException {

Question: Is it worth moving this to the APKResource iteself? mApkResources.getInstallMessage(msg); ?

::: mobile/android/base/webapp/WebAppAllocator2.java
@@ +126,5 @@
> +
> +    public synchronized void releaseIndex(final int index) {
> +        commit(begin().remove(appKey(index))
> +               .remove(iconKey(index))
> +               .remove(originKey(index)));

Line these calls up.

@@ +129,5 @@
> +               .remove(iconKey(index))
> +               .remove(originKey(index)));
> +    }
> +
> +    private SharedPreferences.Editor mEditor = null;

You imported Editor in this file. Might as well use it.

@@ +131,5 @@
> +    }
> +
> +    private SharedPreferences.Editor mEditor = null;
> +
> +    public synchronized Editor begin() {

This API is a little strange to look at. At times, you return 'this', other times you return the Editor. Sometimes you use mEditor, other times you don't. Can we unify it a bit. i.e. always return the allocator or the editor. Always use mEditor?

@@ +136,5 @@
> +        mEditor = edit();
> +        return mEditor;
> +    }
> +
> +    public WebAppAllocator2 end() {

This return is never used.

@@ +162,5 @@
> +                }
> +            });
> +            mEditor = null;
> +        }
> +        return this;

I'd just not return here either. Callers that want to do things like:

return commit(edit().something());

can just call
commit(edit().something());
return this;

::: mobile/android/base/webapp/WebAppImpl2.java
@@ +78,5 @@
> +        boolean isInstalled = extras.getBoolean("isInstalled", false);
> +        String packageName = extras.getString("packageName");
> +
> +        if (packageName == null) {
> +            // TODO Migration path!

Kill ourself? Show an error toast?

@@ +84,5 @@
> +
> +        try {
> +            mApkResources = new ApkResources(this, packageName);
> +        } catch (NameNotFoundException e) {
> +            Log.e(LOGTAG, "Can't find " + packageName + " package for webapp", e);

Kill ourself.

@@ +136,5 @@
> +        }
> +        // This is where we construct the URL from the Intent from the
> +        // the synthesized APK.
> +
> +        // TODO Translate AndroidIntents into WebActivities here.

Nice

::: mobile/android/modules/WebappManager.jsm
@@ +41,5 @@
> +    let params = {
> +      manifestUrl: manifestUrl,
> +    };
> +    generatorUrl.query =
> +      [p + "=" + encodeURIComponent(params[p]) for (p in params)].join("&");

Nice :)

@@ +66,5 @@
> +          data: JSON.stringify(aMsg),
> +        });
> +      } else { // type == "failure"
> +        // TODO: handle error better.
> +        dump("error downloading APK: " + message);

We should just show a toast here to begin with.

@@ +162,5 @@
> +        message.isPackage = true;
> +        // I'm not convinced that this should be true.
> +        // This makes the assumption that the marketplace hosts the manifest file,
> +        // which I don't think it does.
> +        // Uncommenting this line will make that assumption.

I don't think this is true either, but might be for packaged apps...

Comment 13

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

Comment on attachment 8350560 [details] [diff] [review]
patch v2: addresses wesj's review comments

I had wanted Brian to look at the GeckoProfile section. After looking more closely, it looks good to me.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: addresses wesj's followup review comments

(In reply to Myk Melez [:myk] [@mykmelez] from comment #11)

> > I would pull this logic out and put it in a utils/FileUtils.java class (I've
> > been trying to get a version of that landed for awhile now...) i.e.
> > something like:
> > 
> > boolean FileUtils.copy(File src, File dest)
> 
> Seems reasonable.  I haven't done this yet but will tackle it in the next
> iteration (I'm assuming there will be one!).

Erm, upon further inspection, a FileUtils.copy(File src, File dest) method wouldn't help here, since the input stream comes from an android.resource: URL, for which there is no corresponding file (just an entry in the APK).

I could create a StreamUtils class with StreamUtils.copy(InputStream in, OutputStream out), but it wouldn't refactor much code, and the exception handling would be more complex.

Alternately, I could create Something.copyResourceToFile(Uri resourceUri, File file), but it isn't clear how useful that would be generally.

So I've left this code as-is for now.  Perhaps we can refactor this and similar code blocks in a followup bug?


(In reply to Wesley Johnston (:wesj) from comment #12)

> Nothing major to complain about. mfinkle might want to look at the
> WebAppAllocator2/WappImpl2 and comment on those. I'd prefer if we could hide
> the versioning behind an interface and just have "getAllocator()" return the
> right implementation.

Keep in mind that the situation is temporary, as we'll remove the old implementation once the new one is up and running.

And I don't see how we can hide the versioning behind an interface, since it's not just the implementation that has changed; the API has too.

Thus the two versions have mostly different methods, and an interface to them would have the same problem as in my original patch: each implementation would have to include a bunch of exception-throwing stubs for the methods in the other one.

Nevertheless, on second glance, it's pretty easy to use the fully-qualified name for the new implementation where needed, since it's only needed in a few cases in GeckoAppShell.  The other references are in new files in the webapp/ subdirectory that never import the old implementation, so there's no chance of a name conflict there.

Thus I have renamed the new classes back to WebAppAllocator and WebAppImpl and use fully-qualified names to reference them in GeckoAppShell.

https://github.com/mykmelez/gecko-dev/commit/b4bad20

(FWIW, I considered also adding a getAllocator method that would encapsulate use of the fully-qualified name.  But that would only encapsulate some occurrences, since I would still need to annotate each local variable to which I assign a new-style allocator with its fully-qualified name.)


> ::: dom/apps/src/Webapps.jsm
> @@ +2064,5 @@
> > +        this.confirmInstall(aData);
> > +      } else {
> > +        Services.obs.notifyObservers(aMm, "webapps-ask-install",
> > +                                     JSON.stringify(aData));
> > +      }
> 
> I think I would prefer (long term?) that this whole NativeApp install
> experience was made into a component that could do something like check for
> a native install method, and fallback to a non-native one if that failed.

Fabrice filed bug 910473 to implement a "proper glue" between the mozApps implementation and the native runtimes, and Marco has already written some code for it.  We should redouble our efforts to make that happen tout de suite.


> ::: mobile/android/base/GeckoApp.java
> @@ +634,5 @@
> >                  final String type = message.getString("shortcutType");
> >                  GeckoAppShell.removeShortcut(title, url, origin, type);
> > +            } else if (AppConstants.MOZ_ANDROID_SYNTHAPKS && event.equals("WebApps:InstallApk")) {
> > +                GeckoAppShell.installApk(this, message.getString("filePath"), message.getString("data"));
> > +            } else if (!AppConstants.MOZ_ANDROID_SYNTHAPKS && event.equals("WebApps:PreInstall")) {
> 
> I'd like to move all of this code to a WebAppsHelper in your webapps
> package. We can do that separately though if you'd like :) Can you file a
> bug?

I would like to do that separately!  I'll file a bug (along with other followup bugs we identify over the course of the review process) in conjunction with landing this patch!


> @@ +1911,5 @@
> >                                              Tabs.LOADURL_USER_ENTERED |
> >                                              Tabs.LOADURL_EXTERNAL);
> >          } else if (action != null && action.startsWith(ACTION_WEBAPP_PREFIX)) {
> > +            // TODO Figure out what this branch is actually supposed to do,
> > +            // as it doesn't appear to ever get called.
> 
> We can probably remove this. It was supposed to support everything.me, and
> I've heard their name coming up lately though... I guess leaving it in for
> now is best.

Ok, I left it in but updated the comment to express the general idea of the everything.me use case:

    // A lightweight mechanism for loading a web page as a webapp
    // without installing the app natively nor registering it in the DOM
    // application registry.

https://github.com/mykmelez/gecko-dev/commit/6176a0f


> ::: mobile/android/base/GeckoAppShell.java
> @@ +720,2 @@
> >      public static void postInstallWebApp(String aTitle, String aURI, String aOrigin, String aIconURL, String aOriginalOrigin) {
> > +        WebAppAllocator allocator = WebAppAllocator.getInstance(getContext());
> 
> I also love to move all this logic into the webapp package as well.
> GeckoAppShell is my greatest enemy that I helped create.

Roger that.

Note to self: file followup bug to move webapp event listeners and handlers to a class in the webapp package.


> @@ +728,5 @@
> > +    // The new implementation of postInstallWebApp.  Used by MOZ_ANDROID_SYNTHAPKS.
> > +    public static void postInstallWebApp(String aPackageName, String aOrigin) {
> > +        WebAppAllocator2 allocator = WebAppAllocator2.getInstance(getContext());
> > +        allocator.begin();
> > +        int index = allocator.allocatePackage(aPackageName);
> 
> It might be nice to name thing something like findOrAllocatePackage to make
> it clear that it handle's already allocated package names nicely. We do that
> in sqlite sometimes too. i.e. updateOrInsert

Yup, that's a better name, because it's more accurate, although I generally prefer to implement such interfaces via separate methods, such that a caller can call either or both, ideally in the same statement via a logical operator:

    int index = allocator.findPackage(aPackagename) || allocator.allocatePackage(aPackageName);

That would require findPackage to return a falsy value if it doesn't find the package, though, and it returns -1 in that case, so we can't do that.  So for now I've renamed it to findOrAllocatePackage.

https://github.com/mykmelez/gecko-dev/commit/e4f4bc4


> @@ +874,5 @@
> >              @Override
> >              public void run() {
> > +                int index;
> > +                if (AppConstants.MOZ_ANDROID_SYNTHAPKS) {
> > +                    index = WebAppAllocator2.getInstance(getContext()).releaseIndexForApp(uniqueURI);
> 
> I wonder if we could make this prettier with an interface.
> WebAppAllocator.getInstance() could return either implementation based on
> the SYNCTH_APK pref.

Per my earlier comment, I don't think this'll provide much benefit.


> ::: mobile/android/base/WebAppManifestFragment.xml.frag.in
> @@ +8,5 @@
> > +                  android:launchMode="singleTop"
> > +                  android:exported="true"/>
> > +#else
> > +                  android:launchMode="singleTask"
> > +                  android:taskAffinity="org.mozilla.gecko.WEBAPP@APPNUM@"
> 
> This looks wrong. No end tag is this is in synth apks?

There isn't an end *tag*, but there's an unobtrusive closing slash at the end of the opening tag: />, since the <activity> tag doesn't have children in the new implementation.  I made it more obvious by putting the closing slash and greater-than sign on the next line by themselves.

https://github.com/mykmelez/gecko-dev/commit/e5b3415


> ::: mobile/android/base/webapp/ApkResources.java
> @@ +20,5 @@
> > +import android.os.Environment;
> > +import android.util.Log;
> > +
> > +public class ApkResources {
> > +    private static final String LOGTAG = "Gecko ApkResources";
> 
> We usually don't put spaces in logtags.

D'oh!

https://github.com/mykmelez/gecko-dev/commit/7fd143f


> @@ +52,5 @@
> > +    public String getMiniManifest(Context context) {
> > +        return readResource(context, "mini");
> > +    }
> > +
> > +
> 
> Remove the extra space.

Ok.

https://github.com/mykmelez/gecko-dev/commit/97dd39a


> @@ +58,5 @@
> > +        return metadata().getString("manifestUrl");
> > +    }
> > +
> > +    public boolean isPackaged() {
> > +        return "packaged".equals(metadata().getString("webapp", null));
> 
> Reuse getWebAppType here.

Gotcha.

https://github.com/mykmelez/gecko-dev/commit/97dd39a


> @@ +86,5 @@
> > +    public Uri getAppIconUri() {
> > +        return Uri.parse("android.resource://" + mPackageName + "/" + info().icon);
> > +    }
> > +
> > +    public Drawable loadAppIcon() {
> 
> getAppIcon

Yupski.

https://github.com/mykmelez/gecko-dev/commit/a2e5d0a


> ::: mobile/android/base/webapp/InstallHelper.java
> @@ +84,5 @@
> > +        // we can change the profile to be in the app's area here
> > +        GeckoProfile profile = GeckoProfile.get(mContext, profileName);
> > +
> > +        try {
> > +            message = getInstallMessageFromPackage(message);
> 
> Since getInstallMessageFromPackage asserts if message is null, I don't think
> it really needs to return?

Indeed!

https://github.com/mykmelez/gecko-dev/commit/da4f89e


> @@ +90,5 @@
> > +
> > +            if (mApkResources.isPackaged()) {
> > +                File zipFile = copyApplicationZipFile();
> > +                String zipFilePath = zipFile.toString();
> > +                message.putOpt("zipFilePath", "file://" + zipFilePath);
> 
> Uri.fromFile(zipFile)?

For sure.

https://github.com/mykmelez/gecko-dev/commit/e2a86a3


> @@ +149,5 @@
> > +            GeckoAppShell.registerEventListener(eventName, this);
> > +        }
> > +    }
> > +
> > +    private void calculateColor() {
> 
> Its so important this be done on a backgound thread, I'd be happy if we had
> an assert about it.

Presumably ThreadUtils.assertOnBackgroundThread is the way to do that, although there are no callers of that method in the codebase.

Also, it isn't clear whether this assert should be debug-only or unconditional.  The various assertOn*Thread calls in GeckoEditable and GeckoInputConnection are debug-only, while the assertOnUiThread calls in FormAssistPopup, gfx/GLController, gfx/LayerMarginsAnimator, and prompts/Prompt are unconditional.

In any case, I've made this one unconditional, but let me know if I should change that.

https://github.com/mykmelez/gecko-dev/commit/10ae360


> @@ +156,5 @@
> > +        Bitmap bitmap = drawableToBitmap(mApkResources.loadAppIcon());
> > +        slots.updateColor(index, BitmapUtils.getDominantColor(bitmap));
> > +    }
> > +
> > +    public static Bitmap drawableToBitmap(Drawable drawable) {
> 
> I would put this in our BitmapUtils class.

Done, and also renamed to getBitmapFromDrawable for consistency with getBitmapFromDataUri.

https://github.com/mykmelez/gecko-dev/commit/471fd7d


> @@ +174,5 @@
> > +
> > +        return bitmap;
> > +    }
> > +
> > +    private JSONObject getInstallMessageFromPackage(JSONObject installMessage) throws JSONException {
> 
> Question: Is it worth moving this to the APKResource iteself?
> mApkResources.getInstallMessage(msg); ?

Hmm, getInstallMessageFromPackage gets all its info from ApkResources, but the "install message" in question actually belongs to InstallHelper, and ApkResources is merely helping to populate it.

If anything, I would unrefactor getInstallMessageFromPackage, merging its logic into the *install* method, since *install* is its only caller, and that would allow us to get rid of the *message* in/out parameter, which tends to be confusing (as it was in this case, given that *install* was also assigning the return value of getInstallMessageFromPackage, as you noted earlier).

So that's what I did.

https://github.com/mykmelez/gecko-dev/commit/8d2afbb


> ::: mobile/android/base/webapp/WebAppAllocator2.java
> @@ +126,5 @@
> > +
> > +    public synchronized void releaseIndex(final int index) {
> > +        commit(begin().remove(appKey(index))
> > +               .remove(iconKey(index))
> > +               .remove(originKey(index)));
> 
> Line these calls up.

Done.

https://github.com/mykmelez/gecko-dev/commit/b877f10


> @@ +129,5 @@
> > +               .remove(iconKey(index))
> > +               .remove(originKey(index)));
> > +    }
> > +
> > +    private SharedPreferences.Editor mEditor = null;
> 
> You imported Editor in this file. Might as well use it.

Indeed!  I did so in three places.

https://github.com/mykmelez/gecko-dev/commit/03ad9ac


> @@ +131,5 @@
> > +    }
> > +
> > +    private SharedPreferences.Editor mEditor = null;
> > +
> > +    public synchronized Editor begin() {
> 
> This API is a little strange to look at. At times, you return 'this', other
> times you return the Editor. Sometimes you use mEditor, other times you
> don't. Can we unify it a bit. i.e. always return the allocator or the
> editor. Always use mEditor?

Indeed.  Not only is it strange, but GeckoAppShell and WebAppAllocator use the API inconsistently, with GeckoAppShell calling putPackageName (through findOrAllocatePackage), putOrigin, and *end*, all of which independently call *commit*; while WebAppAllocator bypasses *end* and calls *commit* directly.

And then there's WebAppDispatcher and InstallListener, both of which call putPackageName (through findOrAllocatePackage) without calling *begin* nor *end*.

So it's more than mere inconsistency in the begin/edit/commit/end API; there's also mixture of that API with the higher-level API built on top of it, plus inconsistent use of the APIs.

Meanwhile, Android API Level 9, which Fennec requires, introduced SharedPreferences.Editor.apply <http://developer.android.com/reference/android/content/SharedPreferences.Editor.html#apply%28%29>, which writes to disk asynchronously, presumably obviating the need for us to call SharedPreferences.Editor.commit on a background thread.

I also see Fennec's SharedPreferencesHelper.handleSet, but that would require callers to construct a JSON object, which seems even harder than directly accessing SharedPreferences.Editor (although I imagine it's simpler for some use cases).

So I have removed the begin/edit/commit/end API.  Instead, WebAppAllocator now uses SharedPreferences.Editor directly, and other classes only use the higher-level API.

This does mean that some operations in other classes cause multiple commits.  But that was already the case; all I did was make it more obvious.  And their usage is so minimal that I don't think it's worth making them jump through hoops to batch and commit their changes.  No premature optimization!

While here, I also converted the homegrown call to SharedPreferences.Editor.commit on a background thread with calls to SharedPreferences.Editor.apply.

https://github.com/mykmelez/gecko-dev/commit/927b186


> @@ +136,5 @@
> > +        mEditor = edit();
> > +        return mEditor;
> > +    }
> > +
> > +    public WebAppAllocator2 end() {
> 
> This return is never used.

All your base are belong to us.


> @@ +162,5 @@
> > +                }
> > +            });
> > +            mEditor = null;
> > +        }
> > +        return this;
> 
> I'd just not return here either. Callers that want to do things like:
> 
> return commit(edit().something());
> 
> can just call
> commit(edit().something());
> return this;

You have no chance to survive make your time.


> ::: mobile/android/base/webapp/WebAppImpl2.java
> @@ +78,5 @@
> > +        boolean isInstalled = extras.getBoolean("isInstalled", false);
> > +        String packageName = extras.getString("packageName");
> > +
> > +        if (packageName == null) {
> > +            // TODO Migration path!
> 
> Kill ourself? Show an error toast?

An ideal experience would be to prompt the user to reinstall the app (and walk them through the process).  But for now we will kill ourself.

Note to self: find or file bug on migration experience.

https://github.com/mykmelez/gecko-dev/commit/a19aaca


> @@ +84,5 @@
> > +
> > +        try {
> > +            mApkResources = new ApkResources(this, packageName);
> > +        } catch (NameNotFoundException e) {
> > +            Log.e(LOGTAG, "Can't find " + packageName + " package for webapp", e);
> 
> Kill ourself.

https://github.com/mykmelez/gecko-dev/commit/a19aaca


> @@ +66,5 @@
> > +          data: JSON.stringify(aMsg),
> > +        });
> > +      } else { // type == "failure"
> > +        // TODO: handle error better.
> > +        dump("error downloading APK: " + message);
> 
> We should just show a toast here to begin with.

Note to self: file bug on showing a toast here to begin with.


> @@ +162,5 @@
> > +        message.isPackage = true;
> > +        // I'm not convinced that this should be true.
> > +        // This makes the assumption that the marketplace hosts the manifest file,
> > +        // which I don't think it does.
> > +        // Uncommenting this line will make that assumption.
> 
> I don't think this is true either, but might be for packaged apps...

It isn't true for packaged apps either.  For example, <https://people.mozilla.org/~myk/focusTimer/package.manifest> is a valid mini-manifest for a packaged app that is not hosted by Marketplace (nor is the package itself, which is located at <https://people.mozilla.org/~myk/focusTimer/package.zip>; while the page that installs the app is at <https://people.mozilla.org/~myk/focusTimer/>).

https://github.com/mykmelez/gecko-dev/commit/b919870


I also touched CLOBBER to ensure a clobber, as I see build failures referencing the dynamically-generated WebApp[n] classes otherwise.


wesj: here's a comparison between the patch you reviewed and the updated patch in case you want to revisit your r+:

https://github.com/mykmelez/gecko-dev/compare/feba44e...48dd1f4

Either way, this still needs review from Fabrice for the Webapps.jsm changes.


Try Run: https://tbpl.mozilla.org/?tree=Try&rev=f8af33887e7e

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 8351326 [details] [diff] [review]
patch v3: addresses wesj's followup review comments

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

I checked the dom/apps code, it's nice and clean. Thanks Myk! Nice little worker too ;)

Comment 16

[Myk Melez [:myk] [@mykmelez]]

This is ready to commit, but I'm PTO for the next two days, so I'm going to hold off doing so until Thursday, when I'll be around to address regressions.

In the meantime, I filed dependent bugs on all the followup issues we've discussed in this bug or noted via TODO annotations in the patch.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c31c82d98fc

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4c31c82d98fc