1130834 - First download-notification can’t be deleted since Aurora 37

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Download Manager, defect)

Description

[Jan Manthay]

Download a file.
The animated arrow apears in the android notification bar.
After the download is finished, the arrow is solid.

Problem:
I can’t clear the notification list now. 
Normaly there is a button, I click on it, and all notifications are deleted.
This button is greyed out.

Now download a second file. (Or even more, does not matter)
After the download is finished, there are now two (or even more) notifications.

Now I can delete the notifications.
But...
The first download-notification will still not be deleted, but all others.

This happens since Aurora 37 on my Asus Memopad 7 HD (ME173X)

Comment 1

[Teodora Vermesan (:TeoVermesan)]

Tested with:
Device: Alcatel One Touch (Android 4.1.2)
Builds:
- Aurora 37 and Nightly 38: I try to "Save as PDF" a page. After the download is completed, I try to clear the download notification, by swiping it. I am not able. I save as PDF another page. The second notification can be cleared, but the first no.
- 36 Beta 6 and 35.0.1: the notification can be cleared from the list by swiping it.

I think it's Bug 901360 - Convert to Downloads.jsm. 
I will investigate it.

Comment 2

[Teodora Vermesan (:TeoVermesan)]

good build: 2014-12-16
bad build: 2014-12-17
pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b836016d82b5&tochange=b7eb1ce0237d

Comment 3

[:Margaret Leibovic]

I started debugging this, and it looks like things are happening correctly in DownloadNotifications.jsm. I find that when the download succeeds, we update the notification with a new set of options, and we call setOngoing(false) here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/NotificationHelper.java#253

I followed the logic into NotificationHandler.add, and in there isOngoing(notification) is also false. That's where we're calling mNotificationManager.notify, and according to the docs, this should replace an existing notification with a new one.

wesj, is there something beisdes "ongoing" that would make a notification non-dismissible? Have you seen issues like this before? Maybe it's just not supported to flip a notification from being ongoing to non-ongoing?

Comment 4

[Wesley Johnston (:wesj)]

Hmm.. I'm not sure of any other way to hold on to a notification either.... You might look to see if this code is hit:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AlertNotification.java#98

but it shouldn't be. You might also look at the flags of the notification generated from build.build()...

Comment 6

[:Margaret Leibovic]

Attachment: MozReview Request: bz://1130834/margaret

/r/4151 - Bug 1130834 - Explictly cancel ongoing download notifications instead of trying to update them to be non-ongoing. r=wesj

Pull down this commit:

hg pull review -r fa44f99fab64a00943fd47e5e7f4232a5014bdf7

Comment 7

[:Margaret Leibovic]

I tried digging into this again, and it really seems like NotificationManager#notify just will not update an ongoing notification to be non-ongoing.

While we could update our whole notifications system to handle this ourselves, that feels difficult and risky, especially since this notification code is complicated and used by other parts of the app.

This patch feels kinda dirty, but if we want to uplift something, I think it's safest to restrict the changes to download-specific code.

I'd also be okay with just admitting defeat, and understanding that we need to cancel ongoing notifications and create new non-ongoing notifications, rather than just trying to update them in place.

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 8567745 [details]
MozReview Request: bz://1130834/margaret

I'm fine with this. File a follow up to fix this maybe though? We spent a lot of time making sure these updated correctly, and the only change (I know of?) is downloads.jsm, so I assume that its sending slightly different info and confusing things. One thing I wanted to check was if it was sending "progress: 100%" or something and confusing us into thinking this was still a progress notification.

Comment 9

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/4aad4f99b867

I'll request uplift once this is verified on Nightly.

Filed bug 1136246 to deal with this notification issue more generically.

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/4aad4f99b867

Comment 11

[Teodora Vermesan (:TeoVermesan)]

The notification can be cleared from the android notification list by swiping it, so:
Verified as fixed using:
Device: Alcatel One Touch (Android 4.1.2)
Builds: Firefox for Android 39.0a1 (2015-03-04)

Comment 12

[:Margaret Leibovic]

Comment on attachment 8567745 [details]
MozReview Request: bz://1130834/margaret

Approval Request Comment
[Feature/regressing bug #]: Downloads.jsm (bug 901360)
[User impact if declined]: users can't swipe away system notifications for finished downloads
[Describe test coverage new/current, TreeHerder]: no test coverage, verified on Nightly
[Risks and why]: low-risk, as this change only affects the download notification
[String/UUID change made/needed]: none

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8567745 [details]
MozReview Request: bz://1130834/margaret

Small, user visible fix that has already been verified. Beta+ Aurora+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b3e0687fcf27

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2f6284a0d529

Comment 16

[Teodora Vermesan (:TeoVermesan)]

Notifications can be cleared from the android notification list by swiping it, so:
Verified as fixed using:
Device: Alcatel One Touch (Android 4.1.2)
Builds: Firefox for Android 38.0a2 (2015-03-06)

Comment 17

[Teodora Vermesan (:TeoVermesan)]

Notifications can be cleared from the android notification list by swiping it.
Verified as fixed using:
Device: Alcatel One Touch (Android 4.1.2)
Builds: Firefox for Android 37 Beta 4

Comment 19

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1130834 - Explictly cancel ongoing download notifications instead of trying to update them to be non-ongoing. r=wesj