923238 - Swiping away updater download notification should cancel the download

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Right now if you swipe the updater download notification away, the download continues. That's bad.

Comment 1

[Aaron Train [:aaronmt]]

Mentor bug?

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Could've sworn I set the mentor flags when I filed. Weird.

Assigned this to Michael Boon, our new updater maintainer.

Comment 3

[Eduard Neculaesi (:eduardn)]

I'd like to work on this bug, could you give me some directions? Thanks!

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Eduard Neculaesi (:eduardn) from comment #3)
> I'd like to work on this bug, could you give me some directions? Thanks!

Michael hasn't had time to work on it, and says he's fine giving it up, so it's all yours!

First you need to provide a 'deleteIntent' to the download progress notification. This is the intent that will be fired when the notification is swiped away (see here: http://developer.android.com/reference/android/app/Notification.html#deleteIntent). So you need to provide that intent and then just cancel the download in the handler. I think you'll need to do this in onStartCommand() instead of onPendingIntent(), similar to ACTION_APPLY_UPDATE.

Comment 5

[Eduard Neculaesi (:eduardn)]

Attachment: Fix for 923238

I added a |deleteIntent| to the notification and when that gets called I stop the download and delete the package. The next step (if everything is ok with the patch) would be testing but I don't know where to add tests and how to run them.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8355303 [details] [diff] [review]
Fix for 923238

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

You need to reset mCancelDownload at the beginning of downloadUpdatePackage(), otherwise it will stay canceled forever (or until the service is recreated).

::: mobile/android/base/updater/UpdateServiceHelper.java
@@ +26,5 @@
>      public static final String ACTION_CHECK_FOR_UPDATE = AppConstants.ANDROID_PACKAGE_NAME + ".CHECK_FOR_UPDATE";
>      public static final String ACTION_CHECK_UPDATE_RESULT = AppConstants.ANDROID_PACKAGE_NAME + ".CHECK_UPDATE_RESULT";
>      public static final String ACTION_DOWNLOAD_UPDATE = AppConstants.ANDROID_PACKAGE_NAME + ".DOWNLOAD_UPDATE";
>      public static final String ACTION_APPLY_UPDATE = AppConstants.ANDROID_PACKAGE_NAME + ".APPLY_UPDATE";
> +    public static final String ACTION_CANCEL_UPDATE = AppConstants.ANDROID_PACKAGE_NAME + ".CANCEL_UPDATE";

How about CANCEL_DOWNLOAD, since that's the specific phase that we are talking about.

Comment 7

[Eduard Neculaesi (:eduardn)]

Attachment: Fix for 923238 (2)

Missed that! Renamed to CANCEL_DOWNLOAD and fixed the |mCancelDownload| problem.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8355578 [details] [diff] [review]
Fix for 923238 (2)

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

nice work!

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce80d022ae3f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce80d022ae3f