Combine action parameters sent to Android displayNotification

chrome/browser/notifications/notification_platform_bridge_android.cc

 // Copyright 2014 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.
 
 #include "chrome/browser/notifications/notification_platform_bridge_android.h"
 
 #include <utility>
 #include <vector>
 
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/strings/nullable_string16.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/notifications/native_notification_display_service.h"
 #include "chrome/browser/notifications/notification.h"
 #include "chrome/browser/notifications/notification_common.h"
 #include "chrome/browser/notifications/notification_display_service_factory.h"
+#include "chrome/browser/notifications/notification_platform_types_android.h"
 #include "chrome/browser/notifications/persistent_notification_delegate.h"
 #include "chrome/browser/notifications/platform_notification_service_impl.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #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"
 #include "ui/gfx/image/image.h"
 
 using base::android::AttachCurrentThread;
 using base::android::ConvertJavaStringToUTF8;
 using base::android::ConvertJavaStringToUTF16;
 using base::android::ConvertUTF16ToJavaString;
 using base::android::ConvertUTF8ToJavaString;
 using base::android::JavaParamRef;
 using base::android::ScopedJavaLocalRef;
 
 namespace {
 
-ScopedJavaLocalRef<jobjectArray> ConvertToJavaBitmaps(
+ScopedJavaLocalRef<jobject> ConvertToJavaBitmap(JNIEnv* env,
+                                                const gfx::Image& icon) {
+  SkBitmap skbitmap = icon.AsBitmap();
+  ScopedJavaLocalRef<jobject> j_bitmap = NULL;

[Peter Beverloo, 2016/10/20 15:36:20]

nit: no need to assign NULL - that's the default constructor already.

[awdf, 2016/10/20 16:05:39]

Done.

+  if (!skbitmap.drawsNothing()) {

[Peter Beverloo, 2016/10/20 15:36:20]

micro nit: no {} for one line statements

[awdf, 2016/10/20 16:05:39]

Done.

+    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;
+  }

[Peter Beverloo, 2016/10/20 15:36:20]

Not all compilers like this. Nothing stops a static_cast<> from setting
|button.type| to 42. What do we return in that case?

Instead, it's common for us to output an error and choose a default value.

    NOTREACHED();
    return NotificationActionType::BUTTON;

It *should* never be reached. Where the switch() gives you compile time
guarantees, the NOTREACHED() does the same for runtime guarantees.

[awdf, 2016/10/20 16:05:39]

Done.

+}
+
+ScopedJavaLocalRef<jobjectArray> ConvertToJavaActionInfos(
     const std::vector<message_center::ButtonInfo>& buttons) {
-  std::vector<SkBitmap> skbitmaps;
-  for (const message_center::ButtonInfo& button : buttons)
-    skbitmaps.push_back(button.icon.AsBitmap());
-
   JNIEnv* env = AttachCurrentThread();
-  ScopedJavaLocalRef<jclass> clazz =
-      base::android::GetClass(env, "android/graphics/Bitmap");
-  jobjectArray array = env->NewObjectArray(skbitmaps.size(), clazz.obj(),
+  ScopedJavaLocalRef<jclass> clazz = base::android::GetClass(
+      env, "org/chromium/chrome/browser/notifications/ActionInfo");
+  jobjectArray array = env->NewObjectArray(buttons.size(), clazz.obj(),

[Peter Beverloo, 2016/10/20 15:36:20]

nit: s/array/actions/?

[awdf, 2016/10/20 16:05:39]

Done.

                                            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) {
+    ScopedJavaLocalRef<jstring> title =

[Peter Beverloo, 2016/10/20 15:36:20]

nit: consider having a const reference to the button as the first line in the
loop, allows you to remove all the "[i]"s.

const auto& button = buttons[i];

[awdf, 2016/10/20 16:05:39]

Done.

+        base::android::ConvertUTF16ToJavaString(env, buttons[i].title);
+    int type = GetNotificationActionType(buttons[i]);
+    ScopedJavaLocalRef<jstring> placeholder =
+        base::android::ConvertUTF16ToJavaString(env, buttons[i].placeholder);
+    ScopedJavaLocalRef<jobject> icon =
+        ConvertToJavaBitmap(env, buttons[i].icon);
+    ScopedJavaLocalRef<jobject> action_info =
+        chrome::Java_ActionInfo_createActionInfo(AttachCurrentThread(),
+                                                 title.obj(), icon.obj(), type,
+                                                 placeholder.obj());
+    env->SetObjectArrayElement(array, i, action_info.obj());
   }
 
   return ScopedJavaLocalRef<jobjectArray>(env, array);
 }
 
 // Callback to run once the profile has been loaded in order to perform a
 // given |operation| in a notification.
 // TODO(miguelg) move it to notification_common?
 void ProfileLoadedCallback(NotificationCommon::Operation operation,
                            NotificationCommon::Type notification_type,
@@ skipping 148 matching lines @@
   ScopedJavaLocalRef<jobject> notification_icon;
   SkBitmap notification_icon_bitmap = notification.icon().AsBitmap();
   if (!notification_icon_bitmap.drawsNothing())
     notification_icon = gfx::ConvertToJavaBitmap(&notification_icon_bitmap);
 
   ScopedJavaLocalRef<jobject> badge;
   SkBitmap badge_bitmap = notification.small_image().AsBitmap();
   if (!badge_bitmap.drawsNothing())
     badge = gfx::ConvertToJavaBitmap(&badge_bitmap);
 
   // TODO(crbug.com/650302): Combine these action_* vectors into a single vector

[awdf, 2016/10/20 15:20:34]

Argh, commented out code - thought I removed this. Will fix.

   // 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> action_infos =
+      ConvertToJavaActionInfos(notification.buttons());
 
   ScopedJavaLocalRef<jintArray> vibration_pattern =
       base::android::ToJavaIntArray(env, notification.vibration_pattern());
 
   ScopedJavaLocalRef<jstring> j_profile_id =
       ConvertUTF8ToJavaString(env, profile_id);
 
   Java_NotificationPlatformBridge_displayNotification(
       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(), action_infos);
 
   regenerated_notification_infos_[notification_id] =
       RegeneratedNotificationInfo(origin_url.spec(), notification.tag(),
                                   ConvertJavaStringToUTF8(env, webapk_package));
 }
 
 void NotificationPlatformBridgeAndroid::Close(
     const std::string& profile_id,
     const std::string& notification_id) {
   const auto iterator = regenerated_notification_infos_.find(notification_id);
@@ skipping 47 matching lines @@
     RegeneratedNotificationInfo() {}
 
 NotificationPlatformBridgeAndroid::RegeneratedNotificationInfo::
     RegeneratedNotificationInfo(const std::string& origin,
                                 const std::string& tag,
                                 const std::string& webapk_package)
     : origin(origin), tag(tag), webapk_package(webapk_package) {}
 
 NotificationPlatformBridgeAndroid::RegeneratedNotificationInfo::
     ~RegeneratedNotificationInfo() {}