Add InfoBarIdentifier to GroupedPermissionInfoBarDelegate

chrome/browser/ui/android/infobars/grouped_permission_infobar.cc

 // 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.
 
 #include "chrome/browser/ui/android/infobars/grouped_permission_infobar.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "chrome/browser/android/resource_mapper.h"
-#include "chrome/browser/permissions/grouped_permission_infobar_delegate.h"
+#include "chrome/browser/permissions/grouped_permission_infobar_delegate_android.h"
 #include "jni/GroupedPermissionInfoBar_jni.h"
 
 GroupedPermissionInfoBar::GroupedPermissionInfoBar(
     std::unique_ptr<GroupedPermissionInfoBarDelegate> delegate)
     : ConfirmInfoBar(std::move(delegate)) {}
 
 GroupedPermissionInfoBar::~GroupedPermissionInfoBar() {
 }
 
 bool GroupedPermissionInfoBar::Register(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 void GroupedPermissionInfoBar::SetPermissionState(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& obj,
     const base::android::JavaParamRef<jbooleanArray>& permissions) {
-
-  for (int i = 0; i < GetDelegate()->GetPermissionCount(); i++) {
-      jboolean value;
-      env->GetBooleanArrayRegion(permissions.obj(), i, 1, &value);
-      GetDelegate()->ToggleAccept(i, value);
+  for (size_t i = 0; i < GetDelegate()->permission_count(); i++) {
+    jboolean value;
+    env->GetBooleanArrayRegion(permissions.obj(), i, 1, &value);
+    GetDelegate()->ToggleAccept(i, value);
   }

[Peter Kasting, 2016/09/23 00:42:06]

Nit: Longer-term consideration: Is this the only place we call ToggleAccept() on
this infobar?  It seems like the desired API is really that it sets all the
permissions' state at once, so maybe one call that takes n booleans (in an array
or vector or something) would be better than n one-arg calls.

I don't know JNI so I don't know precisely how to do this efficiently.

[lshang, 2016/09/27 01:50:43]

This is for future work. We're planning to reuse PermissionPrompt and
PermissionRequestManager on Android to improve feature parity between the two
platforms. So in that case, GroupedPermissionInfoBarDelegate will be used by
PermissionPrompt, and PermissionPrompt::Delegate expects to be called
one-at-a-time(https://cs.chromium.org/chromium/src/chrome/browser/ui/website_settings/permission_prompt.h?sq=package:chromium&l=33):-)

 }
 
 void GroupedPermissionInfoBar::ProcessButton(int action) {
   // Check if the delegate asked us to display a persistence toggle. If so,
   // inform it of the toggle state.
   GroupedPermissionInfoBarDelegate* delegate = GetDelegate();
   if (delegate->ShouldShowPersistenceToggle()) {
     delegate->set_persist(Java_GroupedPermissionInfoBar_isPersistSwitchOn(
         base::android::AttachCurrentThread(), GetJavaInfoBar()));
   }
@@ skipping 12 matching lines @@
       base::android::ConvertUTF16ToJavaString(
           env, GetTextFor(ConfirmInfoBarDelegate::BUTTON_OK));
   base::android::ScopedJavaLocalRef<jstring> cancel_button_text =
       base::android::ConvertUTF16ToJavaString(
           env, GetTextFor(ConfirmInfoBarDelegate::BUTTON_CANCEL));
 
   std::vector<base::string16> permission_strings;
   std::vector<int> permission_icons;
   std::vector<int> content_settings_types;
 
-  for (int i = 0; i < delegate->GetPermissionCount(); i++) {
+  for (size_t i = 0; i < delegate->permission_count(); i++) {
     permission_strings.push_back(delegate->GetMessageTextFragment(i));
     permission_icons.push_back(
         ResourceMapper::MapFromChromiumId(delegate->GetIconIdForPermission(i)));
     content_settings_types.push_back(delegate->GetContentSettingType(i));
   }

[Peter Kasting, 2016/09/23 00:42:06]

Nit: Similarly, if this is the only place we call these getters, maybe a single
getter that takes three vectors as outparams and fills all of them would make
more sense.

[lshang, 2016/09/27 01:50:43]

Probably will also need to get info at specific position by PermissionPrompt, as
above, in the following work. But I'm not 100% sure about this at the moment.
This is now also in consistent style with ToggleAccept().

I could come back to address this when we've implemented PermissionPromptAndroid
and find that these one-at-a-time getters are unnessasary indeed:-)

[Peter Kasting, 2016/09/27 02:05:43]

Seems reasonable to me to reevaluate then.

 
   return Java_GroupedPermissionInfoBar_create(
       env, message_text, ok_button_text, cancel_button_text,
       base::android::ToJavaIntArray(env, permission_icons),
       base::android::ToJavaArrayOfStrings(env, permission_strings),
       GetWindowAndroid(),
       base::android::ToJavaIntArray(env, content_settings_types),
       delegate->ShouldShowPersistenceToggle());
 }
 
 void GroupedPermissionInfoBar::SetJavaInfoBar(
     const base::android::JavaRef<jobject>& java_info_bar) {
   InfoBarAndroid::SetJavaInfoBar(java_info_bar);
   JNIEnv* env = base::android::AttachCurrentThread();
   Java_GroupedPermissionInfoBar_setNativePtr(env, java_info_bar,
                                              reinterpret_cast<intptr_t>(this));
 }
 
 GroupedPermissionInfoBarDelegate* GroupedPermissionInfoBar::GetDelegate() {
   return static_cast<GroupedPermissionInfoBarDelegate*>(delegate());
 }
-
-std::unique_ptr<infobars::InfoBar>
-GroupedPermissionInfoBarDelegate::CreateInfoBar(
-    infobars::InfoBarManager* infobar_manager,
-    std::unique_ptr<GroupedPermissionInfoBarDelegate> delegate) {
-  return std::unique_ptr<infobars::InfoBar>(
-      new GroupedPermissionInfoBar(std::move(delegate)));
-}