Combine action parameters sent to Android displayNotification

- Combine the 4 notification action arrays sent through JNI
into a single array of ActionInfo java objects

- Pass action type as a java-exported enum/int

BUG=650302

Review-Url: https://chromiumcodereview.appspot.com/2440483002
Cr-Commit-Position: refs/heads/master@{#426535}

chrome/android/BUILD.gn

@@ -277,6 +277,7 @@ java_cpp_enum("chrome_android_java_enums_srcjar") {
     "//chrome/browser/android/policy/policy_auditor.cc",
     "//chrome/browser/android/shortcut_info.h",
     "//chrome/browser/android/tab_android.h",
+    "//chrome/browser/notifications/notification_platform_bridge_android.cc",
     "//chrome/browser/profiles/profile_metrics.h",
     "//chrome/browser/ui/android/infobars/infobar_android.h",
   ]

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

+// Copyright 2016 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.graphics.Bitmap;
+
+import org.chromium.base.annotations.CalledByNative;
+
+/**
+ * Helper class for passing notification action information over the JNI.
+ */
+class ActionInfo {
+    public final String title;
+    public final Bitmap icon;
+    public final int type;
+    public final String placeholder;
+
+    private ActionInfo(String title, Bitmap icon, int type, String placeholder) {
+        this.title = title;
+        this.icon = icon;
+        this.type = type;
+        this.placeholder = placeholder;
+    }
+
+    @CalledByNative
+    private static ActionInfo createActionInfo(
+            String title, Bitmap icon, int type, String placeholder) {
+        return new ActionInfo(title, icon, type, placeholder);
+    }
+}

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

@@ -458,7 +458,6 @@ public class NotificationPlatformBridge {
     /**
      * Displays a notification with the given details.
      *
-     * TODO(crbug.com/650302): Combine the 'action*' parameters into a single array of objects.
      * @param notificationId The id of the notification.
      * @param origin Full text of the origin, including the protocol, owning this notification.
      * @param profileId Id of the profile that showed the notification.
@@ -482,22 +481,14 @@ public class NotificationPlatformBridge {
      * @param renotify Whether the sound, vibration, and lights should be replayed if the
      *                 notification is replacing another notification.
      * @param silent Whether the default sound, vibration and lights should be suppressed.
-     * @param actionTitles Titles of actions to display alongside the notification.
-     * @param actionIcons Icons of actions to display alongside the notification.
-     * @param actionTypes Types of actions to display alongside the notification.
-     * @param actionPlaceholders Placeholders of actions to display alongside the notification.
+     * @param actions Action buttons to display alongside the notification.
      * @see https://developer.android.com/reference/android/app/Notification.html
      */
     @CalledByNative
     private void displayNotification(String notificationId, String origin, String profileId,
             boolean incognito, String tag, String webApkPackage, String title, String body,
             Bitmap image, Bitmap icon, Bitmap badge, int[] vibrationPattern, long timestamp,
-            boolean renotify, boolean silent, String[] actionTitles, Bitmap[] actionIcons,
-            String[] actionTypes, String[] actionPlaceholders) {
-        if (actionTitles.length != actionIcons.length) {
-            throw new IllegalArgumentException("The number of action titles and icons must match.");
-        }
-
+            boolean renotify, boolean silent, ActionInfo[] actions) {
         Resources res = mAppContext.getResources();
 
         // Record whether it's known whether notifications can be shown to the user at all.
@@ -541,19 +532,19 @@ public class NotificationPlatformBridge {
                         .setOrigin(UrlFormatter.formatUrlForSecurityDisplay(
                                 origin, false /* showScheme */));
 
-        for (int actionIndex = 0; actionIndex < actionTitles.length; actionIndex++) {
+        for (int actionIndex = 0; actionIndex < actions.length; actionIndex++) {
             PendingIntent intent = makePendingIntent(
                     NotificationConstants.ACTION_CLICK_NOTIFICATION, notificationId, origin,
                     profileId, incognito, tag, webApkPackage, actionIndex);
+            ActionInfo action = actions[actionIndex];
             // Don't show action button icons when there's an image, as then action buttons go on
             // the same row as the Site Settings button, so icons wouldn't leave room for text.
-            Bitmap actionIcon = hasImage ? null : actionIcons[actionIndex];
-            // TODO(crbug.com/650302): Encode actionTypes with an enum, not a magic string!
-            if (actionTypes[actionIndex].equals("text")) {
-                notificationBuilder.addTextAction(actionIcon, actionTitles[actionIndex], intent,
-                        actionPlaceholders[actionIndex]);
+            Bitmap actionIcon = hasImage ? null : action.icon;
+            if (action.type == NotificationActionType.TEXT) {
+                notificationBuilder.addTextAction(
+                        actionIcon, action.title, intent, action.placeholder);
             } else {
-                notificationBuilder.addButtonAction(actionIcon, actionTitles[actionIndex], intent);
+                notificationBuilder.addButtonAction(actionIcon, action.title, intent);
             }
         }
 
@@ -561,7 +552,7 @@ public class NotificationPlatformBridge {
         // label and icon, so abbreviate it. This has the unfortunate side-effect of unnecessarily
         // abbreviating it on Android Wear also (crbug.com/576656). If custom layouts are enabled,
         // the label and icon provided here only affect Android Wear, so don't abbreviate them.
-        boolean abbreviateSiteSettings = actionTitles.length > 0 && !useCustomLayouts(hasImage);
+        boolean abbreviateSiteSettings = actions.length > 0 && !useCustomLayouts(hasImage);
         int settingsIconId = abbreviateSiteSettings ? 0 : R.drawable.settings_cog;
         CharSequence settingsTitle = abbreviateSiteSettings
                                      ? res.getString(R.string.notification_site_settings_button)

chrome/android/java_sources.gni

@@ -519,6 +519,7 @@ chrome_java_sources = [
   "java/src/org/chromium/chrome/browser/nfc/BeamCallback.java",
   "java/src/org/chromium/chrome/browser/nfc/BeamController.java",
   "java/src/org/chromium/chrome/browser/nfc/BeamProvider.java",
+  "java/src/org/chromium/chrome/browser/notifications/ActionInfo.java",
   "java/src/org/chromium/chrome/browser/notifications/CustomNotificationBuilder.java",
   "java/src/org/chromium/chrome/browser/notifications/GoogleServicesNotificationController.java",
   "java/src/org/chromium/chrome/browser/notifications/NotificationBuilderBase.java",

chrome/browser/BUILD.gn

@@ -3853,6 +3853,7 @@ if (android_java_ui) {
       "../android/java/src/org/chromium/chrome/browser/mojo/ChromeInterfaceRegistrar.java",
       "../android/java/src/org/chromium/chrome/browser/net/qualityprovider/ExternalEstimateProviderAndroid.java",
       "../android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java",
+      "../android/java/src/org/chromium/chrome/browser/notifications/ActionInfo.java",
       "../android/java/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridge.java",
       "../android/java/src/org/chromium/chrome/browser/ntp/ForeignSessionHelper.java",
       "../android/java/src/org/chromium/chrome/browser/ntp/LogoBridge.java",

chrome/browser/notifications/notification_platform_bridge_android.cc

@@ -27,6 +27,7 @@
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "content/public/common/persistent_notification_status.h"
 #include "content/public/common/platform_notification_data.h"
+#include "jni/ActionInfo_jni.h"
 #include "jni/NotificationPlatformBridge_jni.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/android/java_bitmap.h"
@@ -42,27 +43,59 @@ using base::android::ScopedJavaLocalRef;
 
 namespace {
 
-ScopedJavaLocalRef<jobjectArray> ConvertToJavaBitmaps(
-    const std::vector<message_center::ButtonInfo>& buttons) {
-  std::vector<SkBitmap> skbitmaps;
-  for (const message_center::ButtonInfo& button : buttons)
-    skbitmaps.push_back(button.icon.AsBitmap());
+// A Java counterpart will be generated for this enum.
+// GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.notifications
+enum NotificationActionType {
+  // NB. Making this a one-line enum breaks code generation! crbug.com/657847
+  BUTTON,
+  TEXT
+};
+
+ScopedJavaLocalRef<jobject> ConvertToJavaBitmap(JNIEnv* env,
+                                                const gfx::Image& icon) {
+  SkBitmap skbitmap = icon.AsBitmap();
+  ScopedJavaLocalRef<jobject> j_bitmap;
+  if (!skbitmap.drawsNothing())
+    j_bitmap = gfx::ConvertToJavaBitmap(&skbitmap);
+  return j_bitmap;
+}
+
+NotificationActionType GetNotificationActionType(
+    message_center::ButtonInfo button) {
+  switch (button.type) {
+    case message_center::ButtonType::BUTTON:
+      return NotificationActionType::BUTTON;
+    case message_center::ButtonType::TEXT:
+      return NotificationActionType::TEXT;
+  }
+  NOTREACHED();
+  return NotificationActionType::TEXT;
+}
 
+ScopedJavaLocalRef<jobjectArray> ConvertToJavaActionInfos(
+    const std::vector<message_center::ButtonInfo>& buttons) {
   JNIEnv* env = AttachCurrentThread();
-  ScopedJavaLocalRef<jclass> clazz =
-      base::android::GetClass(env, "android/graphics/Bitmap");
-  jobjectArray array = env->NewObjectArray(skbitmaps.size(), clazz.obj(),
-                                           nullptr /* initialElement */);
+  ScopedJavaLocalRef<jclass> clazz = base::android::GetClass(
+      env, "org/chromium/chrome/browser/notifications/ActionInfo");
+  jobjectArray actions = env->NewObjectArray(buttons.size(), clazz.obj(),
+                                             nullptr /* initialElement */);
   base::android::CheckException(env);
 
-  for (size_t i = 0; i < skbitmaps.size(); ++i) {
-    if (!skbitmaps[i].drawsNothing()) {
-      env->SetObjectArrayElement(
-          array, i, gfx::ConvertToJavaBitmap(&(skbitmaps[i])).obj());
-    }
+  for (size_t i = 0; i < buttons.size(); ++i) {
+    const auto& button = buttons[i];
+    ScopedJavaLocalRef<jstring> title =
+        base::android::ConvertUTF16ToJavaString(env, button.title);
+    int type = GetNotificationActionType(button);
+    ScopedJavaLocalRef<jstring> placeholder =
+        base::android::ConvertUTF16ToJavaString(env, button.placeholder);
+    ScopedJavaLocalRef<jobject> icon = ConvertToJavaBitmap(env, button.icon);
+    ScopedJavaLocalRef<jobject> action_info =
+        Java_ActionInfo_createActionInfo(AttachCurrentThread(), title.obj(),
+                                         icon.obj(), type, placeholder.obj());
+    env->SetObjectArrayElement(actions, i, action_info.obj());
   }
 
-  return ScopedJavaLocalRef<jobjectArray>(env, array);
+  return ScopedJavaLocalRef<jobjectArray>(env, actions);
 }
 
 // Callback to run once the profile has been loaded in order to perform a
@@ -228,36 +261,8 @@ void NotificationPlatformBridgeAndroid::Display(
   if (!badge_bitmap.drawsNothing())
     badge = gfx::ConvertToJavaBitmap(&badge_bitmap);
 
-  // TODO(crbug.com/650302): Combine these action_* vectors into a single vector
-  // of objects.
-  std::vector<base::string16> action_titles_vector;
-  std::vector<base::string16> action_types_vector;
-  std::vector<base::string16> action_placeholders_vector;
-  for (const message_center::ButtonInfo& button : notification.buttons()) {
-    action_titles_vector.push_back(button.title);
-    action_placeholders_vector.push_back(button.placeholder);
-    base::string16 type;
-    switch (button.type) {
-      case message_center::ButtonType::BUTTON:
-        type = base::ASCIIToUTF16("button");
-        break;
-      case message_center::ButtonType::TEXT:
-        type = base::ASCIIToUTF16("text");
-        break;
-    }
-    action_types_vector.push_back(std::move(type));
-  }
-  ScopedJavaLocalRef<jobjectArray> action_titles =
-      base::android::ToJavaArrayOfStrings(env, action_titles_vector);
-
-  ScopedJavaLocalRef<jobjectArray> action_types =
-      base::android::ToJavaArrayOfStrings(env, action_types_vector);
-
-  ScopedJavaLocalRef<jobjectArray> action_placeholders =
-      base::android::ToJavaArrayOfStrings(env, action_placeholders_vector);
-
-  ScopedJavaLocalRef<jobjectArray> action_icons =
-      ConvertToJavaBitmaps(notification.buttons());
+  ScopedJavaLocalRef<jobjectArray> actions =
+      ConvertToJavaActionInfos(notification.buttons());
 
   ScopedJavaLocalRef<jintArray> vibration_pattern =
       base::android::ToJavaIntArray(env, notification.vibration_pattern());
@@ -269,8 +274,7 @@ void NotificationPlatformBridgeAndroid::Display(
       env, java_object_, j_notification_id, j_origin, j_profile_id, incognito,
       tag, webapk_package, title, body, image, notification_icon, badge,
       vibration_pattern, notification.timestamp().ToJavaTime(),
-      notification.renotify(), notification.silent(), action_titles,
-      action_icons, action_types, action_placeholders);
+      notification.renotify(), notification.silent(), actions);
 
   regenerated_notification_infos_[notification_id] =
       RegeneratedNotificationInfo(origin_url.spec(), notification.tag(),