Move the caption color handling code from WindowStateDelegate to CustomFrameViewAsh.

A client may set custom WindowStateDelegate, so the code should be
in the CustomFrameViewAsh.

BUG=33693796
TEST=covered by unit tests.

Change-Id: Iccacddf8a0bce01ed1a3819c959e73d591a8d8ef
Reviewed-on: https://chromium-review.googlesource.com/972677
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545092}

ash/frame/caption_buttons/frame_caption_button_container_view.cc

@@ -114,10 +114,7 @@ const char FrameCaptionButtonContainerView::kViewClassName[] =
 
 FrameCaptionButtonContainerView::FrameCaptionButtonContainerView(
     views::Widget* frame)
-    : frame_(frame),
-      minimize_button_(NULL),
-      size_button_(NULL),
-      close_button_(NULL) {
+    : frame_(frame) {
   constexpr int kTouchOptimizedCaptionButtonsSpacing = 8;
   auto layout = std::make_unique<views::BoxLayout>(
       views::BoxLayout::kHorizontal, gfx::Insets(),

ash/frame/caption_buttons/frame_caption_button_container_view.h

@@ -138,9 +138,9 @@ class ASH_EXPORT FrameCaptionButtonContainerView
 
   // The buttons. In the normal button style, at most one of |minimize_button_|
   // and |size_button_| is visible.
-  FrameCaptionButton* minimize_button_;
-  FrameCaptionButton* size_button_;
-  FrameCaptionButton* close_button_;
+  FrameCaptionButton* minimize_button_ = nullptr;
+  FrameCaptionButton* size_button_ = nullptr;
+  FrameCaptionButton* close_button_ = nullptr;
 
   // Mapping of the image needed to paint a button for each of the values of
   // CaptionButtonIcon.

ash/frame/custom_frame_view_ash.cc

@@ -53,9 +53,8 @@ class CustomFrameViewAshWindowStateDelegate : public wm::WindowStateDelegate,
  public:
   CustomFrameViewAshWindowStateDelegate(wm::WindowState* window_state,
                                         CustomFrameViewAsh* custom_frame_view,
-                                        HeaderView* header_view,
                                         bool enable_immersive)
-      : window_state_(nullptr), header_view_(header_view) {
+      : window_state_(nullptr) {
     // Add a window state observer to exit fullscreen properly in case
     // fullscreen is exited without going through
     // WindowState::ToggleFullscreen(). This is the case when exiting
@@ -130,26 +129,6 @@ class CustomFrameViewAshWindowStateDelegate : public wm::WindowStateDelegate,
     window_state_ = nullptr;
   }
 
-  void OnWindowPropertyChanged(aura::Window* window,
-                               const void* key,
-                               intptr_t old) override {
-    DCHECK_EQ(window_state_->window(), window);
-    if (key != ash::kFrameActiveColorKey &&
-        key != ash::kFrameInactiveColorKey) {
-      return;
-    }
-
-    if (key == ash::kFrameActiveColorKey) {
-      header_view_->SetFrameColors(
-          window->GetProperty(ash::kFrameActiveColorKey),
-          header_view_->GetInactiveFrameColor());
-      return;
-    }
-    header_view_->SetFrameColors(
-        header_view_->GetActiveFrameColor(),
-        window->GetProperty(ash::kFrameInactiveColorKey));
-  }
-
   // wm::WindowStateObserver:
   void OnPostWindowStateTypeChange(wm::WindowState* window_state,
                                    mojom::WindowStateType old_type) override {
@@ -179,7 +158,6 @@ class CustomFrameViewAshWindowStateDelegate : public wm::WindowStateDelegate,
   }
 
   wm::WindowState* window_state_;
-  HeaderView* const header_view_;
   std::unique_ptr<ImmersiveFullscreenController>
       immersive_fullscreen_controller_;
 
@@ -342,13 +320,15 @@ CustomFrameViewAsh::CustomFrameViewAsh(
                             header_view_->GetActiveFrameColor());
   frame_window->SetProperty(ash::kFrameInactiveColorKey,
                             header_view_->GetInactiveFrameColor());
+  frame_window->AddObserver(this);
+
   // A delegate for a more complex way of fullscreening the window may already
   // be set. This is the case for packaged apps.
   wm::WindowState* window_state = wm::GetWindowState(frame_window);
   if (!window_state->HasDelegate()) {
     window_state->SetDelegate(std::unique_ptr<wm::WindowStateDelegate>(
-        new CustomFrameViewAshWindowStateDelegate(
-            window_state, this, header_view_, enable_immersive)));
+        new CustomFrameViewAshWindowStateDelegate(window_state, this,
+                                                  enable_immersive)));
   }
   Shell::Get()->AddShellObserver(this);
   Shell::Get()->split_view_controller()->AddObserver(this);
@@ -358,6 +338,10 @@ CustomFrameViewAsh::~CustomFrameViewAsh() {
   Shell::Get()->RemoveShellObserver(this);
   if (Shell::Get()->split_view_controller())
     Shell::Get()->split_view_controller()->RemoveObserver(this);
+  if (frame_ && frame_->GetNativeWindow() &&
+      frame_->GetNativeWindow()->HasObserver(this)) {
+    frame_->GetNativeWindow()->RemoveObserver(this);
+  }
 }
 
 void CustomFrameViewAsh::InitImmersiveFullscreenControllerForView(
@@ -370,12 +354,9 @@ void CustomFrameViewAsh::SetFrameColors(SkColor active_frame_color,
                                         SkColor inactive_frame_color) {
   header_view_->SetFrameColors(active_frame_color, inactive_frame_color);
   aura::Window* frame_window = frame_->GetNativeWindow();
-  frame_window->SetProperty(aura::client::kTopViewColor,
-                            header_view_->GetInactiveFrameColor());
-  frame_window->SetProperty(ash::kFrameActiveColorKey,
-                            header_view_->GetActiveFrameColor());
-  frame_window->SetProperty(ash::kFrameInactiveColorKey,
-                            header_view_->GetInactiveFrameColor());
+  frame_window->SetProperty(aura::client::kTopViewColor, inactive_frame_color);
+  frame_window->SetProperty(ash::kFrameActiveColorKey, active_frame_color);
+  frame_window->SetProperty(ash::kFrameInactiveColorKey, inactive_frame_color);
 }
 
 void CustomFrameViewAsh::SetBackButtonState(FrameBackButtonState state) {
@@ -434,7 +415,6 @@ void CustomFrameViewAsh::SizeConstraintsChanged() {
 void CustomFrameViewAsh::ActivationChanged(bool active) {
   // The icons differ between active and inactive.
   header_view_->SchedulePaint();
-
   frame_->non_client_view()->Layout();
 }
 
@@ -511,6 +491,28 @@ void CustomFrameViewAsh::SetVisible(bool visible) {
   InvalidateLayout();
 }
 
+void CustomFrameViewAsh::OnWindowDestroying(aura::Window* window) {
+  DCHECK_EQ(frame_->GetNativeWindow(), window);
+  window->RemoveObserver(this);
+}
+
+void CustomFrameViewAsh::OnWindowPropertyChanged(aura::Window* window,
+                                                 const void* key,
+                                                 intptr_t old) {
+  DCHECK_EQ(frame_->GetNativeWindow(), window);
+  if (key != ash::kFrameActiveColorKey && key != ash::kFrameInactiveColorKey)
+    return;
+
+  if (key == ash::kFrameActiveColorKey) {
+    header_view_->SetFrameColors(window->GetProperty(ash::kFrameActiveColorKey),
+                                 header_view_->GetInactiveFrameColor());
+    return;
+  }
+  header_view_->SetFrameColors(
+      header_view_->GetActiveFrameColor(),
+      window->GetProperty(ash::kFrameInactiveColorKey));
+}
+
 const views::View* CustomFrameViewAsh::GetAvatarIconViewForTest() const {
   return header_view_->avatar_icon();
 }

ash/frame/custom_frame_view_ash.h

@@ -14,6 +14,7 @@
 #include "base/macros.h"
 #include "base/optional.h"
 #include "third_party/skia/include/core/SkColor.h"
+#include "ui/aura/window_observer.h"
 #include "ui/views/window/non_client_view.h"
 
 namespace views {
@@ -41,7 +42,8 @@ enum class FrameBackButtonState {
 // BrowserNonClientFrameViewAsh.
 class ASH_EXPORT CustomFrameViewAsh : public views::NonClientFrameView,
                                       public ShellObserver,
-                                      public SplitViewController::Observer {
+                                      public SplitViewController::Observer,
+                                      public aura::WindowObserver {
  public:
   // Internal class name.
   static const char kViewClassName[];
@@ -102,6 +104,12 @@ class ASH_EXPORT CustomFrameViewAsh : public views::NonClientFrameView,
   void SchedulePaintInRect(const gfx::Rect& r) override;
   void SetVisible(bool visible) override;
 
+  // aura::WindowObserver:
+  void OnWindowDestroying(aura::Window* window) override;
+  void OnWindowPropertyChanged(aura::Window* window,
+                               const void* key,
+                               intptr_t old) override;
+
   // If |paint| is false, we should not paint the header. Used for overview mode
   // with OnOverviewModeStarting() and OnOverviewModeEnded() to hide/show the
   // header of v2 and ARC apps.

ash/frame/custom_frame_view_ash_unittest.cc

@@ -17,6 +17,7 @@
 #include "ash/wm/overview/window_selector_controller.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "ash/wm/window_state.h"
+#include "ash/wm/window_state_delegate.h"
 #include "ash/wm/wm_event.h"
 #include "services/ui/public/interfaces/window_manager_constants.mojom.h"
 #include "ui/aura/client/aura_constants.h"
@@ -499,10 +500,47 @@ TEST_F(CustomFrameViewAshTest, FrameVisibility) {
   EXPECT_TRUE(widget->non_client_view()->frame_view()->visible());
 }
 
+namespace {
+
+class CustomFrameViewAshFrameColorTest
+    : public CustomFrameViewAshTest,
+      public testing::WithParamInterface<bool> {
+ public:
+  CustomFrameViewAshFrameColorTest() = default;
+  ~CustomFrameViewAshFrameColorTest() override = default;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CustomFrameViewAshFrameColorTest);
+};
+
+class TestWidgetDelegate : public TestWidgetConstraintsDelegate {
+ public:
+  TestWidgetDelegate(bool custom) : custom_(custom) {}
+  ~TestWidgetDelegate() override = default;
+
+  // views::WidgetDelegate:
+  views::NonClientFrameView* CreateNonClientFrameView(
+      views::Widget* widget) override {
+    if (custom_) {
+      ash::wm::WindowState* window_state =
+          ash::wm::GetWindowState(widget->GetNativeWindow());
+      window_state->SetDelegate(std::make_unique<wm::WindowStateDelegate>());
+    }
+    return TestWidgetConstraintsDelegate::CreateNonClientFrameView(widget);
+  }
+
+ private:
+  bool custom_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestWidgetDelegate);
+};
+
+}  // namespace
+
 // Verify that CustomFrameViewAsh updates the active color based on the
 // ash::kFrameActiveColorKey window property.
-TEST_F(CustomFrameViewAshTest, kFrameActiveColorKey) {
-  TestWidgetConstraintsDelegate* delegate = new TestWidgetConstraintsDelegate;
+TEST_P(CustomFrameViewAshFrameColorTest, kFrameActiveColorKey) {
+  TestWidgetDelegate* delegate = new TestWidgetDelegate(GetParam());
   std::unique_ptr<views::Widget> widget(CreateWidget(delegate));
 
   SkColor active_color =
@@ -520,8 +558,8 @@ TEST_F(CustomFrameViewAshTest, kFrameActiveColorKey) {
 
 // Verify that CustomFrameViewAsh updates the inactive color based on the
 // ash::kFrameInactiveColorKey window property.
-TEST_F(CustomFrameViewAshTest, KFrameInactiveColor) {
-  TestWidgetConstraintsDelegate* delegate = new TestWidgetConstraintsDelegate;
+TEST_P(CustomFrameViewAshFrameColorTest, KFrameInactiveColor) {
+  TestWidgetDelegate* delegate = new TestWidgetDelegate(GetParam());
   std::unique_ptr<views::Widget> widget(CreateWidget(delegate));
 
   SkColor active_color =
@@ -538,4 +576,7 @@ TEST_F(CustomFrameViewAshTest, KFrameInactiveColor) {
             delegate->custom_frame_view()->GetInactiveFrameColorForTest());
 }
 
+// Run frame color tests with and without custom wm::WindowStateDelegate.
+INSTANTIATE_TEST_CASE_P(, CustomFrameViewAshFrameColorTest, testing::Bool());
+
 }  // namespace ash

ash/frame/default_frame_header.cc

@@ -306,6 +306,13 @@ void DefaultFrameHeader::PaintHeaderContentSeparator(gfx::Canvas* canvas) {
 
 void DefaultFrameHeader::UpdateAllButtonImages() {
   caption_button_container_->SetUseLightImages(ShouldUseLightImages());
+  if (back_button_) {
+    back_button_->set_use_light_images(ShouldUseLightImages());
+    back_button_->SetImage(CAPTION_BUTTON_ICON_BACK,
+                           FrameCaptionButton::ANIMATE_NO,
+                           kWindowControlBackIcon);
+  }
+
   caption_button_container_->SetButtonImage(CAPTION_BUTTON_ICON_MINIMIZE,
                                             kWindowControlMinimizeIcon);
 

ash/frame/header_view.cc

@@ -194,6 +194,8 @@ void HeaderView::SetShouldPaintHeader(bool paint) {
 void HeaderView::OnImmersiveRevealStarted() {
   fullscreen_visible_fraction_ = 0;
   SetPaintToLayer();
+  // The immersive layer should always be top.
+  layer()->parent()->StackAtTop(layer());
   parent()->Layout();
 }