[Android] Adding content settings provider for notification channels

chrome/browser/notifications/notification_content_settings_provider_android.cc

Index: chrome/browser/notifications/notification_content_settings_provider_android.cc
diff --git a/chrome/browser/notifications/notification_content_settings_provider_android.cc b/chrome/browser/notifications/notification_content_settings_provider_android.cc
new file mode 100644
index 0000000000000000000000000000000000000000..6d89be83b86df9e68031b5604815f7f099cc9569
--- /dev/null
+++ b/chrome/browser/notifications/notification_content_settings_provider_android.cc
@@ -0,0 +1,108 @@
+// 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/notifications/notification_content_settings_provider_android.h"

[raymes, 2017/05/17 03:13:38]

Renaming is really a pain so I hate to suggest this but if we wanted to be more
consistent with other provider names, I would suggest
content_settings_notifications_provider_android.h/cc. I think all our other
providers start with content_settings...

[awdf, 2017/05/17 17:14:41]

I was considering changing it to notification_channels_provider.h/cc  actually,
to be a bit more descriptive, WDYT? (If not, I'm happy to change it to
content_settings_notifications_provider_android instead)

[raymes, 2017/05/18 00:39:37]

So a few quick notes:
1) Keeping the _android suffix seems good, since this is android only but I'm no
expert with android code/conventions.
2) All our other providers filenames have a content_settings_ prefixes. However
the name of the classes don't have ContentSettings prefixes which is kind of
inconsistent with the naming style. So I don't feel strongly whether we keep the
content_settings_ prefixes on the filenames.
3) I don't feel strongly about keeping "channels" in the name or not, I'll leave
it up to you folks.
4) Do we tend to use Notifications (plural) when naming things? I'll also leave
that to you folks.

So any of the below are fine with me (the last one is probably might slight
preference):
notifications_channels_provider_android.h/cc ->
NotificationsChannelsProviderAndroid
notifications_provider_android.h/cc -> NotificationsProviderAndroid
content_settings_notifications_provider_android.h/cc ->
NotificationsProviderAndroid

+
+#include "base/android/jni_android.h"
+#include "base/android/jni_string.h"
+#include "base/logging.h"
+#include "base/strings/string_util.h"
+#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "components/content_settings/core/browser/content_settings_details.h"
+#include "components/content_settings/core/browser/content_settings_utils.h"
+#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 "jni/NotificationSettingsBridge_jni.h"
+#include "url/gurl.h"
+#include "url/url_constants.h"
+
+using base::android::AttachCurrentThread;
+using base::android::ConvertUTF8ToJavaString;
+using base::android::ScopedJavaLocalRef;
+
+namespace {
+
+// A Java counterpart will be generated for this enum.
+// GENERATED_JAVA_ENUM_PACKAGE: org.chromium.chrome.browser.notifications
+enum NotificationChannelStatus { ENABLED, BLOCKED, UNAVAILABLE };
+
+}  // anonymous namespace
+
+NotificationContentSettingsProviderAndroid::
+    NotificationContentSettingsProviderAndroid()
+    : should_use_channels_(
+          Java_NotificationSettingsBridge_shouldUseChannelSettings(
+              AttachCurrentThread())) {}
+
+NotificationContentSettingsProviderAndroid::
+    ~NotificationContentSettingsProviderAndroid() = default;
+
+std::unique_ptr<content_settings::RuleIterator>
+NotificationContentSettingsProviderAndroid::GetRuleIterator(

[awdf, 2017/05/17 17:14:41]

@raymes I'm considering how to implement this and I think I can get away with
using something like this ListIterator implementation defined in one of the
tests:
https://cs.chromium.org/chromium/src/components/content_settings/core/browser...
- since none of the channel rules should overlap with each other, we don't need
a RuleIteratorImpl (which takes precedence into account), right?

So I plan to just grab a list/vector of existing channels from java, transform
them into Rules & stick them in a ListIterator, sound ok?

[raymes, 2017/05/18 00:39:37]

Overall that sounds good. Let's land this CL as it is before adding more to it
though, since we're getting close :) 

Also, I think we may need to sort the list before returning it.
ContentSettingsPatterns have a particular precedence order even when they are
disjoint (which is basically a form of sorting by host/port/etc). Using
std::sort on a vector of ContentSettingsPatterns should do the trick?

Also don't worry too much about factoring out that ListIterator from the test to
share code. I think it's fine to add a new subclass.

[awdf, 2017/05/19 17:59:57]

OK. Happy to land this one with just the creating-channels part and leave the
rule iterator to a further cl.

+    ContentSettingsType content_type,
+    const content_settings::ResourceIdentifier& resource_identifier,
+    bool incognito) const {
+  // TODO(crbug.com/700377) return rule iterator over all channels
+  return nullptr;
+}
+
+bool NotificationContentSettingsProviderAndroid::SetWebsiteSetting(
+    const ContentSettingsPattern& primary_pattern,
+    const ContentSettingsPattern& secondary_pattern,
+    ContentSettingsType content_type,
+    const content_settings::ResourceIdentifier& resource_identifier,
+    base::Value* value) {
+  if (content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS ||
+      !should_use_channels_) {
+    return false;
+  }
+  GURL primary_url = GURL(primary_pattern.ToString());
+  // Ignore patterns that are not valid urls - we only want to expose
+  // notification channels for particular origins, not wildcard rules, since
+  // users should not be able to grant or deny wildcard notification permissions
+  // (although these may be set by policy).
+  if (!primary_url.is_valid())
+    return false;
+  JNIEnv* env = AttachCurrentThread();
+  const std::string origin = primary_url.spec();
+  ScopedJavaLocalRef<jstring> jorigin = ConvertUTF8ToJavaString(env, origin);
+  ContentSetting setting = content_settings::ValueToContentSetting(value);
+  switch (setting) {
+    case CONTENT_SETTING_ALLOW:
+    case CONTENT_SETTING_BLOCK: {
+      auto channel_status = static_cast<NotificationChannelStatus>(
+          Java_NotificationSettingsBridge_getChannelStatus(env, jorigin));
+      if (channel_status == NotificationChannelStatus::UNAVAILABLE) {
+        bool result = Java_NotificationSettingsBridge_createChannel(
+            env, jorigin, setting == CONTENT_SETTING_ALLOW /* enabled */);
+        return result;
+      } else {
+        DCHECK(!(setting == CONTENT_SETTING_ALLOW &&
+                 channel_status == NotificationChannelStatus::BLOCKED));
+        DCHECK(!(setting == CONTENT_SETTING_BLOCK &&
+                 channel_status == NotificationChannelStatus::ENABLED));

[Peter Beverloo, 2017/05/16 16:15:34]

These frighten me a bit. Until the mitigations are in place to avoid us from
hitting these (e.g. the infobar changes) it'd be possible for Android O users to
hit this, right?

[raymes, 2017/05/17 03:13:38]

Won't these help to find the callsites that need to be changed though?

My motivation for suggesting this was just to ensure that we don't have any UI
that is expecting to update content settings, but not actually succeeding.

[awdf, 2017/05/17 17:14:41]

Right now it wouldn't crash because the 'else' is never hit since the java
always returns UNAVAILABLE.. but if the java side to create/check channels gets
implemented before we adapt the site settings UI, (which seems likely now I'm
developing behind a flag) you're right that this would crash if someone
blocks/enables a site from site settings with that flag enabled. However if that
flag is disabled by default is this actually a problem?

+        return true;
+      }
+    }
+    case CONTENT_SETTING_DEFAULT:
+      Java_NotificationSettingsBridge_deleteChannel(env, jorigin);
+      return false;

[Peter Beverloo, 2017/05/16 16:15:34]

Is returning FALSE here the right thing to do? We did delete the channel.

[raymes, 2017/05/17 03:13:38]

Good catch, we probably should return true.

[awdf, 2017/05/17 17:14:41]

I think you're right and we should be returning true here actually to indicate
we handled it.

Done.

+    default:
+      // We rely on notification settings being one of ALLOW/BLOCK/DEFAULT.
+      NOTREACHED();
+      return false;

[raymes, 2017/05/17 03:13:38]

I think it's safer to return true here (and in all the above return statements,
except the first one). The reason being that I think we should try to keep the
user prefs for notifications clean, even if we fail to write a pref here.

[awdf, 2017/05/17 17:14:41]

shouldn't really make a difference since this is after a NOTREACHED, and I've
added another DCHECK to HCSM that there are no other possible content setting
values passed in for notifications on android, but changed to true here anyway
for consistency.

+  }
+}
+
+void NotificationContentSettingsProviderAndroid::ClearAllContentSettingsRules(
+    ContentSettingsType content_type) {
+  // TODO(crbug.com/700377): If |content_type| == NOTIFICATIONS, delete
+  // all channels.
+}
+
+void NotificationContentSettingsProviderAndroid::ShutdownOnUIThread() {
+  RemoveAllObservers();
+}