brave · petemill · Apr 8, 2019 · Apr 5, 2019
diff --git a/browser/extensions/api/brave_theme_api_browsertest.cc b/browser/extensions/api/brave_theme_api_browsertest.cc
@@ -4,54 +4,24 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "brave/browser/extensions/api/brave_theme_api.h"
-#include "brave/browser/extensions/brave_theme_event_router.h"
 #include "brave/browser/themes/brave_theme_service.h"
-#include "brave/browser/themes/theme_properties.h"
 #include "brave/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/browser/extensions/extension_function_test_utils.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/themes/theme_properties.h"
-#include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/ui/browser.h"
 #include "components/prefs/pref_service.h"
 #include "extensions/common/extension_builder.h"
-#include "testing/gmock/include/gmock/gmock.h"
 
-using BraveThemeEventRouterBrowserTest = InProcessBrowserTest;
+using BTS = BraveThemeService;
+using extensions::api::BraveThemeGetBraveThemeTypeFunction;
+using extension_function_test_utils::RunFunctionAndReturnSingleResult;
 
 namespace {
-class MockBraveThemeEventRouter : public extensions::BraveThemeEventRouter {
- public:
-  MockBraveThemeEventRouter() {}
-  ~MockBraveThemeEventRouter() override {}
-
-  MOCK_METHOD1(OnBraveThemeTypeChanged, void(Profile*));
-};
-
 void SetBraveThemeType(Profile* profile, BraveThemeType type) {
   profile->GetPrefs()->SetInteger(kBraveThemeType, type);
 }
 }  // namespace
-
-IN_PROC_BROWSER_TEST_F(BraveThemeEventRouterBrowserTest,
-                       BraveThemeEventRouterTest) {
-  Profile* profile = browser()->profile();
-  SetBraveThemeType(profile, BraveThemeType::BRAVE_THEME_TYPE_DARK);
-
-  MockBraveThemeEventRouter* mock_router_ = new MockBraveThemeEventRouter;
-  EXPECT_CALL(*mock_router_, OnBraveThemeTypeChanged(profile)).Times(1);
-
-  BraveThemeService* service = static_cast<BraveThemeService*>(
-      ThemeServiceFactory::GetForProfile(browser()->profile()));
-  service->SetBraveThemeEventRouterForTesting(mock_router_);
-  SetBraveThemeType(profile, BraveThemeType::BRAVE_THEME_TYPE_LIGHT);
-}
-
-using BTS = BraveThemeService;
-using extensions::api::BraveThemeGetBraveThemeTypeFunction;
-using extension_function_test_utils::RunFunctionAndReturnSingleResult;
-
 class BraveThemeAPIBrowserTest : public InProcessBrowserTest {
  public:
   void SetUpOnMainThread() override {

diff --git a/browser/extensions/brave_theme_event_router.cc b/browser/extensions/brave_theme_event_router.cc
@@ -5,45 +5,69 @@
 
 #include "brave/browser/extensions/brave_theme_event_router.h"
 
-#include <utility>
+#include <memory>
 #include <string>
+#include <utility>
 
 #include "brave/browser/themes/brave_theme_service.h"
 #include "brave/common/extensions/api/brave_theme.h"
 #include "chrome/browser/profiles/profile.h"
 #include "extensions/browser/event_router.h"
-#include "extensions/browser/extension_event_histogram_value.h"
+#include "ui/native_theme/native_theme_dark_aura.h"
 
 namespace extensions {
 
-class BraveThemeEventRouterImpl : public BraveThemeEventRouter {
- public:
-  BraveThemeEventRouterImpl() {}
-  ~BraveThemeEventRouterImpl() override {}
+BraveThemeEventRouter::BraveThemeEventRouter(Profile* profile)
+    : profile_(profile),
+      using_dark_(IsDarkModeEnabled()),
+      observer_(this) {
+  ResetThemeObserver();
+}
+
+BraveThemeEventRouter::~BraveThemeEventRouter() {}
 
-  void OnBraveThemeTypeChanged(Profile* profile) override;
+void BraveThemeEventRouter::OnNativeThemeUpdated(
+    ui::NativeTheme* observed_theme) {
+  DCHECK(observer_.IsObserving(observed_theme));
+  ResetThemeObserver();
 
- private:
-  DISALLOW_COPY_AND_ASSIGN(BraveThemeEventRouterImpl);
-};
+  bool use_dark = IsDarkModeEnabled();
+  if (use_dark == using_dark_)
+    return;
 
-void BraveThemeEventRouterImpl::OnBraveThemeTypeChanged(Profile* profile) {
-  EventRouter* event_router = EventRouter::Get(profile);
-  const std::string theme_type = BraveThemeService::GetStringFromBraveThemeType(
-      BraveThemeService::GetActiveBraveThemeType(profile));
+  using_dark_ = use_dark;
+  Notify();
+}
+
+void BraveThemeEventRouter::Notify() {
+  EventRouter* event_router = EventRouter::Get(profile_);
+  const std::string theme_type =
+      BraveThemeService::GetStringFromBraveThemeType(
+          BraveThemeService::GetActiveBraveThemeType(profile_));
 
   auto event = std::make_unique<extensions::Event>(
       extensions::events::BRAVE_ON_BRAVE_THEME_TYPE_CHANGED,
       api::brave_theme::OnBraveThemeTypeChanged::kEventName,
       api::brave_theme::OnBraveThemeTypeChanged::Create(theme_type),
-      profile);
+      profile_);
 
   event_router->BroadcastEvent(std::move(event));
 }
 
-// static
-std::unique_ptr<BraveThemeEventRouter> BraveThemeEventRouter::Create() {
-  return std::make_unique<BraveThemeEventRouterImpl>();
+bool BraveThemeEventRouter::IsDarkModeEnabled() const {
+  return BraveThemeService::GetActiveBraveThemeType(profile_) ==
+      BRAVE_THEME_TYPE_DARK;
+}
+
+void BraveThemeEventRouter::ResetThemeObserver() {
+  auto* current_native_theme =
+      IsDarkModeEnabled() ? ui::NativeThemeDarkAura::instance()
+                          : ui::NativeTheme::GetInstanceForNativeUi();
+  if (!observer_.IsObserving(current_native_theme)) {
+    observer_.RemoveAll();
+    observer_.Add(current_native_theme);
+    current_native_theme_for_testing_ = current_native_theme;
+  }
 }
 
 }  // namespace extensions
diff --git a/browser/extensions/brave_theme_event_router.h b/browser/extensions/brave_theme_event_router.h
@@ -1,23 +1,42 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public
+/* Copyright (c) 2019 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #ifndef BRAVE_BROWSER_EXTENSIONS_BRAVE_THEME_EVENT_ROUTER_H_
 #define BRAVE_BROWSER_EXTENSIONS_BRAVE_THEME_EVENT_ROUTER_H_
 
-#include <memory>
+#include "base/gtest_prod_util.h"
+#include "base/scoped_observer.h"
+#include "ui/native_theme/native_theme_observer.h"
 
 class Profile;
 
 namespace extensions {
 
-class BraveThemeEventRouter {
+class BraveThemeEventRouter : public ui::NativeThemeObserver {
  public:
-  static std::unique_ptr<BraveThemeEventRouter> Create();
+  explicit BraveThemeEventRouter(Profile* profile);
+  ~BraveThemeEventRouter() override;
 
-  virtual ~BraveThemeEventRouter() {}
+ private:
+  friend class MockBraveThemeEventRouter;
 
-  virtual void OnBraveThemeTypeChanged(Profile* profile) = 0;
+  // ui::NativeThemeObserver overrides:
+  void OnNativeThemeUpdated(ui::NativeTheme* observed_theme) override;
+
+  bool IsDarkModeEnabled() const;
+  void ResetThemeObserver();
+
+  // Make virtual for testing.
+  virtual void Notify();
+
+  ui::NativeTheme* current_native_theme_for_testing_ = nullptr;
+  Profile* profile_;
+  bool using_dark_;
+  ScopedObserver<ui::NativeTheme, BraveThemeEventRouter> observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(BraveThemeEventRouter);
 };
 
 }  // namespace extensions

diff --git a/browser/extensions/brave_theme_event_router_browsertest.cc b/browser/extensions/brave_theme_event_router_browsertest.cc
@@ -0,0 +1,69 @@
+/* Copyright (c) 2019 The Brave Authors. All rights reserved.
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include "brave/browser/extensions/brave_theme_event_router.h"
+#include "brave/browser/themes/brave_theme_service.h"
+#include "brave/common/pref_names.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/browser/themes/theme_service_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "components/prefs/pref_service.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "ui/native_theme/native_theme_dark_aura.h"
+
+using BraveThemeEventRouterBrowserTest = InProcessBrowserTest;
+
+namespace {
+
+void SetBraveThemeType(Profile* profile, BraveThemeType type) {
+  profile->GetPrefs()->SetInteger(kBraveThemeType, type);
+}
+
+}  // namespace
+
+namespace extensions {
+
+class MockBraveThemeEventRouter : public BraveThemeEventRouter {
+ public:
+  using BraveThemeEventRouter::BraveThemeEventRouter;
+  ~MockBraveThemeEventRouter() override {}
+
+  ui::NativeTheme* current_native_theme_for_testing() const {
+    return current_native_theme_for_testing_;
+  }
+
+  MOCK_METHOD0(Notify, void());
+};
+
+}  // namespace extensions
+
+
+IN_PROC_BROWSER_TEST_F(BraveThemeEventRouterBrowserTest,
+                       ThemeChangeTest) {
+  Profile* profile = browser()->profile();
+  SetBraveThemeType(profile, BraveThemeType::BRAVE_THEME_TYPE_DARK);
+
+  extensions::MockBraveThemeEventRouter* mock_router =
+      new extensions::MockBraveThemeEventRouter(profile);
+  BraveThemeService* service = static_cast<BraveThemeService*>(
+      ThemeServiceFactory::GetForProfile(browser()->profile()));
+  service->SetBraveThemeEventRouterForTesting(mock_router);
+
+  EXPECT_CALL(*mock_router, Notify()).Times(1);
+  SetBraveThemeType(profile, BraveThemeType::BRAVE_THEME_TYPE_LIGHT);
+  EXPECT_EQ(
+      ui::NativeTheme::GetInstanceForNativeUi(),
+      mock_router->current_native_theme_for_testing());
+
+  EXPECT_CALL(*mock_router, Notify()).Times(1);
+  SetBraveThemeType(profile, BraveThemeType::BRAVE_THEME_TYPE_DARK);
+  EXPECT_EQ(
+      ui::NativeThemeDarkAura::instance(),
+      mock_router->current_native_theme_for_testing());
+
+  EXPECT_CALL(*mock_router, Notify()).Times(0);
+  SetBraveThemeType(profile, BraveThemeType::BRAVE_THEME_TYPE_DARK);
+}
diff --git a/browser/themes/brave_theme_service.cc b/browser/themes/brave_theme_service.cc
@@ -163,6 +163,9 @@ void BraveThemeService::Init(Profile* profile) {
       profile->GetPrefs(),
       base::Bind(&BraveThemeService::OnPreferenceChanged,
                  base::Unretained(this)));
+
+    brave_theme_event_router_.reset(
+        new extensions::BraveThemeEventRouter(profile));
   }
 
   ThemeService::Init(profile);
@@ -209,11 +212,6 @@ void BraveThemeService::OnPreferenceChanged(const std::string& pref_name) {
         ? ui::NativeThemeDarkAura::instance()->NotifyObservers()
         : ui::NativeTheme::GetInstanceForNativeUi()->NotifyObservers();
   }
-
-  if (!brave_theme_event_router_)
-    brave_theme_event_router_ = extensions::BraveThemeEventRouter::Create();
-
-  brave_theme_event_router_->OnBraveThemeTypeChanged(profile());
 }
 
 void BraveThemeService::RecoverPrefStates(Profile* profile) {

diff --git a/browser/themes/brave_theme_service.h b/browser/themes/brave_theme_service.h
@@ -52,7 +52,7 @@ class BraveThemeService : public ThemeService {
  private:
   friend class BraveThemeServiceTestWithoutSystemTheme;
   FRIEND_TEST_ALL_PREFIXES(BraveThemeEventRouterBrowserTest,
-                           BraveThemeEventRouterTest);
+                           ThemeChangeTest);
   FRIEND_TEST_ALL_PREFIXES(BraveThemeServiceTest, GetBraveThemeListTest);
   FRIEND_TEST_ALL_PREFIXES(BraveThemeServiceTest, SystemThemeChangeTest);
 
@@ -72,6 +72,11 @@ class BraveThemeService : public ThemeService {
 
   IntegerPrefMember brave_theme_type_pref_;
 
+  // Make BraveThemeService own BraveThemeEventRouter.
+  // BraveThemeEventRouter does its job independently with BraveThemeService.
+  // However, both are related with brave theme and have similar life cycle.
+  // So, Owning BraveThemeEventRouter by BraveThemeService seems fine.
+  // Use smart ptr for testing by SetBraveThemeEventRouterForTesting.
   std::unique_ptr<extensions::BraveThemeEventRouter> brave_theme_event_router_;
 
   DISALLOW_COPY_AND_ASSIGN(BraveThemeService);

diff --git a/test/BUILD.gn b/test/BUILD.gn
@@ -302,6 +302,7 @@ test("brave_browser_tests") {
     "//brave/browser/extensions/brave_extension_functional_test.h",
     "//brave/browser/extensions/api/brave_shields_api_browsertest.cc",
     "//brave/browser/extensions/api/brave_theme_api_browsertest.cc",
+    "//brave/browser/extensions/brave_theme_event_router_browsertest.cc",
     "//brave/browser/net/brave_network_delegate_browsertest.cc",
     "//brave/browser/net/brave_network_delegate_hsts_fingerprinting_browsertest.cc",
     "//brave/browser/renderer_context_menu/brave_mock_render_view_context_menu.cc",