1450447 - Start using notification channels

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Jan Henning [:JanH]]

As per https://developer.android.com/guide/topics/ui/notifiers/notifications.html#ManageChannels, we must start using notification channels if we target API26 or higher.

Comment 1

[Andrei Lazar]

Attachment: Bug 1450447 - Start using notification channels.

Made all the notifications within the app to use notification channel for devices with API26 and higher.

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

Comment 2

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

Hey Andrei – I'll get to this tomorrow.

Comment 3

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

Comment on attachment 8979635 [details]
Bug 1450447 - Start using notification channels.

https://reviewboard.mozilla.org/r/245774/#review253078

This looks like it'd work but it's using the deprecated Notification.Builder constructor: we might as well fix that now so we don't have to fix that later.

We can also improve performance and the UX create notification channels on process init, and accessing those channels from the NotificationManager later: see my comments below.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
(Diff revision 1)
>                  .action(R.string.updater_permission_allow)
>                  .callback(allowCallback)
>                  .buildAndShow();
>      }
>  
> -    private void conditionallyNotifyEOL() {

I'm guessing this was removed because it's unused? 

For future reference, it helps reviewers if "Remove unused code" are divided into separate commits because then it's easy to scan, say, "seems reasonable to remove" and move on to the next commit. Instead, I have to figure out why it was removed.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
(Diff revision 1)
>  
> -    private void conditionallyNotifyEOL() {
> -        final StrictMode.ThreadPolicy savedPolicy = StrictMode.allowThreadDiskReads();
> -        try {
> -            final SharedPreferences prefs = GeckoSharedPrefs.forProfile(this);
> -            if (!prefs.contains(EOL_NOTIFIED)) {

nit: EOL_NOTIFIED can be removed, I think

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
(Diff revision 1)
> -        try {
> -            final SharedPreferences prefs = GeckoSharedPrefs.forProfile(this);
> -            if (!prefs.contains(EOL_NOTIFIED)) {
> -
> -                // Launch main App to load SUMO url on EOL notification.
> -                final String link = getString(R.string.eol_notification_url,

nit: unused

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
(Diff revision 1)
> -                        .setContentTitle(getString(R.string.eol_notification_title))
> -                        .setContentText(getString(R.string.eol_notification_summary))

nit: unused

::: mobile/android/base/java/org/mozilla/gecko/DataReportingNotification.java:100
(Diff revision 1)
>              String tickerString = resources.getString(R.string.datareporting_notification_ticker_text);
>              SpannableString tickerText = new SpannableString(tickerString);
>              // Bold the notification title of the ticker text, which is the same string as notificationTitle.
>              tickerText.setSpan(new StyleSpan(Typeface.BOLD), 0, notificationTitle.length(), Spannable.SPAN_INCLUSIVE_EXCLUSIVE);
>  
> -            Notification notification = new NotificationCompat.Builder(context)
> +            Notification.Builder notificationBuilder = new Notification.Builder(context)

This builder is deprecated: we should use `NotificationCompat.Builder(Context, String)

::: mobile/android/base/java/org/mozilla/gecko/DataReportingNotification.java:112
(Diff revision 1)
>                                                                          .bigText(notificationBigSummary))
>                                          .addAction(R.drawable.firefox_settings_alert, notificationAction, contentIntent)
> -                                        .setTicker(tickerText)
> -                                        .build();
> +                                        .setTicker(tickerText);
> +
> +            if (!AppConstants.Versions.preO) {
> +                notificationBuilder.setChannelId(new GeckoNotificationChannel(context).getChannel().getId());

We should the channelId init to the non-deprecated constructor.

::: mobile/android/base/java/org/mozilla/gecko/notifications/GeckoNotificationChannel.java:1
(Diff revision 1)
> +package org.mozilla.gecko.notifications;

nit: include file license

::: mobile/android/base/java/org/mozilla/gecko/notifications/GeckoNotificationChannel.java:10
(Diff revision 1)
> +import android.app.NotificationManager;
> +import android.content.Context;
> +import android.support.annotation.NonNull;
> +
> +@TargetApi(26)
> +public class GeckoNotificationChannel {

nit: Rename to one of
- `DefaultNotificationChannel`
- `NotificationChannelWrapper`, which can have `getDefaultChannel...` methods or `getChannel(ChannelType)` methods where `ChannelType` is an enum for our channels (which is just default atm).

`Gecko*` is a legacy naming scheme from the JS/C++ programmers who initially wrote the app to namespace things but namespacing isn't as important in Java since we have imports and the absolute package names (e.g. `org.mozilla.gecko.notifications.GeckoNotificationChannel.createNotificationChannel(...);`).

::: mobile/android/base/java/org/mozilla/gecko/notifications/GeckoNotificationChannel.java:21
(Diff revision 1)
> +
> +    public GeckoNotificationChannel(@NonNull Context context) {
> +        this(context, DEFAULT_CHANNEL, DEFAULT_NAME, DEFAULT_IMPORTANCE);
> +    }
> +
> +    public GeckoNotificationChannel(@NonNull Context context, String channel, String name, int importance) {

This object/constructor wraps the `notificationManager.createNotificationChannel`, which seems redundant to me.

Could we:
- Create the notification channels in Application.init
- Create a static method in this class called, `getDefaultChannelId` which delegates to `notificationManager.getNotificationChannel` or some default string, based on API level.

This will:
- Remove the redundant wrapper
- Reduce allocations
- Create the notification channels when the app starts, which means users won't need to receive their first notification in order to see the default channel in their settings.

::: mobile/android/base/java/org/mozilla/gecko/notifications/NotificationClient.java:315
(Diff revision 1)
>          mForegroundNotification = name;
>  
>          final Intent intent = new Intent(mContext, NotificationService.class);
>          intent.putExtra(NotificationService.EXTRA_NOTIFICATION, notification);
> +
> +        if(AppConstants.Versions.preO) {

nit: run checkstyle locally (or push to CI): this style is not right

::: mobile/android/base/java/org/mozilla/gecko/notifications/NotificationClient.java:318
(Diff revision 1)
>          intent.putExtra(NotificationService.EXTRA_NOTIFICATION, notification);
> +
> +        if(AppConstants.Versions.preO) {
> -        mContext.startService(intent);
> +            mContext.startService(intent);
> +        } else {
> +            mContext.startForegroundService(intent);

nit: Do we have to do this in this bug?

Comment 4

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

Another thing to note: we have a Notifications preference built into the app to enable/disable different types of preferences. Ideally, we'd make this preference deep-link to the NotificationChannel Settings on O+ and use are in-app setting for prior releases. That being said, that's a lot of work and a PITA to maintain.

Andrei, do you mind filing a follow-up bug and NI'ing Susheel on it so that we can prioritize this more complex functionality over the MVP that we have in this bug? Thanks. (To be explicit, I think we're doing the right amount of work in this bug already :)

Comment 5

[Andrei Lazar]

Comment on attachment 8979635 [details]
Bug 1450447 - Start using notification channels.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245774/diff/1-2/

Comment 6

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

Looks like Jan has the review here (if that's okay with you, Jan!).

Comment 7

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

Comment on attachment 8979635 [details]
Bug 1450447 - Start using notification channels.

https://reviewboard.mozilla.org/r/245774/#review261874

::: mobile/android/base/java/org/mozilla/gecko/DataReportingNotification.java:110
(Diff revision 2)
>                                          .setStyle(new NotificationCompat.BigTextStyle()
>                                                                          .bigText(notificationBigSummary))
>                                          .addAction(R.drawable.firefox_settings_alert, notificationAction, contentIntent)
> -                                        .setTicker(tickerText)
> -                                        .build();
> +                                        .setTicker(tickerText);
> +
> +            if (!AppConstants.Versions.preO) {

Just use `Build.VERSION.SDK_INT >= 26`

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:420
(Diff revision 2)
> +        final String DEFAULT_CHANNEL = "Fennec channel";
> +        final String DEFAULT_NAME = "Fennec";

These should be the product name from `AppConstants.MOZ_APP_DISPLAYNAME`

Comment 8

[Andrei Lazar]

Comment on attachment 8979635 [details]
Bug 1450447 - Start using notification channels.

https://reviewboard.mozilla.org/r/245774/#review253078

> nit: Rename to one of
> - `DefaultNotificationChannel`
> - `NotificationChannelWrapper`, which can have `getDefaultChannel...` methods or `getChannel(ChannelType)` methods where `ChannelType` is an enum for our channels (which is just default atm).
> 
> `Gecko*` is a legacy naming scheme from the JS/C++ programmers who initially wrote the app to namespace things but namespacing isn't as important in Java since we have imports and the absolute package names (e.g. `org.mozilla.gecko.notifications.GeckoNotificationChannel.createNotificationChannel(...);`).

Totally removed this wrapper, as you said it was pretty redundant indeed, but I just wanted to hide the whole get/create process for these channels.

> nit: Do we have to do this in this bug?

Nope, sorry, I forgot this line when I had to test some notifications which came from a service (with target 26), thank you!

Comment 9

[Andrei Lazar]

Comment on attachment 8979635 [details]
Bug 1450447 - Start using notification channels.

https://reviewboard.mozilla.org/r/245774/#review261874

> Just use `Build.VERSION.SDK_INT >= 26`

I am using preO constant for its purpose, why should I change it to this?

Comment 10

[Andrei Lazar]

Comment on attachment 8979635 [details]
Bug 1450447 - Start using notification channels.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245774/diff/2-3/

Comment 11

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1450447 - Start using notification channels.

Made all the notifications within the app to use notification channel for devices with API26 and higher.

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce0a6b10d392
Start using notification channels. r=jchen

Comment 13

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

Comment on attachment 8992032 [details]
Bug 1450447 - Start using notification channels.

https://reviewboard.mozilla.org/r/256934/#review264140

::: mobile/android/base/java/org/mozilla/gecko/DataReportingNotification.java:68
(Diff revision 1)
>      }
>  
>      /**
>       * Launch a notification of the data policy, and record notification time and version.
>       */
> +    @SuppressWarnings("NewApi")

Can we get rid of `@SuppressWarnings("NewApi")` if we use `Build.VERSION.SDK_INT` instead of `AppConstants.Versions`?

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

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