WebAppFrameToolbarView should respond directly to theme changes.

Specifically, the caption colors need to update.  On Ash this was happening
indirectly already, since BrowserNonClientFrameViewAsh::OnThemeChanged() was
calling UpdateFrameColor().  But on other platforms, it was not.

Handling this eliminates the need to set these colors at construction or in
BrowserNonClientFrameViewAsh::Init(), since OnThemeChanged() will always be
called when the frame is added to a widget, which happens after both those.
This also eliminates the need to null-check the frame view in
WebAppFrameToolbarView::UpdateCaptionColors(), since by OnThemeChanged() that
will always be set.  Finally, this eliminates the need for
BrowserNonClientFrameViewAsh::OnThemeChanged() to indirectly call
WebAppFrameToolbarView::UpdateCaptionColors(), since that is now called locally.

Also, the BrowserNonClientFrameView need not explicitly SchedulePaint(), since
if any children of the web app frame actually change color, they will invalidate
themselves as necessary.

Bug: none
Change-Id: If0a24ccebcf8ce105dc3662ee6468d6b9b36d419
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1980166Reviewed-by: Alan Cutter <alancutter@chromium.org>
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728425}

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

@@ -162,10 +162,8 @@ SkColor BrowserNonClientFrameView::GetFrameColor(
 
 void BrowserNonClientFrameView::UpdateFrameColor() {
   // Only web-app windows support dynamic frame colors set by HTML meta tags.
-  if (!web_app_frame_toolbar_)
-    return;
-  web_app_frame_toolbar_->UpdateCaptionColors();
-  SchedulePaint();
+  if (web_app_frame_toolbar_)
+    web_app_frame_toolbar_->UpdateCaptionColors();
 }
 
 SkColor BrowserNonClientFrameView::GetToolbarTopSeparatorColor() const {

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

@@ -165,8 +165,6 @@ void BrowserNonClientFrameViewAsh::Init() {
     SetUpForWebApp();
 
   browser_view()->immersive_mode_controller()->AddObserver(this);
-
-  UpdateFrameColor();
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -226,30 +224,7 @@ int BrowserNonClientFrameViewAsh::GetThemeBackgroundXInset() const {
 }
 
 void BrowserNonClientFrameViewAsh::UpdateFrameColor() {
-  aura::Window* window = frame()->GetNativeWindow();
-  base::Optional<SkColor> active_color, inactive_color;
-  if (!UsePackagedAppHeaderStyle(browser_view()->browser())) {
-    active_color = GetFrameColor(BrowserFrameActiveState::kActive);
-    inactive_color = GetFrameColor(BrowserFrameActiveState::kInactive);
-  } else if (browser_view()->IsBrowserTypeWebApp()) {
-    active_color = browser_view()->browser()->app_controller()->GetThemeColor();
-  } else if (!browser_view()->browser()->deprecated_is_app()) {
-    // TODO(crbug.com/836128): Remove when System Web Apps flag is removed, as
-    // the above web-app branch will render the theme color.
-    active_color = SK_ColorWHITE;
-  }
-
-  if (active_color) {
-    window->SetProperty(ash::kFrameActiveColorKey, *active_color);
-    window->SetProperty(ash::kFrameInactiveColorKey,
-                        inactive_color.value_or(*active_color));
-  } else {
-    window->ClearProperty(ash::kFrameActiveColorKey);
-    window->ClearProperty(ash::kFrameInactiveColorKey);
-  }
-
-  frame_header_->UpdateFrameColors();
-
+  OnUpdateFrameColor();
   BrowserNonClientFrameView::UpdateFrameColor();
 }
 
@@ -433,7 +408,7 @@ gfx::Size BrowserNonClientFrameViewAsh::GetMinimumSize() const {
 }
 
 void BrowserNonClientFrameViewAsh::OnThemeChanged() {
-  UpdateFrameColor();
+  OnUpdateFrameColor();
   BrowserNonClientFrameView::OnThemeChanged();
 }
 
@@ -715,11 +690,8 @@ void BrowserNonClientFrameViewAsh::SetUpForWebApp() {
     return;
 
   // Add the container for extra web app buttons (e.g app menu button).
-  set_web_app_frame_toolbar(new WebAppFrameToolbarView(
-      frame(), browser_view(),
-      GetCaptionColor(BrowserFrameActiveState::kActive),
-      GetCaptionColor(BrowserFrameActiveState::kInactive)));
-  AddChildView(web_app_frame_toolbar());
+  set_web_app_frame_toolbar(AddChildView(
+      std::make_unique<WebAppFrameToolbarView>(frame(), browser_view())));
 }
 
 void BrowserNonClientFrameViewAsh::UpdateTopViewInset() {
@@ -791,6 +763,32 @@ bool BrowserNonClientFrameViewAsh::IsInOverviewMode() const {
   return GetFrameWindow()->GetProperty(ash::kIsShowingInOverviewKey);
 }
 
+void BrowserNonClientFrameViewAsh::OnUpdateFrameColor() {
+  aura::Window* window = frame()->GetNativeWindow();
+  base::Optional<SkColor> active_color, inactive_color;
+  if (!UsePackagedAppHeaderStyle(browser_view()->browser())) {
+    active_color = GetFrameColor(BrowserFrameActiveState::kActive);
+    inactive_color = GetFrameColor(BrowserFrameActiveState::kInactive);
+  } else if (browser_view()->IsBrowserTypeWebApp()) {
+    active_color = browser_view()->browser()->app_controller()->GetThemeColor();
+  } else if (!browser_view()->browser()->deprecated_is_app()) {
+    // TODO(crbug.com/836128): Remove when System Web Apps flag is removed, as
+    // the above web-app branch will render the theme color.
+    active_color = SK_ColorWHITE;
+  }
+
+  if (active_color) {
+    window->SetProperty(ash::kFrameActiveColorKey, *active_color);
+    window->SetProperty(ash::kFrameInactiveColorKey,
+                        inactive_color.value_or(*active_color));
+  } else {
+    window->ClearProperty(ash::kFrameActiveColorKey);
+    window->ClearProperty(ash::kFrameInactiveColorKey);
+  }
+
+  frame_header_->UpdateFrameColors();
+}
+
 const aura::Window* BrowserNonClientFrameViewAsh::GetFrameWindow() const {
   return const_cast<BrowserNonClientFrameViewAsh*>(this)->GetFrameWindow();
 }

chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h

@@ -196,6 +196,9 @@ class BrowserNonClientFrameViewAsh
   // Returns whether this window is currently in the overview list.
   bool IsInOverviewMode() const;
 
+  // Called any time the frame color may have changed.
+  void OnUpdateFrameColor();
+
   // Returns the top level aura::Window for this browser window.
   const aura::Window* GetFrameWindow() const;
   aura::Window* GetFrameWindow();

chrome/browser/ui/views/frame/browser_non_client_frame_view_mac.mm

@@ -70,11 +70,8 @@ BrowserNonClientFrameViewMac::BrowserNonClientFrameViewMac(
 
   if (browser_view->IsBrowserTypeWebApp()) {
     if (browser_view->browser()->app_controller()->HasTitlebarToolbar()) {
-      set_web_app_frame_toolbar(
-          AddChildView(std::make_unique<WebAppFrameToolbarView>(
-              frame, browser_view,
-              GetCaptionColor(BrowserFrameActiveState::kActive),
-              GetCaptionColor(BrowserFrameActiveState::kInactive))));
+      set_web_app_frame_toolbar(AddChildView(
+          std::make_unique<WebAppFrameToolbarView>(frame, browser_view)));
     }
 
     // The window title appears above the web app frame toolbar (if present),

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

@@ -110,14 +110,8 @@ GlassBrowserFrameView::GlassBrowserFrameView(BrowserFrame* frame,
   web_app::AppBrowserController* controller =
       browser_view->browser()->app_controller();
   if (controller && controller->HasTitlebarToolbar()) {
-    // TODO(alancutter): Avoid snapshotting GetCaptionColor() values here and
-    // call it on demand in WebAppFrameToolbarView::UpdateIconsColor() via a
-    // delegate interface.
-    set_web_app_frame_toolbar(
-        AddChildView(std::make_unique<WebAppFrameToolbarView>(
-            frame, browser_view,
-            GetCaptionColor(BrowserFrameActiveState::kActive),
-            GetCaptionColor(BrowserFrameActiveState::kInactive))));
+    set_web_app_frame_toolbar(AddChildView(
+        std::make_unique<WebAppFrameToolbarView>(frame, browser_view)));
   }
 
   // The window title appears above the web app frame toolbar (if present),

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

@@ -177,11 +177,8 @@ void OpaqueBrowserFrameView::InitViews() {
   web_app::AppBrowserController* controller =
       browser_view()->browser()->app_controller();
   if (controller && controller->HasTitlebarToolbar()) {
-    set_web_app_frame_toolbar(
-        AddChildView(std::make_unique<WebAppFrameToolbarView>(
-            frame(), browser_view(),
-            GetCaptionColor(BrowserFrameActiveState::kActive),
-            GetCaptionColor(BrowserFrameActiveState::kInactive))));
+    set_web_app_frame_toolbar(AddChildView(
+        std::make_unique<WebAppFrameToolbarView>(frame(), browser_view())));
   }
 
   // The window title appears above the web app frame toolbar (if present),

chrome/browser/ui/views/web_apps/web_app_frame_toolbar_view.cc

@@ -53,7 +53,6 @@
 #include "ui/compositor/layer_animation_sequence.h"
 #include "ui/compositor/scoped_layer_animation_settings.h"
 #include "ui/events/event.h"
-#include "ui/gfx/color_palette.h"
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/geometry/rect.h"
@@ -650,12 +649,8 @@ void WebAppFrameToolbarView::ToolbarButtonContainer::OnWidgetVisibilityChanged(
 }
 
 WebAppFrameToolbarView::WebAppFrameToolbarView(views::Widget* widget,
-                                               BrowserView* browser_view,
-                                               SkColor active_color,
-                                               SkColor inactive_color)
-    : browser_view_(browser_view),
-      active_color_(active_color),
-      inactive_color_(inactive_color) {
+                                               BrowserView* browser_view)
+    : browser_view_(browser_view) {
   DCHECK(browser_view_);
   DCHECK(web_app::AppBrowserController::IsForWebAppBrowser(
       browser_view_->browser()));
@@ -699,7 +694,6 @@ WebAppFrameToolbarView::WebAppFrameToolbarView(views::Widget* widget,
                                     views::MaximumFlexSizeRule::kPreferred)
                                     .WithOrder(1));
 
-  UpdateChildrenColor();
   UpdateStatusIconsVisibility();
 
   DCHECK(!browser_view_->toolbar_button_provider() ||
@@ -720,10 +714,8 @@ void WebAppFrameToolbarView::UpdateStatusIconsVisibility() {
 void WebAppFrameToolbarView::UpdateCaptionColors() {
   const BrowserNonClientFrameView* frame_view =
       browser_view_->frame()->GetFrameView();
+  DCHECK(frame_view);
 
-  // frame_view is nullptr during BrowserNonClientFrameViewAsh::Init().
-  if (!frame_view)
-    return;
   active_color_ = frame_view->GetCaptionColor(BrowserFrameActiveState::kActive);
   inactive_color_ =
       frame_view->GetCaptionColor(BrowserFrameActiveState::kInactive);
@@ -868,6 +860,10 @@ void WebAppFrameToolbarView::ChildPreferredSizeChanged(views::View* child) {
   PreferredSizeChanged();
 }
 
+void WebAppFrameToolbarView::OnThemeChanged() {
+  UpdateCaptionColors();
+}
+
 views::View* WebAppFrameToolbarView::GetContentSettingContainerForTesting() {
   return right_container_->content_settings_container();
 }

chrome/browser/ui/views/web_apps/web_app_frame_toolbar_view.h

@@ -13,6 +13,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/ui/views/frame/toolbar_button_provider.h"
 #include "third_party/skia/include/core/SkColor.h"
+#include "ui/gfx/color_palette.h"
 #include "ui/views/accessible_pane_view.h"
 
 namespace views {
@@ -46,16 +47,13 @@ class WebAppFrameToolbarView : public views::AccessiblePaneView,
   // The total duration of the origin fade animation.
   static base::TimeDelta OriginTotalDuration();
 
-  // |active_color| and |inactive_color| indicate the colors to use
-  // for button icons when the window is focused and blurred respectively.
-  WebAppFrameToolbarView(views::Widget* widget,
-                         BrowserView* browser_view,
-                         SkColor active_color,
-                         SkColor inactive_color);
+  WebAppFrameToolbarView(views::Widget* widget, BrowserView* browser_view);
   ~WebAppFrameToolbarView() override;
 
   void UpdateStatusIconsVisibility();
 
+  // Called when the caption colors may have changed; updates the local values
+  // and triggers a repaint if necessary.
   void UpdateCaptionColors();
 
   // Sets the container to paints its buttons the active/inactive color.
@@ -94,6 +92,7 @@ class WebAppFrameToolbarView : public views::AccessiblePaneView,
   // views::AccessiblePaneView:
   const char* GetClassName() const override;
   void ChildPreferredSizeChanged(views::View* child) override;
+  void OnThemeChanged() override;
 
  private:
   friend class WebAppNonClientFrameViewAshTest;
@@ -118,8 +117,8 @@ class WebAppFrameToolbarView : public views::AccessiblePaneView,
 
   // Button and text colors.
   bool paint_as_active_ = true;
-  SkColor active_color_;
-  SkColor inactive_color_;
+  SkColor active_color_ = gfx::kPlaceholderColor;
+  SkColor inactive_color_ = gfx::kPlaceholderColor;
 
   class NavigationButtonContainer;
   class ToolbarButtonContainer;