Constant notification ringing when downloading a file

VERIFIED FIXED in Firefox 63

Status

()

defect
VERIFIED FIXED
Last year
11 months ago

People

(Reporter: oana.horvath, Assigned: andrei.a.lazar)

Tracking

(Blocks 1 bug, {regression})

Firefox 63
Firefox 63
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 unaffected, firefox62 unaffected, firefox63+ verified)

Details

(Whiteboard: --do_not_change--[priority:high])

Attachments

(1 attachment)

Devices:
Samsung Galaxy S8 (Android 8.0)
Huawei Nexus 6P (Android 8.1.0)
Google Pixel (Android P beta)

Build: Nightly 63.0a1 (2018-07-19);

Steps to reproduce:
1. Download a large file or a build update.
2. Check the download progress push notification.

Expected result:
A simple notification should be displayed that the download has started.

Actual result:
The phone keeps ringing as the download goes on.


Notes:
not reproducing on 
Nokia 6 (Android 7.1.1)
HTC Desire 820 (Android 6.0.1)
I'm relatively certain that bug 1465102 isn't to blame here, which means bug 1450447 is the next likely candidate to have caused this.
Blocks: 1450447
Keywords: regression
Hardware: ARM → All
Assignee: nobody → andrei.a.lazar
See Also: → 1477700
Whiteboard: --do_not_change--[priority:high]
Comment on attachment 8994776 [details]
Bug 1476966 Constant notification ringing when downloading a file

https://reviewboard.mozilla.org/r/259310/#review266334

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:453
(Diff revision 1)
>              notificationManager.createNotificationChannel(defaultNotificationChannel);
>          }
>      }
>  
> +    @TargetApi(26)
> +    private void createProgressiveNotificationChannel() {

Unfortunately we can't fix the priority of the default channel for Nightly users that have already updated, but maybe we should still change it to something more appropriate if something still slips through to Beta and/or Release? We've [only been using a higher priority](https://dxr.mozilla.org/mozilla-central/search?q=path%3Amobile*java+notification+priority&redirect=false) for tab queue tabs and synced tabs I think (and it appears in the Leanplum thirdparty code, although no idea if that bit is actually used), so in hindsight using IMPORTANCE_HIGH for the default channel seems a bit questionable, but I guess it's easy to be wise after the fact.

But in order to fix things for Nightly users who have already updated, I guess we have no choice but to create an additional channel with better priority settings.

However, if we're doing that, wouldn't it make more sense to create a channel that actually makes sense from a user facing perspective, i.e. a channel specifically dedicated to Downloads?

So maybe leave this bug for creating the additional channel to unbreak users that have already updated as well, and then subsequently create (an) additional channel(s) for things that actually are high priority (tab queue/synced tabs arriving) and then change the priority of the default channel.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:455
(Diff revision 1)
>      }
>  
> +    @TargetApi(26)
> +    private void createProgressiveNotificationChannel() {
> +        final String PROGRESSIVE_CHANNEL = AppConstants.MOZ_APP_DISPLAYNAME + " progress channel";
> +        final String PROGRESSIVE_NAME = AppConstants.MOZ_APP_DISPLAYNAME + " progress channel";

The channel's name must be localisable.
Attachment #8994776 - Flags: review?(sdaswani) → review?(nchen)
Attachment #8994776 - Flags: review?(nchen)
Comment on attachment 8994776 [details]
Bug 1476966 Constant notification ringing when downloading a file

https://reviewboard.mozilla.org/r/259310/#review266418

Downloading an update causes the exact same problem as downloading a normal file, so can you please add a second patch adding an "Updates" notification channel and fix the updater's notification display to use that channel?

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:362
(Diff revision 1)
>  
>          IntentHelper.init();
>  
>          if (!AppConstants.Versions.preO) {
>              createDefaultNotificationChannel();
> +            createProgressiveNotificationChannel();

What I've written in bug 1477700 - I think it might make sense to group the whole NotificationChannel infrastructure in a separate helper class.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:455
(Diff revision 1)
>      }
>  
> +    @TargetApi(26)
> +    private void createProgressiveNotificationChannel() {
> +        final String PROGRESSIVE_CHANNEL = AppConstants.MOZ_APP_DISPLAYNAME + " progress channel";
> +        final String PROGRESSIVE_NAME = AppConstants.MOZ_APP_DISPLAYNAME + " progress channel";

What I've written in bug 1477700 - I don't think we should include the app name within the user-visible channel name.
Attachment #8994776 - Flags: review-
Comment on attachment 8994776 [details]
Bug 1476966 Constant notification ringing when downloading a file

https://reviewboard.mozilla.org/r/259310/#review266334

> The channel's name must be localisable.

It might be reasonable to re-use `R.string.downloads` here? (Originally the Downloads button in the main menu)
Blocks: 1479532
Duplicate of this bug: 1479574
Comment on attachment 8994776 [details]
Bug 1476966 Constant notification ringing when downloading a file

https://reviewboard.mozilla.org/r/259310/#review268806

This is fine with the comments fixed, but if you're going to handle only downloads in this patch, then please open a follow-up bug to fix the Updater service as well, since that one needs to
- use .setOnlyAlertOnce(true) as well, so we don't re-alert each time the download progress gets updated
- and generally use a separate and more low-key notifications channel, too, in order to match the previous behaviour on pre-O devices

::: mobile/android/base/java/org/mozilla/gecko/notifications/NotificationClient.java:159
(Diff revision 2)
>                          .bigText(alertText)
>                          .setSummaryText(host));
>  
>          if (!AppConstants.Versions.preO) {
>              builder.setChannelId(NotificationHelper.getInstance(mContext)
> -                    .getNotificationChannel(NotificationHelper.Channel.DEFAULT).getId());
> +                    .getNotificationChannel(NotificationHelper.Channel.DOWNLOAD).getId());

As per the javadoc for this function, this is used for web notifications and therefore doesn't belong in the downloads channel.

::: mobile/android/base/java/org/mozilla/gecko/notifications/NotificationClient.java:237
(Diff revision 2)
>          final Notification.Builder notificationBuilder = new Notification.Builder(mContext)
>                  .setContentText(alertText)
>                  .setSmallIcon(notification.icon)
>                  .setWhen(notification.when)
>                  .setContentIntent(notification.contentIntent)
> +                .setOnlyAlertOnce(true)

Over in the NotificationHelper you only `setOnlyAlertOnce(true)` for notifications that are explicitly marked as belonging to downloads, whereas here you're enabling it unconditionally for all kinds of notifications.

Just wondering, was that done intentionally, i.e. is the logic that we never want to re-alert after explicitly calling `update()` (which in practice seems unused, but that's neither here nor there now), but re-alerting after calling `add()` again is okay, except for the special case of downloads?

::: mobile/android/base/java/org/mozilla/gecko/notifications/NotificationClient.java:242
(Diff revision 2)
> +                .setOnlyAlertOnce(true)
>                  .setProgress((int) progressMax, (int) progress, false);
>  
>          if (!AppConstants.Versions.preO) {
>              notificationBuilder.setChannelId(NotificationHelper.getInstance(mContext)
> -                    .getNotificationChannel(NotificationHelper.Channel.DEFAULT).getId());
> +                    .getNotificationChannel(NotificationHelper.Channel.DOWNLOAD).getId());

This one isn't right, as at least in theory this function could handle app notifications other than downloads, too.

Just use `setChannelId(notification.getChannelId())` to reuse the channel already associated with that notification.

::: mobile/android/base/locales/en-US/android_strings.dtd:900
(Diff revision 2)
>  <!ENTITY pip_pause_button_description "Pause playing">
>  
>  <!-- Notification channels names -->
>  <!ENTITY default_notification_channel "&brandShortName;">
>  <!ENTITY mls_notification_channel "&vendorShortName; Location Service">
> +<!ENTITY download_notification_channel "Downloads notification">

See my comment for the media playback notification - I think just saying "Downloads" suffices.
Attachment #8994776 - Flags: review?(jh+bugzilla) → review+
See Also: → 1482105
Keywords: checkin-needed
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/925d2fd06a24
Constant notification ringing when downloading a file r=JanH
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/925d2fd06a24
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Depends on: 1482478
Duplicate of this bug: 1482553
Devices:
 - Nexus 5 (Android 6.0.1);
 - Huawei P10 (Android 8.0).

Verified in the latest Nightly following the steps provided in Comment 1, the issue is no longer reproducible.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.