refactor to Introduce AUDIO_CAPTURE and VIDEO_CAPTURE permissions.

chrome/browser/permissions/permission_manager.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/permissions/permission_manager.h"
 
 #include "base/callback.h"
 #include "chrome/browser/permissions/permission_context.h"
 #include "chrome/browser/permissions/permission_context_base.h"
 #include "chrome/browser/permissions/permission_request_id.h"
@@ skipping 45 matching lines @@
       return CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER;
 #else
       NOTIMPLEMENTED();
       break;
 #endif
     case PermissionType::DURABLE_STORAGE:
       return CONTENT_SETTINGS_TYPE_DURABLE_STORAGE;
     case PermissionType::MIDI:
       // This will hit the NOTREACHED below.
       break;
+    case PermissionType::AUDIO_CAPTURE:
+      return CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC;
+    case PermissionType::VIDEO_CAPTURE:
+      return CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA;
     case PermissionType::NUM:
       // This will hit the NOTREACHED below.

[palmer, 2015/09/01 18:31:04]

Same here: just put the default case inside the switch with an explicit default:
label.

(a) it's more straightforward ("The purpose of this function is to map 1 enum to
another, and nothing else"); (b) I wonder if the compiler finds it easier to
optimize such pure-switch functions.

[guoweis_left_chromium, 2015/09/01 19:36:34]

intentionally leave out default to have compiler catch the case when enum is
missing.

       break;
   }
 
   NOTREACHED() << "Unknown content setting for permission "
                << static_cast<int>(permission);
   return CONTENT_SETTINGS_TYPE_DEFAULT;
 }
 
 // Helper method that wraps a callback a void(PermissionStatus)
 // callback into a void(ContentSetting) callback.
@@ skipping 122 matching lines @@
     PermissionType permission,
     const GURL& requesting_origin,
     const GURL& embedding_origin) {
   if (IsConstantPermission(permission))
     return GetPermissionStatusForConstantPermission(permission);
 
   PermissionContextBase* context = PermissionContext::Get(profile_, permission);
   if (!context)
     return content::PERMISSION_STATUS_DENIED;
 
-  return ContentSettingToPermissionStatus(
-      context->GetPermissionStatus(requesting_origin.GetOrigin(),
-                                   embedding_origin.GetOrigin()));
+  return ContentSettingToPermissionStatus(context->GetPermissionStatus(
+      requesting_origin.GetOrigin(), embedding_origin.GetOrigin()));

[palmer, 2015/09/01 18:31:04]

I'm still really anxious about this. I believe it may be incorrect, given the
documented contract of GetOrigin:

166   // A helper function to return a GURL containing just the scheme, host,
167   // and port from a URL. Equivalent to clearing any username and password,
168   // replacing the path with a slash, and clearing everything after that. If
169   // this URL is not a standard URL, then the result will be an empty,
170   // invalid GURL. If the URL has neither username nor password, this
171   // degenerates to GetWithEmptyPath().
172   //
173   // It is an error to get the origin of an invalid URL. The result
174   // will be the empty URL.
175   GURL GetOrigin() const;

That is, all non-standard URLs will be equivalent, which is a violation of the
same origin policy. Note the implementation of the comparators:

182 bool GURL::operator==(const GURL& other) const {
183   return spec_ == other.spec_;
184 }
185 
186 bool GURL::operator!=(const GURL& other) const {
187   return spec_ != other.spec_;
188 }
189 
190 bool GURL::operator<(const GURL& other) const {
191   return spec_ < other.spec_;
192 }
193 
194 bool GURL::operator>(const GURL& other) const {
195   return spec_ > other.spec_;
196 }

[guoweis_left_chromium, 2015/09/01 19:36:34]

crbug.com/527149 filed for this.

 }
 
 void PermissionManager::RegisterPermissionUsage(PermissionType permission,
                                                 const GURL& requesting_origin,
                                                 const GURL& embedding_origin) {
   // This is required because constant permissions don't have a
   // ContentSettingsType.
   if (IsConstantPermission(permission))
     return;
 
@@ skipping 71 matching lines @@
     // Add the callback to |callbacks| which will be run after the loop to
     // prevent re-entrance issues.
     callbacks.push_back(
         base::Bind(subscription->callback,
                    ContentSettingToPermissionStatus(new_value)));
   }
 
   for (const auto& callback : callbacks)
     callback.Run();
 }