[subresource_filter] Implement the "Smart" UI on Android

chrome/browser/subresource_filter/chrome_subresource_filter_client.cc

Index: chrome/browser/subresource_filter/chrome_subresource_filter_client.cc
diff --git a/chrome/browser/subresource_filter/chrome_subresource_filter_client.cc b/chrome/browser/subresource_filter/chrome_subresource_filter_client.cc
index 02b40dbbb4661b71b3982f6173eed4d10a333e8f..db67f021678e77628e0465198ad6c02b1ab73889 100644
--- a/chrome/browser/subresource_filter/chrome_subresource_filter_client.cc
+++ b/chrome/browser/subresource_filter/chrome_subresource_filter_client.cc
@@ -4,27 +4,25 @@
 
 #include "chrome/browser/subresource_filter/chrome_subresource_filter_client.h"
 
-#include <string>
-
 #include "base/metrics/histogram_macros.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/content_settings/tab_specific_content_settings.h"
 #include "chrome/browser/infobars/infobar_service.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h"
 #include "chrome/browser/subresource_filter/subresource_filter_content_settings_manager_factory.h"
 #include "chrome/browser/ui/android/content_settings/subresource_filter_infobar_delegate.h"
-#include "components/content_settings/core/browser/host_content_settings_map.h"
-#include "components/content_settings/core/common/content_settings_types.h"
 #include "components/subresource_filter/content/browser/content_ruleset_service.h"
+#include "components/subresource_filter/core/common/activation_level.h"
+#include "components/subresource_filter/core/common/activation_state.h"
 
 ChromeSubresourceFilterClient::ChromeSubresourceFilterClient(
     content::WebContents* web_contents)
     : web_contents_(web_contents), shown_for_navigation_(false) {
   DCHECK(web_contents);
-  // Ensure the content settings manager is initialized.
-  SubresourceFilterContentSettingsManagerFactory::EnsureForProfile(
-      Profile::FromBrowserContext(web_contents_->GetBrowserContext()));
+  settings_manager_ =

[engedy, 2017/04/12 14:02:50]

nit: Can we put this on the initializer list? We would lose the DCHECK, though.

[Charlie Harrison, 2017/04/12 17:53:44]

Done.

+      SubresourceFilterContentSettingsManagerFactory::EnsureForProfile(
+          Profile::FromBrowserContext(web_contents_->GetBrowserContext()));
 }
 
 ChromeSubresourceFilterClient::~ChromeSubresourceFilterClient() {}
@@ -33,21 +31,25 @@ void ChromeSubresourceFilterClient::ToggleNotificationVisibility(
     bool visibility) {
   if (shown_for_navigation_ && visibility)
     return;
-
-  shown_for_navigation_ = visibility;
-  TabSpecificContentSettings* content_settings =
-      TabSpecificContentSettings::FromWebContents(web_contents_);
+  shown_for_navigation_ = false;
 
   // |visibility| is false when a new navigation starts.
   if (visibility) {
-    content_settings->OnContentBlocked(
-        CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER);
-    LogAction(kActionUIShown);
+    const GURL& url = web_contents_->GetLastCommittedURL();
+    if (!settings_manager_->ShouldShowUIForSite(url)) {
+      LogAction(kActionUISuppressed);
+      return;
+    }
 #if defined(OS_ANDROID)
     InfoBarService* infobar_service =
         InfoBarService::FromWebContents(web_contents_);
     SubresourceFilterInfobarDelegate::Create(infobar_service);
 #endif
+    TabSpecificContentSettings* content_settings =
+        TabSpecificContentSettings::FromWebContents(web_contents_);
+    content_settings->OnContentBlocked(
+        CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER);
+    OnDidShowUI(url);
   } else {
     LogAction(kActionNavigationStarted);
   }
@@ -55,40 +57,38 @@ void ChromeSubresourceFilterClient::ToggleNotificationVisibility(
 
 bool ChromeSubresourceFilterClient::IsWhitelistedByContentSettings(
     const GURL& url) {
-  return GetContentSettingForUrl(url) == CONTENT_SETTING_BLOCK;
+  return settings_manager_->GetContentSetting(url) == CONTENT_SETTING_BLOCK;
 }
 
 void ChromeSubresourceFilterClient::WhitelistByContentSettings(
     const GURL& url) {

[engedy, 2017/04/12 14:02:50]

nit: Do you think it would add some clarity to s/url/top_level_url/gc where
relevant in method signatures throughout this CL?

[Charlie Harrison, 2017/04/12 17:53:44]

Done.

   // Whitelist via content settings.
-  Profile* profile =
-      Profile::FromBrowserContext(web_contents_->GetBrowserContext());
-  DCHECK(profile);
-  HostContentSettingsMap* settings_map =
-      HostContentSettingsMapFactory::GetForProfile(profile);
-  settings_map->SetContentSettingDefaultScope(
-      url, url, ContentSettingsType::CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER,
-      std::string(), CONTENT_SETTING_BLOCK);
-
+  settings_manager_->SetContentSetting(url, CONTENT_SETTING_BLOCK,
+                                       true /* log_metrics */);
   LogAction(kActionContentSettingsBlockedFromUI);
 }
 
+void ChromeSubresourceFilterClient::OnActivationComputed(
+    const GURL& url,
+    const subresource_filter::ActivationState& state) {
+  // Clear the content setting when the site does not trigger activation.
+  if (state.activation_level == subresource_filter::ActivationLevel::DISABLED &&

[engedy, 2017/04/12 14:02:50]

Should we also remove the content setting if this is DRYRUN?

[Charlie Harrison, 2017/04/12 17:53:44]

Sure. Done.

+      settings_manager_->GetContentSetting(url) == CONTENT_SETTING_ALLOW) {
+    settings_manager_->ClearContentSetting(url);
+  }
+}
+
 // static
 void ChromeSubresourceFilterClient::LogAction(SubresourceFilterAction action) {
   UMA_HISTOGRAM_ENUMERATION("SubresourceFilter.Actions", action,
                             kActionLastEntry);
 }
 
-ContentSetting ChromeSubresourceFilterClient::GetContentSettingForUrl(
-    const GURL& url) {
-  Profile* profile =
-      Profile::FromBrowserContext(web_contents_->GetBrowserContext());
-  DCHECK(profile);
-  HostContentSettingsMap* settings_map =
-      HostContentSettingsMapFactory::GetForProfile(profile);
-  return settings_map->GetContentSetting(
-      url, url, ContentSettingsType::CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER,
-      std::string());
+void ChromeSubresourceFilterClient::OnDidShowUI(const GURL& url) {

[engedy, 2017/04/12 14:02:50]

This is now three lines and called from a single call site. WDYT about inlining
this?

[Charlie Harrison, 2017/04/12 17:53:44]

Done.

+  LogAction(kActionUIShown);
+  shown_for_navigation_ = true;
+
+  settings_manager_->OnDidShowUI(url);
 }
 
 subresource_filter::VerifiedRulesetDealer::Handle*