Add revocation reporter to permission util

chrome/browser/permissions/permission_util.cc

Index: chrome/browser/permissions/permission_util.cc
diff --git a/chrome/browser/permissions/permission_util.cc b/chrome/browser/permissions/permission_util.cc
index 46ab398ee943b3140578bbae7517f609ad64676d..265f3929220b2000d3e06951bffd4cf4becd5210 100644
--- a/chrome/browser/permissions/permission_util.cc
+++ b/chrome/browser/permissions/permission_util.cc
@@ -9,6 +9,7 @@
 #include "chrome/browser/permissions/permission_uma_util.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "content/public/browser/permission_type.h"
+#include "url/gurl.h"
 
 using content::PermissionType;
 
@@ -78,33 +79,59 @@ bool PermissionUtil::GetPermissionType(ContentSettingsType type,
   return true;
 }
 
-void PermissionUtil::SetContentSettingAndRecordRevocation(
+PermissionUtil::RevocationReporter::RevocationReporter(
     Profile* profile,
     const GURL& primary_url,
     const GURL& secondary_url,
     ContentSettingsType content_type,
     std::string resource_identifier,
-    ContentSetting setting) {
+    PermissionSourceUI source_ui)
+    : profile_(profile),
+      primary_url_(primary_url),
+      secondary_url_(secondary_url),
+      content_type_(content_type),
+      resource_identifier_(resource_identifier),
+      source_ui_(source_ui) {
+  if (!primary_url_.is_valid() ||
+      (!secondary_url_.is_valid() && !secondary_url_.is_empty())) {
+    is_initially_allowed_ = false;
+  }
   HostContentSettingsMap* map =
-      HostContentSettingsMapFactory::GetForProfile(profile);
-  ContentSetting previous_value = map->GetContentSetting(
-      primary_url, secondary_url, content_type, resource_identifier);
-
-  map->SetContentSettingDefaultScope(primary_url, secondary_url, content_type,
-                                     resource_identifier, setting);
+      HostContentSettingsMapFactory::GetForProfile(profile_);
+  ContentSetting initial_content_setting = map->GetContentSetting(
+      primary_url_, secondary_url_, content_type_, resource_identifier_);
+  is_initially_allowed_ = (initial_content_setting == CONTENT_SETTING_ALLOW);
+}
 
-  ContentSetting final_value = map->GetContentSetting(
-      primary_url, secondary_url, content_type, resource_identifier);
+PermissionUtil::RevocationReporter::RevocationReporter(
+    Profile* profile,
+    const ContentSettingsPattern& primary_pattern,
+    const ContentSettingsPattern& secondary_pattern,
+    ContentSettingsType content_type,
+    std::string resource_identifier,
+    PermissionSourceUI source_ui)
+    : RevocationReporter(
+          profile,
+          GURL(primary_pattern.ToString()),

[tsergeant, 2016/07/21 01:32:52]

Is this version of the constructor currently used? It seems useful, but I'd be
worried about just converting the pattern to a string. What happens if the
pattern has a wildcard in it?

[stefanocs, 2016/07/21 01:45:46]

Not in this cl, but we are planning to use this for recording revocation in
content settings (I'm sure if this constructor should be added on that cl
instead). The revocation should only be recorded if the pattern string itself is
a valid url (checked in the first constructor).

[tsergeant, 2016/07/21 07:10:56]

Alright, sounds good to me.

+          GURL((secondary_pattern == ContentSettingsPattern::Wildcard())
+                   ? primary_pattern.ToString()
+                   : secondary_pattern.ToString()),
+          content_type,
+          resource_identifier,
+          source_ui) {}
 
-  if (previous_value == CONTENT_SETTING_ALLOW &&
-      final_value != CONTENT_SETTING_ALLOW) {
+PermissionUtil::RevocationReporter::~RevocationReporter() {
+  if (!is_initially_allowed_)
+    return;
+  HostContentSettingsMap* map =
+      HostContentSettingsMapFactory::GetForProfile(profile_);
+  ContentSetting final_content_setting = map->GetContentSetting(
+      primary_url_, secondary_url_, content_type_, resource_identifier_);
+  if (final_content_setting != CONTENT_SETTING_ALLOW) {
     PermissionType permission_type;
-    if (PermissionUtil::GetPermissionType(content_type, &permission_type)) {
-      // TODO(stefanocs): Report revocations from page action as PAGE_ACTION
-      // source UI instead of SITE_SETTINGS source UI.
-      PermissionUmaUtil::PermissionRevoked(permission_type,
-                                           PermissionSourceUI::SITE_SETTINGS,
-                                           primary_url, profile);
+    if (PermissionUtil::GetPermissionType(content_type_, &permission_type)) {
+      PermissionUmaUtil::PermissionRevoked(permission_type, source_ui_,
+                                           primary_url_, profile_);
     }
   }
 }