Content Settings: Add RevocationObserver to measure when permissions are revoked

components/content_settings/core/browser/host_content_settings_map.cc

Index: components/content_settings/core/browser/host_content_settings_map.cc
diff --git a/components/content_settings/core/browser/host_content_settings_map.cc b/components/content_settings/core/browser/host_content_settings_map.cc
index d1151eab85af2a51fa622b979cda1d05674ff2a3..a3cd4f9fc69dd6fb2590f962dbe0ed0dc1fdbe42 100644
--- a/components/content_settings/core/browser/host_content_settings_map.cc
+++ b/components/content_settings/core/browser/host_content_settings_map.cc
@@ -453,6 +453,9 @@ void HostContentSettingsMap::SetContentSettingDefaultScope(
           content_type);
   DCHECK(info);
 
+  ContentSetting previous_value = GetContentSetting(
+      primary_url, secondary_url, content_type, resource_identifier);
+
   content_settings::PatternPair patterns =
       GetPatternsFromScopingType(info->website_settings_info()->scoping_type(),
                                  primary_url, secondary_url);
@@ -463,6 +466,20 @@ void HostContentSettingsMap::SetContentSettingDefaultScope(
 
   SetContentSetting(primary_pattern, secondary_pattern, content_type,
                     resource_identifier, setting);
+
+  ContentSetting final_value = GetContentSetting(
+      primary_url, secondary_url, content_type, resource_identifier);
+
+  bool final_value_not_allowed =

[raymes, 2016/03/17 02:05:43]

Can you just check whether it's not ALLOW?

[tsergeant, 2016/03/17 06:25:40]

Doing that would not trigger in the case where the permission goes from ALLOW ->
ASK and the default for that content setting type is ALLOW. 

I think this goes back to the question about whether we want to capture user
intent, or capture exactly what happens in the backend.

Kendra, what do you think?

[tsergeant, 2016/03/21 03:38:37]

As discussed, I've simplified this logic without changing the behavior. I was
misunderstanding how content settings works.

+      final_value == CONTENT_SETTING_BLOCK ||
+      (final_value == CONTENT_SETTING_ASK &&
+       GetDefaultContentSetting(content_type, nullptr) !=
+           CONTENT_SETTING_ALLOW);
+
+  if (previous_value == CONTENT_SETTING_ALLOW && final_value_not_allowed) {
+    NotifyRevocation(primary_url, secondary_url, content_type,
+                     resource_identifier);
+  }
 }
 
 ContentSetting HostContentSettingsMap::GetContentSettingAndMaybeUpdateLastUsage(
@@ -531,6 +548,16 @@ void HostContentSettingsMap::RemoveObserver(
   observers_.RemoveObserver(observer);
 }
 
+void HostContentSettingsMap::AddRevocationObserver(
+    content_settings::RevocationObserver* observer) {
+  revocation_observers_.AddObserver(observer);
+}
+
+void HostContentSettingsMap::RemoveRevocationObserver(
+    content_settings::RevocationObserver* observer) {
+  revocation_observers_.RemoveObserver(observer);
+}
+
 void HostContentSettingsMap::FlushLossyWebsiteSettings() {
   prefs_->SchedulePendingLossyWrites();
 }
@@ -747,6 +774,19 @@ scoped_ptr<base::Value> HostContentSettingsMap::GetWebsiteSettingInternal(
   return scoped_ptr<base::Value>();
 }
 
+void HostContentSettingsMap::NotifyRevocation(
+    const GURL& primary_url,
+    const GURL& secondary_url,
+    ContentSettingsType content_type,
+    const std::string& resource_identifier) {
+  FOR_EACH_OBSERVER(content_settings::RevocationObserver,
+                    revocation_observers_,
+                    OnContentSettingRevoked(primary_url,
+                                            secondary_url,
+                                            content_type,
+                                            resource_identifier));

[raymes, 2016/03/17 02:05:43]

nit: perhaps just inline this for now?

[tsergeant, 2016/03/17 06:25:40]

Done.

+}
+
 // static
 scoped_ptr<base::Value>
 HostContentSettingsMap::GetContentSettingValueAndPatterns(