Show policy allowed URLs as policy exceptions in the media settings.

chrome/browser/ui/webui/options/content_settings_handler.cc

Index: chrome/browser/ui/webui/options/content_settings_handler.cc
diff --git a/chrome/browser/ui/webui/options/content_settings_handler.cc b/chrome/browser/ui/webui/options/content_settings_handler.cc
index f3a773cff6ea05c1e68904ac82354bfbdb1703d0..e975b16807e6e9e9e6e278e9dc23304a7cba9c8f 100644
--- a/chrome/browser/ui/webui/options/content_settings_handler.cc
+++ b/chrome/browser/ui/webui/options/content_settings_handler.cc
@@ -10,6 +10,7 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/command_line.h"
 #include "base/logging.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_number_conversions.h"
@@ -24,6 +25,7 @@
 #include "chrome/browser/notifications/desktop_notification_profile_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser_list.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
@@ -141,8 +143,7 @@ ContentSettingsType ContentSettingsTypeFromGroupName(const std::string& name) {
 
 // Create a DictionaryValue* that will act as a data source for a single row
 // in a HostContentSettingsMap-controlled exceptions table (e.g., cookies).
-// Ownership of the pointer is passed to the caller.
-base::DictionaryValue* GetExceptionForPage(
+scoped_ptr<base::DictionaryValue> GetExceptionForPage(
     const ContentSettingsPattern& pattern,
     const ContentSettingsPattern& secondary_pattern,
     const ContentSetting& setting,
@@ -155,13 +156,12 @@ base::DictionaryValue* GetExceptionForPage(
                            secondary_pattern.ToString());
   exception->SetString(kSetting, ContentSettingToString(setting));
   exception->SetString(kSource, provider_name);
-  return exception;
+  return make_scoped_ptr(exception);
 }
 
 // Create a DictionaryValue* that will act as a data source for a single row
-// in the Geolocation exceptions table. Ownership of the pointer is passed to
-// the caller.
-base::DictionaryValue* GetGeolocationExceptionForPage(
+// in the Geolocation exceptions table.
+scoped_ptr<base::DictionaryValue> GetGeolocationExceptionForPage(
     const ContentSettingsPattern& origin,
     const ContentSettingsPattern& embedding_origin,
     ContentSetting setting) {
@@ -169,13 +169,12 @@ base::DictionaryValue* GetGeolocationExceptionForPage(
   exception->SetString(kSetting, ContentSettingToString(setting));
   exception->SetString(kOrigin, origin.ToString());
   exception->SetString(kEmbeddingOrigin, embedding_origin.ToString());
-  return exception;
+  return make_scoped_ptr(exception);
 }
 
 // Create a DictionaryValue* that will act as a data source for a single row
-// in the desktop notifications exceptions table. Ownership of the pointer is
-// passed to the caller.
-base::DictionaryValue* GetNotificationExceptionForPage(
+// in the desktop notifications exceptions table.
+scoped_ptr<base::DictionaryValue> GetNotificationExceptionForPage(
     const ContentSettingsPattern& primary_pattern,
     const ContentSettingsPattern& secondary_pattern,
     ContentSetting setting,
@@ -189,7 +188,7 @@ base::DictionaryValue* GetNotificationExceptionForPage(
   exception->SetString(kOrigin, primary_pattern.ToString());
   exception->SetString(kEmbeddingOrigin, embedding_origin);
   exception->SetString(kSource, provider_name);
-  return exception;
+  return make_scoped_ptr(exception);
 }
 
 // Returns true whenever the |extension| is hosted and has |permission|.
@@ -518,11 +517,21 @@ void ContentSettingsHandler::InitializeHandler() {
                  base::Unretained(this),
                  CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC));
   pref_change_registrar_.Add(
+      prefs::kAudioCaptureAllowedUrls,
+      base::Bind(&ContentSettingsHandler::UpdateExceptionsViewFromModel,
+                 base::Unretained(this),
+                 CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC));
+  pref_change_registrar_.Add(
       prefs::kVideoCaptureAllowed,
       base::Bind(&ContentSettingsHandler::UpdateSettingDefaultFromModel,
                  base::Unretained(this),
                  CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA));
   pref_change_registrar_.Add(
+      prefs::kVideoCaptureAllowedUrls,
+      base::Bind(&ContentSettingsHandler::UpdateExceptionsViewFromModel,
+                 base::Unretained(this),
+                 CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA));
+  pref_change_registrar_.Add(
       prefs::kEnableDRM,
       base::Bind(
           &ContentSettingsHandler::UpdateProtectedContentExceptionsButton,
@@ -1099,6 +1108,49 @@ void ContentSettingsHandler::UpdateExceptionsViewFromOTRHostContentSettingsMap(
                                    type_string, exceptions);
 }
 
+scoped_ptr<base::ListValue> ContentSettingsHandler::GetPolicyAllowedUrls(
+    ContentSettingsType type) {
+  DCHECK(type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC ||
+         type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA);
+
+  PrefService* prefs = Profile::FromWebUI(web_ui())->GetPrefs();
+  const base::ListValue* policy_urls = prefs->GetList(
+      type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC
+          ? prefs::kAudioCaptureAllowedUrls
+          : prefs::kVideoCaptureAllowedUrls);
+
+  // Convert the URLs to |ContentSettingsPattern|s. Ignore any invalid ones.
+  std::vector<ContentSettingsPattern> patterns;
+  for (const base::Value* entry : *policy_urls) {
+    std::string url;
+    bool valid_string = entry->GetAsString(&url);
+    if (!valid_string)
+      continue;
+
+    ContentSettingsPattern pattern = ContentSettingsPattern::FromString(url);
+    if (!pattern.IsValid())
+      continue;
+
+    patterns.push_back(pattern);
+  }
+
+  // The patterns are shown in the UI in a reverse order defined by
+  // |ContentSettingsPattern::operator<|.
+  std::sort(
+      patterns.begin(), patterns.end(), std::greater<ContentSettingsPattern>());
+
+  scoped_ptr<base::ListValue> exceptions(new base::ListValue());
+  for (const ContentSettingsPattern& pattern : patterns) {
+    exceptions->Append(GetExceptionForPage(
+        pattern,
+        ContentSettingsPattern(),
+        CONTENT_SETTING_ALLOW,
+        kPolicyProviderId));
+  }
+
+  return exceptions.Pass();
+}
+
 void ContentSettingsHandler::GetExceptionsFromHostContentSettingsMap(
     const HostContentSettingsMap* map,
     ContentSettingsType type,
@@ -1128,8 +1180,10 @@ void ContentSettingsHandler::GetExceptionsFromHostContentSettingsMap(
 
   // Keep the exceptions sorted by provider so they will be displayed in
   // precedence order.
-  std::vector<std::vector<base::Value*> > all_provider_exceptions;
+  ScopedVector<base::ListValue> all_provider_exceptions;
   all_provider_exceptions.resize(HostContentSettingsMap::NUM_PROVIDER_TYPES);
+  for (auto& one_provider_exceptions : all_provider_exceptions)
+    one_provider_exceptions = new base::ListValue();
 
   // The all_patterns_settings is sorted from the lowest precedence pattern to
   // the highest (see operator< in ContentSettingsPattern), so traverse it in
@@ -1150,8 +1204,8 @@ void ContentSettingsHandler::GetExceptionsFromHostContentSettingsMap(
       parent = one_settings.find(ContentSettingsPattern::Wildcard());
 
     const std::string& source = i->first.second;
-    std::vector<base::Value*>* this_provider_exceptions =
-        &all_provider_exceptions.at(
+    base::ListValue* this_provider_exceptions =
+        all_provider_exceptions.get().at(

[Bernhard Bauer, 2015/07/28 15:35:31]

Could you just use the [] operator? The bounds checking that at() does might be
disabled in our builds anyway (because we don't support exceptions), and you
could in any case do it yourself.

[msramek, 2015/07/29 13:07:09]

Done. Since GetProviderTypeFromSource returns ProviderType and we allocate
NUM_PROVIDER_TYPES, the only thing to check is an invalid source argument, and
that will be handled by the NOTREACHED in GetProviderTypeFromSource.

             HostContentSettingsMap::GetProviderTypeFromSource(source));
 
     // Add the "parent" entry for the non-embedded setting.
@@ -1159,10 +1213,10 @@ void ContentSettingsHandler::GetExceptionsFromHostContentSettingsMap(
         parent == one_settings.end() ? CONTENT_SETTING_DEFAULT : parent->second;
     const ContentSettingsPattern& secondary_pattern =
         parent == one_settings.end() ? primary_pattern : parent->first;
-    this_provider_exceptions->push_back(GetExceptionForPage(primary_pattern,
-                                                            secondary_pattern,
-                                                            parent_setting,
-                                                            source));
+    this_provider_exceptions->Append(GetExceptionForPage(primary_pattern,
+                                                         secondary_pattern,
+                                                         parent_setting,
+                                                         source));
 
     // Add the "children" for any embedded settings.
     for (OnePatternSettings::const_iterator j = one_settings.begin();
@@ -1172,7 +1226,7 @@ void ContentSettingsHandler::GetExceptionsFromHostContentSettingsMap(
         continue;
 
       ContentSetting content_setting = j->second;
-      this_provider_exceptions->push_back(GetExceptionForPage(
+      this_provider_exceptions->Append(GetExceptionForPage(
           primary_pattern,
           j->first,
           content_setting,
@@ -1180,9 +1234,19 @@ void ContentSettingsHandler::GetExceptionsFromHostContentSettingsMap(
     }
   }
 
-  for (size_t i = 0; i < all_provider_exceptions.size(); ++i) {
-    for (size_t j = 0; j < all_provider_exceptions[i].size(); ++j) {
-      exceptions->Append(all_provider_exceptions[i][j]);
+  // For camera and microphone, we do not have policy exceptions, but we do have
+  // the policy-set allowed URLs, which should be displayed in the same manner.
+  if (type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC ||
+      type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA) {
+    base::ListValue* policy_exceptions = all_provider_exceptions.get().at(
+         HostContentSettingsMap::GetProviderTypeFromSource(kPolicyProviderId));
+    DCHECK(policy_exceptions->empty());
+    policy_exceptions->Swap(GetPolicyAllowedUrls(type).release());

[Bernhard Bauer, 2015/07/28 15:35:31]

Doesn't this leak the ListValue object that is returned by GetPolicyAllowedUrls
(which is now empty, but still a heap-allocated object)?

[msramek, 2015/07/29 13:07:09]

Good catch!

+  }
+
+  for (auto& one_provider_exceptions : all_provider_exceptions) {

[Bernhard Bauer, 2015/07/28 15:35:31]

Can you use const auto& here?

[msramek, 2015/07/29 13:07:09]

Done.

+    for (auto& exception : *one_provider_exceptions) {
+      exceptions->Append(exception->DeepCopy());

[Bernhard Bauer, 2015/07/28 15:35:31]

Hm... it's a pity that ListValue doesn't support releasing its contents. What
you could do to avoid the DeepCopy is call Remove() with index 0 repeatedly.

[msramek, 2015/07/29 13:07:09]

But ListValue uses std::vector (not std::list) under the hood, so that would be
quadratic.

Alternatively, we can Remove objects from the back, which will be linear, but
then we'll have to push them to a vector or stack to reverse them, and push them
back to |exceptions|. But that seems to me like a lot of boilerplate code for a
small performance gain.

So a third idea - don't reverse the exceptions while processing them, then
explain the reversion when appending (implemented in the last patchset).

[Bernhard Bauer, 2015/07/29 13:19:24]

Heh. I like that ☺

     }
   }
 }