[subresource_filter] Add metrics for UI / related things

chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/subresource_filter/subresource_filter_content_settings_observer_factory.h"
+
+#include "base/logging.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"
+#include "components/content_settings/core/browser/content_settings_details.h"
+#include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/content_settings/core/common/content_settings_pattern.h"
+#include "components/content_settings/core/common/content_settings_types.h"
+#include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "url/gurl.h"
+
+SubresourceFilterContentSettingsObserver::
+    SubresourceFilterContentSettingsObserver(Profile* profile)
+    : settings_map_(HostContentSettingsMapFactory::GetForProfile(profile)) {
+  DCHECK(profile);
+  settings_map_->AddObserver(this);
+}
+
+void SubresourceFilterContentSettingsObserver::OnContentSettingChanged(

[msramek, 2017/03/29 09:41:25]

This method probably deserves a unittest. You mention a browsertest in your
comment, but that's probably not needed; it seems to me that you just need a
TestingProfile and the UI thread here.

[Charlie Harrison, 2017/03/29 15:23:32]

Done.

+    const ContentSettingsPattern& primary_pattern,
+    const ContentSettingsPattern& secondary_pattern,

[msramek, 2017/03/29 09:41:25]

Do we care about the secondary pattern?

If a user sets the content setting through a permission prompt on "google.com",
they'll get ["google.com", "*"].

The settings UI only allows specifying primary patterns, so again, they can only
add ["google.com", "*"].

That's fine. Below you call GetContentSetting(url, url, ...) =
GetContentSettingForUrl("google.com", "google.com", ...), which will return the
value for ["google.com", "*"], because "*" matches "google.com".

However, an extension or administrator could theoretically add an exception for
["google.com", "example.org"].

Then below, you call GetContentSetting(url, url, ...) =
GetContentSettingForUrl("google.com", "google.com") which would not match
["google.com", "example.org"] and would return something broader, e.g. the
default setting.

As far as I see, we don't have subresource filter in the extension API or admin
policy, so I just propose:

DCHECK(secondary_pattern == ContentSettingsPattern::Wildcard());

[Charlie Harrison, 2017/03/29 15:23:32]

Done. We may add this functionality in the future but for now I've just made a
note to remove this if we do.

+    ContentSettingsType content_type,
+    std::string resource_identifier) {
+  DCHECK(settings_map_);
+  if (content_type != CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER ||
+      !primary_pattern.IsValid()) {
+    return;
+  }
+
+  const ContentSettingsDetails details(primary_pattern, secondary_pattern,
+                                       content_type, resource_identifier);
+  if (details.update_all()) {
+    ContentSetting global_setting =
+        GetContentSettingForUrl(GURL("https://example.com"));

[msramek, 2017/03/29 09:41:25]

I'm so going to mess with your metrics by flipping the setting on example.com :)

You can use HostContentSettingsMap::GetDefaultContentSetting(). This returns
ContentSetting, which is safe, because SUBRESOURCE_FILTER indeed is a content
setting (and not just a website setting).

[Charlie Harrison, 2017/03/29 15:23:32]

Ha! Done. I knew there was a better way than this.

+    if (global_setting == CONTENT_SETTING_BLOCK) {

[msramek, 2017/03/29 09:41:25]

style nit: Use a switch instead of NOTREACHED() to ensure that the enumeration
is exhaustive at compile-time

[Charlie Harrison, 2017/03/29 15:23:32]

There are 5 content settings that I'm not handling here. Do you propose I add
them all in the switch with a NOTREACHED() if they are encountered? This is what
I've done (+ hoisted it to a helper function since we do it twice).

[msramek, 2017/03/29 17:37:30]

Sigh... For a moment, I completely forgot that we're not working with just a
2-member enum. This is actually worse than what you had, because it forces
everyone who adds a new value for their setting to also update your code.

So, we *could* have a 2-set enum:

enum SETTING { ALLOW = CONTENT_SETTING_ALLOW, BLOCK = CONTENT_SETTING_BLOCK };

But I'd probably just revert this part to the original. Sorry!

[Charlie Harrison, 2017/03/29 17:47:34]

No worries! Reverted back to the if chain.

+      ChromeSubresourceFilterClient::LogSubresourceFilterAction(
+          kActionContentSettingsBlockedGlobal);
+    } else if (global_setting == CONTENT_SETTING_ALLOW) {
+      ChromeSubresourceFilterClient::LogSubresourceFilterAction(
+          kActionContentSettingsAllowedGlobal);
+    } else {
+      NOTREACHED();
+    }
+    return;
+  }
+
+  GURL url = GURL(primary_pattern.ToString());
+  if (!url.is_valid()) {
+    ChromeSubresourceFilterClient::LogSubresourceFilterAction(
+        kActionContentSettingsWildcardUpdate);
+    return;
+  }
+
+  ContentSetting setting = GetContentSettingForUrl(url);
+  if (setting == CONTENT_SETTING_BLOCK) {

[msramek, 2017/03/29 09:41:25]

Ditto.

[Charlie Harrison, 2017/03/29 15:23:32]

Done.

+    ChromeSubresourceFilterClient::LogSubresourceFilterAction(
+        kActionContentSettingsBlocked);
+  } else if (setting == CONTENT_SETTING_ALLOW) {
+    ChromeSubresourceFilterClient::LogSubresourceFilterAction(
+        kActionContentSettingsAllowed);
+  } else {
+    NOTREACHED();
+  }
+}
+
+void SubresourceFilterContentSettingsObserver::Shutdown() {
+  settings_map_->RemoveObserver(this);
+  settings_map_ = nullptr;
+}
+
+ContentSetting
+SubresourceFilterContentSettingsObserver::GetContentSettingForUrl(
+    const GURL& url) {
+  return settings_map_->GetContentSetting(
+      url, url, ContentSettingsType::CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER,
+      std::string());
+}
+
+// static
+SubresourceFilterContentSettingsObserver*
+SubresourceFilterContentSettingsObserverFactory::GetForProfile(
+    Profile* profile) {
+  return static_cast<SubresourceFilterContentSettingsObserver*>(
+      GetInstance()->GetServiceForBrowserContext(profile, true /* create */));
+}
+
+// static
+SubresourceFilterContentSettingsObserverFactory*
+SubresourceFilterContentSettingsObserverFactory::GetInstance() {
+  return base::Singleton<
+      SubresourceFilterContentSettingsObserverFactory>::get();
+}
+
+SubresourceFilterContentSettingsObserverFactory::
+    SubresourceFilterContentSettingsObserverFactory()
+    : BrowserContextKeyedServiceFactory(
+          "SubresourceFilterContentSettingsObserver",
+          BrowserContextDependencyManager::GetInstance()) {}
+
+KeyedService*
+SubresourceFilterContentSettingsObserverFactory::BuildServiceInstanceFor(
+    content::BrowserContext* profile) const {
+  return new SubresourceFilterContentSettingsObserver(
+      static_cast<Profile*>(profile));
+}