[subresource_filter] Implement the "Smart" UI on Android

chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc

Index: chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc
diff --git a/chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc b/chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc
index 42cf2fb5983a2b2f6408dfd9985db6ea704bd9cd..26a92e1928127f604c1b0233c34b940035bd6116 100644
--- a/chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc
+++ b/chrome/browser/subresource_filter/subresource_filter_content_settings_manager.cc
@@ -4,7 +4,11 @@
 
 #include "chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h"
 
+#include "base/auto_reset.h"
+#include "base/feature_list.h"
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
+#include "base/time/default_clock.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/subresource_filter/chrome_subresource_filter_client.h"
@@ -12,11 +16,33 @@
 #include "components/content_settings/core/browser/host_content_settings_map.h"
 #include "components/content_settings/core/common/content_settings.h"
 #include "components/content_settings/core/common/content_settings_pattern.h"
+#include "components/subresource_filter/core/browser/subresource_filter_features.h"
 #include "url/gurl.h"
 
+namespace {
+
+// Key into the website setting dict for the smart UI.
+const char kUILastShowTime[] = "LastShowTime";

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

nit: InfobarLastShownTime ?

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

Done.

+
+bool ShouldUseSmartUI() {
+#if defined(OS_ANDROID)
+  return base::FeatureList::IsEnabled(
+      subresource_filter::kSafeBrowsingSubresourceFilterExperimentalUI);
+#endif
+  return false;
+}
+
+}  // namespace
+
+constexpr base::TimeDelta
+    SubresourceFilterContentSettingsManager::kUIShowThresholdTime =

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

nit: Something along the lines of kDelayBeforeShowingInfobarAgain or
kInfobarCooldown[Duration]?

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

Done.

+        base::TimeDelta::FromHours(24);
+
 SubresourceFilterContentSettingsManager::
     SubresourceFilterContentSettingsManager(Profile* profile)
-    : settings_map_(HostContentSettingsMapFactory::GetForProfile(profile)) {
+    : settings_map_(HostContentSettingsMapFactory::GetForProfile(profile)),
+      clock_(base::MakeUnique<base::DefaultClock>(base::DefaultClock())),
+      should_use_smart_ui_(ShouldUseSmartUI()) {
   DCHECK(profile);
   DCHECK(settings_map_);
   settings_map_->AddObserver(this);
@@ -30,6 +56,88 @@ void SubresourceFilterContentSettingsManager::Shutdown() {
   settings_map_ = nullptr;
 }
 
+ContentSetting SubresourceFilterContentSettingsManager::GetContentSetting(
+    const GURL& url) const {
+  return settings_map_->GetContentSetting(
+      url, GURL(),
+      ContentSettingsType::CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER,
+      std::string());
+}
+
+void SubresourceFilterContentSettingsManager::SetContentSetting(
+    const GURL& url,
+    ContentSetting setting,
+    bool log_metrics) {
+  base::AutoReset<bool>(&ignore_settings_changes_, !log_metrics);

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

|ignore_settings_changes_| is never read. Let's also add a unit test.

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

Done. Great catch. Fallout from the previous refactor.

+  settings_map_->SetContentSettingDefaultScope(
+      url, GURL(),
+      ContentSettingsType::CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER,
+      std::string(), setting);
+}
+
+void SubresourceFilterContentSettingsManager::ClearContentSetting(
+    const GURL& url) {
+  base::AutoReset<bool>(&ignore_settings_changes_, true);
+  auto predicate = base::Bind(

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

#include "base/bind.h"

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

Done.

+      [](const ContentSettingsPattern& pattern_to_delete,
+         const ContentSettingsPattern& primary,
+         const ContentSettingsPattern& seconary) {
+        return pattern_to_delete == primary;
+      },
+      ContentSettingsPattern::FromURLNoWildcard(url));

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

If I understand correctly, this is per-origin granularity. Safe Browsing allows
site-chunk based activation (using at most 4 path segments). I believe this is
very much an edge case, but might be worth thinking through that nothing really
bad happens.

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

Ok. So, if safebrowsing does chunking it means that an origin could have
matching URLs and non-matching URLs.

If a user navigates to a non-matching URL, the entire origin is cleared and it
no longer shows up in the content setting UI. Subsequent navigations to that
origin will trigger the UI to show and ignore all previous whitelisting.

I think this is fine, unless the safebrowsing lists use a lot of chunking.

+  settings_map_->ClearSettingsForOneTypeWithPredicate(
+      ContentSettingsType::CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER,
+      std::move(predicate));
+}
+
+void SubresourceFilterContentSettingsManager::OnDidShowUI(const GURL& url) {
+  // Explicitly set the content setting to "Allow" to ensure it always shows up
+  // in the settings UI.
+  SetContentSetting(url, CONTENT_SETTING_ALLOW, false /* log_metrics */);
+
+  if (!should_use_smart_ui())
+    return;
+  auto dict = base::MakeUnique<base::DictionaryValue>();

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

nit: Do we need to include "base/values.h"?

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

Done.

+  double now = clock_->Now().ToDoubleT();
+  dict->SetDouble(kUILastShowTime, now);
+  SetWebsiteSetting(url, std::move(dict));
+}
+
+bool SubresourceFilterContentSettingsManager::ShouldShowUIForSite(
+    const GURL& url) const {
+  if (!should_use_smart_ui())
+    return true;
+
+  std::unique_ptr<base::DictionaryValue> dict = GetWebsiteSetting(url);
+  if (!dict)
+    return true;
+
+  double last_shown_time_double = 0;
+  if (dict->GetDouble(kUILastShowTime, &last_shown_time_double)) {
+    base::Time last_shown = base::Time::FromDoubleT(last_shown_time_double);
+    if (clock_->Now() - last_shown < kUIShowThresholdTime)
+      return false;
+  }
+  return true;
+}
+
+std::unique_ptr<base::DictionaryValue>
+SubresourceFilterContentSettingsManager::GetWebsiteSetting(
+    const GURL& url) const {
+  return base::DictionaryValue::From(settings_map_->GetWebsiteSetting(
+      url, GURL(), CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER_DATA, std::string(),
+      nullptr));
+}
+
+void SubresourceFilterContentSettingsManager::SetWebsiteSetting(
+    const GURL& url,
+    std::unique_ptr<base::DictionaryValue> dict) {
+  settings_map_->SetWebsiteSettingDefaultScope(
+      url, GURL(),
+      ContentSettingsType::CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER_DATA,
+      std::string(), std::move(dict));
+}
+
 void SubresourceFilterContentSettingsManager::OnContentSettingChanged(
     const ContentSettingsPattern& primary_pattern,
     const ContentSettingsPattern& secondary_pattern,
@@ -79,4 +187,15 @@ void SubresourceFilterContentSettingsManager::OnContentSettingChanged(
   } else {
     NOTREACHED();
   }
+
+  if (!should_use_smart_ui())
+    return;
+
+  if (!ShouldShowUIForSite(url)) {
+    ChromeSubresourceFilterClient::LogAction(
+        kActionContentSettingsBlockedWhileUISuppressed);

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

Wouldn't we reach this even if the user toggled the feature on the infobar
itself we had just shown for the very first time?

Let's add a unit test for this.

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

I've fixed this by changing "log_metrics" to "from_ui" in the code, and
SetContentSetting never logs metrics here. Now this method will only trigger for
content settings changed via the content setting UI.

+  }
+
+  // Reset the smart UI to ensure tne user can easily change their decision.
+  SetWebsiteSetting(url, nullptr);
 }