970200 - let users restrict webapp updates to wifi connections

Status: RESOLVED FIXED

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

Description

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

Over in bug 934760, wesj noted:

> ::: 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...

Settings > Customize > "Download updates automatically" is vague enough to apply to webapps as well, although I'm unsure that's what users would expect.  But let's start by applying the existing pref to webapp updates as well, then gather feedback and use it to figure out whether or not we should create a webapp-specific pref.

Comment 1

[Martyn Haigh (:mhaigh)]

Attachment: let users restrict webapp updates to wifi connections

I believe this will suffice but I can't get the code to run.  
I've altered the value of the browser.webapps.updateInterval to a low number (10) but can't see any evidence of webappsUpdateTimer.js actually being called.

Any ideas?

Comment 2

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

Comment on attachment 8379674 [details] [diff] [review]
let users restrict webapp updates to wifi connections

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

::: mobile/android/base/webapp/EventListener.java
@@ +112,4 @@
>  
>                  // preInstallWebapp will return a File object pointing to the profile directory of the webapp
>                  mCurrentResponse = preInstallWebapp(name, manifestURL, origin).toString();
> +            } else if (event.equals("Webapps:GetApkVersions")) {

This looks left over from another change.  Merge detritus?

::: mobile/android/components/WebappsUpdateTimer.js
@@ +59,5 @@
> +
> +    if (!this._updateAllowed()) {
> +      return;
> +    }
> +    

Nit: trailing whitespace.

@@ +69,5 @@
> +    return (network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_WIFI || network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_ETHERNET);
> +  },
> +
> +  _updateAllowed: function() {
> +    let autoUpdatePref = Services.prefs.getChardPref("app.update.autodownload");

getChardPref -> getCharPref

@@ +70,5 @@
> +  },
> +
> +  _updateAllowed: function() {
> +    let autoUpdatePref = Services.prefs.getChardPref("app.update.autodownload");
> +    let updateAllowed = (autoUpdatePref == "Always") || 

Nit: trailing whitespace.

@@ +71,5 @@
> +
> +  _updateAllowed: function() {
> +    let autoUpdatePref = Services.prefs.getChardPref("app.update.autodownload");
> +    let updateAllowed = (autoUpdatePref == "Always") || 
> +                        (autoUpdatePref != "Never" && _usingLAN());

I might misunderstand how Android preferences work, but it looks like the values to which the pref can be set are the ones in the android:entryValues array, which are "enabled", "wifi", and "disabled".  So "Always" -> "enabled" and "Never" -> "disabled.

But I would also invert the "Never" conditional so it checks for == "wifi", which is more explicit and robust against changes to the value set in the future.

@@ +72,5 @@
> +  _updateAllowed: function() {
> +    let autoUpdatePref = Services.prefs.getChardPref("app.update.autodownload");
> +    let updateAllowed = (autoUpdatePref == "Always") || 
> +                        (autoUpdatePref != "Never" && _usingLAN());
> +    return updateAllowed;

Nit: since you only use updateAllowed once, you might as well simply return the value to which it gets set instead of defining a variable and then immediately returning it.

::: mobile/android/modules/WebappManager.jsm
@@ +297,4 @@
>        let manifestUrlToApkVersion = {};
>        let manifestUrlToApp = {};
>        for (let app of installedApps) {
> +        manifestUrlToApkVersion[app.manifestURL] = apkNameToVersion[app.apkPackageName] || 0;

These too look left over from another change.  More merge detritus?

Comment 3

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

(In reply to Martyn Haigh (:mhaigh) from comment #1)
> I believe this will suffice but I can't get the code to run.  
> I've altered the value of the browser.webapps.updateInterval to a low number
> (10) but can't see any evidence of webappsUpdateTimer.js actually being
> called.

The mechanics of update timers are complex:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateTimerManager.js

So it's hard to get them to fire on such a predictable interval.  Although the unit tests may do it:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/unit_timermanager/consumerNotifications.js#139

You could also use the Browser Console to load the component and fake a notification, something like:

Cc[@mozilla.org/b2g/webapps-update-timer;1].getService(Ci.nsITimerCallback).notify();

(You might have to pass an nsITimer to "notify", although it might be sufficient to simply pass |null| or an object literal.)

Comment 4

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

Self: please provide info on how to successfully trigger an "automated" update check!

Comment 5

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

Erm, I'm still on the hook to provide info on how to successfully trigger an "automated" update check.

Comment 6

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

(In reply to Myk Melez [:myk] [@mykmelez] from comment #5)
> Erm, I'm still on the hook to provide info on how to successfully trigger an
> "automated" update check.

The way to do this is indeed what I had earlier suggested (with minor fixes): enable remote debugging in Fennec, forward port 6000 on your desktop, connect Firefox remote tools to Fennec's "Main Process", and evaluate the following statement in the remote Browser Console:

  Cc["@mozilla.org/b2g/webapps-update-timer;1"].getService(Ci.nsITimerCallback).notify({});

But bug 985184 (and possibly also bug 985195) currently prevent this from working!  So you'll need to build Fennec with the patches in those bugs applied (or wait for them to land) in order for this to work.

Comment 7

[Martyn Haigh (:mhaigh)]

Attachment: check against user general update settings for webapp updates

Comment 8

[Martyn Haigh (:mhaigh)]

The actual call which worked in the console was :

Cc["@mozilla.org/webapps-update-timer;1"].getService(Ci.nsITimerCallback).notify({});

Comment 9

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

(In reply to Martyn Haigh (:mhaigh) from comment #8)
> The actual call which worked in the console was :
> 
> Cc["@mozilla.org/webapps-update-timer;1"].getService(Ci.nsITimerCallback).
> notify({});

Erm, of course, I recently changed that contract ID!

Comment 10

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

Comment on attachment 8397842 [details] [diff] [review]
check against user general update settings for webapp updates

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

Other than the nit, this looks good, except that I've been researching the behavior of app.update.autodownload, as implemented by UpdateService.java, and it turns out that it's supposed to govern downloading of the updates, not checking for updates, which is always enabled.

And we should do the same: always check for updates, then use the value of the pref to determine whether or not to automatically download them.  I think this means we'll want to move this check to WebappManager.jsm.

::: mobile/android/components/WebappsUpdateTimer.js
@@ +56,2 @@
>      log("network back online for webapp update check; commencing");
>      // TODO: observe pref to do this only on wifi.

Nit: remove this TODO now that we're to-doing it.

Comment 11

[Martyn Haigh (:mhaigh)]

Attachment: check against user general update settings for webapp updates

Comment 12

[Martyn Haigh (:mhaigh)]

One concern I have is that there is no user feedback when an update doesn't happen.  This feels alright if it's an automated update but when the user has explicitly requested an update we should tell them that we aren't updating because of their update connectivity settings.

Comment 13

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

Comment on attachment 8411824 [details] [diff] [review]
check against user general update settings for webapp updates

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

(In reply to Martyn Haigh (:mhaigh) from comment #12)
> One concern I have is that there is no user feedback when an update doesn't
> happen.  This feels alright if it's an automated update but when the user
> has explicitly requested an update we should tell them that we aren't
> updating because of their update connectivity settings.

Indeed, and there's a userInitiated boolean in checkForUpdates that we can use to modify behavior based on whether or not the user has manually requested an update check.

But we shouldn't need to use that here, because this pref shouldn't actually prevent an update from happening, it should just determine whether or not an available update gets downloaded automatically.

If the pref is set to "enabled", or it's set to "wifi" and the user is on a broadband connection, then a found update should be downloaded automatically; otherwise, the user should be prompted to download the update.

In other words, it should only affect step 3 of this outline of the overall update process:

1. check for updates (either automatically or by user request);
2. find an update;
3. possibly prompt user to download update (depending on pref);
4. download update;
5. prompt user to install update;
6. install update.

::: mobile/android/modules/WebappManager.jsm
@@ +323,5 @@
> +               (autoUpdatePref == "wifi" && usingLAN());
> +      }
> +
> +      if (!updateAllowed()) {
> +        dump("Webapp update not allowed");

Nit: use "debug" instead of "dump".

Comment 14

[Martyn Haigh (:mhaigh)]

Attachment: check against user general update settings for webapp updates

Updated according to last feedback with a corrected flow as suggested.

Comment 15

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

Comment on attachment 8416548 [details] [diff] [review]
check against user general update settings for webapp updates

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

Looks great and works as expected!  Just a few nits.  Let's get mfinkle to review it.

::: mobile/android/modules/WebappManager.jsm
@@ +311,4 @@
>          return;
>        }
>  
> +      let usingLAN = function() {

Nit: call this usingLan per the default style in this file.

@@ +312,5 @@
>        }
>  
> +      let usingLAN = function() {
> +        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);

Nit: line lengths in Fennec code are flexible, but this one is really long!  I know you're just copying it from elsewhere, but it'd be nice to shorten it a bit for readability.

Pro-tip: XPCOM interface constants are defined on both the interface and its instances, so this line could be written as:

      return (network.linkType == network.LINK_TYPE_WIFI || network.linkType == network.LINK_TYPE_ETHERNET);

Or simply leave it as-is but wrap it:

      return (network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_WIFI ||
              network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_ETHERNET);

@@ +320,5 @@
> +        let autoUpdatePref = Services.prefs.getCharPref("app.update.autodownload");
> +
> +        return (autoUpdatePref == "enabled") ||
> +               (autoUpdatePref == "wifi" && usingLAN());
> +      }

Nit: close the usingLAN and updateAllowed assignments with semicolons, since the functions are expressions <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/function> rather than statements <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function>.

Also, the return statement here has the opposite style nit of the one in usingLAN: it's fairly short but is nevertheless wrapped, which seems unnecessary.

Comment 16

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

Comment on attachment 8416548 [details] [diff] [review]
check against user general update settings for webapp updates

Nice. Just make Myk's suggested changes and I am fine with it.

Comment 17

[Martyn Haigh (:mhaigh)]

Attachment: check against user general update settings for webapp updates

Nits addressed

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Do you have a link to a recent Try run for this?

Comment 19

[Martyn Haigh (:mhaigh)]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

What are you asking?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/9fb35ece0bde

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/9fb35ece0bde