refactor to Introduce AUDIO_CAPTURE and VIDEO_CAPTURE permissions.

chrome/browser/media/media_stream_device_permission_context.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_stream_device_permission_context.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"
+
+MediaStreamDevicePermissionContext::MediaStreamDevicePermissionContext(
+    Profile* profile,
+    const ContentSettingsType permission_type)
+    : PermissionContextBase(profile, permission_type),
+      permission_type_(permission_type) {

[raymes, 2015/08/27 05:54:18]

I'd suggest naming this content_type_ (to distinguish from PermissionType).

[xhwang, 2015/08/27 19:25:17]

+1, or just strictly follow the type name: content_settings_type_, which will
leave no room for ambiguity.

[guoweis_left_chromium, 2015/08/27 23:18:34]

Done.

+  CHECK(permission_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC ||

[raymes, 2015/08/27 05:54:18]

Should be DCHECK - use CHECK sparingly

[guoweis_left_chromium, 2015/08/27 23:18:34]

Done.

+        permission_type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA);

[mlamouri (slow - plz ping), 2015/08/27 10:03:40]

nit: you might want to check the member, not the argument.

[guoweis_left_chromium, 2015/08/27 23:18:34]

Done.

+}
+
+MediaStreamDevicePermissionContext::~MediaStreamDevicePermissionContext() {}
+
+void MediaStreamDevicePermissionContext::RequestPermission(
+    content::WebContents* web_contents,
+    const PermissionRequestID& id,
+    const GURL& requesting_frame,
+    bool user_gesture,
+    const BrowserPermissionCallback& callback) {
+  NOTREACHED() << "RequestPermission is not implemented";

[mlamouri (slow - plz ping), 2015/08/27 10:03:40]

This would only fail in DEBUG. Could you resolve the callback so a release build
wouldn't break.

[guoweis_left_chromium, 2015/08/27 23:18:34]

Done.

+}
+
+ContentSetting MediaStreamDevicePermissionContext::GetPermissionStatus(
+    const GURL& requesting_origin,
+    const GURL& embedding_origin) const {
+  // TODO(raymes): Merge this policy check into content settings
+  // crbug.com/244389.
+  const char* policy_name = nullptr;
+  const char* urls_policy_name = nullptr;
+  if (permission_type_ == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC) {
+    policy_name = prefs::kAudioCaptureAllowed;
+    urls_policy_name = prefs::kAudioCaptureAllowedUrls;

[xhwang, 2015/08/27 19:25:17]

err, here we are using another set of names:

AudioCapture
VideoCapture

Is there any reason we can't make the names consistent for, say, AudioCapture,
AudioRecording and Mic?

+  } else if (permission_type_ == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA) {
+    policy_name = prefs::kVideoCaptureAllowed;
+    urls_policy_name = prefs::kVideoCaptureAllowedUrls;
+  } else {
+    NOTREACHED();

[mlamouri (slow - plz ping), 2015/08/27 10:03:39]

NOTREACHED() probably not needed, we are checking this in the ctor already.
Alternatively, you can do:
if (permission_type_ == MEDIASTREAM_MIC) {
  // ...
} else {
  DCHECK_EQ(permission_type_, MEDIASTREAM_CAMERA);
  // ...
}

So at least, you only have two paths.

[guoweis_left_chromium, 2015/08/27 23:18:34]

Done.

+  }
+
+  MediaStreamDevicePolicy policy = GetDevicePolicy(
+      profile(), requesting_origin, policy_name, urls_policy_name);
+
+  if (policy == ALWAYS_DENY)
+    return CONTENT_SETTING_BLOCK;

[raymes, 2015/08/27 05:54:18]

nit: include components/content_settings/core/common/content_settings.h

[guoweis_left_chromium, 2015/08/27 23:18:34]

Done.

+
+  if (policy == ALWAYS_ALLOW)
+    return CONTENT_SETTING_ALLOW;
+
+  DCHECK_EQ(policy, POLICY_NOT_SET);

[raymes, 2015/08/27 05:54:18]

nit: the arguments are in the wrong order

[guoweis_left_chromium, 2015/08/27 23:18:34]

Done.

+  // Check the content setting.
+  ContentSetting setting = PermissionContextBase::GetPermissionStatus(
+      requesting_origin, embedding_origin);
+
+  if (setting == CONTENT_SETTING_DEFAULT)
+    return CONTENT_SETTING_ASK;
+
+  return setting;

[mlamouri (slow - plz ping), 2015/08/27 10:03:39]

nit: you might want to do:
return setting == CONTENT_SETTING_DEFAULT ? CONTENT_SETTING_ASK : setting;

[guoweis_left_chromium, 2015/08/27 23:18:34]

Done.

+}
+
+void MediaStreamDevicePermissionContext::ResetPermission(
+    const GURL& requesting_origin,
+    const GURL& embedding_origin) {
+  NOTREACHED() << "ResetPermission is not implemented";
+}
+
+void MediaStreamDevicePermissionContext::CancelPermissionRequest(
+    content::WebContents* web_contents,
+    const PermissionRequestID& id) {
+  NOTREACHED() << "CancelPermissionRequest is not implemented";
+}
+
+bool MediaStreamDevicePermissionContext::IsRestrictedToSecureOrigins() const {
+  return true;
+}

[raymes, 2015/08/27 05:54:18]

Mounir: we actually can't do this yet. The reason is that this will be used for
Flash too which currently does allow http origins to request audio/video.

[mlamouri (slow - plz ping), 2015/08/27 10:03:40]

I don't think we should try to use PermissionContext for Flash based
permissions.

[guoweis_left_chromium, 2015/08/27 23:18:34]

Are we saying to keep media_permission the same as it is without depending on
PermissionContext added here?