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 7a90f34d50cdd2ab0a81da8aeb74ce85b8d1bcd7..1015b4d243ee1d979bf4b7d8194ca7d05f79be4f 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.content.Context;
 import android.content.res.Resources;
 import android.graphics.Bitmap;
@@ -32,21 +33,31 @@ import javax.annotation.Nullable;
  */
 public abstract class NotificationBuilderBase {
     protected static class Action {
+        enum Type { BUTTON, TEXT }
+
         public int iconId;
         public Bitmap iconBitmap;
         public CharSequence title;
         public PendingIntent intent;
+        public Type type;
+        public String placeholder;
 
-        Action(int iconId, CharSequence title, PendingIntent intent) {
+        Action(int iconId, CharSequence title, PendingIntent intent, Type type,
+                String placeholder) {
             this.iconId = iconId;
             this.title = title;
             this.intent = intent;
+            this.type = type;
+            this.placeholder = placeholder;
         }
 
-        Action(Bitmap iconBitmap, CharSequence title, PendingIntent intent) {
+        Action(Bitmap iconBitmap, CharSequence title, PendingIntent intent, Type type,
+                String placeholder) {
             this.iconBitmap = iconBitmap;
             this.title = title;
             this.intent = intent;
+            this.type = type;
+            this.placeholder = placeholder;
         }
     }
 
@@ -201,11 +212,29 @@ public abstract class NotificationBuilderBase {
     }
 
     /**
-     * Adds an action to the notification. Actions are typically displayed as a button adjacent to
-     * the notification content.
+     * Adds an action to the notification, displayed as a button adjacent to the notification
+     * content.
      */
-    public NotificationBuilderBase addAction(@Nullable Bitmap iconBitmap,
+    public NotificationBuilderBase addButtonAction(@Nullable Bitmap iconBitmap,
             @Nullable CharSequence title, @Nullable PendingIntent intent) {
+        addAuthorProvidedAction(iconBitmap, title, intent, Action.Type.BUTTON, null);
+        return this;
+    }
+
+    /**
+     * Adds an action to the notification, displayed as a button adjacent to the notification
+     * content, which when tapped will display a text box within the notification for inline
+     * replies.
+     * @param placeholder - placeholder text for the text box

[Peter Beverloo, 2016/09/22 18:07:55]

nit: why document only |placeholder|? Either document everything, or nothing.

[awdf, 2016/09/23 15:24:56]

Done.

+     */
+    public NotificationBuilderBase addTextAction(@Nullable Bitmap iconBitmap,
+            @Nullable CharSequence title, @Nullable PendingIntent intent, String placeholder) {
+        addAuthorProvidedAction(iconBitmap, title, intent, Action.Type.TEXT, placeholder);
+        return this;
+    }
+
+    private void addAuthorProvidedAction(@Nullable Bitmap iconBitmap, @Nullable CharSequence title,
+            @Nullable PendingIntent intent, Action.Type actionType, String placeholder) {

[Peter Beverloo, 2016/09/22 18:07:55]

@Nullable String placeholder

(You pass NULL on line 220.)

[awdf, 2016/09/23 15:24:56]

Done.

         if (mActions.size() == MAX_AUTHOR_PROVIDED_ACTION_BUTTONS) {
             throw new IllegalStateException(
                     "Cannot add more than " + MAX_AUTHOR_PROVIDED_ACTION_BUTTONS + " actions.");
@@ -213,8 +242,7 @@ public abstract class NotificationBuilderBase {
         if (iconBitmap != null) {
             applyWhiteOverlayToBitmap(iconBitmap);
         }
-        mActions.add(new Action(iconBitmap, limitLength(title), intent));
-        return this;
+        mActions.add(new Action(iconBitmap, limitLength(title), intent, actionType, placeholder));
     }
 
     /**
@@ -222,7 +250,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, Action.Type.BUTTON, null);
         return this;
     }
 
@@ -368,15 +396,35 @@ public abstract class NotificationBuilderBase {
      * level is high enough, otherwise a resource id is used.
      */
     @SuppressWarnings("deprecation") // For addAction(int, CharSequence, PendingIntent)
-    @TargetApi(Build.VERSION_CODES.M) // For the Icon class.
     protected static void addActionToBuilder(Notification.Builder builder, Action action) {
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.KITKAT_WATCH) {
+            // Notification.Action.Builder and RemoteInput were added in API level 20.
+            Notification.Action.Builder actionBuilder = getActionBuilder(action);
+            if (action.type == Action.Type.TEXT) {
+                actionBuilder.addRemoteInput(
+                        new RemoteInput.Builder("key_text_reply")

[Peter Beverloo, 2016/09/22 18:07:55]

nit: make this a constant in NotificationConstants? (The project generally
doesn't like random strings with semantic meaning like this.)

[awdf, 2016/09/23 15:24:56]

Done.

I'm sorry for all these obvious rookie mistakes. I should have gone through this
properly and fixed it up first, or started from scratch, instead of asking you
to review the initial hacky patch (broke the rule of always throwing away
prototype code!)

Never mind though. I have now gone through it all so please continue reviewing
as normal.

What's your thoughts on pushing this (when ready) before the rest of the inline
reply work (to send on the replies) is done? I know it's good to commit little
and often instead of having a long-running branch, but this feature doesn't
really make sense without the rest, so, hide it behind a new flag? Or maybe
these changes are small and isolated enough that it's ok to keep them on my own
branch while working on the rest..?

[Peter Beverloo, 2016/09/23 16:01:22]

Don't be! Really, Chromium is a super complicated project with lots of
preferences, far from all of which are written down.
Absolutely. I'm a big fan of smaller patches - make them as small and granular
as you'd like, with the nit that it's a good idea to set the bar at "something
that's testable", no matter how bare bone.

[awdf, 2016/09/26 15:33:28]

Acknowledged.

+                                .setLabel(action.placeholder != null ? action.placeholder : "")
+                                .build());
+            }
+            builder.addAction(actionBuilder.build());
+        } else {
+            builder.addAction(action.iconId, action.title, action.intent);
+        }
+    }
+
+    @TargetApi(Build.VERSION_CODES.KITKAT_WATCH) // For Notification.Action.Builder
+    @SuppressWarnings("deprecation") // For Builder(int, CharSequence, PendingIntent)
+    private static Notification.Action.Builder getActionBuilder(Action action) {
+        Notification.Action.Builder actionBuilder;
         if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M && action.iconBitmap != null) {
+            // Icon was added in Android M.
             Icon icon = Icon.createWithBitmap(action.iconBitmap);
-            builder.addAction(
-                    new Notification.Action.Builder(icon, action.title, action.intent).build());
+            actionBuilder = new Notification.Action.Builder(icon, action.title, action.intent);
         } else {
-            builder.addAction(action.iconId, action.title, action.intent);
+            actionBuilder =
+                    new Notification.Action.Builder(action.iconId, action.title, action.intent);
         }
+        return actionBuilder;
     }
 
     /**