[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..e879c5a3594c888abcf3fc57c705d33256518efe 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,13 @@
 
 #include "chrome/browser/subresource_filter/subresource_filter_content_settings_manager.h"
 
+#include "base/auto_reset.h"
+#include "base/bind.h"
+#include "base/feature_list.h"
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
+#include "base/time/default_clock.h"
+#include "base/values.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 +18,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 kInfobarLastShownTimeKey[] = "InfobarLastShownTime";
+
+bool ShouldUseSmartUI() {
+#if defined(OS_ANDROID)
+  return base::FeatureList::IsEnabled(
+      subresource_filter::kSafeBrowsingSubresourceFilterExperimentalUI);
+#endif
+  return false;
+}
+
+}  // namespace
+
+constexpr base::TimeDelta
+    SubresourceFilterContentSettingsManager::kDelayBeforeShowingInfobarAgain =
+        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,12 +58,100 @@ 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 from_ui) {
+  base::AutoReset<bool> resetter(&ignore_settings_changes_, true);
+  settings_map_->SetContentSettingDefaultScope(
+      url, GURL(),
+      ContentSettingsType::CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER,
+      std::string(), setting);
+  if (from_ui) {
+    ChromeSubresourceFilterClient::LogAction(
+        kActionContentSettingsBlockedFromUI);

[msramek, 2017/04/13 16:08:27]

We're logging "BlockedFromUI" regardless of |setting|. Can we either make a part
of the condition, or DCHECK() and document why we think this can never be ALLOW?

[Charlie Harrison, 2017/04/13 17:54:01]

DCHECK sounds good. As of now, the "standard" UI for subresource filtering
doesn't allow for changing the setting to ALLOW.

+    ChromeSubresourceFilterClient::LogAction(kActionContentSettingsBlocked);

[jkarlin, 2017/04/13 12:57:18]

Is this meant to be part of the same condition?

[Charlie Harrison, 2017/04/13 14:13:09]

Hm, now that you mention it it does kind of make sense for this not to be logged
here, so the buckets are exclusive.

+  }
+}
+
+void SubresourceFilterContentSettingsManager::ClearContentSetting(
+    const GURL& url) {
+  base::AutoReset<bool> resetter(&ignore_settings_changes_, true);
+  auto predicate = base::Bind(
+      [](const ContentSettingsPattern& pattern_to_delete,
+         const ContentSettingsPattern& primary,
+         const ContentSettingsPattern& seconary) {
+        return pattern_to_delete == primary;
+      },
+      ContentSettingsPattern::FromURLNoWildcard(url));
+  settings_map_->ClearSettingsForOneTypeWithPredicate(

[msramek, 2017/04/13 16:08:27]

Do you expect to match more than one setting? You only use origin-scoped primary
patterns, and no secondary patterns, and for a given pattern pair you can't have
more than one value.

SetContentSetting(url, CONTENT_SETTING_DEFAULT, false /* from_ui */) should do
the trick.

[Charlie Harrison, 2017/04/13 17:54:01]

Done. That makes sense.

+      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 /* from_ui */);
+
+  if (!should_use_smart_ui())
+    return;
+  auto dict = base::MakeUnique<base::DictionaryValue>();
+  double now = clock_->Now().ToDoubleT();
+  dict->SetDouble(kInfobarLastShownTimeKey, 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(kInfobarLastShownTimeKey, &last_shown_time_double)) {
+    base::Time last_shown = base::Time::FromDoubleT(last_shown_time_double);
+    if (clock_->Now() - last_shown < kDelayBeforeShowingInfobarAgain)
+      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(

[msramek, 2017/04/13 16:08:27]

optional nit: I appreciate that I can now point people to your codebase to
explain the difference between content settings and website settings. However,
wouldn't it be more readable for you to call them more semantically? Like
"permission" and "metadata" or something such?

[Charlie Harrison, 2017/04/13 17:54:01]

Sure.

+    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,
     ContentSettingsType content_type,
     std::string resource_identifier) {
-  if (content_type != CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER)
+  if (content_type != CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER ||
+      ignore_settings_changes_)
     return;
 
   const ContentSettingsDetails details(primary_pattern, secondary_pattern,
@@ -79,4 +195,15 @@ void SubresourceFilterContentSettingsManager::OnContentSettingChanged(
   } else {
     NOTREACHED();
   }
+
+  if (!should_use_smart_ui())
+    return;
+
+  if (!ShouldShowUIForSite(url)) {
+    ChromeSubresourceFilterClient::LogAction(
+        kActionContentSettingsBlockedWhileUISuppressed);
+  }
+
+  // Reset the smart UI to ensure tne user can easily change their decision.

[msramek, 2017/04/13 16:08:27]

typo: the

[Charlie Harrison, 2017/04/13 17:54:01]

Done.

+  SetWebsiteSetting(url, nullptr);
 }