Integrate MediaPermission with PermissionManager by using PermissionContext.

chrome/browser/media/media_permission.cc

 // Copyright 2015 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_permission.h"
 
 #include "chrome/browser/media/media_capture_devices_dispatcher.h"
 #include "chrome/browser/media/media_stream_device_permissions.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
-#include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "content/public/browser/permission_manager.h"
+#include "content/public/browser/permission_type.h"
 #include "content/public/common/url_constants.h"
 #include "extensions/common/constants.h"
 
+namespace {

[raymes, 2015/08/31 01:51:45]

nit: newline below

[guoweis_left_chromium, 2015/09/01 07:08:51]

Done.

+ContentSetting PermissionStatusToContentSetting(
+    content::PermissionStatus status) {
+  switch (status) {
+    case content::PERMISSION_STATUS_GRANTED:
+      return CONTENT_SETTING_ALLOW;
+    case content::PERMISSION_STATUS_DENIED:
+      return CONTENT_SETTING_BLOCK;
+    case content::PERMISSION_STATUS_ASK:
+      return CONTENT_SETTING_ASK;
+  }
+
+  NOTREACHED();
+  return CONTENT_SETTING_BLOCK;
+}
+
+content::PermissionType ContentSettingsTypeToPermission(
+    ContentSettingsType content_setting) {
+  switch (content_setting) {
+    case CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC:
+      return content::PermissionType::AUDIO_RECORDING;
+    case CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA:
+      return content::PermissionType::VIDEO_RECORDING;
+    default:
+      // This will hit the NOTREACHED below.
+      break;
+  }
+
+  NOTREACHED() << "Unexpected content setting for permission "
+               << static_cast<int>(content_setting);
+  return content::PermissionType::NUM;
+}
+
+}  // namespace
+
 MediaPermission::MediaPermission(ContentSettingsType content_type,
                                  content::MediaStreamRequestType request_type,
                                  const GURL& origin,
                                  Profile* profile)
     : content_type_(content_type),
       request_type_(request_type),
       origin_(origin),
       profile_(profile) {
 }
 
@@ skipping 20 matching lines @@
   // Deny the request if there is no device attached to the OS of the requested
   // type.
   if (!HasAvailableDevices(device_id)) {
     *denial_reason = content::MEDIA_DEVICE_NO_HARDWARE;
     return CONTENT_SETTING_BLOCK;
   }
 
   return GetPermissionStatus(denial_reason);
 }
 
 ContentSetting MediaPermission::GetStoredContentSetting() const {

[mlamouri (slow - plz ping), 2015/08/28 10:51:35]

Could you change MediaPermission::GetStoredContentSetting() to
MediaPermission::GetPermissionStatus() and return a PermissionStatus? It seems
that it would simply this code greatly.

[raymes, 2015/08/29 00:09:02]

This unfortunately comes as a result of doing a partial refactoring.
ContentSetting values are used throughout MediaStreamDevicesController which
uses this file. The end state that we want to get to is to remove
MediaStreamDevicesController altogether and have the callsites access the
PermissionContexts for mic/cam directly. 

I agree, though, that it seems nasty to have to do the
permissionstatus/contentsetting conversion in this file too. I'll have to have a
bit more of a think about how we can make this nicer in the short term. I'll try
to add more comments on Monday.

[raymes, 2015/08/31 01:51:45]

It's probably possible to do some refactoring in MediaStreamDevicesController to
use PermissionType but it would take some effort and I'm not sure if it's worth
the benefit without doing the full refactor to remove the class altogether.

Mounir, do you have anyone who might be interested in doing this refactoring to
replace MediaStreamDevicesController with PermissionContext? It would be a
pre-requisite for making the media PermissionContexts work properly.

[mlamouri (slow - plz ping), 2015/08/31 13:09:58]

Hmmm, my comment was only applying to GetStoredContentSettings but I didn't
realise that GetPermissionStatus above was using the returned value as a
ContentSettings which was used in more places.

I guess it's fine to keep that as-is then. Unfortunately, I can't think of any
one with cycles to take care of this refactoring.

-  // TODO(raymes): Merge this policy check into content settings
-  // crbug.com/244389.
-  const char* policy_name = nullptr;
-  const char* urls_policy_name = nullptr;
-  if (content_type_ == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) {
-    policy_name = prefs::kAudioCaptureAllowed;
-    urls_policy_name = prefs::kAudioCaptureAllowedUrls;
-  } else if (content_type_ == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA) {
-    policy_name = prefs::kVideoCaptureAllowed;
-    urls_policy_name = prefs::kVideoCaptureAllowedUrls;
-  } else {
-    NOTREACHED();
+  ContentSetting setting = CONTENT_SETTING_BLOCK;
+  if (profile_) {

[mlamouri (slow - plz ping), 2015/08/28 10:51:35]

Why are you checking for |profile_| while the previous code didn't?

[guoweis_left_chromium, 2015/09/01 07:08:51]

Done.

+    content::PermissionManager* permission_manager =
+        profile_->GetPermissionManager();
+    if (permission_manager) {

[mlamouri (slow - plz ping), 2015/08/28 10:51:35]

Maybe:
if (!permission_manager)
  return CONTENT_SETTING_BLOCK;

[guoweis_left_chromium, 2015/09/01 07:08:51]

Done.

+      setting = PermissionStatusToContentSetting(
+          permission_manager->GetPermissionStatus(
+              ContentSettingsTypeToPermission(content_type_), origin_,
+              origin_));

[mlamouri (slow - plz ping), 2015/08/28 10:51:35]

Shouldn't you take into account the embedder?

[raymes, 2015/08/29 00:09:02]

Mounir, you should read my replies :) As I mentioned in the other CL, the
current behavior for microphone and video permissions is not to consider the
embedder. Even if we wanted to make the behavioral change, this CL wouldn't be
the best place to do it :)

[mlamouri (slow - plz ping), 2015/08/31 13:09:58]

I remember your comment. Though, I would prefer the PermissionContext to make
that decision, not the callers. That way, we can easily change the policy
without having to track all the callers.

[raymes, 2015/08/31 23:32:49]

Ahh, sorry, that makes sense and I agree with you. 

Unfortunately we don't have access to the embedder easily here - we would have
to modify MediaStreamRequest and pass it through. We should add a TODO at least
but it might be best done separately.

[guoweis_left_chromium, 2015/09/01 07:08:51]

Done.

+    }
   }
 
-  MediaStreamDevicePolicy policy =
-      GetDevicePolicy(profile_, origin_, policy_name, urls_policy_name);
-
-  if (policy == ALWAYS_DENY)
-    return CONTENT_SETTING_BLOCK;
-
-  if (policy == ALWAYS_ALLOW)
-    return CONTENT_SETTING_ALLOW;
-
-  DCHECK(policy == POLICY_NOT_SET);
-  // Check the content setting.
-  ContentSetting setting =
-      profile_->GetHostContentSettingsMap()->GetContentSetting(
-          origin_, origin_, content_type_,
-          content_settings::ResourceIdentifier());
-
-  if (setting == CONTENT_SETTING_DEFAULT)
-    return CONTENT_SETTING_ASK;
-
   // TODO(raymes): This is here for safety to ensure that we always ask the user
   // even if a content setting is set to "allow" if the origin is insecure. In
   // reality we shouldn't really need to check this here as we should respect
   // the user's content setting. The problem is that pepper requests allow
   // insecure origins to be persisted. We should stop allowing this, do some
   // sort of migration and remove this check. See crbug.com/512301.
   if (!ShouldPersistContentSetting(setting, origin_, request_type_) &&
       !origin_.SchemeIs(extensions::kExtensionScheme) &&
       !origin_.SchemeIs(content::kChromeUIScheme) &&
       !origin_.SchemeIs(content::kChromeDevToolsScheme)) {
     return CONTENT_SETTING_ASK;
   }

[raymes, 2015/08/31 01:51:45]

I think this should be moved into the PermissionContext too, otherwise it's
going to return the wrong result for some requests. We can't pass in
request_type_ but what we can do is create an extra method hanging off the
PermissionContext: GetPermissionStatusForPepper() which would be called if
request_type == content:MEDIA_OPEN_DEVICE. We would change
ShouldPersistContentSetting to accept a bool is_pepper_request instead of
request_type.

[guoweis_left_chromium, 2015/09/01 07:08:51]

We can't use PermissionManager for this. Instead, we use PermissionContext::Get
and cast it to the MediaStreamDevicePermissionContext type for this.

 
   return setting;
 }
 
 bool MediaPermission::HasAvailableDevices(const std::string& device_id) const {
   const content::MediaStreamDevices* devices = nullptr;
   if (content_type_ == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) {
     devices =
         &MediaCaptureDevicesDispatcher::GetInstance()->GetAudioCaptureDevices();
   } else if (content_type_ == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA) {
@@ skipping 12 matching lines @@
     return false;
 
   // Note: we check device_id before dereferencing devices. If the requested
   // device id is non-empty, then the corresponding device list must not be
   // NULL.
   if (!device_id.empty() && !devices->FindById(device_id))
     return false;
 
   return true;
 }