[views] Ensure browser theme change notifications propagate to children

Currently browser theme changes are only propagated to the BrowserFrame
root Widget. However currently other owned Widgets in the frame's
hierarchy inherit theme information but will not receive a notification
that this theme information has changed.

This CL ensures that owned Widgets receive ThemeChange() notifications
when the browser theme has changed.

Note: this is not an issue for NativeTheme changes as owned Widgets
maintain their own NativeTheme observations.

This CL also eliminates redundant code in status_bubble_views that
existed to propagate these browser theme change notifications to
the status bubble (which is now covered by the more general change
in this CL).

Bug: 1472289
Change-Id: I6bfb30cafc4c24510b2c382eef7cafe2fb615677
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4775743
Auto-Submit: Thomas Lukaszewicz <tluk@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1188597}

chrome/browser/themes/theme_service.cc

@@ -552,8 +552,11 @@ SkColor ThemeService::GetPolicyThemeColor() const {
 
 void ThemeService::SetBrowserColorScheme(
     ThemeService::BrowserColorScheme color_scheme) {
-  profile_->GetPrefs()->SetInteger(prefs::kBrowserColorScheme,
-                                   static_cast<int>(color_scheme));
+  {
+    base::AutoReset<bool> resetter(&should_suppress_theme_updates_, true);
+    profile_->GetPrefs()->SetInteger(prefs::kBrowserColorScheme,
+                                     static_cast<int>(color_scheme));
+  }
   NotifyThemeChanged();
 }
 
@@ -585,8 +588,11 @@ absl::optional<SkColor> ThemeService::GetUserColor() const {
 
 void ThemeService::SetBrowserColorVariant(
     ui::mojom::BrowserColorVariant color_variant) {
-  profile_->GetPrefs()->SetInteger(prefs::kBrowserColorVariant,
-                                   static_cast<int>(color_variant));
+  {
+    base::AutoReset<bool> resetter(&should_suppress_theme_updates_, true);
+    profile_->GetPrefs()->SetInteger(prefs::kBrowserColorVariant,
+                                     static_cast<int>(color_variant));
+  }
   NotifyThemeChanged();
 }
 

chrome/browser/ui/views/frame/browser_frame.cc

@@ -251,9 +251,22 @@ void BrowserFrame::UserChangedTheme(BrowserThemeChangeType theme_change_type) {
 
   // When the browser theme changes, the NativeTheme may also change.
   // In Incognito, the usage of dark or normal hinges on the browser theme.
-  if (theme_change_type == BrowserThemeChangeType::kBrowserTheme)
+  // TODO(tluk): This should no longer be necessary as the dark NativeTheme is
+  // no longer used for dark mode.
+  if (theme_change_type == BrowserThemeChangeType::kBrowserTheme) {
     SetNativeTheme(SelectNativeTheme());
 
+    // Browser theme changes are directly observed by the BrowserFrame. However
+    // the other Widgets in the frame's hierarchy may inherit this new theme
+    // information in their ColorProviderKeys and thus should also be forwarded
+    // theme change notifications.
+    Widget::Widgets widgets;
+    GetAllOwnedWidgets(GetNativeView(), &widgets);
+    for (auto* widget : widgets) {
+      widget->ThemeChanged();
+    }
+  }
+
   if (!RegenerateFrameOnThemeChange(theme_change_type)) {
     // If RegenerateFrame() returns true, ThemeChanged() was implicitly called,
     // so no need to call it explicitly.

chrome/browser/ui/views/frame/browser_frame_browsertest.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ui/views/frame/browser_frame.h"
 
+#include "base/scoped_observation.h"
 #include "base/test/bind.h"
 #include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
@@ -33,6 +34,7 @@
 #include "ui/color/color_provider_manager.h"
 #include "ui/color/color_recipe.h"
 #include "ui/native_theme/test_native_theme.h"
+#include "ui/views/bubble/bubble_dialog_model_host.h"
 #include "ui/views/views_delegate.h"
 
 namespace {
@@ -104,6 +106,45 @@ IN_PROC_BROWSER_TEST_F(BrowserFrameTest, WebAppsHasBoundsOnOpen) {
   app_browser->window()->Close();
 }
 
+class MockThemeObserver : public views::WidgetObserver {
+ public:
+  explicit MockThemeObserver(views::Widget* widget) {
+    widget_observation_.Observe(widget);
+  }
+  MOCK_METHOD(void, OnWidgetThemeChanged, (views::Widget*));
+
+ private:
+  base::ScopedObservation<views::Widget, views::WidgetObserver>
+      widget_observation_{this};
+};
+
+// Verifies that theme change notifications are propagated to child widgets for
+// browser theme changes.
+IN_PROC_BROWSER_TEST_F(BrowserFrameTest, ChildWidgetsReceiveThemeUpdates) {
+  BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser());
+
+  // Create a child popup Widget for the BrowserFrame.
+  const auto child_widget = std::make_unique<views::Widget>();
+  views::Widget::InitParams params(views::Widget::InitParams::TYPE_POPUP);
+  params.shadow_elevation = 1;
+  params.shadow_type = views::Widget::InitParams::ShadowType::kDrop;
+  params.bounds = {0, 0, 200, 200};
+  params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  params.parent = browser_view->GetWidget()->GetNativeView();
+  params.child = true;
+  child_widget->Init(std::move(params));
+
+  // Add a bubble widget and set up the theme change observer.
+  MockThemeObserver widget_child_observer(child_widget.get());
+
+  // Propagate a browser theme change notification to the root BrowserFrame
+  // widget and ensure the child widget is forwarded the theme change
+  // notification.
+  EXPECT_CALL(widget_child_observer, OnWidgetThemeChanged(testing::_)).Times(1);
+  static_cast<BrowserFrame*>(browser_view->GetWidget())
+      ->UserChangedTheme(BrowserThemeChangeType::kBrowserTheme);
+}
+
 // Runs browser color provider tests with ChromeRefresh2023 enabled and
 // disabled.
 class BrowserFrameColorProviderTest : public BrowserFrameTest,

chrome/browser/ui/views/frame/browser_view.cc

@@ -4186,9 +4186,6 @@ void BrowserView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
 void BrowserView::OnThemeChanged() {
   views::ClientView::OnThemeChanged();
 
-  if (status_bubble_)
-    status_bubble_->OnThemeChanged();
-
   FrameColorsChanged();
 }
 

chrome/browser/ui/views/status_bubble_views.cc

@@ -807,11 +807,6 @@ int StatusBubbleViews::GetWidthForURL(const std::u16string& url_string) {
   return elided_url_width + (kShadowThickness + kTextHorizPadding) * 2 + 1;
 }
 
-void StatusBubbleViews::OnThemeChanged() {
-  if (popup_)
-    popup_->ThemeChanged();
-}
-
 void StatusBubbleViews::SetStatus(const std::u16string& status_text) {
   if (size_.IsEmpty())
     return;  // We have no bounds, don't attempt to show the popup.

chrome/browser/ui/views/status_bubble_views.h

@@ -66,9 +66,6 @@ class StatusBubbleViews : public StatusBubble {
   // Gets the width that a bubble should be for a given string
   int GetWidthForURL(const std::u16string& url_string);
 
-  // Notifies the bubble's popup that browser's theme is changed.
-  void OnThemeChanged();
-
   // Overridden from StatusBubble:
   void SetStatus(const std::u16string& status) override;
   void SetURL(const GURL& url) override;

ui/views/widget/widget.h

@@ -516,11 +516,15 @@ class VIEWS_EXPORT Widget : public internal::NativeWidgetDelegate,
 
   // Returns all Widgets in |native_view|'s hierarchy, including itself if
   // it is one.
+  // TODO(tluk): This API should be updated to return Widgets rather than take
+  // an out param.
   static void GetAllChildWidgets(gfx::NativeView native_view,
                                  Widgets* children);
 
   // Returns all Widgets owned by |native_view| (including child widgets, but
   // not including itself).
+  // TODO(tluk): This API should be updated to return Widgets rather than take
+  // an out param.
   static void GetAllOwnedWidgets(gfx::NativeView native_view, Widgets* owned);
 
   // Re-parent a NativeView and notify all Widgets in |native_view|'s hierarchy