[Android] Adding content settings provider for notification channels

chrome/browser/notifications/android_settings_provider.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/notifications/android_settings_provider.h"
+
+#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
+
+namespace content_settings {
+
+AndroidSettingsProvider::AndroidSettingsProvider()
+    : should_use_channels_(
+          Java_NotificationSettingsBridge_shouldUseChannelSettings(
+              AttachCurrentThread())) {}
+
+AndroidSettingsProvider::~AndroidSettingsProvider() {}

[Peter Beverloo, 2017/05/15 15:10:08]

micro nit: {} -> = default;

[awdf, 2017/05/16 15:17:35]

Done.

+
+std::unique_ptr<RuleIterator> AndroidSettingsProvider::GetRuleIterator(
+    ContentSettingsType content_type,
+    const ResourceIdentifier& resource_identifier,
+    bool incognito) const {
+  // TODO(crbug.com/700377) return rule iterator over all channels
+  return nullptr;
+}
+
+bool AndroidSettingsProvider::SetWebsiteSetting(
+    const ContentSettingsPattern& primary_pattern,
+    const ContentSettingsPattern& secondary_pattern,
+    ContentSettingsType content_type,
+    const ResourceIdentifier& resource_identifier,
+    base::Value* value) {
+  if (content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS ||

[raymes, 2017/05/16 01:08:18]

Should we just DCHECK that it's only notifications for now?

[awdf, 2017/05/16 15:17:35]

But won't this get all types of content settings updates?

Looking at
https://cs.chromium.org/chromium/src/components/content_settings/core/browser...
, it seems all providers may receive all content settings updates that go
through setWebsiteSetting (until one of them returns true), but maybe I'm
missing something?

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

Ah good point, you're right. Sorry for the misguidance.

+      !should_use_channels_)
+    return false;

[Peter Beverloo, 2017/05/15 15:10:08]

micro nit: {} (multi-line statement)

[awdf, 2017/05/16 15:17:35]

Done.

+  GURL primary_url = GURL(primary_pattern.ToString());
+  if (!primary_url.is_valid())

[Peter Beverloo, 2017/05/15 15:10:08]

This warrants a comment

[raymes, 2017/05/16 01:08:18]

Would this be better as  DCHECK too?

[awdf, 2017/05/16 15:17:35]

Done.

[awdf, 2017/05/16 15:17:35]

hmmm good question. 

I agree it might be useful to document and enforce our assumption that
notification permissions cannot be set dynamically on wildcard primary urls
through SetWebsiteSetting, but I wonder if this is the best place to do that. 

How do you feel about adding a DCHECK to
HostContentSettingsMap::SetWebsiteSettingCustomScope to ensure this? Feels like
it's safer to expect people writing new callers to respect DCHECKs set there,
but they are likely not gonna test their code on Android O and notice a DCHECK
if we just add it here.

However, adding a DCHECK on content type + url validity to
HostContentSettingsMap::SetWebsiteSettingCustomScope feels like it might be
unnecessarily specific, given we can just gracefully ignore that case here.
While it's an assumption that might produce surprising behaviour if broken, it's
not an assumption we rely on for this code to function correctly.

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

I guess my thought is that if there are callers passing in non-origins, we
should try to find out about them because they are bugs that should be fixed. If
this is a DCHECK, we have a much better chance of finding out about them,
because we'll get a crash in tests, etc. 

I would be ok adding the specific DCHECK to
HostContentSettingsMap::SetWebsiteSettingCustomScope but we would only be able
to do it on Android, as other platforms can set non-origin patterns. That's why
having it here seemed ok too. If we are worried about trying to write invalid
URLs to Android and messing up the settings then we could use a CHECK.

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

Okay I have added the DCHECK here and in HCSM on Android for additional safety.

+    return false;
+  JNIEnv* env = AttachCurrentThread();
+  const std::string origin = primary_url.spec();
+  ScopedJavaLocalRef<jstring> jorigin = ConvertUTF8ToJavaString(env, origin);
+  ContentSetting setting = 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;

[Peter Beverloo, 2017/05/15 15:10:08]

When would creating a channel fail?

[awdf, 2017/05/16 15:17:35]

Well right now it does because I haven't implemented it yet :D

Right now there's also the chance that reflection could fail with an exception. 

Happy to take this out in future though, once implemented & the reflection is
gone (when we target O).

+      }

[raymes, 2017/05/16 01:08:18]

What if there was already a channel created but the channel has the opposite
setting to what is being set? I feel as though maybe we should DCHECK in that
case?

[awdf, 2017/05/16 15:17:35]

Done.

+      return true;
+    }
+    case CONTENT_SETTING_ASK:

[raymes, 2017/05/16 01:08:18]

This should never be passed in so I think we can remove this and fall through to
the default in that case

[awdf, 2017/05/16 15:17:35]

Done.

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

[raymes, 2017/05/16 01:08:17]

I'd suggest DCHECKing here too

[awdf, 2017/05/16 15:17:35]

again I wonder about dchecking this earlier on, if this is an assumption we
really care about.

In an ideal world I would love it if these assumptions could be enforced at
compile time, not runtime, eg by using a specialized content setting type for
notifications that only has three possible values, rather than attempting to
generalize everything. 

however i suppose maybe dchecking here is better than nothing in terms of adding
a sanity check for our assumptions.

Done.

+  }
+}
+
+void AndroidSettingsProvider::ClearAllContentSettingsRules(
+    ContentSettingsType content_type) {
+  // TODO(crbug.com/700377) delete all channels

[Peter Beverloo, 2017/05/15 15:10:08]

nit: if |content_type| == NOTIFICATIONS

[raymes, 2017/05/16 01:08:17]

DCHECK here too?

[awdf, 2017/05/16 15:17:35]

again, I don't think I can?

[awdf, 2017/05/16 15:17:35]

Done.

+}
+
+void AndroidSettingsProvider::ShutdownOnUIThread() {
+  RemoveAllObservers();
+}
+
+}  // namespace content_settings