Integrate MediaPermission with PermissionManager by using PermissionContext.

chrome/browser/media/media_permission.cc

Index: chrome/browser/media/media_permission.cc
diff --git a/chrome/browser/media/media_permission.cc b/chrome/browser/media/media_permission.cc
index d09f8c80b52693c1cf9ed00e1a760862df785fa3..60c47e3899c6c4081e549663011a33460ad1a572 100644
--- a/chrome/browser/media/media_permission.cc
+++ b/chrome/browser/media/media_permission.cc
@@ -8,10 +8,46 @@
 #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,
@@ -53,39 +89,18 @@ ContentSetting MediaPermission::GetPermissionStatusWithDeviceRequired(
 }
 
 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