[Android] Adding content settings provider for notification channels

chrome/browser/notifications/notification_channels_provider_android_unittest.cc

+// Copyright (c) 2017 The Chromium Authors. All rights reserved.

[raymes, 2017/05/24 00:59:51]

nit: I don't think we use (c) anymore
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[awdf, 2017/06/01 15:11:22]

Done.

+// 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/memory/ptr_util.h"
+#include "base/values.h"
+#include "components/content_settings/core/browser/content_settings_pref.h"
+#include "components/content_settings/core/common/content_settings_pattern.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+using ::testing::Return;
+
+class NotificationChannelsProviderAndroidTest : public testing::Test {
+ public:
+  NotificationChannelsProviderAndroidTest() {}

[Peter Beverloo, 2017/05/24 10:57:43]

TEST_F(X, Y) means that you're creating a test with a given fixture, i.e. a
class that (indirectly) extends testing::Test with additional functionality.
You're not adding anything here. In those cases you can just use TEST(X, Y) w/o
declaring the class.

However, each test here starts with the same block of code:

      auto* mock_bridge = new MockNotificationChannelsBridge();
      EXPECT_CALL(*mock_bridge,
ShouldUseChannelSettings()).WillOnce(Return(true));
      NotificationChannelsProviderAndroid channels_provider(
          base::WrapUnique(mock_bridge));
      // ...
      channels_provider.ShutdownOnUIThread();

It would be great if |channels_provider| can be a protected member of the
fixture. This reduces duplication, and a single point of allocation makes the
friend declaration you need in NCPA easier. 

[awdf, 2017/06/01 15:11:22]

Done.

+};
+
+class MockNotificationChannelsBridge
+    : public NotificationChannelsProviderAndroid::NotificationChannelsBridge {
+ public:

[Peter Beverloo, 2017/05/24 10:57:43]

nit: define dtor

[awdf, 2017/06/01 15:11:22]

Done. Do I also need to define a ctor?

[Peter Beverloo, 2017/06/01 15:26:11]

No, that's fine. It's just virtual destructors that we need to make sure we
override.

+  MOCK_METHOD0(ShouldUseChannelSettings, bool());
+  MOCK_METHOD2(CreateChannel, void(const std::string&, bool));
+  MOCK_METHOD1(GetChannelStatus, NotificationChannelStatus(const std::string&));
+  MOCK_METHOD1(DeleteChannel, void(const std::string&));
+};
+
+TEST_F(
+    NotificationChannelsProviderAndroidTest,
+    SetWebsiteSettingAllowedWhenChannelUnavailable_CreatesEnabledChannelAndReturnsTrue) {

[Peter Beverloo, 2017/05/24 10:57:42]

Such descriptive names are super rare in Chromium C++ code. Can we just call
this "CreatesEnabledChannelWhenUnavailable"? Especially the "AndReturnsTrue" is
not super interesting.

[awdf, 2017/06/01 15:11:22]

I'd argue that 'CreatesEnabledChannelWhenUnavailable' is not at all clear on its
own (when What is unavailable?).

I think when tests fail the test name should be useful not cryptic.

I agree wholeheartedly that the rest of chromium sucks at this. No reason not to
make our own test failure messages more useful though? When something fails I
wanna know what happened before checking the logs!

However, I have compromised by removing 'AndReturnsTrue' in some places in the
interests of keeping line length <80 chars.

[Peter Beverloo, 2017/06/01 15:26:11]

Okay!

+  auto* mock_bridge = new MockNotificationChannelsBridge();
+  EXPECT_CALL(*mock_bridge, ShouldUseChannelSettings()).WillOnce(Return(true));
+
+  NotificationChannelsProviderAndroid channels_provider(
+      base::WrapUnique(mock_bridge));
+
+  EXPECT_CALL(*mock_bridge, GetChannelStatus("https://example.com/"))

[Peter Beverloo, 2017/05/24 10:57:43]

nit: consider using a constant for the origin.

[awdf, 2017/06/01 15:11:22]

Done.

[awdf, 2017/06/01 15:11:22]

Done.

+      .WillOnce(Return(NotificationChannelStatus::UNAVAILABLE));
+  EXPECT_CALL(*mock_bridge,
+              CreateChannel("https://example.com/", true /* enabled */));
+
+  bool result = channels_provider.SetWebsiteSetting(
+      ContentSettingsPattern::FromString("https://example.com"),
+      ContentSettingsPattern(), CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+      std::string(), new base::Value(CONTENT_SETTING_ALLOW));
+
+  EXPECT_EQ(true, result);

[raymes, 2017/05/24 00:59:51]

nit (here and below): EXPECT_TRUE(result)

[awdf, 2017/06/01 15:11:22]

Done.

+
+  channels_provider.ShutdownOnUIThread();
+}
+
+TEST_F(
+    NotificationChannelsProviderAndroidTest,
+    SetWebsiteSettingBlockedWhenChannelUnavailable_CreatesDisabledChannelAndReturnsTrue) {
+  auto* mock_bridge = new MockNotificationChannelsBridge();
+  EXPECT_CALL(*mock_bridge, ShouldUseChannelSettings()).WillOnce(Return(true));
+
+  NotificationChannelsProviderAndroid channels_provider(
+      base::WrapUnique(mock_bridge));
+
+  EXPECT_CALL(*mock_bridge, GetChannelStatus("https://example.com/"))
+      .WillOnce(Return(NotificationChannelStatus::UNAVAILABLE));
+  EXPECT_CALL(*mock_bridge,
+              CreateChannel("https://example.com/", false /* enabled */));
+
+  bool result = channels_provider.SetWebsiteSetting(
+      ContentSettingsPattern::FromString("https://example.com"),
+      ContentSettingsPattern(), CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+      std::string(), new base::Value(CONTENT_SETTING_BLOCK));
+
+  EXPECT_EQ(true, result);
+
+  channels_provider.ShutdownOnUIThread();
+}
+
+TEST_F(NotificationChannelsProviderAndroidTest,
+       SetWebsiteSettingAllowedWhenChannelAllowed_NoopAndReturnsTrue) {
+  auto* mock_bridge = new MockNotificationChannelsBridge();
+  EXPECT_CALL(*mock_bridge, ShouldUseChannelSettings()).WillOnce(Return(true));
+
+  NotificationChannelsProviderAndroid channels_provider(
+      base::WrapUnique(mock_bridge));
+
+  EXPECT_CALL(*mock_bridge, GetChannelStatus("https://example.com/"))
+      .WillOnce(Return(NotificationChannelStatus::ENABLED));
+
+  bool result = channels_provider.SetWebsiteSetting(
+      ContentSettingsPattern::FromString("https://example.com"),
+      ContentSettingsPattern(), CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+      std::string(), new base::Value(CONTENT_SETTING_ALLOW));
+
+  EXPECT_EQ(true, result);
+
+  channels_provider.ShutdownOnUIThread();
+}
+
+TEST_F(NotificationChannelsProviderAndroidTest,
+       SetWebsiteSettingBlockedWhenChannelBlocked_NoopAndReturnsTrue) {
+  auto* mock_bridge = new MockNotificationChannelsBridge();
+  EXPECT_CALL(*mock_bridge, ShouldUseChannelSettings()).WillOnce(Return(true));
+
+  NotificationChannelsProviderAndroid channels_provider(
+      base::WrapUnique(mock_bridge));
+
+  EXPECT_CALL(*mock_bridge, GetChannelStatus("https://example.com/"))
+      .WillOnce(Return(NotificationChannelStatus::BLOCKED));
+
+  bool result = channels_provider.SetWebsiteSetting(
+      ContentSettingsPattern::FromString("https://example.com"),
+      ContentSettingsPattern(), CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+      std::string(), new base::Value(CONTENT_SETTING_BLOCK));
+
+  EXPECT_EQ(true, result);
+
+  channels_provider.ShutdownOnUIThread();
+}
+
+TEST_F(NotificationChannelsProviderAndroidTest,
+       SetWebsiteSettingDefault_DeletesChannelAndReturnsTrue) {
+  auto* mock_bridge = new MockNotificationChannelsBridge();
+  EXPECT_CALL(*mock_bridge, ShouldUseChannelSettings()).WillOnce(Return(true));
+
+  NotificationChannelsProviderAndroid channels_provider(
+      base::WrapUnique(mock_bridge));
+
+  EXPECT_CALL(*mock_bridge, DeleteChannel("https://example.com/"));
+  bool result = channels_provider.SetWebsiteSetting(
+      ContentSettingsPattern::FromString("https://example.com"),
+      ContentSettingsPattern(), CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+      std::string(), nullptr);
+
+  EXPECT_EQ(true, result);
+
+  channels_provider.ShutdownOnUIThread();
+}

[raymes, 2017/05/24 00:59:51]

nit: could we have a test which checks that the provider returns false if
ShouldUseChannelSettings returns false?

[awdf, 2017/06/01 15:11:22]

Done.