Use vector of PermissionRequest instead of ContentSettingsTypes for GroupedPermissionInfoBarDelegate

chrome/browser/permissions/grouped_permission_infobar_delegate.cc

Index: chrome/browser/permissions/grouped_permission_infobar_delegate.cc
diff --git a/chrome/browser/permissions/grouped_permission_infobar_delegate.cc b/chrome/browser/permissions/grouped_permission_infobar_delegate.cc
index 6ea8084bb31673e93eb5b64aec9129488334cd86..6666c5fc6bb9712e6c62a159246f9db3bb6a7de3 100644
--- a/chrome/browser/permissions/grouped_permission_infobar_delegate.cc
+++ b/chrome/browser/permissions/grouped_permission_infobar_delegate.cc
@@ -2,19 +2,50 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/browser/media/webrtc/media_stream_devices_controller.h"

[tsergeant, 2016/09/22 06:53:56]

Is this include necessary?

[lshang, 2016/09/27 07:21:52]

Removed.

 #include "chrome/browser/permissions/grouped_permission_infobar_delegate.h"
+#include "chrome/browser/permissions/permission_request.h"
 #include "chrome/browser/permissions/permission_util.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/grit/theme_resources.h"
 #include "components/url_formatter/elide_url.h"
 #include "ui/base/l10n/l10n_util.h"
 
+namespace {
+
+ContentSettingsType PermissionRequestTypeToContentType(

[tsergeant, 2016/09/22 06:53:56]

I think it would be good to avoid adding this conversion method if possible. It
won't work for media, and PermissionRequestType is currently only used for UMA.

The easiest way would be to add a new method to PermissionRequest like

PermissionRequest::GetContentSettingsTypes()

that returns a vector of the content settings types for by that request.

You can then write an implementation for PermissionRequestImpl and
MediaStreamDevicesController, which are the only interesting cases.

Annoyingly, PermissionRequestImpl currently only has PermissionTypes, not
ContentSettingsTypes. I think it would be fine to write a conversion method in
there temporarily. Longer term, we might be able to convert the Android OS
permissions system to use PermissionTypes instead of ContentSettingsTypes.

[lshang, 2016/09/27 07:21:52]

I moved this conversion to PermissionRequest, but haven't override it in
media_stream_devices_controller, since I think now is not a proper time to add
support of media in grouped permission infobar delegate. See as below.

+    PermissionRequestType request_type) {
+  ContentSettingsType type = CONTENT_SETTINGS_TYPE_DEFAULT;
+  switch (request_type) {
+    case PermissionRequestType::PERMISSION_GEOLOCATION:
+      type = CONTENT_SETTINGS_TYPE_GEOLOCATION;
+      break;
+    case PermissionRequestType::PERMISSION_NOTIFICATIONS:
+      type = CONTENT_SETTINGS_TYPE_NOTIFICATIONS;
+      break;
+    case PermissionRequestType::PERMISSION_MIDI_SYSEX:
+      type = CONTENT_SETTINGS_TYPE_MIDI_SYSEX;
+      break;
+    case PermissionRequestType::PERMISSION_PUSH_MESSAGING:
+      type = CONTENT_SETTINGS_TYPE_NOTIFICATIONS;
+      break;
+    case PermissionRequestType::PERMISSION_PROTECTED_MEDIA_IDENTIFIER:
+      type = CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER;
+      break;
+    default:
+      NOTREACHED();
+  }
+  return type;
+}
+
+}  // namespace
+
 GroupedPermissionInfoBarDelegate::GroupedPermissionInfoBarDelegate(
     const GURL& requesting_origin,
-    const std::vector<ContentSettingsType>& types)
+    const std::vector<PermissionRequest*>& requests)
     : requesting_origin_(requesting_origin),
-      types_(types),
-      accept_states_(types_.size(), true),
+      requests_(requests),
+      accept_states_(requests_.size(), true),
       persist_(true) {}
 
 GroupedPermissionInfoBarDelegate::~GroupedPermissionInfoBarDelegate() {}
@@ -52,49 +83,34 @@ base::string16 GroupedPermissionInfoBarDelegate::GetMessageText() const {
 }
 
 int GroupedPermissionInfoBarDelegate::GetPermissionCount() const {
-  return types_.size();
+  return requests_.size();
 }
 
 ContentSettingsType GroupedPermissionInfoBarDelegate::GetContentSettingType(
     int index) const {
-  DCHECK(index >= 0 && index < static_cast<int>(types_.size()));
-  return types_[index];
+  DCHECK(index >= 0 && index < static_cast<int>(requests_.size()));
+  return PermissionRequestTypeToContentType(

[tsergeant, 2016/09/22 06:53:56]

The way that this method was written doesn't necessarily work once we have
PermissionRequests, since a single PermissionRequest can have multiple content
settings types (media will do this).

PermissionInfoBarDelegate now has a content_settings() method which returns the
full vector of content settings requested by an infobar.

Can we override that method and use it instead? This method is only called from
one place,
https://cs.chromium.org/chromium/src/chrome/browser/ui/android/infobars/group...

[lshang, 2016/09/27 07:21:52]

I think this CL is not ready for adding support for medias in grouped permission
infobar delegate, as we discussed, since if a request can have multiple content
settings types, we need to figure out what |index/position| means in getters and
ToggleAccept(), which might not be consistent. 

[tsergeant, 2016/09/28 00:01:06]

Yeah, you're right -- there are complications in the Android layer to do with
using multiple content settings types per request. This seems fine for now.

+      requests_[index]->GetPermissionRequestType());
 }
 
 int GroupedPermissionInfoBarDelegate::GetIconIdForPermission(int index) const {
-  DCHECK(index >= 0 && index < static_cast<int>(types_.size()));
-  ContentSettingsType type = types_[index];
-  if (type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) {
-    return IDR_INFOBAR_MEDIA_STREAM_MIC;
-  } else if (type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA) {
-    return IDR_INFOBAR_MEDIA_STREAM_CAMERA;
-  }
-  return IDR_INFOBAR_WARNING;
+  DCHECK(index >= 0 && index < static_cast<int>(requests_.size()));
+  return requests_[index]->GetIconId();
 }
 
 base::string16 GroupedPermissionInfoBarDelegate::GetMessageTextFragment(
     int index) const {
-  DCHECK(index >= 0 && index < static_cast<int>(types_.size()));
-  ContentSettingsType type = types_[index];
-  int message_id;
-  if (type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) {
-    message_id = IDS_MEDIA_CAPTURE_AUDIO_ONLY_PERMISSION_FRAGMENT;
-  } else if (type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA) {
-    message_id = IDS_MEDIA_CAPTURE_VIDEO_ONLY_PERMISSION_FRAGMENT;
-  } else {
-    NOTREACHED();
-    return base::string16();
-  }
-  return l10n_util::GetStringUTF16(message_id);
+  DCHECK(index >= 0 && index < static_cast<int>(requests_.size()));
+  return requests_[index]->GetMessageTextFragment();
 }
 
 void GroupedPermissionInfoBarDelegate::ToggleAccept(int position,
                                                     bool new_value) {
-  DCHECK(position >= 0 && position < static_cast<int>(types_.size()));
+  DCHECK(position >= 0 && position < static_cast<int>(requests_.size()));
   accept_states_[position] = new_value;
 }
 
 bool GroupedPermissionInfoBarDelegate::GetAcceptState(int position) {
-  DCHECK(position >= 0 && position < static_cast<int>(types_.size()));
+  DCHECK(position >= 0 && position < static_cast<int>(requests_.size()));
   return accept_states_[position];
 }