Plumb through notification action types and placeholders on Android

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

Index: chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java
index c5b0fc072f4f25b359cd9df80dd291e1933047ca..9f884d868eb3011bde1a75ee45baab71c41cca42 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java
@@ -7,6 +7,7 @@ package org.chromium.chrome.browser.notifications;
 import android.annotation.TargetApi;
 import android.app.Notification;
 import android.app.PendingIntent;
+import android.app.RemoteInput;
 import android.graphics.Bitmap;
 import android.graphics.Canvas;
 import android.graphics.Color;
@@ -33,17 +34,21 @@ public abstract class NotificationBuilderBase {
         public Bitmap iconBitmap;
         public CharSequence title;
         public PendingIntent intent;
+        public RemoteInput remoteInput;

[Peter Beverloo, 2016/09/14 16:27:12]

Both the NotificationBuilderBase and the Action sub-class store the data, then
apply this when actually building the notification.

Should it perhaps be the responsibility of the NotificationBuilder to translate
the type/placeholder into a RemoteInput object too, as opposed to the
NotificationPlatformBridge doing so?

[awdf, 2016/09/22 14:17:11]

Done.

 
-        Action(int iconId, CharSequence title, PendingIntent intent) {
+        Action(int iconId, CharSequence title, PendingIntent intent, RemoteInput remoteInput) {
             this.iconId = iconId;
             this.title = title;
             this.intent = intent;
+            this.remoteInput = remoteInput;
         }
 
-        Action(Bitmap iconBitmap, CharSequence title, PendingIntent intent) {
+        Action(Bitmap iconBitmap, CharSequence title, PendingIntent intent,
+                RemoteInput remoteInput) {
             this.iconBitmap = iconBitmap;
             this.title = title;
             this.intent = intent;
+            this.remoteInput = remoteInput;
         }
     }
 
@@ -186,7 +191,25 @@ public abstract class NotificationBuilderBase {
         if (iconBitmap != null) {
             applyWhiteOverlayToBitmap(iconBitmap);
         }
-        mActions.add(new Action(iconBitmap, limitLength(title), intent));
+        mActions.add(new Action(iconBitmap, limitLength(title), intent, null));
+        return this;
+    }
+
+    /**
+     * Adds an action to the notification. Actions are typically displayed as a button adjacent to
+     * the notification content.
+     */
+    public NotificationBuilderBase addAction(@Nullable Bitmap iconBitmap,
+            @Nullable CharSequence title, @Nullable PendingIntent intent,
+            @Nullable RemoteInput remoteInput) {

[Peter Beverloo, 2016/09/14 16:27:12]

This gives me the feeling that we can't decide on an API contract and instead
settled on "give me whatever. or nothing." Not necessarily for this patch, but
we should really decide that e.g. |intent| must be passed.

[awdf, 2016/09/22 14:17:11]

Acknowledged.

+        if (mActions.size() == MAX_AUTHOR_PROVIDED_ACTION_BUTTONS) {
+            throw new IllegalStateException(
+                    "Cannot add more than " + MAX_AUTHOR_PROVIDED_ACTION_BUTTONS + " actions.");
+        }
+        if (iconBitmap != null) {
+            applyWhiteOverlayToBitmap(iconBitmap);
+        }
+        mActions.add(new Action(iconBitmap, limitLength(title), intent, remoteInput));
         return this;
     }
 
@@ -195,7 +218,7 @@ public abstract class NotificationBuilderBase {
      */
     public NotificationBuilderBase addSettingsAction(
             int iconId, @Nullable CharSequence title, @Nullable PendingIntent intent) {
-        mSettingsAction = new Action(iconId, limitLength(title), intent);
+        mSettingsAction = new Action(iconId, limitLength(title), intent, null);
         return this;
     }
 
@@ -272,10 +295,19 @@ public abstract class NotificationBuilderBase {
     protected static void addActionToBuilder(Notification.Builder builder, Action action) {
         if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M && action.iconBitmap != null) {
             Icon icon = Icon.createWithBitmap(action.iconBitmap);
-            builder.addAction(
-                    new Notification.Action.Builder(icon, action.title, action.intent).build());
+            Notification.Action.Builder actionBuilder =
+                    new Notification.Action.Builder(icon, action.title, action.intent);
+            if (action.remoteInput != null) {
+                actionBuilder.addRemoteInput(action.remoteInput);
+            }
+            builder.addAction(actionBuilder.build());
         } else {
-            builder.addAction(action.iconId, action.title, action.intent);
+            Notification.Action.Builder actionBuilder =
+                    new Notification.Action.Builder(action.iconId, action.title, action.intent);
+            if (action.remoteInput != null) {
+                actionBuilder.addRemoteInput(action.remoteInput);
+            }
+            builder.addAction(actionBuilder.build());

[Peter Beverloo, 2016/09/14 16:27:12]

The four lines of duplication are unfortunate. Could we instead differentiate
how we instantiate |actionBuilder|, and then have lines 307-310 in a common code
path?

[awdf, 2016/09/22 14:17:11]

Done.

         }
     }