Last Comment Bug 786380 - Write new Android service-based updater
: Write new Android service-based updater
Status: VERIFIED FIXED
[Updater]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: Firefox 18
Assigned To: James Willcox (:snorp) (jwillcox@mozilla.com)
: Kevin Brosnan [:kbrosnan]
Mentors:
Depends on: 802118 802364 808297 789791 791475 792992 793276 794642 802396 866487
Blocks: 723365 750077 692827 714739 729780 774432 791387
  Show dependency treegraph
 
Reported: 2012-08-28 11:15 PDT by James Willcox (:snorp) (jwillcox@mozilla.com)
Modified: 2016-07-29 14:29 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified


Attachments
Implement new Java-based updater for Android (60.51 KB, patch)
2012-08-28 11:17 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
no flags Details | Diff | Splinter Review
Implement new Java-based updater for Android (61.01 KB, patch)
2012-08-29 07:03 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
cpeterson: review+
Details | Diff | Splinter Review
Implement new Java-based updater for Android (61.38 KB, patch)
2012-08-29 13:11 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
snorp: review+
mark.finkle: review-
Details | Diff | Splinter Review
Implement new Java-based updater for Android (60.95 KB, patch)
2012-08-29 13:24 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
no flags Details | Diff | Splinter Review
Implement new Java-based updater for Android (77.69 KB, patch)
2012-08-30 10:39 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
mark.finkle: review+
Details | Diff | Splinter Review
Implement new Java-based updater for Android (77.05 KB, patch)
2012-08-31 06:30 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
snorp: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description James Willcox (:snorp) (jwillcox@mozilla.com) 2012-08-28 11:15:22 PDT
The current updater for Aurora/Nightly suffers from some issues which would be easily solved by implementing our own updater as an Android service. Some of the advantages are:

* Schedule update checks to be performed in the background automatically instead of relying on the application to start

* Automatically download updates in the background

* Apply updates without having to launch the entire application

* Fix many bugs around state management that exist with the current updater
Comment 1 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-08-28 11:17:19 PDT
Created attachment 656115 [details] [diff] [review]
Implement new Java-based updater for Android
Comment 2 Chris Peterson [:cpeterson] 2012-08-28 14:05:00 PDT
Comment on attachment 656115 [details] [diff] [review]
Implement new Java-based updater for Android

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

Awesome! I can't wait!

::: mobile/android/app/mobile.js
@@ +501,5 @@
>  pref("app.update.url", "https://aus2.mozilla.org/update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%-@MOZ_PKG_SPECIAL@/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%PLATFORM_VERSION%/update.xml");
>  #else
>  pref("app.update.url", "https://aus2.mozilla.org/update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%PLATFORM_VERSION%/update.xml");
>  #endif
> +*/

Why comment out this code instead of removing it?

::: mobile/android/base/GeckoApp.java
@@ +1704,5 @@
>  
>          GeckoNetworkManager.getInstance().init();
>          GeckoNetworkManager.getInstance().start();
>  
> +        UpdateServiceHelper.registerForUpdates(this);

Will registering for updates here cause us to register an additional repeating alarm every time Firefox is launched?

::: mobile/android/base/UpdateService.java
@@ +46,5 @@
> +import org.w3c.dom.NodeList;
> +
> +import java.util.Date;
> +
> +import android.util.Log;

Google's Android style guide [1] recommends that imports be grouped by namespace (project-specific, libraries, system, then language) and separated by blank lines. The imports within a grouping should be alphabetized. Don't make the import police (Kats and me) come after you later! <:)

  import org.mozilla.gecko...

  import org.w3c...

  import android...

  import java...

  import javax...

[1] http://source.android.com/source/code-style.html#order-import-statements

@@ +50,5 @@
> +import android.util.Log;
> +
> +public class UpdateService extends IntentService {
> +    private static final int BUFSIZE = 8192;
> +    private static final int NOTIFICATION_ID = 0xdeadbeef;

I would suggest using a notification tag string like "org.mozilla.gecko.updater"/whatever instead of 0xdeadbeef (which is a popular magic number that might conflict with another app's notifications).

@@ +66,5 @@
> +    private NotificationManager mNotificationManager;
> +    private ConnectivityManager mConnectivityManager;
> +
> +    private boolean mDownloading; // = false;
> +    private boolean mApplyImmediately; // = false;

These "= false" comments are unnecessary.

@@ +119,5 @@
> +    }
> +
> +    private void startUpdate(int flags) {
> +        NetworkInfo netInfo = mConnectivityManager.getActiveNetworkInfo();
> +        if (!netInfo.isConnected()) {

You need to handle the case when getActiveNetworkInfo() returns null because no networks are available.

@@ +365,5 @@
> +                if (input != null)
> +                    input.close();
> +
> +                if (output != null)
> +                    output.close();

If input.close() throws, then you won't output.close().

::: mobile/android/base/UpdateServiceHelper.java.in
@@ +93,5 @@
> +            return;
> +
> +        // Register for the update check to be performed at UDPATE_HOUR, once a day
> +        manager.setRepeating(AlarmManager.RTC_WAKEUP, getUpdateMillis(), AlarmManager.INTERVAL_DAY,
> +                             PendingIntent.getService(context, 0, new Intent(UpdateServiceHelper.ACTION_CHECK_FOR_UPDATE, null, context, UpdateService.class), 0));

1. setInexactRepeating() is preferable to setRepeating().
2. Do we really want to wake a sleeping device with an RTC_WAKEUP alarm instead of RTC?
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-28 14:21:45 PDT
(In reply to Chris Peterson (:cpeterson) from comment #2)

> ::: mobile/android/app/mobile.js
> @@ +501,5 @@
> >  pref("app.update.url", "https://aus2.mozilla.org/update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%-@MOZ_PKG_SPECIAL@/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%PLATFORM_VERSION%/update.xml");
> >  #else
> >  pref("app.update.url", "https://aus2.mozilla.org/update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%PLATFORM_VERSION%/update.xml");
> >  #endif
> > +*/
> 
> Why comment out this code instead of removing it?

Because anyone updating the app.update.url will look here. I want them to act as a guidepost to send people to the new location.
Comment 4 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-08-28 14:28:03 PDT
(In reply to Chris Peterson (:cpeterson) from comment #2)
> Comment on attachment 656115 [details] [diff] [review]
> Implement new Java-based updater for Android
> 
> Review of attachment 656115 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome! I can't wait!
> 
> ::: mobile/android/app/mobile.js
> @@ +501,5 @@
> >  pref("app.update.url", "https://aus2.mozilla.org/update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%-@MOZ_PKG_SPECIAL@/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%PLATFORM_VERSION%/update.xml");
> >  #else
> >  pref("app.update.url", "https://aus2.mozilla.org/update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%PLATFORM_VERSION%/update.xml");
> >  #endif
> > +*/
> 
> Why comment out this code instead of removing it?

I dunno. I guess just to know what used to be there, but I guess that's what version control is for. I'll remove it.

> 
> ::: mobile/android/base/GeckoApp.java
> @@ +1704,5 @@
> >  
> >          GeckoNetworkManager.getInstance().init();
> >          GeckoNetworkManager.getInstance().start();
> >  
> > +        UpdateServiceHelper.registerForUpdates(this);
> 
> Will registering for updates here cause us to register an additional
> repeating alarm every time Firefox is launched?

No, it will just replace the alarm we had already registered (if any).

> 
> ::: mobile/android/base/UpdateService.java
> @@ +46,5 @@
> > +import org.w3c.dom.NodeList;
> > +
> > +import java.util.Date;
> > +
> > +import android.util.Log;
> 
> Google's Android style guide [1] recommends that imports be grouped by
> namespace (project-specific, libraries, system, then language) and separated
> by blank lines. The imports within a grouping should be alphabetized. Don't
> make the import police (Kats and me) come after you later! <:)

Ugh...ok

> 
>   import org.mozilla.gecko...
> 
>   import org.w3c...
> 
>   import android...
> 
>   import java...
> 
>   import javax...
> 
> [1] http://source.android.com/source/code-style.html#order-import-statements
> 
> @@ +50,5 @@
> > +import android.util.Log;
> > +
> > +public class UpdateService extends IntentService {
> > +    private static final int BUFSIZE = 8192;
> > +    private static final int NOTIFICATION_ID = 0xdeadbeef;
> 
> I would suggest using a notification tag string like
> "org.mozilla.gecko.updater"/whatever instead of 0xdeadbeef (which is a
> popular magic number that might conflict with another app's notifications).

Well it has to be an integer, but yeah, 0xdeadbeef is not great. I meant to change that.

> 
> @@ +66,5 @@
> > +    private NotificationManager mNotificationManager;
> > +    private ConnectivityManager mConnectivityManager;
> > +
> > +    private boolean mDownloading; // = false;
> > +    private boolean mApplyImmediately; // = false;
> 
> These "= false" comments are unnecessary.

Fair enough, removed

> 
> @@ +119,5 @@
> > +    }
> > +
> > +    private void startUpdate(int flags) {
> > +        NetworkInfo netInfo = mConnectivityManager.getActiveNetworkInfo();
> > +        if (!netInfo.isConnected()) {
> 
> You need to handle the case when getActiveNetworkInfo() returns null because
> no networks are available.

Indeed.

> 
> @@ +365,5 @@
> > +                if (input != null)
> > +                    input.close();
> > +
> > +                if (output != null)
> > +                    output.close();
> 
> If input.close() throws, then you won't output.close().

Nice catch!

> 
> ::: mobile/android/base/UpdateServiceHelper.java.in
> @@ +93,5 @@
> > +            return;
> > +
> > +        // Register for the update check to be performed at UDPATE_HOUR, once a day
> > +        manager.setRepeating(AlarmManager.RTC_WAKEUP, getUpdateMillis(), AlarmManager.INTERVAL_DAY,
> > +                             PendingIntent.getService(context, 0, new Intent(UpdateServiceHelper.ACTION_CHECK_FOR_UPDATE, null, context, UpdateService.class), 0));
> 
> 1. setInexactRepeating() is preferable to setRepeating().

Ah, yes. Changed.

> 2. Do we really want to wake a sleeping device with an RTC_WAKEUP alarm
> instead of RTC?

Maybe. If the update is scheduled in the middle of the (which it's not now, but maybe we want that?), then I think you'd want the phone to wake up and do its thing. Regardless, we're only causing one wakeup a day so I don't think we should worry about it too much.
Comment 5 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-08-28 14:28:39 PDT
(In reply to Mark Finkle (:mfinkle) from comment #3)
> (In reply to Chris Peterson (:cpeterson) from comment #2)
> 
> > ::: mobile/android/app/mobile.js
> > @@ +501,5 @@
> > >  pref("app.update.url", "https://aus2.mozilla.org/update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%-@MOZ_PKG_SPECIAL@/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%PLATFORM_VERSION%/update.xml");
> > >  #else
> > >  pref("app.update.url", "https://aus2.mozilla.org/update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%PLATFORM_VERSION%/update.xml");
> > >  #endif
> > > +*/
> > 
> > Why comment out this code instead of removing it?
> 
> Because anyone updating the app.update.url will look here. I want them to
> act as a guidepost to send people to the new location.

Yeah, I guess that's what I was thinking too
Comment 6 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-08-29 07:03:35 PDT
Created attachment 656428 [details] [diff] [review]
Implement new Java-based updater for Android
Comment 7 Chris Peterson [:cpeterson] 2012-08-29 10:00:50 PDT
Comment on attachment 656428 [details] [diff] [review]
Implement new Java-based updater for Android

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

LGTM. Ship it!
Comment 8 Kevin Brosnan [:kbrosnan] 2012-08-29 10:07:55 PDT
Just to cover all the bases is there a way we can test this before checking it into m-c?
Comment 9 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-08-29 13:11:03 PDT
Created attachment 656575 [details] [diff] [review]
Implement new Java-based updater for Android
Comment 10 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-08-29 13:14:46 PDT
A build with the patch applied can be found here: http://people.mozilla.org/~jwillcox/fennec-new-updater.apk

This will update from the Nightly channel, but the apk itself is of the 'org.mozilla.fennec_snorp' variety, so it does not update itself. The build id on this is newer than any of the nightlies too, so there won't actually be any updates for a while. You can, however, trigger a "reinstall" by using the following command:

adb shell am startservice -a org.mozilla.fennec_snorp.CHECK_FOR_UPDATE -n org.mozilla.fennec_snorp/org.mozilla.gecko.updater.UpdateService --ei updateFlags 4

This just tells the update service that our current build ID is 0, so it returns whatever the latest update is.
Comment 11 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-08-29 13:24:51 PDT
Created attachment 656583 [details] [diff] [review]
Implement new Java-based updater for Android
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-29 13:45:09 PDT
Comment on attachment 656575 [details] [diff] [review]
Implement new Java-based updater for Android

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

> #ifdef MOZ_UPDATER
> /* prefs used specifically for updating the app */
> pref("app.update.enabled", true);
> pref("app.update.auto", false);
> pref("app.update.channel", "@MOZ_UPDATE_CHANNEL@");
> pref("app.update.mode", 1);
> pref("app.update.silent", false);
>-#ifdef MOZ_PKG_SPECIAL
>-pref("app.update.url", "https://aus2.mozilla.org/update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%-@MOZ_PKG_SPECIAL@/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%PLATFORM_VERSION%/update.xml");
>-#else
>-pref("app.update.url", "https://aus2.mozilla.org/update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%PLATFORM_VERSION%/update.xml");
>-#endif
>+
>+// If you are looking for app.update.url, we no longer use it.
>+// See mobile/android/base/UpdateServiceHelper.java.in
>+
> pref("app.update.promptWaitTime", 43200);
> pref("app.update.idletime", 60);
> pref("app.update.showInstalledUI", false);
> pref("app.update.incompatible.mode", 0);
> pref("app.update.download.backgroundInterval", 0);
> 
> #ifdef MOZ_OFFICIAL_BRANDING
> pref("app.update.interval", 86400);
> pref("app.update.url.manual", "http://www.mozilla.com/%LOCALE%/m/");
> pref("app.update.url.details", "http://www.mozilla.com/%LOCALE%/mobile/releases/");

How many of these preferences are still used?

Should we actually disable the Gecko updater?

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

>+    public void notifyCheckUpdateResult(boolean result) {
>+        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Update:CheckResult", result ? "true" : "false"));
>+    }

I wonder if we care about this part anymore. The new updater will show notifications or toasts right?

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

>+    private void startUpdate(int flags) {

>+        if (!hasFlag(flags, UpdateServiceHelper.FLAG_FORCE_DOWNLOAD) &&
>+            connectionType != ConnectivityManager.TYPE_WIFI &&
>+            connectionType != ConnectivityManager.TYPE_ETHERNET) {
>+            Log.i(LOGTAG, "not connected via wifi or ethernet");

What are the rules here for downloading an update? No downloads over cell data?

>+            // We aren't autodownloading here, so prompt to start the update

What are the rules for autodownloading? I think we want to avoid as many prompts as possible.

>+        // If we have root, we always apply the update immediately because it happens in the background
>+        if (mApplyImmediately || checkRoot()) {
>+            applyUpdate(pkg);
>+        } else {
>+            // Prompt to apply the update

Again, when would we need to prompt?

>\ No newline at end of file

nit: add one

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

>+    // The hour of the day when we want to poll for updates, in UTC. Generally, Nightly builds
>+    // are published shortly before this time.
>+    private static final int UPDATE_HOUR = 12;

I am not a big fan of this approach. Nightly and Aurora should probably poll more than once a day anyway (they do now). Keying this off the Nightly "time" won't be useful for Beta and Final release either.

>+    private static long getUpdateMillis() {
>+        GregorianCalendar cal = new GregorianCalendar(TimeZone.getTimeZone("GMT"));
>+        if (cal.get(Calendar.HOUR_OF_DAY) >= UPDATE_HOUR) {
>+            // We're past the update time for today, so we'll try tomorrow at UPDATE_HOUR (which is set below)
>+            cal.set(Calendar.DATE, cal.get(Calendar.DATE) + 1);
>+        }
>+
>+        cal.set(Calendar.HOUR_OF_DAY, UPDATE_HOUR);
>+
>+        // Add some random amount of minutes so we don't have every
>+        // client in the world hitting the update server at once
>+        cal.set(Calendar.MINUTE, new Random().nextInt(60));
>+
>+        return cal.getTimeInMillis();
>+    }

I like the random stuff, but I think this should be a configurable poll, like Gecko uses.

>\ No newline at end of file

nit: Add one

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd

>+<!-- Updater notifications -->
>+<!ENTITY updater_start_title "&brandShortName;">
>+<!ENTITY updater_start_ticker "&brandShortName; update available...">
>+<!ENTITY updater_start_select "Select to download update.">
>+
>+<!ENTITY updater_downloading_title "Downloading &brandShortName;">
>+<!ENTITY updater_downloading_ticker "Downloading &brandShortName; update...">
>+<!ENTITY updater_downloading_ticker_failed "Failed to download &brandShortName; update...">
>+<!ENTITY updater_downloading_select "Select to apply update when complete.">
>+<!ENTITY updater_downloading_retry "Select to retry update download.">
>+<!ENTITY updater_downloading_willapply "Waiting for download to complete...">
>+
>+<!ENTITY updater_apply_title "&brandShortName;">
>+<!ENTITY updater_apply_ticker "&brandShortName; update available...">
>+<!ENTITY updater_apply_select "Select to apply downloaded update.">
>+
>+<!ENTITY updater_installing_title "&brandShortName;">
>+<!ENTITY updater_installing_ticker "Updating &brandShortName;...">
>+<!ENTITY updater_installing_ticker_success "Successfully updated &brandShortName;">
>+<!ENTITY updater_installing_ticker_fail "Failed to update &brandShortName;">
>+<!ENTITY updater_installing_text "Installing update...">
>+<!ENTITY updater_installing_text_success "Succesfully updated.">
>+<!ENTITY updater_installing_text_fail "Failed to install update.">

An impressive amount of strings. I am a little worried we are too chatty with the user. Testing will tell.

>diff --git a/mobile/android/chrome/content/about.xhtml b/mobile/android/chrome/content/about.xhtml

Since we remove all the Gecko updater clients here, we should be able to remove the UpdatePrompt component:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/UpdatePrompt.js
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/Makefile.in#47
http://mxr.mozilla.org/mozilla-central/source/mobile/android/installer/package-manifest.in#520
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/MobileComponents.manifest#100

r-, just to get some answers first and figure out about disabling the Gecko updater
Comment 13 Chris Peterson [:cpeterson] 2012-08-29 13:52:04 PDT
(In reply to Mark Finkle (:mfinkle) from comment #12)
> >+        if (!hasFlag(flags, UpdateServiceHelper.FLAG_FORCE_DOWNLOAD) &&
> >+            connectionType != ConnectivityManager.TYPE_WIFI &&
> >+            connectionType != ConnectivityManager.TYPE_ETHERNET) {
> >+            Log.i(LOGTAG, "not connected via wifi or ethernet");
> 
> What are the rules here for downloading an update? No downloads over cell
> data?

If we decide to allow auto-downloads over the cell network, we should play nice and check !NetworkInfo.isRoaming() and !ConnectivityManager.isActiveNetworkMetered() (API Level 16).
Comment 14 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-08-29 14:04:46 PDT
(In reply to Mark Finkle (:mfinkle) from comment #12)
> Comment on attachment 656575 [details] [diff] [review]
> Implement new Java-based updater for Android
> 
> >diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js
> 
> > #ifdef MOZ_UPDATER
> > /* prefs used specifically for updating the app */
> > pref("app.update.enabled", true);
> > pref("app.update.auto", false);
> > pref("app.update.channel", "@MOZ_UPDATE_CHANNEL@");
> > pref("app.update.mode", 1);
> > pref("app.update.silent", false);
> >-#ifdef MOZ_PKG_SPECIAL
> >-pref("app.update.url", "https://aus2.mozilla.org/update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%-@MOZ_PKG_SPECIAL@/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%PLATFORM_VERSION%/update.xml");
> >-#else
> >-pref("app.update.url", "https://aus2.mozilla.org/update/4/%PRODUCT%/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/%PLATFORM_VERSION%/update.xml");
> >-#endif
> >+
> >+// If you are looking for app.update.url, we no longer use it.
> >+// See mobile/android/base/UpdateServiceHelper.java.in
> >+
> > pref("app.update.promptWaitTime", 43200);
> > pref("app.update.idletime", 60);
> > pref("app.update.showInstalledUI", false);
> > pref("app.update.incompatible.mode", 0);
> > pref("app.update.download.backgroundInterval", 0);
> > 
> > #ifdef MOZ_OFFICIAL_BRANDING
> > pref("app.update.interval", 86400);
> > pref("app.update.url.manual", "http://www.mozilla.com/%LOCALE%/m/");
> > pref("app.update.url.details", "http://www.mozilla.com/%LOCALE%/mobile/releases/");
> 
> How many of these preferences are still used?

None, as far as the new updater is concerned

> 
> Should we actually disable the Gecko updater?

Does anything else rely on this stuff? I didn't want to break anything unrelated...

> 
> >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java
> 
> >+    public void notifyCheckUpdateResult(boolean result) {
> >+        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Update:CheckResult", result ? "true" : "false"));
> >+    }
> 
> I wonder if we care about this part anymore. The new updater will show
> notifications or toasts right?

Right, but this is so about:firefox can show the right thing after you click the 'check for updates' button. I think UX wants to change how this works soon, though.

> 
> >diff --git a/mobile/android/base/UpdateService.java b/mobile/android/base/UpdateService.java
> 
> >+    private void startUpdate(int flags) {
> 
> >+        if (!hasFlag(flags, UpdateServiceHelper.FLAG_FORCE_DOWNLOAD) &&
> >+            connectionType != ConnectivityManager.TYPE_WIFI &&
> >+            connectionType != ConnectivityManager.TYPE_ETHERNET) {
> >+            Log.i(LOGTAG, "not connected via wifi or ethernet");
> 
> What are the rules here for downloading an update? No downloads over cell
> data?

No automatic downloads over cell data. If we find that we have an update but can't automatically download it, we show a notification prompting you to start the download (similar to the current updater).

> 
> >+            // We aren't autodownloading here, so prompt to start the update
> 
> What are the rules for autodownloading? I think we want to avoid as many
> prompts as possible.

See above. I agree we should make the number of prompts as small as possible, but don't want to get blamed for ruining someone's life with data charges. Maybe a follow-up patch could add an option for when to automatically download...

> 
> >+        // If we have root, we always apply the update immediately because it happens in the background
> >+        if (mApplyImmediately || checkRoot()) {
> >+            applyUpdate(pkg);
> >+        } else {
> >+            // Prompt to apply the update
> 
> Again, when would we need to prompt?

We only install the update immediately if you select the notification while the download is in progress, or if you have root. Don't want to randomly pop up the package installer.

> 
> >\ No newline at end of file
> 
> nit: add one

Done

> 
> >diff --git a/mobile/android/base/UpdateServiceHelper.java.in b/mobile/android/base/UpdateServiceHelper.java.in
> 
> >+    // The hour of the day when we want to poll for updates, in UTC. Generally, Nightly builds
> >+    // are published shortly before this time.
> >+    private static final int UPDATE_HOUR = 12;
> 
> I am not a big fan of this approach. Nightly and Aurora should probably poll
> more than once a day anyway (they do now). Keying this off the Nightly
> "time" won't be useful for Beta and Final release either.

Yeah. I'm open to suggestions. We could also poll whenever the app is started, for instance.

> 
> >+    private static long getUpdateMillis() {
> >+        GregorianCalendar cal = new GregorianCalendar(TimeZone.getTimeZone("GMT"));
> >+        if (cal.get(Calendar.HOUR_OF_DAY) >= UPDATE_HOUR) {
> >+            // We're past the update time for today, so we'll try tomorrow at UPDATE_HOUR (which is set below)
> >+            cal.set(Calendar.DATE, cal.get(Calendar.DATE) + 1);
> >+        }
> >+
> >+        cal.set(Calendar.HOUR_OF_DAY, UPDATE_HOUR);
> >+
> >+        // Add some random amount of minutes so we don't have every
> >+        // client in the world hitting the update server at once
> >+        cal.set(Calendar.MINUTE, new Random().nextInt(60));
> >+
> >+        return cal.getTimeInMillis();
> >+    }
> 
> I like the random stuff, but I think this should be a configurable poll,
> like Gecko uses.

I guess I can use the app.update.interval values.

> 
> >\ No newline at end of file
> 
> nit: Add one
> 

Done

> >diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd
> 
> >+<!-- Updater notifications -->
> >+<!ENTITY updater_start_title "&brandShortName;">
> >+<!ENTITY updater_start_ticker "&brandShortName; update available...">
> >+<!ENTITY updater_start_select "Select to download update.">
> >+
> >+<!ENTITY updater_downloading_title "Downloading &brandShortName;">
> >+<!ENTITY updater_downloading_ticker "Downloading &brandShortName; update...">
> >+<!ENTITY updater_downloading_ticker_failed "Failed to download &brandShortName; update...">
> >+<!ENTITY updater_downloading_select "Select to apply update when complete.">
> >+<!ENTITY updater_downloading_retry "Select to retry update download.">
> >+<!ENTITY updater_downloading_willapply "Waiting for download to complete...">
> >+
> >+<!ENTITY updater_apply_title "&brandShortName;">
> >+<!ENTITY updater_apply_ticker "&brandShortName; update available...">
> >+<!ENTITY updater_apply_select "Select to apply downloaded update.">
> >+
> >+<!ENTITY updater_installing_title "&brandShortName;">
> >+<!ENTITY updater_installing_ticker "Updating &brandShortName;...">
> >+<!ENTITY updater_installing_ticker_success "Successfully updated &brandShortName;">
> >+<!ENTITY updater_installing_ticker_fail "Failed to update &brandShortName;">
> >+<!ENTITY updater_installing_text "Installing update...">
> >+<!ENTITY updater_installing_text_success "Succesfully updated.">
> >+<!ENTITY updater_installing_text_fail "Failed to install update.">
> 
> An impressive amount of strings. I am a little worried we are too chatty
> with the user. Testing will tell.

Yeah, it is a lot. I would like the UX folks to go over it once it gets in and do any necessary fine tuning to the strings or flow.

> 
> >diff --git a/mobile/android/chrome/content/about.xhtml b/mobile/android/chrome/content/about.xhtml
> 
> Since we remove all the Gecko updater clients here, we should be able to
> remove the UpdatePrompt component:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/
> UpdatePrompt.js
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/
> Makefile.in#47
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/installer/
> package-manifest.in#520
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/
> MobileComponents.manifest#100

Done

> 
> r-, just to get some answers first and figure out about disabling the Gecko
> updater
Comment 15 Kevin Brosnan [:kbrosnan] 2012-08-29 17:58:11 PDT
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #10)
> A build with the patch applied can be found here:
> http://people.mozilla.org/~jwillcox/fennec-new-updater.apk
> 
> This will update from the Nightly channel, but the apk itself is of the
> 'org.mozilla.fennec_snorp' variety, so it does not update itself. The build
> id on this is newer than any of the nightlies too, so there won't actually
> be any updates for a while. You can, however, trigger a "reinstall" by using
> the following command:
> 
> adb shell am startservice -a org.mozilla.fennec_snorp.CHECK_FOR_UPDATE -n
> org.mozilla.fennec_snorp/org.mozilla.gecko.updater.UpdateService --ei
> updateFlags 4
> 
> This just tells the update service that our current build ID is 0, so it
> returns whatever the latest update is.

Tested using this build on Android 2.2, 2.3 and 4.0. Seems to be in good enough shape to test on a real nightly tomorrow morning.

One thing I noticed was that on Android 2.3 and 4.0 the downloader launched the installer for nightly. So in theory we could move users around to different channels?

Note for unrooted phones the command to force an update check needs to use run-as

adb shell run-as org.mozilla.fennec_snorp am startservice -a org.mozilla.fennec_snorp.CHECK_FOR_UPDATE -n org.mozilla.fennec_snorp/org.mozilla.gecko.updater.UpdateService --ei updateFlags 4
Comment 16 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-08-30 10:39:49 PDT
Created attachment 656943 [details] [diff] [review]
Implement new Java-based updater for Android
Comment 17 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-30 22:15:40 PDT
Comment on attachment 656943 [details] [diff] [review]
Implement new Java-based updater for Android

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

>+    private static final String KEY_NEXT_ATTEMPT_DATE = "UpdateService.nextAttemptDate";

You might be able to remove this if we don't need any of the nextAttempt stuff

>+    private void registerForUpdates(boolean isRetry) {
>+        Calendar lastAttempt = getLastAttemptDate();
>+        Calendar nextAttempt = getNextAttemptDate();

nextAttempt is not used in this method. Should it be? Looks like you get around needing it.

>+    private Calendar getNextAttemptDate() {

You call this once, but don't really use the result. Remove it?

>+    private void setNextAttemptDate(Calendar cal) {

You never call this


This file has a lot of Log.i debugging in it. Maybe too much. We can keep it while on nightly to help find issues, but we should consider removing what we can in a followup bug before moving to Aurora.

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

>+	public static URL getUpdateUrl(boolean force) {
>+		Locale locale = Locale.getDefault();
>+		String url = UPDATE_URL.replace("%LOCALE%", locale.getLanguage() + "-" + locale.getCountry()).
>+            replace("%OS_VERSION%", Build.VERSION.RELEASE).
>+            replace("%BUILDID%", force ? "0" : BUILDID);

TABs -> spaces

>+
>+        try {
>+		    return new URL(url);

TABs -> spaces

>diff --git a/mobile/android/base/locales/en-US/android_strings.dtd b/mobile/android/base/locales/en-US/android_strings.dtd
>--- a/mobile/android/base/locales/en-US/android_strings.dtd
>+++ b/mobile/android/base/locales/en-US/android_strings.dtd
>@@ -217,10 +217,34 @@ just addresses the organization to follo
> <!ENTITY bookmarkhistory_button_import "Import">
> <!ENTITY bookmarkhistory_import_both "Importing bookmarks and history
>                                       from Android">
> <!ENTITY bookmarkhistory_import_bookmarks "Importing bookmarks
>                                            from Android">
> <!ENTITY bookmarkhistory_import_history "Importing history
>                                          from Android">
> <!ENTITY bookmarkhistory_import_wait "Please wait...">
> 
> <!ENTITY webapp_generic_name "App">
>+
>+<!-- Updater notifications -->
>+<!ENTITY updater_start_title "&brandShortName;">
>+<!ENTITY updater_start_ticker "&brandShortName; update available...">
>+<!ENTITY updater_start_select "Select to download update.">
>+
>+<!ENTITY updater_downloading_title "Downloading &brandShortName;">
>+<!ENTITY updater_downloading_ticker "Downloading &brandShortName; update...">
>+<!ENTITY updater_downloading_ticker_failed "Failed to download &brandShortName; update...">
>+<!ENTITY updater_downloading_select "Select to apply update when complete.">
>+<!ENTITY updater_downloading_retry "Select to retry update download.">
>+<!ENTITY updater_downloading_willapply "Waiting for download to complete...">
>+
>+<!ENTITY updater_apply_title "&brandShortName;">
>+<!ENTITY updater_apply_ticker "&brandShortName; update available...">
>+<!ENTITY updater_apply_select "Select to apply downloaded update.">
>+
>+<!ENTITY updater_installing_title "&brandShortName;">
>+<!ENTITY updater_installing_ticker "Updating &brandShortName;...">
>+<!ENTITY updater_installing_ticker_success "Successfully updated &brandShortName;">
>+<!ENTITY updater_installing_ticker_fail "Failed to update &brandShortName;">
>+<!ENTITY updater_installing_text "Installing update...">
>+<!ENTITY updater_installing_text_success "Succesfully updated.">
>+<!ENTITY updater_installing_text_fail "Failed to install update.">

nit: In general we try to use the unicode ellipsis character instead of 3 periods (…)
Also, should we be using a period to end some of these phrases? If using an ending period is consistent with other parts of Firefox and Android in general then OK.

r+, but check into the "ending period" thing and fix up the other nits
Comment 18 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-08-31 06:30:32 PDT
Created attachment 657249 [details] [diff] [review]
Implement new Java-based updater for Android
Comment 19 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-08-31 06:34:39 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1c78a682928
Comment 20 Mark Finkle (:mfinkle) (use needinfo?) 2012-08-31 08:49:46 PDT
backed out for complete talos bustage
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8cacc4cd63e
Comment 21 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-09-04 10:58:23 PDT
Test failures were caused by bug 750548. I pushed the original patch with a small workaround.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7f10bbefb8f
Comment 22 Ryan VanderMeulen [:RyanVM] 2012-09-04 18:49:31 PDT
https://hg.mozilla.org/mozilla-central/rev/d7f10bbefb8f
Comment 23 Thomas Stache 2012-09-09 10:56:02 PDT
Is the service working as intended? I haven't noticed a nightly update since this landed... Does Android expose installed services somewhere?
Comment 24 Aaron Train [:aaronmt] 2012-09-09 11:29:03 PDT
(In reply to Thomas Stache from comment #23)
> Is the service working as intended? I haven't noticed a nightly update since
> this landed... Does Android expose installed services somewhere?

It's possible you might be running into bug 789791; where no update is offered if the system language on Android is not en-US?
Comment 25 Thomas Stache 2012-09-09 13:02:36 PDT
(In reply to Aaron Train [:aaronmt] from comment #24)
> (In reply to Thomas Stache from comment #23)
> > Is the service working as intended? I haven't noticed a nightly update since
> > this landed... Does Android expose installed services somewhere?
> 
> It's possible you might be running into bug 789791; where no update is
> offered if the system language on Android is not en-US?

That is so, my devices are on de-de. Thanks for adding the dependency!
Comment 26 Cristian Nicolae (:xti) 2012-09-11 07:46:45 PDT
The updater is changed on the latest Nightly build. Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-11)
Device: Galaxy Note
OS: Android 4.0.4
Comment 27 Ed Morley [:emorley] 2012-09-12 05:20:48 PDT
The updater no longer shows download progress after this patch, was this intended?
Comment 28 Mark Finkle (:mfinkle) (use needinfo?) 2012-09-12 05:29:13 PDT
(In reply to Ed Morley [:edmorley UTC+1] from comment #27)
> The updater no longer shows download progress after this patch, was this
> intended?

Yes. We might want to add it back though.
Comment 29 Sebastian H. [:aryx][:archaeopteryx] 2012-09-15 10:14:29 PDT
(In reply to Mark Finkle (:mfinkle) from comment #28)
> (In reply to Ed Morley [:edmorley UTC+1] from comment #27)
> > The updater no longer shows download progress after this patch, was this
> > intended?
> 
> Yes. We might want to add it back though.

See bug 791475 for this.
Comment 30 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-09-17 08:32:22 PDT
Comment on attachment 657249 [details] [diff] [review]
Implement new Java-based updater for Android

[Approval Request Comment]
Apparently we'd like to have this in 17. Fairly low-risk, as it has been working fine in 18 (assuming we also merge the fix for bugs 789964 and 789791)
Comment 31 Nick Thomas [:nthomas] 2012-09-17 14:00:00 PDT
Will we need to disable the service when the code merges to beta ? If so, we need to let lsblakk/akeybl know what is required.
Comment 32 Karen Rudnitski [:kar] 2012-09-17 14:05:08 PDT
Hi - I'd like to have this in 17 please, so am requesting for aurora approval. I don't want to miss the boat and wait for the next ESR round, and this may very well be an important path for future partners - may ease a barrier to entry. However, this should be watched closely for bugs.
Comment 33 Alex Keybl [:akeybl] 2012-09-17 14:53:49 PDT
(In reply to Karen Rudnitski from comment #32)
> Hi - I'd like to have this in 17 please, so am requesting for aurora
> approval. I don't want to miss the boat and wait for the next ESR round, and
> this may very well be an important path for future partners - may ease a
> barrier to entry. However, this should be watched closely for bugs.

Hi Karen - mobile doesn't currently build from the ESR, so that shouldn't be a concern for uplift.

I'd also like to point out that this patch has string changes, which should have been called out in Comment 30. Let's discuss the necessity for an early uplift at tomorrow morning's meeting, since having an updater for single locale l10n would be the only release-user benefit here (and would have l10n impact).
Comment 34 Alex Keybl [:akeybl] 2012-09-17 14:55:33 PDT
(In reply to Nick Thomas [:nthomas] from comment #31)
> Will we need to disable the service when the code merges to beta ? If so, we
> need to let lsblakk/akeybl know what is required.

If this patch regresses the assumption that beta/release do not update themselves, we definitely need a bug on file to fix that.
Comment 35 Aki Sasaki [:aki] 2012-09-17 15:04:38 PDT
This bug may be the cause of https://bugzilla.mozilla.org/show_bug.cgi?id=791387#c2 (relying on non-existent config/buildid during l10n repacks).
Comment 36 Axel Hecht [:Pike] 2012-09-18 08:28:04 PDT
As per our chat on vidyo, let's take this in support of a possible ESR on mobile. Make sure to take this together with bug 789791.

Aki, do we need additional fixes here to not break repacks when we uplift this?
Comment 38 Aki Sasaki [:aki] 2012-09-18 09:34:04 PDT
(In reply to Axel Hecht [:Pike] from comment #36)
> Aki, do we need additional fixes here to not break repacks when we uplift
> this?

I currently have a workaround in bug 791387, namely to explicitly run 'make export' in config/ to create config/buildid.

If we can find where this regressed (possibly https://hg.mozilla.org/mozilla-central/rev/d7f10bbefb8f#l6.49 ? ) and add the appropriate makefile dependencies so we no longer need the workaround, that would be ideal.
Comment 39 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-09-18 12:13:54 PDT
(In reply to Aki Sasaki [:aki] from comment #38)
> (In reply to Axel Hecht [:Pike] from comment #36)
> > Aki, do we need additional fixes here to not break repacks when we uplift
> > this?
> 
> I currently have a workaround in bug 791387, namely to explicitly run 'make
> export' in config/ to create config/buildid.
> 
> If we can find where this regressed (possibly
> https://hg.mozilla.org/mozilla-central/rev/d7f10bbefb8f#l6.49 ? ) and add
> the appropriate makefile dependencies so we no longer need the workaround,
> that would be ideal.

The buildid file was already used in that Makefile (4 lines lower), so I'm not sure how this could have caused the problem.
Comment 40 Aki Sasaki [:aki] 2012-09-18 12:17:39 PDT
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #39)
> (In reply to Aki Sasaki [:aki] from comment #38)
> > (In reply to Axel Hecht [:Pike] from comment #36)
> > > Aki, do we need additional fixes here to not break repacks when we uplift
> > > this?
> > 
> > I currently have a workaround in bug 791387, namely to explicitly run 'make
> > export' in config/ to create config/buildid.
> > 
> > If we can find where this regressed (possibly
> > https://hg.mozilla.org/mozilla-central/rev/d7f10bbefb8f#l6.49 ? ) and add
> > the appropriate makefile dependencies so we no longer need the workaround,
> > that would be ideal.
> 
> The buildid file was already used in that Makefile (4 lines lower), so I'm
> not sure how this could have caused the problem.

Yeah, that's why I'm not sure it caused it.
However, we don't hit that line if $(ANDROID_VERSION_CODE) is not null, so it's still possible it's the culprit.
Comment 41 Scoobidiver (away) 2012-09-20 14:29:12 PDT
Pushed to Aurora: http://hg.mozilla.org/releases/mozilla-aurora/rev/25a31e8d9ae8
Comment 42 Dão Gottwald [:dao] 2012-09-21 05:23:50 PDT
Is there any way to disable this when not on wifi or entirely? Nightly updates are killing my data plan since this landed.
Comment 43 Aaron Train [:aaronmt] 2012-09-21 06:37:31 PDT
(In reply to Dão Gottwald [:dao] from comment #42)
> Is there any way to disable this when not on wifi or entirely? Nightly
> updates are killing my data plan since this landed.

So, ConnectivityManager not working or what?

> +        int connectionType = netInfo.getType();
> +        if (!hasFlag(flags, UpdateServiceHelper.FLAG_FORCE_DOWNLOAD) &&
> +            connectionType != ConnectivityManager.TYPE_WIFI &&
> +            connectionType != ConnectivityManager.TYPE_ETHERNET) 
> ...
Comment 44 Dão Gottwald [:dao] 2012-09-21 07:09:47 PDT
(In reply to Aaron Train [:aaronmt] from comment #43)
> (In reply to Dão Gottwald [:dao] from comment #42)
> > Is there any way to disable this when not on wifi or entirely? Nightly
> > updates are killing my data plan since this landed.
> 
> So, ConnectivityManager not working or what?

If it's expected to prevent the behavior I'm getting, then it seems like it doesn't. Or maybe FLAG_FORCE_DOWNLOAD is always set.

> > +        int connectionType = netInfo.getType();
> > +        if (!hasFlag(flags, UpdateServiceHelper.FLAG_FORCE_DOWNLOAD) &&
> > +            connectionType != ConnectivityManager.TYPE_WIFI &&
> > +            connectionType != ConnectivityManager.TYPE_ETHERNET) 
> > ...

Note You need to log in before you can comment on or make changes to this bug.