[Android Notifications] Delete NotificationBuilderForO.java

- No longer necessary now that we compile against O and use a new
enough version of the support library that can handle channels.

Change-Id: I454d8cbc63744a277e05423998594e44cb4d7768
Reviewed-on: https://chromium-review.googlesource.com/1156550
Commit-Queue: Anita Woodruff <awdf@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579766}

chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilder.java

@@ -15,16 +15,23 @@ import android.os.Bundle;
 import android.support.v4.media.session.MediaSessionCompat;
 import android.widget.RemoteViews;
 
+import org.chromium.chrome.browser.notifications.channels.ChannelsInitializer;
+
 /**
  * Wraps a Notification.Builder object.
  */
 public class NotificationBuilder implements ChromeNotificationBuilder {
-    protected final Notification.Builder mBuilder;
+    private final Notification.Builder mBuilder;
     private final Context mContext;
 
-    public NotificationBuilder(Context context) {
+    NotificationBuilder(
+            Context context, String channelId, ChannelsInitializer channelsInitializer) {
         mContext = context;
         mBuilder = new Notification.Builder(mContext);
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
+            channelsInitializer.safeInitialize(channelId);
+            mBuilder.setChannelId(channelId);
+        }
     }
 
     @Override

chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderFactory.java

@@ -4,10 +4,8 @@
 
 package org.chromium.chrome.browser.notifications;
 
-import android.annotation.SuppressLint;
 import android.app.NotificationManager;
 import android.content.Context;
-import android.os.Build;
 
 import org.chromium.base.ContextUtils;
 import org.chromium.chrome.browser.notifications.channels.ChannelsInitializer;
@@ -19,14 +17,9 @@ import org.chromium.chrome.browser.notifications.channels.ChannelsInitializer;
 public class NotificationBuilderFactory {
     /**
      * Creates either a Notification.Builder or NotificationCompat.Builder under the hood, wrapped
-     * in our own common interface.
-     *
-     * TODO(crbug.com/704152) Remove this once we've updated to revision 26 of the support library.
-     * Then we can use NotificationCompat.Builder and set the channel directly everywhere.
-     * Although we will still need to ensure the channel is always initialized first.
+     * in our own common interface, and ensures the notification channel has been initialized.
      *
      * @param preferCompat true if a NotificationCompat.Builder is preferred.
-     *                     A Notification.Builder will be used regardless on Android O.
      * @param channelId The ID of the channel the notification should be posted to. This channel
      *                  will be created if it did not already exist. Must be a known channel within
      *                  {@link ChannelsInitializer#ensureInitialized(String)}.
@@ -34,19 +27,14 @@ public class NotificationBuilderFactory {
     public static ChromeNotificationBuilder createChromeNotificationBuilder(
             boolean preferCompat, String channelId) {
         Context context = ContextUtils.getApplicationContext();
-        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
-            return createNotificationBuilderForO(channelId, context);
-        }
-        return preferCompat ? new NotificationCompatBuilder(context)
-                            : new NotificationBuilder(context);
-    }
 
-    @SuppressLint("NewApi") // for Context.getSystemService(Class)
-    private static ChromeNotificationBuilder createNotificationBuilderForO(
-            String channelId, Context context) {
-        return new NotificationBuilderForO(context, channelId,
-                new ChannelsInitializer(new NotificationManagerProxyImpl(context.getSystemService(
-                                                NotificationManager.class)),
-                        context.getResources()));
+        NotificationManagerProxyImpl notificationManagerProxy = new NotificationManagerProxyImpl(
+                (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE));
+
+        ChannelsInitializer channelsInitializer =
+                new ChannelsInitializer(notificationManagerProxy, context.getResources());
+
+        return preferCompat ? new NotificationCompatBuilder(context, channelId, channelsInitializer)
+                            : new NotificationBuilder(context, channelId, channelsInitializer);
     }
 }

chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderForO.java

-// Copyright 2017 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-package org.chromium.chrome.browser.notifications;
-
-import android.annotation.TargetApi;
-import android.content.Context;
-import android.os.Build;
-import android.text.TextUtils;
-
-import org.chromium.chrome.browser.notifications.channels.ChannelsInitializer;
-import org.chromium.chrome.browser.webapps.WebApkServiceClient;
-
-/**
- * Builder to be used on Android O until we target O and these APIs are in the support library.
- */
-@TargetApi(Build.VERSION_CODES.O)
-public class NotificationBuilderForO extends NotificationBuilder {
-    private static final String TAG = "NotifBuilderForO";
-
-    public NotificationBuilderForO(
-            Context context, String channelId, ChannelsInitializer channelsInitializer) {
-        super(context);
-        assert Build.VERSION.SDK_INT >= Build.VERSION_CODES.O;
-        if (channelId == null) {
-            // The channelId may be null if the notification will be posted by another app that
-            // does not target O or sets its own channels. E.g. Web apk notifications.
-            return;
-        }
-
-        // If the channel ID matches {@link WebApkServiceClient#CHANNEL_ID_WEBAPKS}, we don't create
-        // the channel in Chrome. Instead, the channel will be created in WebAPKs.
-        if (!TextUtils.equals(channelId, WebApkServiceClient.CHANNEL_ID_WEBAPKS)) {
-            channelsInitializer.ensureInitialized(channelId);
-        }
-        mBuilder.setChannelId(channelId);
-    }
-}

chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java

@@ -9,19 +9,26 @@ import android.app.PendingIntent;
 import android.content.Context;
 import android.graphics.Bitmap;
 import android.graphics.drawable.Icon;
+import android.os.Build;
 import android.os.Bundle;
 import android.support.v4.app.NotificationCompat;
 import android.support.v4.media.session.MediaSessionCompat;
 import android.widget.RemoteViews;
 
+import org.chromium.chrome.browser.notifications.channels.ChannelsInitializer;
+
 /**
  * Wraps a NotificationCompat.Builder object.
  */
 public class NotificationCompatBuilder implements ChromeNotificationBuilder {
     private final NotificationCompat.Builder mBuilder;
 
-    public NotificationCompatBuilder(Context context) {
-        mBuilder = new NotificationCompat.Builder(context);
+    NotificationCompatBuilder(
+            Context context, String channelId, ChannelsInitializer channelsInitializer) {
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
+            channelsInitializer.safeInitialize(channelId);
+        }
+        mBuilder = new NotificationCompat.Builder(context, channelId);
     }
 
     @Override

chrome/android/java/src/org/chromium/chrome/browser/notifications/channels/ChannelsInitializer.java

@@ -10,8 +10,10 @@ import android.app.NotificationChannelGroup;
 import android.app.NotificationManager;
 import android.content.res.Resources;
 import android.os.Build;
+import android.text.TextUtils;
 
 import org.chromium.chrome.browser.notifications.NotificationManagerProxy;
+import org.chromium.chrome.browser.webapps.WebApkServiceClient;
 
 /**
  * Initializes our notification channels.
@@ -93,4 +95,22 @@ public class ChannelsInitializer {
         }
         mNotificationManager.createNotificationChannel(channel);
     }
+
+    /**
+     * This calls ensureInitialized after checking this isn't a WebAPK channel ID or null.
+     * @param channelId Id of the channel to be initialized.
+     */
+    public void safeInitialize(String channelId) {
+        // The channelId may be null if the notification will be posted by another app that
+        // does not target O or sets its own channels. E.g. WebAPK notifications.
+        if (channelId == null) {
+            return;
+        }
+        // If the channel ID matches {@link WebApkServiceClient#CHANNEL_ID_WEBAPKS}, we don't create
+        // the channel in Chrome. Instead, the channel will be created in WebAPKs.
+        if (TextUtils.equals(channelId, WebApkServiceClient.CHANNEL_ID_WEBAPKS)) {
+            return;
+        }
+        ensureInitialized(channelId);
+    }
 }

chrome/android/java_sources.gni

@@ -820,7 +820,6 @@ chrome_java_sources = [
   "java/src/org/chromium/chrome/browser/notifications/NotificationBuilder.java",
   "java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java",
   "java/src/org/chromium/chrome/browser/notifications/NotificationBuilderFactory.java",
-  "java/src/org/chromium/chrome/browser/notifications/NotificationBuilderForO.java",
   "java/src/org/chromium/chrome/browser/notifications/NotificationCompatBuilder.java",
   "java/src/org/chromium/chrome/browser/notifications/NotificationConstants.java",
   "java/src/org/chromium/chrome/browser/notifications/NotificationManagerProxy.java",
@@ -1847,6 +1846,7 @@ chrome_test_java_sources = [
   "javatests/src/org/chromium/chrome/browser/multiwindow/MultiWindowIntegrationTest.java",
   "javatests/src/org/chromium/chrome/browser/multiwindow/MultiWindowTestHelper.java",
   "javatests/src/org/chromium/chrome/browser/multiwindow/MultiWindowUtilsTest.java",
+  "javatests/src/org/chromium/chrome/browser/notifications/ChromeNotificationBuilderTest.java",
   "javatests/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilderTest.java",
   "javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeIntentTest.java",
   "javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeTest.java",

chrome/android/javatests/src/org/chromium/chrome/browser/notifications/ChromeNotificationBuilderTest.java

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.notifications;
+
+import android.app.Notification;
+import android.app.NotificationChannel;
+import android.app.NotificationManager;
+import android.content.Context;
+import android.os.Build;
+import android.support.test.InstrumentationRegistry;
+import android.support.test.filters.MediumTest;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.chromium.base.test.util.CommandLineFlags;
+import org.chromium.chrome.R;
+import org.chromium.chrome.browser.ChromeSwitches;
+import org.chromium.chrome.browser.notifications.channels.ChannelDefinitions;
+import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+
+/**
+ * Tests that ChromeNotificationBuilders created using
+ * {@link NotificationBuilderFactory#createChromeNotificationBuilder(boolean, String)} can be built
+ * and the notifications they build don't cause a crash when passed to NotificationManager#notify.
+ */
+@RunWith(ChromeJUnit4ClassRunner.class)
+@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
+public class ChromeNotificationBuilderTest {
+    private static final int TEST_NOTIFICATION_ID = 101;
+
+    private NotificationManager mNotificationManager;
+
+    @Before
+    public void setUp() {
+        Context context = InstrumentationRegistry.getTargetContext();
+
+        mNotificationManager =
+                (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE);
+
+        // Don't rely on channels already being registered.
+        clearNotificationChannels(mNotificationManager);
+    }
+
+    @After
+    public void tearDown() throws Exception {
+        // Let's leave things in a clean state.
+        mNotificationManager.cancelAll();
+    }
+
+    private static void clearNotificationChannels(NotificationManager notificationManager) {
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
+            for (NotificationChannel channel : notificationManager.getNotificationChannels()) {
+                if (!channel.getId().equals(NotificationChannel.DEFAULT_CHANNEL_ID)) {
+                    notificationManager.deleteNotificationChannel(channel.getId());
+                }
+            }
+        }
+    }
+
+    @MediumTest
+    @Test
+    public void buildNotificationAndNotifyDoesNotCrash() {
+        ChromeNotificationBuilder notificationBuilder =
+                NotificationBuilderFactory.createChromeNotificationBuilder(
+                        false, ChannelDefinitions.ChannelId.BROWSER);
+
+        Notification notification = notificationBuilder.setContentTitle("Title")
+                                            .setSmallIcon(R.drawable.ic_chrome)
+                                            .build();
+
+        mNotificationManager.notify(TEST_NOTIFICATION_ID, notification);
+    }
+
+    @MediumTest
+    @Test
+    public void buildCompatNotificationAndNotifyDoesNotCrash() {
+        ChromeNotificationBuilder notificationBuilder =
+                NotificationBuilderFactory.createChromeNotificationBuilder(
+                        true, ChannelDefinitions.ChannelId.BROWSER);
+
+        Notification notification = notificationBuilder.setContentTitle("Title")
+                                            .setSmallIcon(R.drawable.ic_chrome)
+                                            .build();
+
+        mNotificationManager.notify(TEST_NOTIFICATION_ID, notification);
+    }
+}
\ No newline at end of file