[Android] Adding content settings provider for notification channels

chrome/browser/notifications/android_settings_provider.h

+// 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.
+
+#ifndef CHROME_BROWSER_NOTIFICATIONS_ANDROID_SETTINGS_PROVIDER_H_
+#define CHROME_BROWSER_NOTIFICATIONS_ANDROID_SETTINGS_PROVIDER_H_
+
+#include <string>
+
+#include "base/macros.h"
+#include "components/content_settings/core/browser/content_settings_observable_provider.h"
+#include "components/content_settings/core/browser/content_settings_observer.h"
+#include "components/content_settings/core/common/content_settings.h"
+#include "components/content_settings/core/common/content_settings_types.h"
+#include "components/keyed_service/core/keyed_service.h"
+
+namespace content_settings {

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

nit: only code that's part of the content_settings component should be in this
namespace.

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

Done (I note every other content settings observable provider declares itself
within the content_settings namespace..)

+
+// This class provides notification content settings from system notification
+// channels on Android O+.

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

It would be good to document the priority order of this provider. (I.e. it takes
precedence over normal content settings, but makes place for supervised users
and policy settings.)

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

Done.

+class AndroidSettingsProvider : public ObservableProvider {

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

I'd suggest calling this NotificationContentSettingsProviderAndroid. It's super
long, I realise, but:

  (a) We shouldn't generalize this yet. It'd need to move to a new place anyway
if it ends up being generalized.
  (b) "Settings" is much more overloaded than "ContentSettings"

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

Done. ..Although, what about 'NotificationChannelsProviderAndroid' - given we
are restricting its scope anyway, that would be perhaps more descriptive?

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

Up to you.

+ public:
+  AndroidSettingsProvider();
+  ~AndroidSettingsProvider() override;
+
+  // ProviderInterface methods:
+  std::unique_ptr<RuleIterator> GetRuleIterator(
+      ContentSettingsType content_type,
+      const ResourceIdentifier& resource_identifier,
+      bool incognito) const override;
+
+  bool SetWebsiteSetting(const ContentSettingsPattern& primary_pattern,
+                         const ContentSettingsPattern& secondary_pattern,
+                         ContentSettingsType content_type,
+                         const ResourceIdentifier& resource_identifier,
+                         base::Value* value) override;
+
+  void ClearAllContentSettingsRules(ContentSettingsType content_type) override;
+
+  void ShutdownOnUIThread() override;

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

micro nit: I wouldn't put the empty lines between these methods, because the
comment on line 26 applies to all of them.

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

interesting, even if they're multiline? That would make this inconsistent with
other ProviderInterface implementations..

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

Yeah, think of it as a "block" of implementations of the interface indicated by
the comment. (Which should have s/ProviderInterface/ObservableProvider/ btw.)

+
+ private:
+  bool should_use_channels_;
+
+  DISALLOW_COPY_AND_ASSIGN(AndroidSettingsProvider);
+};
+
+}  // namespace content_settings
+
+#endif  // CHROME_BROWSER_NOTIFICATIONS_ANDROID_SETTINGS_PROVIDER_H_