Site Settings Desktop: Support adding exceptions for incognito mode.

chrome/browser/ui/webui/site_settings_helper.cc

Index: chrome/browser/ui/webui/site_settings_helper.cc
diff --git a/chrome/browser/ui/webui/site_settings_helper.cc b/chrome/browser/ui/webui/site_settings_helper.cc
index a0b57cf132ffdb93416861016d055715c122dbe9..716baad3d1087f89e753e99151021bb8b29a4ba4 100644
--- a/chrome/browser/ui/webui/site_settings_helper.cc
+++ b/chrome/browser/ui/webui/site_settings_helper.cc
@@ -19,6 +19,7 @@ const char kSetting[] = "setting";
 const char kOrigin[] = "origin";
 const char kPolicyProviderId[] = "policy";
 const char kSource[] = "source";
+const char kIncognito[] = "incognito";
 const char kEmbeddingOrigin[] = "embeddingOrigin";
 const char kPreferencesSource[] = "preference";
 const char kObject[] = "object";
@@ -80,7 +81,8 @@ std::unique_ptr<base::DictionaryValue> GetExceptionForPage(
     const ContentSettingsPattern& pattern,
     const ContentSettingsPattern& secondary_pattern,
     const ContentSetting& setting,
-    const std::string& provider_name) {
+    const std::string& provider_name,
+    bool incognito) {
   base::DictionaryValue* exception = new base::DictionaryValue();
   exception->SetString(kOrigin, pattern.ToString());
   exception->SetString(kEmbeddingOrigin,
@@ -94,12 +96,14 @@ std::unique_ptr<base::DictionaryValue> GetExceptionForPage(
 
   exception->SetString(kSetting, setting_string);
   exception->SetString(kSource, provider_name);
+  exception->SetBoolean(kIncognito, incognito);
   return base::WrapUnique(exception);
 }
 
 void GetExceptionsFromHostContentSettingsMap(const HostContentSettingsMap* map,
                                              ContentSettingsType type,
                                              content::WebUI* web_ui,
+                                             bool incognito,
                                              base::ListValue* exceptions) {
   ContentSettingsForOneType entries;
   map->GetSettingsForOneType(type, std::string(), &entries);
@@ -157,7 +161,7 @@ void GetExceptionsFromHostContentSettingsMap(const HostContentSettingsMap* map,
     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));
+        primary_pattern, secondary_pattern, parent_setting, source, incognito));
 
     // Add the "children" for any embedded settings.
     for (OnePatternSettings::const_iterator j = one_settings.begin();
@@ -168,7 +172,7 @@ void GetExceptionsFromHostContentSettingsMap(const HostContentSettingsMap* map,
 
       ContentSetting content_setting = j->second;
       this_provider_exceptions.push_back(GetExceptionForPage(
-          primary_pattern, j->first, content_setting, source));
+          primary_pattern, j->first, content_setting, source, incognito));
     }
   }
 
@@ -179,7 +183,7 @@ void GetExceptionsFromHostContentSettingsMap(const HostContentSettingsMap* map,
     auto& policy_exceptions = all_provider_exceptions
         [HostContentSettingsMap::GetProviderTypeFromSource(kPolicyProviderId)];
     DCHECK(policy_exceptions.empty());
-    GetPolicyAllowedUrls(type, &policy_exceptions, web_ui);
+    GetPolicyAllowedUrls(type, &policy_exceptions, web_ui, incognito);
   }
 
   for (auto& one_provider_exceptions : all_provider_exceptions) {
@@ -191,7 +195,8 @@ void GetExceptionsFromHostContentSettingsMap(const HostContentSettingsMap* map,
 void GetPolicyAllowedUrls(
     ContentSettingsType type,
     std::vector<std::unique_ptr<base::DictionaryValue>>* exceptions,
-    content::WebUI* web_ui) {
+    content::WebUI* web_ui,
+    bool incognito) {
   DCHECK(type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_MIC ||
          type == CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA);
 
@@ -224,7 +229,7 @@ void GetPolicyAllowedUrls(
   for (const ContentSettingsPattern& pattern : patterns) {
     exceptions->push_back(GetExceptionForPage(pattern, ContentSettingsPattern(),
                                               CONTENT_SETTING_ALLOW,
-                                              kPolicyProviderId));
+                                              kPolicyProviderId, incognito));
   }
 }
 
@@ -242,6 +247,7 @@ std::unique_ptr<base::DictionaryValue> GetChooserExceptionForPage(
     const GURL& requesting_origin,
     const GURL& embedding_origin,
     const std::string& provider_name,
+    bool incognito,
     const std::string& name,
     const base::DictionaryValue* object) {
   std::unique_ptr<base::DictionaryValue> exception(new base::DictionaryValue());
@@ -255,6 +261,7 @@ std::unique_ptr<base::DictionaryValue> GetChooserExceptionForPage(
   exception->SetString(
       site_settings::kEmbeddingOrigin, embedding_origin.spec());
   exception->SetString(site_settings::kSource, provider_name);
+  exception->SetBoolean(kIncognito, incognito);

[stevenjb, 2016/09/01 15:47:55]

It *looks* like the only thing we use |incognito| for is to set
exception[kIncognito] = true.

If that is the case, I think this code would be more clear with a separate call:
site_settings::SetIncognito(exceptions).

Otherwise, anywhere we call these functions we need to document the boolean,
e.g. true /* incognito */ or false /* not incognito */. (Or use a named
variable).

[Finnur, 2016/09/01 16:48:17]

Correct. 

We send down (to .js) a single Array<SiteException> where some of the
SiteExceptions have the |incognito| bit set while others don't.

I think your comment can be read in two ways. Either you are saying:

1) Once we've gathered the exceptions for incognito, we should (before gathering
the other exceptions) iterate through the incognito exceptions and add the
incognito flag to each one. 

... OR ...

2) We should change what we send down to .js, from:
  Array<SiteException>
... to something like ...
[ {incognito : boolean, exceptions: Array<SiteException>}, ... ]


My concern about 1 is that it is less efficient than the current solution and my
concern about 2 is that changing the format we send down is a breaking change
for the old settings code, which I was hoping to avoid (with a non-breaking
change like adding an extra field)...

[stevenjb, 2016/09/01 17:07:54]

I was suggesting (1). I don't think the inefficiency of an extra call to
SetBoolean is a concern, but since you commented all of the calls to this,
that's fine.

   if (object) {
     exception->SetString(kObjectName, name);
     exception->Set(kObject, object->CreateDeepCopy());
@@ -316,8 +323,9 @@ void GetChooserExceptionsFromProfile(
 
       for (const auto& sorted_objects_entry : sorted_objects) {
         this_provider_exceptions.push_back(GetChooserExceptionForPage(
-            requesting_origin, embedding_origin, source,
-            sorted_objects_entry.first, sorted_objects_entry.second));
+            requesting_origin, embedding_origin, source, incognito,
+            sorted_objects_entry.first,
+            sorted_objects_entry.second));
       }
     }
 
@@ -326,7 +334,8 @@ void GetChooserExceptionsFromProfile(
       // where |requesting_origin| has been embedded.
       this_provider_exceptions.push_back(
           GetChooserExceptionForPage(requesting_origin, requesting_origin,
-                                     source, std::string(), nullptr));
+                                     source, incognito, std::string(),
+                                     nullptr));
 
       // Add the "children" for any embedded settings.
       for (const auto& one_origin_objects_entry : one_origin_objects) {
@@ -339,7 +348,7 @@ void GetChooserExceptionsFromProfile(
 
         for (const auto& sorted_objects_entry : sorted_objects) {
           this_provider_exceptions.push_back(GetChooserExceptionForPage(
-              requesting_origin, embedding_origin, source,
+              requesting_origin, embedding_origin, source, incognito,
               sorted_objects_entry.first, sorted_objects_entry.second));
         }
       }