1358598 - Multiple downloads no longer show multiple notifications

Status: VERIFIED FIXED

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

Description

[Jan Henning [:JanH]]

I've got a feeling that I've already seen a bug report for this recently, but if it actually exists, I can't find it now.

In any case the issue is that with multiple downloads running concurrently, we no longer show multiple notifications.

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

I remember this being a limitation from bug 1305498.

Comment 2

[Jan Henning [:JanH]]

That's indeed what the blame is showing for the current notification code, however the original implementation in bug 1304145 apparently broke this as well.

Comment 3

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Download manager is more on the platform side rather than front-end?

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

This bug is more about notifications, which is front-end.

Comment 5

[Max Liu [:maliu]]

Hi Jim,

Would you help to give us more advice in how to differentiate the download progress updating, please? Thanks.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

You need a unique ID for each foreground notification [1], instead of all using `R.id.foregroundNotification`. So you need a way to map a notification to an ID.

[1] http://searchfox.org/mozilla-central/rev/abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/mobile/android/base/java/org/mozilla/gecko/notifications/NotificationService.java#23

Comment 7

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

Tim this needs a front-end assignee

Comment 9

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

I think it's too late for 54.

Comment 10

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 11

[Mike Taylor [:miketaylr]]

Max, do you have plans to work on this?

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Max, or Tim, is this still on your radar? It's been a regression since 52 or so. 
If we can fix it by 59 then it will be in the new version of ESR.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I am no longer being assiged to Fennec.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

[triage] non-critical.

Comment 15

[Jan Henning [:JanH]]

Attachment: Bug 1358598 - Show ongoing notifications even if they're not the "official" foreground notification.

We can have only one "official" foreground notification that's associated with
the NotificationService to keep the OS from background-killing us at the same
time, but that doesn't mean that we should simply ignore any further ongoing
notifications possibly arriving afterwards.

Review commit: https://reviewboard.mozilla.org/r/226872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226872/

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8957953 [details]
Bug 1358598 - Show ongoing notifications even if they're not the "official" foreground notification.

https://reviewboard.mozilla.org/r/226872/#review232642

Ideally we would map each ongoing notification to an ID that can be used with setForegroundNotification (so we have multiple foreground notifications), but this is okay.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/689a0e5ce54d
Show ongoing notifications even if they're not the "official" foreground notification. r=jchen

Comment 18

[Jan Henning [:JanH]]

Comment on attachment 8957953 [details]
Bug 1358598 - Show ongoing notifications even if they're not the "official" foreground notification.

https://reviewboard.mozilla.org/r/226872/#review232642

As far as I can tell from searching around and looking at the framework sources, a service can have only one associated foreground notification, i.e. a call to `setForeground()` from the service will replace the previous notification, so we still need the current system of notifying the service only
- when we start requiring a foreground notification
- when we no longer require a foreground notification
- when we need to replace the current foreground notification because the old one is going away

I think we could get rid of the hardcoded ID, though, by simply passing the desired notification name to the service as an additional intent extra.

Comment 19

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/689a0e5ce54d

Comment 20

[Jan Henning [:JanH]]

Comment on attachment 8957953 [details]
Bug 1358598 - Show ongoing notifications even if they're not the "official" foreground notification.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1304145 
[User impact if declined]: We show only one download notification even if multiple downloads are running.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Tested locally.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: We already handled those additional ongoing notifications, but until now we simply didn't pass them on to the OS if we already had a designated foreground notification.
[String changes made/needed]: none

Comment 21

[Julien Cristau [:jcristau]]

(In reply to Jan Henning [:JanH] from comment #20)
> [Is this code covered by automated tests?]: No.
> [Has the fix been verified in Nightly?]: Tested locally.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Mind providing STR anyway?

Comment 22

[Jan Henning [:JanH]]

Just start multiple file downloads (preferably bigger files, so that they'll definitively run in parallel).

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8957953 [details]
Bug 1358598 - Show ongoing notifications even if they're not the "official" foreground notification.

fennec notifications fix, for 60.0b5

Comment 24

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e51c41264513

Comment 25

[Sorina Florean [:sflorean]]

Verified as fixed: build 60.0b5.
Device: Samsung Galaxy Tab S3 (Android 7.0) and Nexus 6(Android 6.0.1).
Screenshot: https://i.imgur.com/2n0aRkn.png.

Comment 26

[Sorina Florean [:sflorean]]

Verified as fixed  on latest Nightly build (61.0a1 - 04.17).
Device: Motorola Nexus 6(Android 7.1.1).