Enable the optional permission prompt persistence toggle on grouped infobars

chrome/browser/media/media_stream_devices_controller.cc

Index: chrome/browser/media/media_stream_devices_controller.cc
diff --git a/chrome/browser/media/media_stream_devices_controller.cc b/chrome/browser/media/media_stream_devices_controller.cc
index 4f25a33c3114cd212f221514ea19c4ed564da28d..95437bb4fee41c63029864d969fb95f5722dad9d 100644
--- a/chrome/browser/media/media_stream_devices_controller.cc
+++ b/chrome/browser/media/media_stream_devices_controller.cc
@@ -19,6 +19,7 @@
 #include "chrome/browser/media/media_stream_capture_indicator.h"
 #include "chrome/browser/media/media_stream_device_permissions.h"
 #include "chrome/browser/permissions/permission_uma_util.h"
+#include "chrome/browser/permissions/permission_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/chrome_switches.h"
@@ -31,7 +32,6 @@
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/url_formatter/elide_url.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/permission_type.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_widget_host_view.h"
@@ -255,6 +255,16 @@ base::string16 MediaStreamDevicesController::GetMessageText() const {
           GetOrigin(), url_formatter::SchemeDisplay::OMIT_CRYPTOGRAPHIC));
 }
 
+content::PermissionType
+MediaStreamDevicesController::GetPermissionTypeForContentSettingsType(
+    ContentSettingsType content_type) const {
+  DCHECK(content_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC ||
+         content_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA);
+  content::PermissionType permission = content::PermissionType::NUM;
+  DCHECK(PermissionUtil::GetPermissionType(content_type, &permission));
+  return permission;
+}
+
 void MediaStreamDevicesController::ForcePermissionDeniedTemporarily() {
   set_persist(false);
   // TODO(tsergeant): Determine whether it is appropriate to record permission
@@ -464,8 +474,9 @@ void MediaStreamDevicesController::RunCallback(
 
   // If the kill switch is on we don't update the tab context or persist the
   // setting.
-  if (persist() && denial_reason != content::MEDIA_DEVICE_KILL_SWITCH_ON) {
-    StorePermission(audio_setting, video_setting);
+  if (denial_reason != content::MEDIA_DEVICE_KILL_SWITCH_ON) {
+    if (persist())

[raymes, 2016/08/22 04:37:19]

I don't see persist/set_persist defined anywhere.

[dominickn, 2016/08/22 05:30:02]

It's defined in PermissionRequest.

[raymes, 2016/08/22 05:35:23]

Oh..it's in the dependent CL. I feel like it could be better to pass this to
PermissionGranted/PermissionDenied since it's a bit harder to reason about the
state otherwise. At the very least we should ensure that set_persist is only
ever called once.

[dominickn, 2016/08/22 06:00:15]

I considered that. The problem would be that PermissionRequest has many
subclasses, each of which would need to take the persist boolean as an argument,
and ignore it. I talked this over with tsergeant and we agreed that solution was
worse because it introduced an otherwise ignored parameter to the download
request limiter request, the protocol request, etc.

WDYT? I don't really mind either way, but I feel like the way I've done it is
slightly nicer. The media stream device controller may set the variable multiple
times (see ForceDeniedTemporarily in the dependency CL for this patchset), so I
don't think logic to make sure set_persist is set once is appropriate.

[raymes, 2016/08/22 06:59:11]

I think that's a reasonable argument. My main concern would be that it is
meaningless for other subclasses.

I haven't looked closely at the dependent CL. I think we could untangle the two
by just putting the persist_ variable inside the MediaStreamDevicesController
for now and then we could look at the larger picture in the desktop CL. That
might help getting this landed quicker. Do you think that's possible and makes
sense?
The invariant that I was thinking of is that set_persist should never be called
after Accept/Deny or Granted/Denied because it won't do anything. We don't have
to add it in this CL but I think it could be a good sanity check.

[dominickn, 2016/08/22 07:08:44]

MediaStreamDevicesController currently has the persist boolean (it's called
persist_permission_changes_). The CL that this depends on upstreams that boolean
(and renames it to persist_) into PermissionRequest because PermissionManager
operates over a list of PermissionRequest objects; therefore passing persist_
must be somewhere on PermissionRequest or else I don't think we can push the
persistence state up from the UI code to the permission manager. For instance,
geolocation permissions use PermissionRequestImpl, not
MediaStreamDevicesController. Both get routed via PermissionManager through the
PermissionRequest base class.

The two options were PermissionGranted(bool) and PermissionDenied(bool), which
has the drawbacks I previously mentioned, or adding the boolean and a setter to
PermissionRequest, which contains the setting more cleanly.
Thanks for clarifying this; I'll see what I can do.

+      StorePermission(audio_setting, video_setting);
     UpdateTabSpecificContentSettings(audio_setting, video_setting);

[raymes, 2016/08/22 04:37:19]

I'm not sure this should actually be in this block either, though it's not
related to your CL. Perhaps kcarattini knows.

[dominickn, 2016/08/22 05:30:02]

Acknowledged. +kcarattini to look at.

[kcarattini, 2016/08/22 05:44:38]

This was done to suppress the Page Action icon from displaying when the kill
switch icon. This is consistent with the other permissions, but may not be 100%
correct if the function now does other things. That being said, the kill switch
is only on if there is a major API vulnerability, so this would not be the
normal everyday state.

[dominickn, 2016/08/22 06:00:15]

Acknowledged.

   }
 
@@ -580,14 +591,8 @@ ContentSetting MediaStreamDevicesController::GetContentSetting(
     return CONTENT_SETTING_BLOCK;
   }
 
-  content::PermissionType permission_type;
-  if (content_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) {
-    permission_type = content::PermissionType::AUDIO_CAPTURE;
-  } else {
-    DCHECK(content_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA);
-    permission_type = content::PermissionType::VIDEO_CAPTURE;
-  }
-
+  content::PermissionType permission_type =
+      GetPermissionTypeForContentSettingsType(content_type);
   if (ContentTypeIsRequested(permission_type, request)) {
     DCHECK(content::IsOriginSecure(request_.security_origin) ||
            request_.request_type == content::MEDIA_OPEN_DEVICE_PEPPER_ONLY);