Theme Preferences for Distilled Pages

components/dom_distiller/core/distilled_page_prefs_unittests.cc

Index: components/dom_distiller/core/distilled_page_prefs_unittests.cc
diff --git a/components/dom_distiller/core/distilled_page_prefs_unittests.cc b/components/dom_distiller/core/distilled_page_prefs_unittests.cc
new file mode 100644
index 0000000000000000000000000000000000000000..dc32bbc4c6dabfb78ae249a3e778cf7a4dd04c5b
--- /dev/null
+++ b/components/dom_distiller/core/distilled_page_prefs_unittests.cc
@@ -0,0 +1,80 @@
+// Copyright 2014 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 "components/dom_distiller/core/distilled_page_prefs.h"
+
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "components/pref_registry/testing_pref_service_syncable.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace dom_distiller {
+
+namespace {

[nyquist, 2014/07/07 19:12:49]

I think you can end this namespace after the observer definition (i.e. just
before you declare the DistilledPagePrefsTest class).

[smaslo, 2014/07/08 19:58:11]

Done.

+
+void RunLoop() {

[nyquist, 2014/07/07 19:12:49]

Could you inline this to base::RunLoop().RunUntilIdle() ?

[smaslo, 2014/07/08 19:58:11]

Done.

+ base::RunLoop run_loop;
+ run_loop.RunUntilIdle();
+}
+
+class TestingObserver
+     : public DistilledPagePrefs::Observer {

[nyquist, 2014/07/07 19:12:49]

Nit: Does this really need to be on its own line?

[smaslo, 2014/07/08 19:58:11]

Done.

+ public:
+  virtual void OnChangeTheme(DistilledPagePrefs::Theme) OVERRIDE;

[nyquist, 2014/07/07 19:12:48]

I think this method and GetTheme can probably just be defined inline here.

[smaslo, 2014/07/08 19:58:11]

Done.

+  DistilledPagePrefs::Theme GetTheme();

[nyquist, 2014/07/07 19:12:49]

How about adding using DistilledPagePrefs::Observer given this is a unit test?

[smaslo, 2014/07/08 19:58:11]

Could not add "using DistilledPagePrefs::Theme;" because of compiler error
message: "using declaration cannot refer to class member".
On 2014/07/07 19:12:49, nyquist wrote:

+
+ private:
+  DistilledPagePrefs::Theme theme;

[nyquist, 2014/07/07 19:12:48]

theme_

[smaslo, 2014/07/08 19:58:11]

Done.

+};
+
+void TestingObserver::OnChangeTheme(

[nyquist, 2014/07/07 19:12:48]

same line after adding 'using'?

[smaslo, 2014/07/08 19:58:11]

Done.

+    DistilledPagePrefs::Theme new_theme) {
+  theme = new_theme;
+}
+
+DistilledPagePrefs::Theme TestingObserver::GetTheme() {
+  return theme;
+}
+
+class DistilledPagePrefsTest : public testing::Test {
+ protected:
+  virtual void SetUp() OVERRIDE {
+    user_prefs::TestingPrefServiceSyncable* pref_service =
+        new user_prefs::TestingPrefServiceSyncable();
+    DistilledPagePrefs::RegisterProfilePrefs(pref_service->registry());
+    distilled_page_prefs_ = new DistilledPagePrefs(pref_service);
+  }
+
+  DistilledPagePrefs* distilled_page_prefs_;
+
+ private:
+  base::MessageLoop message_loop_;
+};
+
+TEST_F(DistilledPagePrefsTest, TestingOnChangeThemeIsBeingCalled) {
+  TestingObserver* obs = new TestingObserver();
+  distilled_page_prefs_->AddObserver(obs);
+  distilled_page_prefs_->SetTheme(DistilledPagePrefs::Theme::kLight);
+  RunLoop();

[nyquist, 2014/07/07 19:12:49]

Add an EXPECT_EQ("", ...) before the call to RunLoop() to ensure that the task
was posted correctly. This requires the implementation to always post a task
instead of just notifying the observers in the SetTheme method.

[smaslo, 2014/07/08 19:58:11]

Done. Constructor for TestingObserver now initializes theme_ to kLight.

+  EXPECT_EQ(DistilledPagePrefs::Theme::kLight, obs->GetTheme());
+  distilled_page_prefs_->SetTheme(DistilledPagePrefs::Theme::kDark);
+  RunLoop();
+  EXPECT_EQ(DistilledPagePrefs::Theme::kDark, obs->GetTheme());
+}
+
+TEST_F(DistilledPagePrefsTest, TestingMultipleObservers) {

[nyquist, 2014/07/07 19:12:48]

This is not currently testing multiple observers, but I agree that it should.
See comment below.

[smaslo, 2014/07/08 19:58:11]

Done.

+  TestingObserver* obs = new TestingObserver();
+  distilled_page_prefs_->AddObserver(obs);
+  distilled_page_prefs_->SetTheme(DistilledPagePrefs::Theme::kSepia);

[nyquist, 2014/07/07 19:12:49]

This has already been tested in the method above. How about adding two observers
first, setting theme, removing one observer, and calling SetTheme again? You
naturally need to add the appropriate EXPECT_EQ calls to ensure the state is as
expected.

Since the test will then be a bit more convoluted, you might want to add a
comment or two. For example that in the last EXPECT_EQ series on the two
observers, only one observer should have been updated to the new theme or
something along those lines.

[smaslo, 2014/07/08 19:58:11]

Done.

+  RunLoop();
+  EXPECT_EQ(DistilledPagePrefs::Theme::kSepia, obs->GetTheme());
+  distilled_page_prefs_->RemoveObserver(obs);
+  distilled_page_prefs_->SetTheme(DistilledPagePrefs::Theme::kLight);
+  RunLoop();
+  EXPECT_EQ(DistilledPagePrefs::Theme::kSepia, obs->GetTheme());
+}
+
+}  // namespace
+
+}  // namespace dom_distiller