Closed Bug 786380 Opened 12 years ago Closed 12 years ago

Write new Android service-based updater

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox17 fixed, firefox18 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 18
Tracking Flags:
Tracking Status
firefox17 --- fixed
firefox18 --- verified

People

(Reporter: snorp, Assigned: snorp)

References

Details

(Whiteboard: [Updater])

Attachments

(1 file, 5 obsolete files)

Implement new Java-based updater for Android
77.05 KB, patch
snorp
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review
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
Assignee: nobody → snorp
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?
(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.
(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.
(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
Attachment #656115 - Attachment is obsolete: true
Attachment #656115 - Flags: review?(mark.finkle)
Attachment #656428 - Flags: review?(mark.finkle)
Comment on attachment 656428 [details] [diff] [review] Implement new Java-based updater for Android Review of attachment 656428 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Ship it!
Attachment #656428 - Flags: review?(cpeterson) → review+
Just to cover all the bases is there a way we can test this before checking it into m-c?
Attachment #656428 - Attachment is obsolete: true
Attachment #656428 - Flags: review?(mark.finkle)
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.
Attachment #656575 - Attachment is obsolete: true
Attachment #656583 - Flags: review?(mark.finkle)
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
Attachment #656575 - Attachment is obsolete: false
Attachment #656575 - Flags: review-
(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).
QA Contact: kbrosnan
(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
(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
Attachment #656575 - Attachment is obsolete: true
Attachment #656583 - Attachment is obsolete: true
Attachment #656583 - Flags: review?(mark.finkle)
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
Attachment #656943 - Flags: review?(mark.finkle) → review+
Attachment #656943 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/d7f10bbefb8f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Is the service working as intended? I haven't noticed a nightly update since this landed... Does Android expose installed services somewhere?
Depends on: 789791
(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?
(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!
Blocks: 774432
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
Status: RESOLVED → VERIFIED
status-firefox18: --- → verified
The updater no longer shows download progress after this patch, was this intended?
(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.
(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 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)
Attachment #657249 - Flags: approval-mozilla-aurora?
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.
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.
(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).
(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.
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).
Blocks: 791387
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?
(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.
(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.
(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.
Attachment #657249 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 792992
Is there any way to disable this when not on wifi or entirely? Nightly updates are killing my data plan since this landed.
(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) > ...
(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) > > ...
Depends on: 793276
Whiteboard: [Updater]
Depends on: 802118
Depends on: 802115
Blocks: 802139
No longer blocks: 802139
Depends on: 802364
Depends on: 802396
No longer depends on: 802115
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: