Frame button should use the target widget passed in HeaderView.

This is a regression in http://crrev.com/c/1053116.
The target widget is used in DetachedTitleAreaRenderer and WideFrameView.

BUG=b/33693796
TEST=covered by unittest

Change-Id: I839d13113a40abde4d25d7e634ad549937dac30b
Reviewed-on: https://chromium-review.googlesource.com/1067319
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561355}

ash/frame/custom_frame_header.cc

@@ -118,26 +118,23 @@ void PaintFrameImagesInRoundRect(gfx::Canvas* canvas,
 ///////////////////////////////////////////////////////////////////////////////
 // CustomFrameHeader, public:
 
-CustomFrameHeader::CustomFrameHeader() = default;
-
-CustomFrameHeader::~CustomFrameHeader() = default;
-
-void CustomFrameHeader::Init(
+CustomFrameHeader::CustomFrameHeader(
+    views::Widget* target_widget,
     views::View* view,
     AppearanceProvider* appearance_provider,
     bool incognito,
-    FrameCaptionButtonContainerView* caption_button_container) {
-  DCHECK(view);
+    FrameCaptionButtonContainerView* caption_button_container)
+    : FrameHeader(target_widget, view) {
   DCHECK(appearance_provider);
   DCHECK(caption_button_container);
-
-  set_view(view);
   appearance_provider_ = appearance_provider;
   is_incognito_ = incognito;
 
   SetCaptionButtonContainer(caption_button_container);
 }
 
+CustomFrameHeader::~CustomFrameHeader() = default;
+
 ///////////////////////////////////////////////////////////////////////////////
 // CustomFrameHeader, protected:
 
@@ -148,7 +145,7 @@ void CustomFrameHeader::DoPaintHeader(gfx::Canvas* canvas) {
 }
 
 AshLayoutSize CustomFrameHeader::GetButtonLayoutSize() const {
-  return GetWidget()->IsMaximized() || GetWidget()->IsFullscreen() ||
+  return target_widget()->IsMaximized() || target_widget()->IsFullscreen() ||
                  appearance_provider_->IsTabletMode()
              ? AshLayoutSize::kBrowserCaptionMaximized
              : AshLayoutSize::kBrowserCaptionRestored;
@@ -189,7 +186,7 @@ void CustomFrameHeader::PaintFrameImages(gfx::Canvas* canvas, bool active) {
       SkColorSetA(appearance_provider_->GetFrameHeaderColor(active), 0xFF);
 
   int corner_radius = 0;
-  if (!GetWidget()->IsMaximized() && !GetWidget()->IsFullscreen())
+  if (!target_widget()->IsMaximized() && !target_widget()->IsFullscreen())
     corner_radius = FrameHeaderUtil::GetTopCornerRadiusWhenRestored();
 
   PaintFrameImagesInRoundRect(canvas, frame_image, frame_overlay_image, alpha,

ash/frame/custom_frame_header.h

@@ -26,16 +26,16 @@ class ASH_EXPORT CustomFrameHeader : public FrameHeader {
     virtual bool IsTabletMode() = 0;
   };
 
-  CustomFrameHeader();
-  ~CustomFrameHeader() override;
-
   // BrowserFrameHeaderAsh does not take ownership of any of the parameters.
+  // |target_widget| is the widget that the caption buttons act on.
   // |view| is the view into which |this| will paint. |back_button| can be
   // nullptr, and the frame will not have a back button.
-  void Init(views::View* view,
-            AppearanceProvider* appearance_provider,
-            bool incognito,
-            FrameCaptionButtonContainerView* caption_button_container);
+  CustomFrameHeader(views::Widget* target_widget,
+                    views::View* view,
+                    AppearanceProvider* appearance_provider,
+                    bool incognito,
+                    FrameCaptionButtonContainerView* caption_button_container);
+  ~CustomFrameHeader() override;
 
  protected:
   // FrameHeader:

ash/frame/custom_frame_view_ash_unittest.cc

@@ -35,6 +35,7 @@
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_unittest_util.h"
+#include "ui/gfx/vector_icon_types.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_delegate.h"
 
@@ -616,20 +617,16 @@ TEST_F(CustomFrameViewAshTest, CustomButtonModel) {
   custom_frame_view->SizeConstraintsChanged();
   EXPECT_TRUE(test_api.menu_button()->enabled());
 
-// The addresses in library and in the main binary differ in
-// comoponent build.
-#if !defined(COMPONENT_BUILD)
   // zoom button
-  EXPECT_EQ(&kWindowControlMaximizeIcon,
-            test_api.size_button()->icon_definition_for_test());
+  EXPECT_STREQ(kWindowControlMaximizeIcon.name,
+               test_api.size_button()->icon_definition_for_test()->name);
   model_ptr->set_zoom_mode(true);
   custom_frame_view->SizeConstraintsChanged();
-  EXPECT_EQ(&ash::kWindowControlZoomIcon,
-            test_api.size_button()->icon_definition_for_test());
+  EXPECT_STREQ(kWindowControlZoomIcon.name,
+               test_api.size_button()->icon_definition_for_test()->name);
   widget->Maximize();
-  EXPECT_EQ(&ash::kWindowControlDezoomIcon,
-            test_api.size_button()->icon_definition_for_test());
-#endif
+  EXPECT_STREQ(kWindowControlDezoomIcon.name,
+               test_api.size_button()->icon_definition_for_test()->name);
 }
 
 TEST_F(CustomFrameViewAshTest, WideFrame) {
@@ -705,6 +702,40 @@ TEST_F(CustomFrameViewAshTest, WideFrame) {
             wide_frame_view->GetWidget()->GetWindowBoundsInScreen().width());
 }
 
+TEST_F(CustomFrameViewAshTest, WideFrameButton) {
+  auto* delegate = new CustomFrameTestWidgetDelegate();
+  std::unique_ptr<views::Widget> widget = CreateTestWidget(
+      delegate, kShellWindowId_DefaultContainer, gfx::Rect(100, 0, 400, 500));
+
+  WideFrameView* wide_frame_view = WideFrameView::Create(widget.get());
+  wide_frame_view->GetWidget()->Show();
+  HeaderView* header_view = wide_frame_view->header_view();
+  FrameCaptionButtonContainerView::TestApi test_api(
+      header_view->caption_button_container());
+
+  EXPECT_STREQ(kWindowControlMaximizeIcon.name,
+               test_api.size_button()->icon_definition_for_test()->name);
+  widget->Maximize();
+  header_view->Layout();
+  EXPECT_STREQ(kWindowControlRestoreIcon.name,
+               test_api.size_button()->icon_definition_for_test()->name);
+
+  widget->Restore();
+  header_view->Layout();
+  EXPECT_STREQ(kWindowControlMaximizeIcon.name,
+               test_api.size_button()->icon_definition_for_test()->name);
+
+  widget->SetFullscreen(true);
+  header_view->Layout();
+  EXPECT_STREQ(kWindowControlRestoreIcon.name,
+               test_api.size_button()->icon_definition_for_test()->name);
+
+  widget->SetFullscreen(false);
+  header_view->Layout();
+  EXPECT_STREQ(kWindowControlMaximizeIcon.name,
+               test_api.size_button()->icon_definition_for_test()->name);
+}
+
 namespace {
 
 class CustomFrameViewAshFrameColorTest

ash/frame/default_frame_header.cc

@@ -58,13 +58,13 @@ namespace ash {
 // DefaultFrameHeader, public:
 
 DefaultFrameHeader::DefaultFrameHeader(
+    views::Widget* target_widget,
     views::View* header_view,
     FrameCaptionButtonContainerView* caption_button_container)
-    : active_frame_color_(kDefaultFrameColor),
+    : FrameHeader(target_widget, header_view),
+      active_frame_color_(kDefaultFrameColor),
       inactive_frame_color_(kDefaultFrameColor) {
-  DCHECK(header_view);
   DCHECK(caption_button_container);
-  set_view(header_view);
   SetCaptionButtonContainer(caption_button_container);
 }
 
@@ -80,7 +80,7 @@ void DefaultFrameHeader::SetThemeColor(SkColor theme_color) {
 
 void DefaultFrameHeader::DoPaintHeader(gfx::Canvas* canvas) {
   int corner_radius =
-      (GetWidget()->IsMaximized() || GetWidget()->IsFullscreen())
+      (target_widget()->IsMaximized() || target_widget()->IsFullscreen())
           ? 0
           : FrameHeaderUtil::GetTopCornerRadiusWhenRestored();
 

ash/frame/default_frame_header.h

@@ -20,7 +20,8 @@ namespace ash {
 class ASH_EXPORT DefaultFrameHeader : public FrameHeader {
  public:
   // DefaultFrameHeader does not take ownership of any of the parameters.
-  DefaultFrameHeader(views::View* header_view,
+  DefaultFrameHeader(views::Widget* target_widget,
+                     views::View* header_view,
                      FrameCaptionButtonContainerView* caption_button_container);
   ~DefaultFrameHeader() override;
 

ash/frame/default_frame_header_unittest.cc

@@ -31,7 +31,7 @@ TEST_F(DefaultFrameHeaderTest, TitleIconAlignment) {
   w->SetBounds(gfx::Rect(0, 0, 500, 500));
   w->Show();
 
-  DefaultFrameHeader frame_header(w->non_client_view()->frame_view(),
+  DefaultFrameHeader frame_header(w.get(), w->non_client_view()->frame_view(),
                                   &container);
   frame_header.SetLeftHeaderView(&window_icon);
   frame_header.LayoutHeader();
@@ -46,7 +46,7 @@ TEST_F(DefaultFrameHeaderTest, BackButtonAlignment) {
   FrameCaptionButtonContainerView container(w.get());
   FrameBackButton back;
 
-  DefaultFrameHeader frame_header(w->non_client_view()->frame_view(),
+  DefaultFrameHeader frame_header(w.get(), w->non_client_view()->frame_view(),
                                   &container);
   frame_header.SetBackButton(&back);
   frame_header.LayoutHeader();
@@ -67,7 +67,7 @@ TEST_F(DefaultFrameHeaderTest, FrameColors) {
   w->SetBounds(gfx::Rect(0, 0, 500, 500));
   w->Show();
 
-  DefaultFrameHeader frame_header(w->non_client_view()->frame_view(),
+  DefaultFrameHeader frame_header(w.get(), w->non_client_view()->frame_view(),
                                   &container);
 
   // Check frame color is sensitive to mode.

ash/frame/frame_header.cc

@@ -47,7 +47,8 @@ void FrameHeader::PaintHeader(gfx::Canvas* canvas, Mode mode) {
   if (mode_ != old_mode) {
     UpdateCaptionButtonColors();
 
-    if (!initial_paint_ && FrameHeaderUtil::CanAnimateActivation(GetWidget())) {
+    if (!initial_paint_ &&
+        FrameHeaderUtil::CanAnimateActivation(target_widget_)) {
       activation_animation_.SetSlideDuration(kActivationCrossfadeDurationMs);
       if (mode_ == MODE_ACTIVE)
         activation_animation_.Show();
@@ -136,14 +137,10 @@ void FrameHeader::AnimationProgressed(const gfx::Animation* animation) {
 ///////////////////////////////////////////////////////////////////////////////
 // FrameHeader, protected:
 
-FrameHeader::FrameHeader() = default;
-
-views::Widget* FrameHeader::GetWidget() {
-  return view_->GetWidget();
-}
-
-const views::Widget* FrameHeader::GetWidget() const {
-  return view_->GetWidget();
+FrameHeader::FrameHeader(views::Widget* target_widget, views::View* view)
+    : target_widget_(target_widget), view_(view) {
+  DCHECK(target_widget);
+  DCHECK(view);
 }
 
 gfx::Rect FrameHeader::GetPaintedBounds() const {
@@ -160,11 +157,13 @@ void FrameHeader::UpdateCaptionButtonColors() {
 }
 
 void FrameHeader::PaintTitleBar(gfx::Canvas* canvas) {
-  if (GetWidget()->widget_delegate() &&
-      GetWidget()->widget_delegate()->ShouldShowWindowTitle() &&
-      !GetWidget()->widget_delegate()->GetWindowTitle().empty()) {
+  views::WidgetDelegate* target_widget_delegate =
+      target_widget_->widget_delegate();
+  if (target_widget_delegate &&
+      target_widget_delegate->ShouldShowWindowTitle() &&
+      !target_widget_delegate->GetWindowTitle().empty()) {
     canvas->DrawStringRectWithFlags(
-        GetWidget()->widget_delegate()->GetWindowTitle(),
+        target_widget_delegate->GetWindowTitle(),
         views::NativeWidgetAura::GetWindowTitleFontList(), GetTitleColor(),
         GetTitleBounds(), gfx::Canvas::NO_SUBPIXEL_RENDERING);
   }
@@ -189,17 +188,15 @@ void FrameHeader::SetCaptionButtonContainer(
 // FrameHeader, private:
 
 void FrameHeader::LayoutHeaderInternal() {
-  if (!GetWidget())
-    return;
-
   bool use_zoom_icons = caption_button_container()->model()->InZoomMode();
   const gfx::VectorIcon& restore_icon =
       use_zoom_icons ? kWindowControlDezoomIcon : kWindowControlRestoreIcon;
   const gfx::VectorIcon& maximize_icon =
       use_zoom_icons ? kWindowControlZoomIcon : kWindowControlMaximizeIcon;
   const gfx::VectorIcon& icon =
-      GetWidget()->IsMaximized() || GetWidget()->IsFullscreen() ? restore_icon
-                                                                : maximize_icon;
+      target_widget_->IsMaximized() || target_widget_->IsFullscreen()
+          ? restore_icon
+          : maximize_icon;
   caption_button_container()->SetButtonImage(
       CAPTION_BUTTON_ICON_MAXIMIZE_RESTORE, icon);
 

ash/frame/frame_header.h

@@ -75,10 +75,10 @@ class ASH_EXPORT FrameHeader : public gfx::AnimationDelegate {
   void AnimationProgressed(const gfx::Animation* animation) override;
 
  protected:
-  FrameHeader();
+  FrameHeader(views::Widget* target_widget, views::View* view);
 
-  views::Widget* GetWidget();
-  const views::Widget* GetWidget() const;
+  views::Widget* target_widget() { return target_widget_; }
+  const views::Widget* target_widget() const { return target_widget_; }
 
   // Returns bounds of the region in |view_| which is painted with the header
   // images. The region is assumed to start at the top left corner of |view_|
@@ -92,8 +92,6 @@ class ASH_EXPORT FrameHeader : public gfx::AnimationDelegate {
   void SetCaptionButtonContainer(
       FrameCaptionButtonContainerView* caption_button_container);
 
-  void set_view(views::View* view) { view_ = view; }
-
   void set_button_color_mode(FrameCaptionButton::ColorMode button_color_mode) {
     button_color_mode_ = button_color_mode;
   }
@@ -129,10 +127,14 @@ class ASH_EXPORT FrameHeader : public gfx::AnimationDelegate {
 
   gfx::Rect GetTitleBounds() const;
 
+  // The widget that the caption buttons act on. This can be different from
+  // |view_|'s widget.
+  views::Widget* target_widget_;
+
   // The view into which |this| paints.
-  views::View* view_ = nullptr;
-  FrameCaptionButton* back_button_ = nullptr;  // May be nullptr.
-  views::View* left_header_view_ = nullptr;    // May be nullptr.
+  views::View* view_;
+  FrameCaptionButton* back_button_ = nullptr;  // May remain nullptr.
+  views::View* left_header_view_ = nullptr;    // May remain nullptr.
   FrameCaptionButton::ColorMode button_color_mode_ =
       FrameCaptionButton::ColorMode::kDefault;
   FrameCaptionButtonContainerView* caption_button_container_ = nullptr;

ash/frame/header_view.cc

@@ -78,17 +78,17 @@ HeaderView::HeaderView(views::Widget* target_widget,
   AddChildView(caption_button_container_);
 
   if (window_style == mojom::WindowStyle::DEFAULT) {
-    frame_header_ =
-        std::make_unique<DefaultFrameHeader>(this, caption_button_container_);
+    frame_header_ = std::make_unique<DefaultFrameHeader>(
+        target_widget, this, caption_button_container_);
   } else {
     DCHECK_EQ(mojom::WindowStyle::BROWSER, window_style);
     DCHECK_EQ(Config::MASH, Shell::GetAshConfig());
     appearance_provider_ = std::make_unique<WindowPropertyAppearanceProvider>(
         target_widget_->GetNativeWindow());
-    auto frame_header = std::make_unique<CustomFrameHeader>();
     // TODO(estade): pass correct value for |incognito|.
-    frame_header->Init(this, appearance_provider_.get(), false,
-                       caption_button_container_);
+    auto frame_header = std::make_unique<CustomFrameHeader>(
+        target_widget, this, appearance_provider_.get(), false,
+        caption_button_container_);
     frame_header_ = std::move(frame_header);
   }
   aura::Window* window = target_widget->GetNativeWindow();

ash/wm/panels/panel_frame_view.cc

@@ -52,8 +52,8 @@ void PanelFrameView::InitFrameHeader() {
   caption_button_container_ = new FrameCaptionButtonContainerView(frame_);
   AddChildView(caption_button_container_);
 
-  frame_header_ =
-      std::make_unique<DefaultFrameHeader>(this, caption_button_container_);
+  frame_header_ = std::make_unique<DefaultFrameHeader>(
+      frame_, this, caption_button_container_);
   GetWidgetWindow()->SetProperty(aura::client::kTopViewColor,
                                  kDefaultFrameColor);
 

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

@@ -696,14 +696,13 @@ BrowserNonClientFrameViewAsh::CreateFrameHeader() {
 
   Browser* browser = browser_view()->browser();
   if (!UsePackagedAppHeaderStyle()) {
-    auto browser_frame_header = std::make_unique<ash::CustomFrameHeader>();
-    browser_frame_header->Init(this, this,
-                               !browser_view()->IsRegularOrGuestSession(),
-                               caption_button_container_);
+    auto browser_frame_header = std::make_unique<ash::CustomFrameHeader>(
+        frame(), this, this, !browser_view()->IsRegularOrGuestSession(),
+        caption_button_container_);
     header = std::move(browser_frame_header);
   } else {
     std::unique_ptr<ash::DefaultFrameHeader> default_frame_header =
-        std::make_unique<ash::DefaultFrameHeader>(this,
+        std::make_unique<ash::DefaultFrameHeader>(frame(), this,
                                                   caption_button_container_);
     // TODO(alancutter): Move this branch into a new HostedAppFrameHeader class.
     if (extensions::HostedAppBrowserController::