Enable the optional permission prompt persistence toggle on grouped infobars

chrome/browser/media/media_stream_devices_controller.cc

 // Copyright (c) 2012 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/media/media_stream_devices_controller.h"
 
 #include <map>
 #include <utility>
 
 #include "base/callback_helpers.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/content_settings/tab_specific_content_settings.h"
 #include "chrome/browser/media/media_capture_devices_dispatcher.h"
 #include "chrome/browser/media/media_permission.h"
 #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"
 #include "chrome/common/features.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #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"
 #include "content/public/common/media_stream_request.h"
 #include "content/public/common/origin_util.h"
 #include "extensions/common/constants.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
 #if BUILDFLAG(ANDROID_JAVA_UI)
@@ skipping 203 matching lines @@
   if (!IsAskingForAudio())
     message_id = IDS_MEDIA_CAPTURE_VIDEO_ONLY;
   else if (!IsAskingForVideo())
     message_id = IDS_MEDIA_CAPTURE_AUDIO_ONLY;
   return l10n_util::GetStringFUTF16(
       message_id,
       url_formatter::FormatUrlForSecurityDisplay(
           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
   // action metrics here, as this is a different sort of user action.
   RunCallback(CONTENT_SETTING_BLOCK,
               CONTENT_SETTING_BLOCK,
               content::MEDIA_DEVICE_PERMISSION_DENIED);
   set_persist(true);
 }
 
@@ skipping 189 matching lines @@
 }
 
 void MediaStreamDevicesController::RunCallback(
     ContentSetting audio_setting,
     ContentSetting video_setting,
     content::MediaStreamRequestResult denial_reason) {
   CHECK(!callback_.is_null());
 
   // 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.

   }
 
   content::MediaStreamDevices devices =
       GetDevices(audio_setting, video_setting);
 
   // If either audio or video are allowed then the callback should report
   // success, otherwise we report |denial_reason|.
   content::MediaStreamRequestResult request_result = content::MEDIA_DEVICE_OK;
   if (audio_setting != CONTENT_SETTING_ALLOW &&
       video_setting != CONTENT_SETTING_ALLOW) {
@@ skipping 93 matching lines @@
   if (content_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC)
     requested_device_id = request.requested_audio_device_id;
   else if (content_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA)
     requested_device_id = request.requested_video_device_id;
 
   if (!IsUserAcceptAllowed(content_type)) {
     *denial_reason = content::MEDIA_DEVICE_PERMISSION_DENIED;
     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);
     MediaPermission permission(content_type, request.security_origin,
         web_contents_->GetLastCommittedURL().GetOrigin(), profile_);
     return permission.GetPermissionStatusWithDeviceRequired(requested_device_id,
                                                             denial_reason);
   }
   // Return the default content setting if the device is not requested.
   return CONTENT_SETTING_DEFAULT;
@@ skipping 40 matching lines @@
   if (android_permission_blocked)
     return false;
 
   // Don't approve device requests if the tab was hidden.
   // TODO(qinmin): Add a test for this. http://crbug.com/396869.
   // TODO(raymes): Shouldn't this apply to all permissions not just audio/video?
   return web_contents_->GetRenderWidgetHostView()->IsShowing();
 #endif
   return true;
 }