[Android] Implement GetRuleIterator for channels provider

chrome/browser/notifications/notification_channels_provider_android.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/notification_channels_provider_android.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_rule.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/origin.h"
 #include "url/url_constants.h"
 
 using base::android::AttachCurrentThread;
@@ skipping 25 matching lines @@
     return static_cast<NotificationChannelStatus>(
         Java_NotificationSettingsBridge_getChannelStatus(
             env, ConvertUTF8ToJavaString(env, origin)));
   }
 
   void DeleteChannel(const std::string& origin) override {
     JNIEnv* env = AttachCurrentThread();
     Java_NotificationSettingsBridge_deleteChannel(
         env, ConvertUTF8ToJavaString(env, origin));
   }
+
+  std::vector<NotificationChannel> GetChannels() override {
+    JNIEnv* env = AttachCurrentThread();
+    ScopedJavaLocalRef<jobjectArray> raw_channels =
+        Java_NotificationSettingsBridge_getSiteChannels(env);
+    jsize num_channels = env->GetArrayLength(raw_channels.obj());
+    std::vector<NotificationChannel> channels;
+    for (jsize i = 0; i < num_channels; ++i) {
+      jobject jchannel = env->GetObjectArrayElement(raw_channels.obj(), i);
+      channels.emplace_back(
+          ConvertJavaStringToUTF8(Java_SiteChannel_getOrigin(env, jchannel)),
+          static_cast<NotificationChannelStatus>(
+              Java_SiteChannel_getStatus(env, jchannel)));
+    }
+    return channels;
+  }
+};
+
+ContentSetting ChannelStatusToContentSetting(NotificationChannelStatus status) {
+  switch (status) {
+    case NotificationChannelStatus::ENABLED:
+      return CONTENT_SETTING_ALLOW;
+    case NotificationChannelStatus::BLOCKED:
+      return CONTENT_SETTING_BLOCK;
+    case NotificationChannelStatus::UNAVAILABLE:
+      return CONTENT_SETTING_DEFAULT;

[raymes, 2017/06/04 23:51:21]

Arguably UNAVAILABLE doesn't really map to a content setting. I would suggest
adding NOTREACHED here too to help aid in this not being misused.

[awdf, 2017/06/05 16:29:26]

Done.

+  }
+  NOTREACHED();
+  return CONTENT_SETTING_DEFAULT;
+}
+
+class ChannelsRuleIterator : public content_settings::RuleIterator {
+ public:
+  explicit ChannelsRuleIterator(std::vector<NotificationChannel> channels)
+      : channels_(std::move(channels)), index_(0) {}
+
+  ~ChannelsRuleIterator() override {}

[Peter Beverloo, 2017/06/05 12:40:24]

micro nit: {} -> = default;

+
+  bool HasNext() const override { return index_ < channels_.size(); }
+
+  content_settings::Rule Next() override {
+    DCHECK(HasNext());
+    DCHECK_NE(channels_[index_].status_,
+              NotificationChannelStatus::UNAVAILABLE);
+    content_settings::Rule rule = content_settings::Rule(
+        ContentSettingsPattern::FromString(channels_[index_].origin_),
+        ContentSettingsPattern(),

[raymes, 2017/06/04 23:51:21]

I think returning ContentSettingsPattern::Wildcard here might be more in-line
with what other providers would do. An empty pattern is invalid and won't match
anything.

[awdf, 2017/06/05 16:29:26]

Done.

+        new base::Value(
+            ChannelStatusToContentSetting(channels_[index_].status_)));
+    index_++;
+    return rule;
+  }
+
+ private:
+  std::vector<NotificationChannel> channels_;
+  size_t index_;
 };

[Peter Beverloo, 2017/06/05 12:40:24]

nit: DISALLOW_COPY_AND_ASSIGN

[awdf, 2017/06/05 16:29:26]

Done.

 
 }  // anonymous namespace
 
 NotificationChannelsProviderAndroid::NotificationChannelsProviderAndroid()
     : NotificationChannelsProviderAndroid(
           base::MakeUnique<NotificationChannelsBridgeImpl>()) {}
 
 NotificationChannelsProviderAndroid::NotificationChannelsProviderAndroid(
     std::unique_ptr<NotificationChannelsBridge> bridge)
     : bridge_(std::move(bridge)),
       should_use_channels_(bridge_->ShouldUseChannelSettings()) {}
 
 NotificationChannelsProviderAndroid::~NotificationChannelsProviderAndroid() =
     default;
 
 std::unique_ptr<content_settings::RuleIterator>
 NotificationChannelsProviderAndroid::GetRuleIterator(
     ContentSettingsType content_type,
     const content_settings::ResourceIdentifier& resource_identifier,
     bool incognito) const {
-  // TODO(crbug.com/700377) return rule iterator over all channels
-  return nullptr;
+  if (content_type != CONTENT_SETTINGS_TYPE_NOTIFICATIONS || incognito ||

[Peter Beverloo, 2017/06/05 12:40:24]

nit: please document what this means

We don't enable notifications in incognito today, but there's no guarantee that
we won't ever. However, what we definitely won't want is for Incognito to spill
over to system UI, so this case should be considered orthogonal from the current
restrictions.

[awdf, 2017/06/05 16:29:26]

Right. But I have two questions:
1. Since this is in GetRuleIterator, where we are checking what permissions
exist, if one day we did allow notifications in incognito mode, wouldn't we
allow them from sites that already have permission in normal mode, in which case
reading those settings here is perfectly reasonable? Or do you envisage a system
where permission would have to be granted in both incognito and normal mode?
Either way I agree we want to make sure channels are definitely not *created*
from incognito mode, but right now there's no channel creation within this
method.

2. To make sure the current rules don't get applied in incognito mode right now,
should I actually be explicitly returning a global BLOCK content setting if
incognito=true? According to the documentation of this interface method, "If
|incognito| is true, the iterator returns only the content settings which are
applicable to the incognito mode and differ from the normal mode." ..Which
sounds a bit like it's saying that if no rules are returned when incognito is
true, the rules returned when incognito is false should apply. - OTOH this
doesn't seem like the correct place to be defining that rule and I'm assuming
there's some other place where 'if (incognito) then permission denied!' is
applied. So maybe I'm misinterpreting the documentation?

[Peter Beverloo, 2017/06/05 17:12:50]

To be frank: I don't know whether we'd inherit the notification permission in
Incognito profiles. It automatically means that sites can create push
subscriptions too, which can be surprising. Something for the very far future.

In what scenario would GetRuleIterator() actually create channels?
 
Returning a wildcard block here would definitely surprise me. The incognito
checks are in place elsewhere and are tested thoroughly. Raymes can comment on
the documentation I'm sure.

[awdf, 2017/06/05 17:33:36]

Re:
Sorry, think I was getting mixed up there, channels should only ever get created
via SetWebsiteSetting.

[raymes, 2017/06/06 01:39:04]

If incognito is true, this should just return settings that are different to
settings that apply to non-incognito. So returning nullptr is the right thing to
do here (I think!).

The normal pref provider deals with incognito settings in a separate in-memory
map. e.g. consider someone visiting a site in incognito and enabling
geolocation. We want that setting to apply but we don't want it to be written.
So it gets stored in-memory and only returned for that incognito session.

Technically we need to be careful in SetWebsiteSetting to make sure that we
don't write anything to the android channels when in incognito. Right now
though, as you point out, we don't grant notifications in incognito so it's not
a big deal. But if we did enable that at some point, SetWebsiteSetting would end
writing the incognito setting to disk. I'll leave it up to you/peter whether you
want to plumb through the incognito status in the constructor and check in
SetWebsiteSetting, but it may be worth adding a note for later.

[awdf, 2017/06/08 15:53:46]

Thanks. Added a TODO for now, will discuss with Peter whether it's necessary
when he's back. My instinct is that having another sanity check here just before
writing to system settings for incognito wouldn't hurt.

+      !should_use_channels_) {
+    return nullptr;
+  }
+  std::vector<NotificationChannel> channels = bridge_->GetChannels();
+  return channels.empty()
+             ? nullptr
+             : base::MakeUnique<ChannelsRuleIterator>(std::move(channels));
 }
 
 bool NotificationChannelsProviderAndroid::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_) {
@@ skipping 31 matching lines @@
 
 void NotificationChannelsProviderAndroid::ClearAllContentSettingsRules(
     ContentSettingsType content_type) {
   // TODO(crbug.com/700377): If |content_type| == NOTIFICATIONS, delete
   // all channels.
 }
 
 void NotificationChannelsProviderAndroid::ShutdownOnUIThread() {
   RemoveAllObservers();
 }