934760 - implement synthetic APK update flow

Status: RESOLVED FIXED

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

Description

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

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

* fennec periodically checks app install origins for updates
* fennec finds update
* if user approval required
  * fennec notifies user about available update
  * user taps notification to approve update
* fennec downloads apk
* fennec installs apk without prompting user
* fennec tells registry to update app registration
* fennec notifies user that app was updated

Comment 1

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

Attachment: work in progress

Here's a work-in-progress patch that implements the periodic update checker that determines whether or not an update is available for each installed webapp by pinging a server for a list of outdated apps.  The next pieces I'll implement will prompt the user about an outdated app, download the update, and apply it.

Comment 2

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

Note that the work-in-progress patch depends on the install/launch/uninstall patch up for review in bug 934758.  I built it from the apk-updates branch <https://github.com/mykmelez/gecko-dev/tree/apk-updates>.

Comment 3

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

Erm, I meant to say that it depends on the patch in *bug 934756*.

Comment 4

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

Comment on attachment 8349463 [details] [diff] [review]
work in progress

Mark: this isn't a complete patch, but I'm interested in your feedback on the bits that are in place to make sure I'm not completely off track.  Once the update checker identifies an available update, my plan is to use code similar to the code in /mobile/android/base/updater/ to notify the user about the update and apply the updated APK.

Comment 5

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

Comment on attachment 8349463 [details] [diff] [review]
work in progress

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

>             } else if (!AppConstants.MOZ_ANDROID_SYNTHAPKS && event.equals("WebApps:Uninstall")) {
>                 GeckoAppShell.uninstallWebApp(message.getString("origin"));
>+            } else if (event.equals("WebApps:GetApkVersions")) {
>+                mCurrentResponse = GeckoAppShell.getApkVersionNumbers(this, message.getJSONArray("packageNames"));

This is changing. mCurrentResponse is going away in bug 946344. Wes has details.

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

>+    public static String getApkVersionNumbers(Activity context, JSONArray packageNames) {

I wouldn't mind trying to move this to JavaScript (or at least out of GeckoAppShell). We might be able to move this to JS using some JNI. We also have HelperApps.jsm which might be a nicer place to move the API. We can run it past Wes, who owns both the JNI stuff and HelperApps.jsm

>diff --git a/mobile/android/components/WebappsUpdateTimer.js b/mobile/android/components/WebappsUpdateTimer.js

>+  notify: function(aTimer) {
>+    // If we are offline, wait to be online to start the update check.
>+    if (Services.io.offline) {
>+      dump("Network is offline. Setting up an offline status observer.");
>+      Services.obs.addObserver(this, "network:offline-status-changed", true);
>+      return;
>+    }

In case we want to add a "Wifi only" mode, we could use soemthing like this:

function isWifi() {
    let network = Cc["@mozilla.org/network/network-link-service;1"].getService(Ci.nsINetworkLinkService);
    return (network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_WIFI || network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_ETHERNET);
}

Given that we don't yet have a plan for being able to move this to a Java background service, this approach looks good.

Comment 6

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

Attachment: 934760-wip2.diff

Here's an updated work in progress that successfully updates apps when you manually check for updates via its new "Check for Updates" list item in about:apps.  It depends on the patches in bug 934756 and bug 953328.  I continue to use the apk-updates branch to develop it:

  https://github.com/mykmelez/gecko-dev/tree/apk-updates

The primary remaining missing piece is a notification prompt that updates are available, like the one that notifies you that an update is available for Fennec Nightly.  Currently, the patch proceeds directly from "Check for Updates" to downloading and installing the updates (i.e. triggering install confirmation screens), which is not a particularly friendly experience.

Requesting feedback from Wes on the best way to access getApkVersionNumbers from JavaScript, per mfinkle's comment 5.

Requesting feedback from Fabrice on the changes to Webapps.jsm.

Fabrice: the bulk of the changes are refactoring, moving updatePackagedApp and updateHostedApp out of checkForUpdates, so Fennec's WebappManager can call them directly.  There are only two functional changes:

1. checkForUpdates now exits earlier if the app is a packaged app whose manifest URL starts with app:// and thus can't be updated.

I did this so I wouldn't have to pass aMm to updatePackagedApp, but I think it makes sense even without that benefit to the refactoring, since there's no point in going through the whole process of downloading a new manifest for the app if it can't be updated.

2. getManifestFor now returns a promise, so you can use it in a promise chain.

It still accepts a callback, though, so you can ignore the return value and pass it a callback.  And I haven't touched any existing callers, which all continue to pass a callback.

This change isn't absolutely necessary, but it simplifies the logic in WebappManager (which would otherwise wrap that function in a promise implementation).  And it seems like a good model for adding promise-based interfaces to such functions without breaking backward compatibility.

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 8354983 [details] [diff] [review]
934760-wip2.diff

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

::: dom/apps/src/Webapps.jsm
@@ +3564,5 @@
> +    } else {
> +      this._readManifests([{ id: id }], function(aResult) {
> +        let manifest = aResult[0].manifest;
> +        if (aCallback) {
> +          aCallback();

aCallback(manifest)

@@ +3566,5 @@
> +        let manifest = aResult[0].manifest;
> +        if (aCallback) {
> +          aCallback();
> +        }
> +        deferred.resolve(null);

resolve(manifest)

@@ +3572,3 @@
>      }
>  
> +    return deferred.promise;

we could also not create the promise at all if !!aCallback. I can't think of a good reason to pass a callback and use the promise.

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 8354983 [details] [diff] [review]
934760-wip2.diff

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

This looks really nice myk. My event stuff is causing some test failures, so if you want to land this before its done, I'd be fine with that.

::: mobile/android/base/GeckoApp.java
@@ +661,5 @@
>                  startActivity(intent);
>              } else if (!AppConstants.MOZ_ANDROID_SYNTHAPKS && event.equals("WebApps:Uninstall")) {
>                  GeckoAppShell.uninstallWebApp(message.getString("origin"));
> +            } else if (event.equals("WebApps:GetApkVersions")) {
> +                mCurrentResponse = GeckoAppShell.getApkVersionNumbers(this, message.getJSONArray("packageNames"));

Yeah. We'll have to change this to something like

JSONObject obj = new JSONObject();
obj.put("apps", GeckoAppShell.getAPKVersionNumbers(this, ...));
EventDispatcher.sendResponse(message, obj);

haven't landed that yet though.

::: mobile/android/base/GeckoAppShell.java
@@ +2841,5 @@
>              }
>          });
>      }
> +
> +    public static String getApkVersionNumbers(Activity context, JSONArray packageNames) {

Does this need to be in GeckoAppShell? I'd love to keep webapp related code in the webapp namespace, even if we have to add a WebAppUtils.java class there... (but maybe this could be a static method in another class we already have?)

GeckoAppShell was useful when we were manually writing a lot of C++ JNI code. We're not anymore, so we don't need it nearly as much.

::: mobile/android/chrome/content/aboutApps.xhtml
@@ +58,5 @@
> +      <img class="icon" src="chrome://browser/skin/images/update.png" />
> +      <div class="inner">
> +        <div id="browse-title" class="title">&aboutApps.checkForUpdates;</div>
> +      </div>
> +    </div>

Nice. Like you said before, this should probably die. Maybe we can add this in settings somewhere (or on about:firefox?)

::: mobile/android/components/WebappsUpdateTimer.js
@@ +38,5 @@
> +    WebappManager.checkForUpdates();
> +  },
> +
> +  observe: function(aSubject, aTopic, aData) {
> +    if (aTopic !== "network:offline-status-changed" || aData !== "online") {

Hmm... Longer term, we might want to make these updates only happen on wifi. Mark did that here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/SimpleServiceDiscovery.jsm#108

Maybe this should be a pref? Or use the pref we already have for nightly updates...

::: mobile/android/modules/WebappManager.jsm
@@ +23,4 @@
>  
> +let originalDump = dump;
> +let dump = function(message) {
> +  originalDump(message + "\n");

Does \n actually do anything for you? logcat already breaks things up in lines, and strips linebreaks from my messages.

Comment 9

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

Attachment: 934760v1.diff

Ok, I think this is more or less ready for review.  I expect we'll have followup work, especially on the UX, which we'll want to review and revise accordingly.  But this seems like a reasonable start.

The key difference between the last WIP and this patch is that we now notify the user that updates are available and prompt the user to download them via a notification.  Then, once the updates have been downloaded, we prompt the user to install them.


(In reply to Fabrice Desré [:fabrice] from comment #7)
> Comment on attachment 8354983 [details] [diff] [review]
> 934760-wip2.diff
> 
> Review of attachment 8354983 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/src/Webapps.jsm
> @@ +3564,5 @@
> > +    } else {
> > +      this._readManifests([{ id: id }], function(aResult) {
> > +        let manifest = aResult[0].manifest;
> > +        if (aCallback) {
> > +          aCallback();
> 
> aCallback(manifest)

Got it.


> @@ +3566,5 @@
> > +        let manifest = aResult[0].manifest;
> > +        if (aCallback) {
> > +          aCallback();
> > +        }
> > +        deferred.resolve(null);
> 
> resolve(manifest)

Yep.


> @@ +3572,3 @@
> >      }
> >  
> > +    return deferred.promise;
> 
> we could also not create the promise at all if !!aCallback. I can't think of
> a good reason to pass a callback and use the promise.

Neither can I, although it doesn't hurt to create the promise (unless there's a performance implication?), and it complicates the implementation to create it conditionally.

Nevertheless, this version creates it conditionally.  Let me know what you think!


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

> ::: mobile/android/base/GeckoApp.java
> @@ +661,5 @@
> >                  startActivity(intent);
> >              } else if (!AppConstants.MOZ_ANDROID_SYNTHAPKS && event.equals("WebApps:Uninstall")) {
> >                  GeckoAppShell.uninstallWebApp(message.getString("origin"));
> > +            } else if (event.equals("WebApps:GetApkVersions")) {
> > +                mCurrentResponse = GeckoAppShell.getApkVersionNumbers(this, message.getJSONArray("packageNames"));
> 
> Yeah. We'll have to change this to something like
> 
> JSONObject obj = new JSONObject();
> obj.put("apps", GeckoAppShell.getAPKVersionNumbers(this, ...));
> EventDispatcher.sendResponse(message, obj);
> 
> haven't landed that yet though.

Sure.  Last one in the pool is a rotten egg!  Or at least a fixer-upper of patches.  Alternately, we can let the JNI out of the bottle.


> ::: mobile/android/base/GeckoAppShell.java
> @@ +2841,5 @@
> >              }
> >          });
> >      }
> > +
> > +    public static String getApkVersionNumbers(Activity context, JSONArray packageNames) {
> 
> Does this need to be in GeckoAppShell? I'd love to keep webapp related code
> in the webapp namespace, even if we have to add a WebAppUtils.java class
> there... (but maybe this could be a static method in another class we
> already have?)
> 
> GeckoAppShell was useful when we were manually writing a lot of C++ JNI
> code. We're not anymore, so we don't need it nearly as much.

It doesn't have to be there.  But over in bug 957070 mhaigh is moving the other webapp event handlers to a class in the webapp namespace.  Perhaps we can leave this one in GeckoAppShell until he lands that change?


> ::: mobile/android/chrome/content/aboutApps.xhtml
> @@ +58,5 @@
> > +      <img class="icon" src="chrome://browser/skin/images/update.png" />
> > +      <div class="inner">
> > +        <div id="browse-title" class="title">&aboutApps.checkForUpdates;</div>
> > +      </div>
> > +    </div>
> 
> Nice. Like you said before, this should probably die. Maybe we can add this
> in settings somewhere (or on about:firefox?)

Indeed!  I'll look around for somewhere else to put it.  I've left it here for now, though, since we haven't yet decided to kill about:apps (hence bug 953328).


> ::: mobile/android/components/WebappsUpdateTimer.js
> @@ +38,5 @@
> > +    WebappManager.checkForUpdates();
> > +  },
> > +
> > +  observe: function(aSubject, aTopic, aData) {
> > +    if (aTopic !== "network:offline-status-changed" || aData !== "online") {
> 
> Hmm... Longer term, we might want to make these updates only happen on wifi.
> Mark did that here:
>
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/
> SimpleServiceDiscovery.jsm#108
> 
> Maybe this should be a pref? Or use the pref we already have for nightly
> updates...

Good idea, added to the list of todos!


> ::: mobile/android/modules/WebappManager.jsm
> @@ +23,4 @@
> >  
> > +let originalDump = dump;
> > +let dump = function(message) {
> > +  originalDump(message + "\n");
> 
> Does \n actually do anything for you? logcat already breaks things up in
> lines, and strips linebreaks from my messages.

Hmm, indeed, that it does.  So we don't need to override *dump*.  But I'm also leery of having a bunch of dump statements in this file that look different from the others scattered around the codebase.  So I renamed the function to *log* and added a long explanation to it of why we use *dump* instead of *Services.console.logStringMessage*.

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 8360275 [details] [diff] [review]
934760v1.diff

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

Adding mike to this so that he can look through some of this code and get ramped up reviewing this stuff a bit. I started to read through and then my head hurt, so I figure this is a great place for him to start :)

mcomella: No need to go crazy. Just wanted to point you to something. I'd ignore all the changes in dom/apps and mostly focus on things in Android.

::: dom/apps/src/AppsUtils.jsm
@@ +628,5 @@
>      }
>      return {};
>    },
>  
> +  get biggestIconURL() {

Long term, it seems more useful to have this take an (optional?) parameter so that you can request a particular size, as that's more often what you want. "Find me an icon that's 64x64 or larger"

::: mobile/android/base/GeckoApp.java
@@ +660,5 @@
>                      return;
>                  startActivity(intent);
>              } else if (!AppConstants.MOZ_ANDROID_SYNTHAPKS && event.equals("WebApps:Uninstall")) {
>                  GeckoAppShell.uninstallWebApp(message.getString("origin"));
> +            } else if (event.equals("WebApps:GetApkVersions")) {

I'm fine leaving these in here for now, as long as we've got a plan to clean up :)

::: mobile/android/base/GeckoAppShell.java
@@ +2775,5 @@
>          });
>      }
> +
> +    public static String getApkVersionNumbers(Activity context, JSONArray packageNames) {
> +        List <String> packageNameList = new ArrayList <String>();

We don't usually put spaces between the class and <Generic> bit. i.e. List<String>

Comment 11

[[:fabrice] Fabrice Desré]

Comment on attachment 8360275 [details] [diff] [review]
934760v1.diff

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

Will review once it's rebased on current codebase.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8360275 [details] [diff] [review]
934760v1.diff

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

Still skimming through but here's my first bout of pedantic feedback.

::: mobile/android/base/GeckoAppShell.java
@@ +2774,5 @@
>              }
>          });
>      }
> +
> +    public static String getApkVersionNumbers(Activity context, JSONArray packageNames) {

nit: getAPKVersionNumbers

@@ +2780,5 @@
> +        for (int i = 0; i < packageNames.length(); i++) {
> +            try {
> +                packageNameList.add(packageNames.getString(i));
> +            } catch (JSONException e) {
> +                Log.w(LOGTAG, "Exception populating settings items.", e);

nit: "Exception populating settings item.", ...

Since we fail on only one item at a time.

@@ +2786,5 @@
> +        }
> +
> +        final PackageManager pm = context.getPackageManager();
> +        //get a list of installed apps on device
> +        List<ApplicationInfo> packages = pm.getInstalledApplications(PackageManager.GET_META_DATA);

Is this flag used? Sorry, but I'm not entirely sure what it does.

@@ +2791,5 @@
> +
> +        JSONObject jsonMessage = new JSONObject();
> +
> +        for (ApplicationInfo packageInfo : packages) {
> +            if (packageNameList.contains(packageInfo.packageName)) {

Since we only call `contains` on the packageNameList, you should make it a Set instead.

@@ +2792,5 @@
> +        JSONObject jsonMessage = new JSONObject();
> +
> +        for (ApplicationInfo packageInfo : packages) {
> +            if (packageNameList.contains(packageInfo.packageName)) {
> +                int versionCode = 0;

Should this be some less likely value like -1, or is 0 expected if the value is not available?

Also, if this is consistently the same value, perhaps it should be a constant.

@@ +2796,5 @@
> +                int versionCode = 0;
> +                try {
> +                    versionCode = pm.getPackageInfo(packageInfo.packageName, 0).versionCode;
> +                } catch (PackageManager.NameNotFoundException e) {
> +                    Log.e(LOGTAG, "NameNotFoundException", e);

We get NameNotFonudException from the exception given to the log, this should be more descriptive (e.g. "Existing package, " + packageInfo.packageName + " was not found. Was it uninstalled?")

@@ +2801,5 @@
> +                }
> +                try {
> +                    jsonMessage.put(packageInfo.packageName, versionCode);
> +                } catch (JSONException e) {
> +                    Log.e(LOGTAG, "JSONException", e);

We get JSONException from the exception given to log. This should be more descriptive (e.g. "unable to store version code field".)

Comment 13

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

Attachment: 934760v2.diff

Comment 14

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

Comment on attachment 8364363 [details] [diff] [review]
934760v2.diff

Thanks for the review!  I haven't yet updated this patch to address your comments, but I did rebase it against changes upstream so Fabrice could review the Webapps.jsm changes.

Comment 15

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

Attachment: patch v3: addresses review comments and other issues

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

> ::: dom/apps/src/AppsUtils.jsm
> @@ +628,5 @@
> >      }
> >      return {};
> >    },
> >  
> > +  get biggestIconURL() {
> 
> Long term, it seems more useful to have this take an (optional?) parameter
> so that you can request a particular size, as that's more often what you
> want. "Find me an icon that's 64x64 or larger"

Sure, let's burn that bridge when we come to it!


> ::: mobile/android/base/GeckoApp.java
> @@ +660,5 @@
> >                      return;
> >                  startActivity(intent);
> >              } else if (!AppConstants.MOZ_ANDROID_SYNTHAPKS && event.equals("WebApps:Uninstall")) {
> >                  GeckoAppShell.uninstallWebApp(message.getString("origin"));
> > +            } else if (event.equals("WebApps:GetApkVersions")) {
> 
> I'm fine leaving these in here for now, as long as we've got a plan to clean
> up :)

In the process of merging in the fix for bug 957070, which landed yesterday, I moved the implementation to webapp/EventListener.java, although I left the listener in GeckoApp because, like the one for WebApps:PreInstall, it relies on being able to set mCurrentResponse to a value.  Once you land the event listener improvements, we can make this even better.


> ::: mobile/android/base/GeckoAppShell.java
> @@ +2775,5 @@
> >          });
> >      }
> > +
> > +    public static String getApkVersionNumbers(Activity context, JSONArray packageNames) {
> > +        List <String> packageNameList = new ArrayList <String>();
> 
> We don't usually put spaces between the class and <Generic> bit. i.e.
> List<String>

Roger.

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


(In reply to Michael Comella (:mcomella) from comment #12)

> ::: mobile/android/base/GeckoAppShell.java
> @@ +2774,5 @@
> >              }
> >          });
> >      }
> > +
> > +    public static String getApkVersionNumbers(Activity context, JSONArray packageNames) {
> 
> nit: getAPKVersionNumbers

I'm ok with that, but it would be inconsistent with the other names in the file that have "apk" (installApks, apkPath), so I left it as-is.  But let me know if I should instead change all of them.  (Note that this function is now called getApkVersions, for consistency with the name of the event; and it's in the EventListener.java file inside the webapp/ subdirectory, where all Java files do first-letter-only capitalization of symbol names, i.e. fooUrl, barApk, omgWtfBaz, etc.).


> @@ +2780,5 @@
> > +        for (int i = 0; i < packageNames.length(); i++) {
> > +            try {
> > +                packageNameList.add(packageNames.getString(i));
> > +            } catch (JSONException e) {
> > +                Log.w(LOGTAG, "Exception populating settings items.", e);
> 
> nit: "Exception populating settings item.", ...
> 
> Since we fail on only one item at a time.

Indeed!

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


> @@ +2786,5 @@
> > +        }
> > +
> > +        final PackageManager pm = context.getPackageManager();
> > +        //get a list of installed apps on device
> > +        List<ApplicationInfo> packages = pm.getInstalledApplications(PackageManager.GET_META_DATA);
> 
> Is this flag used? Sorry, but I'm not entirely sure what it does.

Hmm, no, I don't think so.  It looks like a leftover from a previous version of the patch that retrieved information from the package's metadata bundle.  I've removed it.

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


> @@ +2791,5 @@
> > +
> > +        JSONObject jsonMessage = new JSONObject();
> > +
> > +        for (ApplicationInfo packageInfo : packages) {
> > +            if (packageNameList.contains(packageInfo.packageName)) {
> 
> Since we only call `contains` on the packageNameList, you should make it a
> Set instead.

Sure.

https://github.com/mykmelez/gecko-dev/commit/28a6e8b


> @@ +2792,5 @@
> > +        JSONObject jsonMessage = new JSONObject();
> > +
> > +        for (ApplicationInfo packageInfo : packages) {
> > +            if (packageNameList.contains(packageInfo.packageName)) {
> > +                int versionCode = 0;
> 
> Should this be some less likely value like -1, or is 0 expected if the value
> is not available?

The APK Factory service doesn't particularly expect 0, but it always generates APKs with positive integer versionCode values, so 0 is sufficient to ensure that the service will always return an update for an app whose versionCode is 0.


> Also, if this is consistently the same value, perhaps it should be a
> constant.

Hmm, I might be missing something (and am not yet well-versed in Java), but I don't quite see how to make this "final" when we want to set it to 0 if retrieving the value throws an exception, given that final variables can only be initialized once.


> @@ +2796,5 @@
> > +                int versionCode = 0;
> > +                try {
> > +                    versionCode = pm.getPackageInfo(packageInfo.packageName, 0).versionCode;
> > +                } catch (PackageManager.NameNotFoundException e) {
> > +                    Log.e(LOGTAG, "NameNotFoundException", e);
> 
> We get NameNotFonudException from the exception given to the log, this
> should be more descriptive (e.g. "Existing package, " +
> packageInfo.packageName + " was not found. Was it uninstalled?")

Good point.  I changed it to "couldn't get version for app", since that's what we're trying to do, and avoided suggesting a cause (which can't be that it was uninstalled, since the packageName in question was just given to us by PackageManager.getInstalledApplications).

I also changed the "packages" variable to "apps" and "packageInfo" to "app" to clarify their values, since getInstalledApplications actually returns a list of ApplicationInfo objects, and it was confusing to see a call to PackageManager.getPackageInfo that passed packageInfo.packageName into the method.

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


> @@ +2801,5 @@
> > +                }
> > +                try {
> > +                    jsonMessage.put(packageInfo.packageName, versionCode);
> > +                } catch (JSONException e) {
> > +                    Log.e(LOGTAG, "JSONException", e);
> 
> We get JSONException from the exception given to log. This should be more
> descriptive (e.g. "unable to store version code field".)

Yup, I changed it to this exact text.

https://github.com/mykmelez/gecko-dev/commit/1e23635


In addition to those changes, I made a variety of improvements to the usability and robustness of the download and update process, including additional error handling and more/better notifications to guide the user through the process.

That included adding progress meters to notifications about actions that are in progress, specifically checking for updates and downloading updates.  And the first of those actions is indeterminate, while the other can be determinate (given enough information about file sizes from the APK Factory service), but the algorithm for calculating progress in the latter case seems complex.  So I made both progress meters indeterminate for now, adding support for that to Notifications.jsm in the process.  (I'll file a followup bug to make the second meter determinate.)

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 8365774 [details] [diff] [review]
patch v3: addresses review comments and other issues

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

r=me for the dom/apps changes

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Just responding to comments, will try to give feedback first thing tomorrow (but it's probably not worth blocking on me if you want to land).

(In reply to Myk Melez [:myk] [@mykmelez] from comment #15)
> (In reply to Michael Comella (:mcomella) from comment #12)
> 
> > ::: mobile/android/base/GeckoAppShell.java
> > @@ +2774,5 @@
> > >              }
> > >          });
> > >      }
> > > +
> > > +    public static String getApkVersionNumbers(Activity context, JSONArray packageNames) {
> > 
> > nit: getAPKVersionNumbers
> 
> I'm ok with that, but it would be inconsistent with the other names in the
> file that have "apk" (installApks, apkPath), so I left it as-is.  But let me
> know if I should instead change all of them.

Honestly, we're inconsistent and do a bit a both. I think the general consensus from the people that care is to make it all capitalized, though I'm not sure what the consensus is on inconsistency. It's not *that* important so I wouldn't stop you from landing as is, but I would probably prefer it if they're all capitalized.

> The APK Factory service doesn't particularly expect 0, but it always
> generates APKs with positive integer versionCode values, so 0 is sufficient
> to ensure that the service will always return an update for an app whose
> versionCode is 0.

Fair enough. I think -1 is more explicit from a readability standpoint though that this is a junk value that the APK factory will never produce (rather than 0, which seems like it could be a valid version). 
 
> > Also, if this is consistently the same value, perhaps it should be a
> > constant.
> 
> Hmm, I might be missing something (and am not yet well-versed in Java), but
> I don't quite see how to make this "final" when we want to set it to 0 if
> retrieving the value throws an exception, given that final variables can
> only be initialized once.

I meant the initializing part, e.g. `int versionCode = INVALID_VERSION_CODE;` where INVALID_VERSION_CODE is `static final`. This would also help with the readability issue above.

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8365774 [details] [diff] [review]
patch v3: addresses review comments and other issues

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

Note: Only skimmed WebappManager.jsm, did not look at Webapps.jsm. I will look at the former more thoroughly next f?, if desired. :)

There are places where you can use the `final` keyword more frequently (e.g. EventListener.java), but it's not vital and a bit pedantic so I didn't mark them.

::: mobile/android/base/NotificationHelper.java
@@ +253,5 @@
>          }
>  
>          if (message.has(PROGRESS_VALUE_ATTR) &&
> +            message.has(PROGRESS_MAX_ATTR) &&
> +            message.has(PROGRESS_INDETERMINATE_ATTR)) {

This changes the behavior of adding a progress bar to the code because we'll skip on setting progress on anything with the indeterminate attribute - is this intentional? Does any other code expect this to work without the attribute?

::: mobile/android/base/webapp/EventListener.java
@@ +219,5 @@
>              }
>          });
>      }
> +
> +    public static String getApkVersions(Activity context, JSONArray packageNames) {

Why is this in EventListener? I think a Utils class is the best place for it, though I think it'd have to be a new file. `private static` in GeckoApp, where it's used, might work too.

@@ +245,5 @@
> +                }
> +                try {
> +                    jsonMessage.put(app.packageName, versionCode);
> +                } catch (JSONException e) {
> +                    Log.e(LOGTAG, "unable to store version code field", e);

nit: I'd include the app name here too.

@@ +250,5 @@
> +                }
> +            }
> +        }
> +
> +        return jsonMessage.toString();

nit: It's probably more correct to return the more generic object (i.e. the JSONObject) and have the caller convert it if they need to, particularly if this goes into a utils class.

::: mobile/android/base/webapp/UninstallListener.java
@@ +32,5 @@
>  
>      @Override
>      public void onReceive(Context context, Intent intent) {
> +        if (intent.getBooleanExtra(Intent.EXTRA_REPLACING, false)) {
> +            Log.i(LOGTAG, "Ignoring removal of package during update");

nit: The debug output does not seem clear to me, not knowing what EXTRA_REPLACING does. If I understand this correctly, I might prefer "Package is being replaced: ignoring removal intent".

::: mobile/android/chrome/content/aboutApps.js
@@ +10,5 @@
>  
>  Cu.import("resource://gre/modules/Services.jsm")
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/AppsUtils.jsm");
> +Cu.import("resource://gre/modules/WebappManager.jsm");

I don't really know how lazy loading works, but we might want to consider that - WebappManager is only used when clicked, which will not necessarily happen every page load, right? [1] looks to be the correct style.

[1]: https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=1a635b8dc7fa#90

::: mobile/android/components/WebappsUpdateTimer.js
@@ +13,5 @@
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/WebappManager.jsm");

nit: Alphabetic order.

@@ +24,5 @@
> +  // and breaks messages into lines automatically at display time (i.e. logcat).
> +  dump(message);
> +}
> +
> +function WebappsUpdateTimer() {}

I'm not sure how the XPCOMUtils work so I might be missing something, but this does not appear to be register with an nsITimer in this patch. Intentional (or am I wrong? :P)?

@@ +51,5 @@
> +
> +    log("network back online for webapp update check; commencing");
> +    // TODO: observe pref to do this only on wifi.
> +    Services.obs.removeObserver(this, "network:offline-status-changed");
> +    WebappManager.checkForUpdates();

Since we don't reset the timer here, we may end up calling checkForUpdates() twice in quick succession. Is that okay?

::: mobile/android/modules/WebappManager.jsm
@@ +73,3 @@
>    },
>  
> +  _installApk: function(aMessage, aMessageManager) { return Task.spawn((function*() {

nit: Knock the return statement down to the next line. The extra indentation makes it more obvious we're inside two functions.

@@ +83,5 @@
> +      aMessageManager.sendAsyncMessage("Webapps:Install:Return:KO", aMessage);
> +      log("error downloading APK: " + ex);
> +    }
> +
> +    sendMessageToJava({

I don't think we want to send this message even when the filePath may be invalid (that's not what the old code did).

On that note, we don't seem to do any File.exists checks on the file path in EventListener.installApk(...) - you might want to verify that is functioning as intended too.

@@ +88,5 @@
> +      type: "WebApps:InstallApk",
> +      filePath: filePath,
> +      data: JSON.stringify(aMessage),
> +    });
> +  }).bind(this)) },

nit: ; after ))

nit: Brace on the newline, with indent change above.

@@ +241,5 @@
>        }
>      });
>    },
>  
> +  _autoUpdate: function(aData, aOldApp) { return Task.spawn((function*() {

nit: Same indentation.

@@ +247,5 @@
> +
> +    // The data object has a manifestUrl property for the manifest URL,
> +    // but updateHostedApp expects it to be called manifestURL, and we pass
> +    // the data object to it, so we need to change the name.
> +    // TODO: rename this to manifestURL upstream, so the data object always has

Just making sure that you filed a bug! :)

@@ +277,5 @@
> +    this._checkingForUpdates = true;
> +
> +    try {
> +      let installedApps = yield this._getInstalledApps();
> +      if (installedApps.length == 0) {

nit: ===

@@ +301,5 @@
> +      }
> +
> +      let outdatedApps = yield this._getOutdatedApps(manifestUrlToApkVersion, userInitiated);
> +
> +      if (outdatedApps.length == 0) {

nit: ===

::: mobile/android/modules/WebappManagerWorker.js
@@ +25,5 @@
>  
>    request.onreadystatechange = function(event) {
>      log("onreadystatechange: " + request.readyState);
>  
> +    if (request.readyState != 4) {

nit: !==

@@ +34,2 @@
>  
> +    if (request.status == 200) {

nit: ===

@@ +35,5 @@
> +    if (request.status == 200) {
> +      postMessage({ type: "success" });
> +    } else {
> +      try {
> +        OS.File.remove(path);

It's unclear why this file is being removed.

Comment 19

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

Attachment: patch v4: address feedback

(In reply to Michael Comella (:mcomella) from comment #17)

> (In reply to Myk Melez [:myk] [@mykmelez] from comment #15)
> > (In reply to Michael Comella (:mcomella) from comment #12)
> > 
> > > ::: mobile/android/base/GeckoAppShell.java
> > > @@ +2774,5 @@
> > > >              }
> > > >          });
> > > >      }
> > > > +
> > > > +    public static String getApkVersionNumbers(Activity context, JSONArray packageNames) {
> > > 
> > > nit: getAPKVersionNumbers
> > 
> > I'm ok with that, but it would be inconsistent with the other names in the
> > file that have "apk" (installApks, apkPath), so I left it as-is.  But let me
> > know if I should instead change all of them.
> 
> Honestly, we're inconsistent and do a bit a both. I think the general
> consensus from the people that care is to make it all capitalized, though
> I'm not sure what the consensus is on inconsistency. It's not *that*
> important so I wouldn't stop you from landing as is, but I would probably
> prefer it if they're all capitalized.

I actually prefer all caps and would be happy to switch to it!

But I generally observe the Mozilla Coding Style Guide's admonition to "use the prevailing style in a file or module" for existing code <https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Naming_and_Formatting_code>.

And the Fennec code in webapp/, of which this is now a part, observes the Android Style Guide's instruction to "treat acronyms as words" <http://source.android.com/source/code-style.html#treat-acronyms-as-words>.

Perhaps it shouldn't have!  And maybe we should search and replace such acronyms with their all-caps equivalents.  But it seems better to do that separately and globally than locally in this patch.


> > The APK Factory service doesn't particularly expect 0, but it always
> > generates APKs with positive integer versionCode values, so 0 is sufficient
> > to ensure that the service will always return an update for an app whose
> > versionCode is 0.
> 
> Fair enough. I think -1 is more explicit from a readability standpoint
> though that this is a junk value that the APK factory will never produce
> (rather than 0, which seems like it could be a valid version). 

-1 is also a valid version code!  But my takeaway from your feedback is that it seems odd to an engineer looking at this code for the first time.  And the server is no more likely to generate an APK with that version.  So I changed this to -1 to eliminate the potential confusion.

https://github.com/mykmelez/gecko-dev/commit/4daeeb2


> > > Also, if this is consistently the same value, perhaps it should be a
> > > constant.
> > 
> > Hmm, I might be missing something (and am not yet well-versed in Java), but
> > I don't quite see how to make this "final" when we want to set it to 0 if
> > retrieving the value throws an exception, given that final variables can
> > only be initialized once.
> 
> I meant the initializing part, e.g. `int versionCode =
> INVALID_VERSION_CODE;` where INVALID_VERSION_CODE is `static final`. This
> would also help with the readability issue above.

Ah, of course.  Ok, I added this const, except that I called it DEFAULT_VERSION_CODE, since the value is valid.

https://github.com/mykmelez/gecko-dev/commit/51c40ba


(In reply to Michael Comella (:mcomella) from comment #18)

> Note: Only skimmed WebappManager.jsm, did not look at Webapps.jsm. I will
> look at the former more thoroughly next f?, if desired. :)

All feedback is welcome!


> There are places where you can use the `final` keyword more frequently (e.g.
> EventListener.java), but it's not vital and a bit pedantic so I didn't mark
> them.

Pedantry is especially welcome!


> ::: mobile/android/base/NotificationHelper.java
> @@ +253,5 @@
> >          }
> >  
> >          if (message.has(PROGRESS_VALUE_ATTR) &&
> > +            message.has(PROGRESS_MAX_ATTR) &&
> > +            message.has(PROGRESS_INDETERMINATE_ATTR)) {
> 
> This changes the behavior of adding a progress bar to the code because we'll
> skip on setting progress on anything with the indeterminate attribute - is
> this intentional? Does any other code expect this to work without the
> attribute?

Yes, this is intentional, as no other code uses this API; Notifications.jsm is its only consumer.


> ::: mobile/android/base/webapp/EventListener.java
> @@ +219,5 @@
> >              }
> >          });
> >      }
> > +
> > +    public static String getApkVersions(Activity context, JSONArray packageNames) {
> 
> Why is this in EventListener? I think a Utils class is the best place for
> it, though I think it'd have to be a new file. `private static` in GeckoApp,
> where it's used, might work too.

Indeed, it would make more sense in a Utils class.  But EventListener is already a grabbag of webapp-related event handlers, so it fits with the other methods in that file.  It's a ripe target for refactoring, but perhaps something we can do en masse in a separate change?


> @@ +245,5 @@
> > +                }
> > +                try {
> > +                    jsonMessage.put(app.packageName, versionCode);
> > +                } catch (JSONException e) {
> > +                    Log.e(LOGTAG, "unable to store version code field", e);
> 
> nit: I'd include the app name here too.

Sure.

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


> @@ +250,5 @@
> > +                }
> > +            }
> > +        }
> > +
> > +        return jsonMessage.toString();
> 
> nit: It's probably more correct to return the more generic object (i.e. the
> JSONObject) and have the caller convert it if they need to, particularly if
> this goes into a utils class.

Indeed!

https://github.com/mykmelez/gecko-dev/commit/83d59e1


> ::: mobile/android/base/webapp/UninstallListener.java
> @@ +32,5 @@
> >  
> >      @Override
> >      public void onReceive(Context context, Intent intent) {
> > +        if (intent.getBooleanExtra(Intent.EXTRA_REPLACING, false)) {
> > +            Log.i(LOGTAG, "Ignoring removal of package during update");
> 
> nit: The debug output does not seem clear to me, not knowing what
> EXTRA_REPLACING does. If I understand this correctly, I might prefer
> "Package is being replaced: ignoring removal intent".

Sure!  I made it that, but with a semi-colon to separate the clauses.

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


> ::: mobile/android/chrome/content/aboutApps.js
> @@ +10,5 @@
> >  
> >  Cu.import("resource://gre/modules/Services.jsm")
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> >  Cu.import("resource://gre/modules/AppsUtils.jsm");
> > +Cu.import("resource://gre/modules/WebappManager.jsm");
> 
> I don't really know how lazy loading works, but we might want to consider
> that - WebappManager is only used when clicked, which will not necessarily
> happen every page load, right? [1] looks to be the correct style.
> 
> [1]:
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.
> js?rev=1a635b8dc7fa#90

Good catch, although defineLazyModuleGetter is actually better in this case, f.e.:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=1a635b8dc7fa#14

https://github.com/mykmelez/gecko-dev/commit/4fee21d


> ::: mobile/android/components/WebappsUpdateTimer.js
> @@ +13,5 @@
> > +const Cu = Components.utils;
> > +
> > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +Cu.import("resource://gre/modules/WebappManager.jsm");
> 
> nit: Alphabetic order.

You bet!

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


> @@ +24,5 @@
> > +  // and breaks messages into lines automatically at display time (i.e. logcat).
> > +  dump(message);
> > +}
> > +
> > +function WebappsUpdateTimer() {}
> 
> I'm not sure how the XPCOMUtils work so I might be missing something, but
> this does not appear to be register with an nsITimer in this patch.
> Intentional (or am I wrong? :P)?

It's quite non-obvious!  But this component is registered as a timer by this declaration in MobileComponents.manifest:

category update-timer WebappsUpdateTimer @mozilla.org/b2g/webapps-update-timer;1,getService,background-update-timer,browser.webapps.updateInterval,86400

B2G also does it this way, and we reuse its contract ID.  But over in bug 898647 we register the equivalent timer for the desktop runtime programmatically via nsIUpdateTimerManager, and maybe we should do that here as well, since it would be simpler and easier to grok.


> @@ +51,5 @@
> > +
> > +    log("network back online for webapp update check; commencing");
> > +    // TODO: observe pref to do this only on wifi.
> > +    Services.obs.removeObserver(this, "network:offline-status-changed");
> > +    WebappManager.checkForUpdates();
> 
> Since we don't reset the timer here, we may end up calling checkForUpdates()
> twice in quick succession. Is that okay?

I think it's ok.  It's possible for checkForUpdates to be called twice in quick succession, but the user would have to go online right before the next timer notification after having been offline for about a day, and WebappManager.checkForUpdates will ignore a call that happens while an update check is in progress anyway, so the likelihood of two update checks happening back-to-back seems small.


> ::: mobile/android/modules/WebappManager.jsm
> @@ +73,3 @@
> >    },
> >  
> > +  _installApk: function(aMessage, aMessageManager) { return Task.spawn((function*() {
> 
> nit: Knock the return statement down to the next line. The extra indentation
> makes it more obvious we're inside two functions.

After discussions this month with Task principals Paolo and Yoric, I'm adding sugar over in bug 966182 to simplify this common pattern, after which I'll refactor occurrences to use the sugar, turning this into:

  _installapk: Task.method(function*(aMessage, aMessageManager) {
    …
  }),

And writing it this funky way here means that fewer lines of code will have misleading indentation-only changes in blame after the refactoring.


> @@ +83,5 @@
> > +      aMessageManager.sendAsyncMessage("Webapps:Install:Return:KO", aMessage);
> > +      log("error downloading APK: " + ex);
> > +    }
> > +
> > +    sendMessageToJava({
> 
> I don't think we want to send this message even when the filePath may be
> invalid (that's not what the old code did).

Erm, indeed, nice catch!

https://github.com/mykmelez/gecko-dev/commit/170ffdd


> On that note, we don't seem to do any File.exists checks on the file path in
> EventListener.installApk(...) - you might want to verify that is functioning
> as intended too.

You bet.

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


> @@ +88,5 @@
> > +      type: "WebApps:InstallApk",
> > +      filePath: filePath,
> > +      data: JSON.stringify(aMessage),
> > +    });
> > +  }).bind(this)) },
> 
> nit: ; after ))

Rightio.

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


> nit: Brace on the newline, with indent change above.

Per above, I'll avoid this to reduce misleading indentation changes in blame.


> @@ +241,5 @@
> >        }
> >      });
> >    },
> >  
> > +  _autoUpdate: function(aData, aOldApp) { return Task.spawn((function*() {
> 
> nit: Same indentation.

As above.


> @@ +247,5 @@
> > +
> > +    // The data object has a manifestUrl property for the manifest URL,
> > +    // but updateHostedApp expects it to be called manifestURL, and we pass
> > +    // the data object to it, so we need to change the name.
> > +    // TODO: rename this to manifestURL upstream, so the data object always has
> 
> Just making sure that you filed a bug! :)

I usually do this in conjunction with the commission of the patch.  Before then, anything can happen!  Like a review that determines that it needs to be to-done already. ;-)


> @@ +277,5 @@
> > +    this._checkingForUpdates = true;
> > +
> > +    try {
> > +      let installedApps = yield this._getInstalledApps();
> > +      if (installedApps.length == 0) {
> 
> nit: ===
> 
> @@ +301,5 @@
> > +      }
> > +
> > +      let outdatedApps = yield this._getOutdatedApps(manifestUrlToApkVersion, userInitiated);
> > +
> > +      if (outdatedApps.length == 0) {
> 
> nit: ===
> 
> ::: mobile/android/modules/WebappManagerWorker.js
> @@ +25,5 @@
> >  
> >    request.onreadystatechange = function(event) {
> >      log("onreadystatechange: " + request.readyState);
> >  
> > +    if (request.readyState != 4) {
> 
> nit: !==
> 
> @@ +34,2 @@
> >  
> > +    if (request.status == 200) {
> 
> nit: ===

All done (plus a fifth one I found).

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


> @@ +35,5 @@
> > +    if (request.status == 200) {
> > +      postMessage({ type: "success" });
> > +    } else {
> > +      try {
> > +        OS.File.remove(path);
> 
> It's unclear why this file is being removed.

If the download failed, a partially-downloaded file may nevertheless remain, and we should remove it to avoid littering the filesystem with useless files.

Comment 20

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

Additional todos to file followup bugs about:

* animate "checking" and "downloading" notification icons
* don't assume update check server returned a valid response

Comment 21

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

Attachment: just the localization strings

Yesterday mfinkle suggested I separate the localization strings in the patch so we can land them in advance on the off chance the patch doesn't get committed before the merge but is a candidate for uplift to Aurora.  That way we wouldn't have to go through a late-l10n process (provided subsequent review didn't turn up problems with the strings, of course).

So here's that separate patch with just the localization strings.  mfinkle, perhaps you can review this portion of the patch?

Comment 22

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

Attachment: patch v5: resolves trivial conflict with recent commit

This is identical to v4 except that it resolves a trivial conflict with the fix for bug 963898, which I landed on Inbound yesterday and just got merged to Central.

Comment 23

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

https://tbpl.mozilla.org/?tree=Try&rev=654dc69f5255

Comment 24

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

Comment on attachment 8368712 [details] [diff] [review]
just the localization strings

These look fine, but we need to change one thing: We don't use periods '.' in system notifications (title or message). It's kind of an Android convention. We only use periods '.' for multiple sentences.

r+ but remove the periods.

Comment 25

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

Attachment: localization strings without periods

(In reply to Mark Finkle (:mfinkle) from comment #24)
> These look fine, but we need to change one thing: We don't use periods '.'
> in system notifications (title or message). It's kind of an Android
> convention. We only use periods '.' for multiple sentences.

Ok, here's a version without periods, although note that the Android update notifications (on which these are based) end messages with periods if they form complete sentences, in particular the one that prompts you to update one or more apps:

  Touch to update Foo.
  Touch to update Foo, Bar and Baz.

(Some single-sentence Fennec messages do too, like "Test the latest Nightly build."  But presumably those are aberrations.)

In any case, this is the version I'll land.  Uploading it again so I can point sheriffs to the exact patch I landed on Inbound.

Comment 26

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

Attachment: DOMApplicationRegistry changes

I will also land the DOMApplicationRegistry changes separately, since they already have review, and they are discrete and separable refactoring changes.  That way we reduce the risk of merge conflicts during a busy tree season.

Comment 27

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

Comment on attachment 8368863 [details] [diff] [review]
localization strings without periods

https://hg.mozilla.org/integration/mozilla-inbound/rev/26bc2b4b1605

Comment 28

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

Comment on attachment 8368871 [details] [diff] [review]
DOMApplicationRegistry changes

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a46630fe387

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/26bc2b4b1605
https://hg.mozilla.org/mozilla-central/rev/7a46630fe387

Comment 30

[Wesley Johnston (:wesj)]

Comment on attachment 8368722 [details] [diff] [review]
patch v5: resolves trivial conflict with recent commit

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

::: mobile/android/base/GeckoApp.java
@@ +641,5 @@
>  
>                  // preInstallWebapp will return a File object pointing to the profile directory of the webapp
>                  mCurrentResponse = EventListener.preInstallWebApp(name, manifestURL, origin).toString();
> +            } else if (event.equals("WebApps:GetApkVersions")) {
> +                mCurrentResponse = EventListener.getApkVersions(this, message.getJSONArray("packageNames")).toString();

Move this out of GeckoApp.

::: mobile/android/base/NotificationHelper.java
@@ +259,5 @@
>              try {
>                  final int progress = message.getInt(PROGRESS_VALUE_ATTR);
>                  final int progressMax = message.getInt(PROGRESS_MAX_ATTR);
> +                final boolean progressIndeterminate = message.getBoolean(PROGRESS_INDETERMINATE_ATTR);
> +                builder.setProgress(progressMax, progress, progressIndeterminate);

I think I'd rather just use a negative progress value for this, instead of a separate js val for indeterminate.

::: mobile/android/base/webapp/InstallListener.java
@@ +58,5 @@
>          } else if (!isCorrectManifest(manifestUrl)) {
> +            // This happens when the updater triggers installation of multiple
> +            // APK updates simultaneously.  If we're the receiver for another
> +            // update, then simply ignore this intent by returning early.
> +            Log.i(LOGTAG, "Waiting to finish installing " + mManifestUrl + " but this is " + manifestUrl + "; ignoring");

You may want to be careful about exposing url's in the log like this.

::: mobile/android/modules/WebappManager.jsm
@@ +353,5 @@
> +        title: Strings.GetStringFromName("checkingForUpdatesTitle"),
> +        message: Strings.GetStringFromName("checkingForUpdatesMessage"),
> +        // TODO: replace this with an animated icon.
> +        icon: "drawable://alert_app",
> +        progress: NaN,

Or we could use NaN for this...

@@ +392,5 @@
> +             replace("#1", aApps.length),
> +      message: Strings.formatStringFromName("downloadingUpdateMessage", [downloadingNames], 1),
> +      // TODO: replace this with an animated icon.  UpdateService uses
> +      // android.R.drawable.stat_sys_download, but I don't think we can reference
> +      // a system icon with a drawable: URL here, so we'll have to craft our own.

We actually avoided this for awhile because I find it annoying :) But we're moving to use the system service, which would use it. Or we could expose these.

Comment 31

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8368722 [details] [diff] [review]
patch v5: resolves trivial conflict with recent commit

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

Read through Webapps.jsm, but I'm not familiar enough with the code to double-check the logic without more time than I have. Unless you *really* want me to, I'm going to pass on looking at WebappManager.jsm again and defer to Wes because I would need to spend a significant amount of time here to get a thorough review.

::: dom/apps/src/Webapps.jsm
@@ +1651,5 @@
>  
> +    // If the app is packaged and its manifestURL has an app:// scheme,
> +    // then we can't have an update.
> +    if (app.origin.startsWith("app://") &&
> +        app.manifestURL.startsWith("app://")) {

I would prefer `app.origin.startsWith...` to go into a function, "isAppPackaged(app)" that wraps that call - it'd be much more intuitive to devs who don't know what makes a package app (having the origin start with "app://" is the criteria, yes? :).

Additionally, then you can drop then comment above (or place a `// Then we can't update` inside the if statement).

@@ +1884,5 @@
>         }
>       }
>    },
>  
> +  updatePackagedApp: function(aData, aId, aApp, aNewManifest) {

nit: Not sure the consensus on variable naming and such but aID. Also, what kind of ID is it? The app ID? Perhaps it should be aAppID.

@@ +1889,5 @@
> +    debug("updatePackagedApp");
> +
> +    // Store the new update manifest.
> +    let dir = this._getAppDir(aId).path;
> +    let manFile = OS.Path.join(dir, "staged-update.webapp");

nit: -> manifestFile. I thought manFile was manualFile.

@@ +1917,5 @@
> +  // for changes in the app cache.
> +  // 'aNewManifest' would contain the updated app manifest if
> +  // it has actually been updated, while 'aOldManifest' contains the
> +  // stored app manifest.
> +  updateHostedApp: function(aData, aId, aApp, aOldManifest, aNewManifest) {

nit: I'd do javadoc style comments.

nit: Rename aID if you did so above.

@@ +1922,5 @@
> +    debug("updateHostedApp " + aData.manifestURL);
> +
> +    // Clean up the deprecated manifest cache if needed.
> +    if (aId in this._manifestCache) {
> +      delete this._manifestCache[aId];

delete seems to work even if the attribute is not present (i.e. remove the `if`).

@@ +1933,5 @@
> +      this.updateAppHandlers(aOldManifest, aNewManifest, aApp);
> +
> +      // Store the new manifest.
> +      let dir = this._getAppDir(aId).path;
> +      let manFile = OS.Path.join(dir, "manifest.webapp");

nit: manFile again.

@@ +1980,5 @@
> +          observe: function(aSubject, aTopic, aObsData) {
> +            debug("updateHostedApp: updateSvc.checkForUpdate return for " +
> +                  aApp.manifestURL + " - event is " + aTopic);
> +            let eventType =
> +              aTopic == "offline-cache-update-available" ? "downloadavailable"

nit: ===

@@ +1982,5 @@
> +                  aApp.manifestURL + " - event is " + aTopic);
> +            let eventType =
> +              aTopic == "offline-cache-update-available" ? "downloadavailable"
> +                                                         : "downloadapplied";
> +            aApp.downloadAvailable = (eventType == "downloadavailable");

nit: ===

@@ +2000,5 @@
> +        };
> +        debug("updateHostedApp: updateSvc.checkForUpdate for " +
> +              manifest.fullAppcachePath());
> +        updateSvc.checkForUpdate(Services.io.newURI(manifest.fullAppcachePath(), null, null),
> +                                 aApp.localId, false, updateObserver);

nit: Perhaps it's just splinter, but this indentation looks funny.

Comment 32

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

Attachment: patch v6: addresses review comments

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

> ::: mobile/android/base/GeckoApp.java
> @@ +641,5 @@
> >  
> >                  // preInstallWebapp will return a File object pointing to the profile directory of the webapp
> >                  mCurrentResponse = EventListener.preInstallWebApp(name, manifestURL, origin).toString();
> > +            } else if (event.equals("WebApps:GetApkVersions")) {
> > +                mCurrentResponse = EventListener.getApkVersions(this, message.getJSONArray("packageNames")).toString();
> 
> Move this out of GeckoApp.

Done for both GetApkVersions and PreInstall (which he'd previously requested we move, although we didn't understand how to do it at the time).

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


> ::: mobile/android/base/NotificationHelper.java
> @@ +259,5 @@
> >              try {
> >                  final int progress = message.getInt(PROGRESS_VALUE_ATTR);
> >                  final int progressMax = message.getInt(PROGRESS_MAX_ATTR);
> > +                final boolean progressIndeterminate = message.getBoolean(PROGRESS_INDETERMINATE_ATTR);
> > +                builder.setProgress(progressMax, progress, progressIndeterminate);
> 
> I think I'd rather just use a negative progress value for this, instead of a
> separate js val for indeterminate.

WesJ and I chatted about this over IRC and agreed to leave it as-is so NotificationHelper's API is aligned with the Android API it wraps.


> ::: mobile/android/base/webapp/InstallListener.java
> @@ +58,5 @@
> >          } else if (!isCorrectManifest(manifestUrl)) {
> > +            // This happens when the updater triggers installation of multiple
> > +            // APK updates simultaneously.  If we're the receiver for another
> > +            // update, then simply ignore this intent by returning early.
> > +            Log.i(LOGTAG, "Waiting to finish installing " + mManifestUrl + " but this is " + manifestUrl + "; ignoring");
> 
> You may want to be careful about exposing url's in the log like this.

Ok, I've removed the URLs from the message.

https://github.com/mykmelez/gecko-dev/commit/7647aca


> ::: mobile/android/modules/WebappManager.jsm
> @@ +353,5 @@
> > +        title: Strings.GetStringFromName("checkingForUpdatesTitle"),
> > +        message: Strings.GetStringFromName("checkingForUpdatesMessage"),
> > +        // TODO: replace this with an animated icon.
> > +        icon: "drawable://alert_app",
> > +        progress: NaN,
> 
> Or we could use NaN for this...

We can use NaN in JavaScript, but we can't pass the value through to NotificationHelper, unfortunately, because NaN is not a valid JSON value.


> @@ +392,5 @@
> > +             replace("#1", aApps.length),
> > +      message: Strings.formatStringFromName("downloadingUpdateMessage", [downloadingNames], 1),
> > +      // TODO: replace this with an animated icon.  UpdateService uses
> > +      // android.R.drawable.stat_sys_download, but I don't think we can reference
> > +      // a system icon with a drawable: URL here, so we'll have to craft our own.
> 
> We actually avoided this for awhile because I find it annoying :) But we're
> moving to use the system service, which would use it. Or we could expose
> these.

Ok, no worries, I'll file the followup bug in any case, and then we can decide what to do.


(In reply to Michael Comella (:mcomella) from comment #31)

> Read through Webapps.jsm, but I'm not familiar enough with the code to
> double-check the logic without more time than I have. Unless you *really*
> want me to, I'm going to pass on looking at WebappManager.jsm again and
> defer to Wes because I would need to spend a significant amount of time here
> to get a thorough review.

Thanks for the feedback!  I agree with all your nits (and how!), but… the Webapps.jsm changes are actually refactoring-only changes, in which I intentionally didn't modify any of the code I moved except to the minimal extent necessary to move it, and I reused the prevailing style for those minimal modifications (hence aId, even though I would be thrilled to make it aID).

Also, the Webapps.jsm changes have already landed over in <https://hg.mozilla.org/mozilla-central/rev/7a46630fe387>.  So I didn't make these changes, but I will keep them in mind in any future Webapps.jsm refactoring work (of which I'm sure there will be plenty!).


> @@ +2000,5 @@
> > +        };
> > +        debug("updateHostedApp: updateSvc.checkForUpdate for " +
> > +              manifest.fullAppcachePath());
> > +        updateSvc.checkForUpdate(Services.io.newURI(manifest.fullAppcachePath(), null, null),
> > +                                 aApp.localId, false, updateObserver);
> 
> nit: Perhaps it's just splinter, but this indentation looks funny.

If you zoom out enough via repeated invocations of View > Zoom > Zoom Out, you'll see that it's just Splinter!


This is the version I'll commit.

Comment 33

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/17aaa1bc6625

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/17aaa1bc6625

Comment 35

[Sylvestre Ledru [:Sylvestre]]

Myk, you want these changes to be uplifted in aurora/29?

Comment 36

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #35)
> Myk, you want these changes to be uplifted in aurora/29?

Yes, I'd like to uplift (most of) these changes to aurora/29.  The automated updates implementation in this bug is part of the Synthetic APKs MVP, which is targeted to Fx29, and it's the only part of the MVP that didn't land before the Fx29 Central -> Aurora merge.  Uplifting it would complete that work for Fx29.

I don't want to uplift the manual updates implementation, however, as it isn't part of the MVP and contains a new string that would trigger late-l10n.  I'll attach a patch containing the specific set of changes I'd like to uplift.

Comment 37

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

Attachment: patch for uplift to aurora

Here's the patch I'd like to uplift to Aurora.  It's identical to patch v6 except that it excludes the "manual updates" changes in the following files:

  mobile/android/themes/core/jar.mn                 (button icon entry)
  mobile/android/themes/core/images/update.png      (button icon)
  mobile/android/chrome/content/aboutApps.js        (event handler)
  mobile/android/chrome/content/aboutApps.xhtml     (button)
  mobile/android/locales/en-US/chrome/aboutApps.dtd (button label)


[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  This is the last piece of the Synthetic APKs feature rather than a bug fix.
User impact if declined:
  Users of webapps will not receive automatic updates for their apps in Fx29.
Testing completed (on m-c, etc.):
  The patch has been in Central for five days, and AaronMT has tested it.
Risk to taking this patch (and alternatives if risky):
  The changes themselves are not particularly risky, although the patch size
  is substantial.
String or IDL/UUID changes made by this patch:
  None.  The patch does add a .properties file to the list of files included
  in a Fennec build, but the .properties file in question was already committed
  to Central for Fx29.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8e0acb91d0ac