Update avatar button to MD (part 1)

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/frame/glass_browser_frame_view.h"
 
 #include <dwmapi.h>
 #include <utility>
 
 #include "base/win/windows_version.h"
@@ skipping 31 matching lines @@
 namespace {
 // Thickness of the frame edge between the non-client area and the web content.
 const int kClientBorderThickness = 3;
 // Besides the frame border, there's empty space atop the window in restored
 // mode, to use to drag the window around.
 const int kNonClientRestoredExtraThickness = 11;
 // At the window corners the resize area is not actually bigger, but the 16
 // pixels at the end of the top and bottom edges trigger diagonal resizing.
 const int kResizeCornerWidth = 16;
 // How far the profile switcher button is from the left of the minimize button.
-const int kProfileSwitcherButtonOffset = 5;
+const int kProfileSwitcherButtonOffset = 1;
 // The content edge images have a shadow built into them.
 const int kContentEdgeShadowThickness = 2;
 // In restored mode, the New Tab button isn't at the same height as the caption
 // buttons, but the space will look cluttered if it actually slides under them,
 // so we stop it when the gap between the two is down to 5 px.
 const int kNewTabCaptionRestoredSpacing = 5;
 // In maximized mode, where the New Tab button and the caption buttons are at
 // similar vertical coordinates, we need to reserve a larger, 16 px gap to avoid
 // looking too cluttered.
 const int kNewTabCaptionMaximizedSpacing = 16;
-// Height of the profile switcher button. Same as the height of the Windows 7/8
-// caption buttons.
-// TODO(bsep): Windows 10 caption buttons look very different and we would like
-// the profile switcher button to match on that platform.
-const int kProfileSwitcherButtonHeight = 20;
 // There is a small one-pixel strip right above the caption buttons in which the
 // resize border "peeks" through.
 const int kCaptionButtonTopInset = 1;
 
 // Converts the |image| to a Windows icon and returns the corresponding HICON
 // handle. |image| is resized to desired |width| and |height| if needed.
 base::win::ScopedHICON CreateHICONFromSkBitmapSizedTo(
     const gfx::ImageSkia& image,
     int width,
     int height) {
@@ skipping 14 matching lines @@
                                              BrowserView* browser_view)
     : BrowserNonClientFrameView(frame, browser_view),
       window_icon_(nullptr),
       window_title_(nullptr),
       profile_switcher_(this),
       minimize_button_(nullptr),
       maximize_button_(nullptr),
       restore_button_(nullptr),
       close_button_(nullptr),
       throbber_running_(false),
-      throbber_frame_(0) {
+      throbber_frame_(0),
+      tab_strip_(nullptr) {
   // We initialize all fields despite some of them being unused in some modes,
   // since it's possible for modes to flip dynamically (e.g. if the user enables
   // a high-contrast theme). Throbber icons are only used when ShowSystemIcon()
   // is true. Everything else here is only used when
   // ShouldCustomDrawSystemTitlebar() is true.
 
   if (browser_view->ShouldShowWindowIcon()) {
     InitThrobberIcons();
 
     window_icon_ = new TabIconView(this, nullptr);
@@ skipping 12 matching lines @@
     AddChildView(window_title_);
   }
 
   minimize_button_ = CreateCaptionButton(VIEW_ID_MINIMIZE_BUTTON);
   maximize_button_ = CreateCaptionButton(VIEW_ID_MAXIMIZE_BUTTON);
   restore_button_ = CreateCaptionButton(VIEW_ID_RESTORE_BUTTON);
   close_button_ = CreateCaptionButton(VIEW_ID_CLOSE_BUTTON);
 }
 
 GlassBrowserFrameView::~GlassBrowserFrameView() {
+  if (tab_strip_) {
+    tab_strip_->RemoveObserver(this);
+    tab_strip_ = nullptr;
+  }
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // GlassBrowserFrameView, BrowserNonClientFrameView implementation:
 
 gfx::Rect GlassBrowserFrameView::GetBoundsForTabStrip(
     views::View* tabstrip) const {
   const int x = incognito_bounds_.right() + kAvatarIconPadding;
   int end_x = width() - ClientBorderThickness(false);
   if (!CaptionButtonsOnLeadingEdge()) {
     end_x = std::min(MinimizeButtonX(), end_x) -
             (IsMaximized() ? kNewTabCaptionMaximizedSpacing
                            : kNewTabCaptionRestoredSpacing);
 
     // The profile switcher button is optionally displayed to the left of the
     // minimize button.
-    if (profile_switcher_.view()) {
+    views::View* button_view = GetProfileSwitcherView();
+    if (button_view) {
       const int old_end_x = end_x;
-      end_x -= profile_switcher_.view()->width() + kProfileSwitcherButtonOffset;
+      end_x -= button_view->width() + kProfileSwitcherButtonOffset;
 
       // In non-maximized mode, allow the new tab button to slide completely
       // under the profile switcher button.
+      // Note that the button should be "cozy" then, even if it's an MD button.
       if (!IsMaximized()) {
         end_x = std::min(end_x + GetLayoutSize(NEW_TAB_BUTTON).width() +
                              kNewTabCaptionRestoredSpacing,
                          old_end_x);
       }
     }
   }
   return gfx::Rect(x, TopAreaHeight(false), std::max(0, end_x - x),
                    tabstrip->GetPreferredSize().height());
 }
@@ skipping 43 matching lines @@
     min_size.set_width(std::max(min_tabstrip_area_width, min_size.width()));
   }
 
   return min_size;
 }
 
 views::View* GlassBrowserFrameView::GetProfileSwitcherView() const {
   return profile_switcher_.view();
 }
 
+void GlassBrowserFrameView::OnBrowserViewInitViewsComplete() {
+  DCHECK(browser_view()->tabstrip());
+  DCHECK(!tab_strip_);
+  tab_strip_ = browser_view()->tabstrip();
+  tab_strip_->AddObserver(this);
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // GlassBrowserFrameView, views::NonClientFrameView implementation:
 
 gfx::Rect GlassBrowserFrameView::GetBoundsForClientView() const {
   return client_view_bounds_;
 }
 
 gfx::Rect GlassBrowserFrameView::GetWindowBoundsForClientBounds(
     const gfx::Rect& client_bounds) const {
   HWND hwnd = views::HWNDForWidget(frame());
@@ skipping 33 matching lines @@
     return HTNOWHERE;
 
   // If the point isn't within our bounds, then it's in the native portion of
   // the frame so again Windows can figure it out.
   if (!bounds().Contains(point))
     return HTNOWHERE;
 
   // See if the point is within the incognito icon or the profile switcher menu.
   if ((profile_indicator_icon() &&
        profile_indicator_icon()->GetMirroredBounds().Contains(point)) ||
-      (profile_switcher_.view() &&
-       profile_switcher_.view()->GetMirroredBounds().Contains(point)))
+      (GetProfileSwitcherView() &&
+       GetProfileSwitcherView()->GetMirroredBounds().Contains(point)))
     return HTCLIENT;
 
   int frame_component = frame()->client_view()->NonClientHitTest(point);
 
   // See if we're in the sysmenu region.  We still have to check the tabstrip
   // first so that clicks in a tab don't get treated as sysmenu clicks.
   int client_border_thickness = ClientBorderThickness(false);
   gfx::Rect sys_menu_region(
       client_border_thickness,
       display::win::ScreenWin::GetSystemMetricsInDIP(SM_CYSIZEFRAME),
@@ skipping 114 matching lines @@
     // must be called prior to LayoutProfileSwitcher().
     LayoutCaptionButtons();
     LayoutTitleBar();
   }
   if (browser_view()->IsRegularOrGuestSession())
     LayoutProfileSwitcher();
   LayoutIncognitoIcon();
   LayoutClientView();
 }
 
+void GlassBrowserFrameView::ChildPreferredSizeChanged(views::View* child) {
+  if (child == profile_switcher_.view()) {

[Evan Stade, 2017/05/02 15:49:20]

nit: GetProfileSwitcherView()? I don't have much of a preference but it would be
good to be consistent, I guess.

[emx, 2017/05/03 17:10:59]

Done.

+    // Need to layout the root view here so that the line below the tabstrip
+    // is re-drawn properly when a profile is added or removed and the avatar
+    // button changes between tall and cozy as a result.
+    frame()->GetRootView()->Layout();
+  }
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // GlassBrowserFrameView, protected:
 
 // BrowserNonClientFrameView:
 void GlassBrowserFrameView::UpdateProfileIcons() {
   if (browser_view()->IsRegularOrGuestSession())
     profile_switcher_.Update(AvatarButtonStyle::NATIVE);
   else
     UpdateProfileIndicatorIcon();
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // GlassBrowserFrameView, private:
 
 // views::NonClientFrameView:
 bool GlassBrowserFrameView::DoesIntersectRect(const views::View* target,
                                               const gfx::Rect& rect) const {
   if (ShouldCustomDrawSystemTitlebar())
     return BrowserNonClientFrameView::DoesIntersectRect(target, rect);
 
   // TODO(bsep): This override has "dead zones" where you can't click on the
   // custom titlebar buttons. It's not clear why it's necessary at all.
   // Investigate tearing this out.
   CHECK_EQ(target, this);
   bool hit_incognito_icon =
       profile_indicator_icon() &&
       profile_indicator_icon()->GetMirroredBounds().Intersects(rect);
   bool hit_profile_switcher_button =
-      profile_switcher_.view() &&
-      profile_switcher_.view()->GetMirroredBounds().Intersects(rect);
+      GetProfileSwitcherView() &&
+      GetProfileSwitcherView()->GetMirroredBounds().Intersects(rect);
   return hit_incognito_icon || hit_profile_switcher_button ||
          !frame()->client_view()->bounds().Intersects(rect);
 }
 
 int GlassBrowserFrameView::ClientBorderThickness(bool restored) const {
   // The frame ends abruptly at the 1 pixel window border drawn by Windows 10.
   if (!browser_view()->HasClientEdge())
     return 0;
 
   if ((IsMaximized() || frame()->IsFullscreen()) && !restored)
@@ skipping 259 matching lines @@
                                                 gfx::Canvas* canvas) const {
   gfx::Rect side(x - kClientEdgeThickness, y, kClientEdgeThickness,
                  bottom + kClientEdgeThickness - y);
   canvas->FillRect(side, color);
   canvas->FillRect(gfx::Rect(x, bottom, right - x, kClientEdgeThickness),
                    color);
   side.set_x(right);
   canvas->FillRect(side, color);
 }
 
+void GlassBrowserFrameView::TabStripMaxXChanged(TabStrip* tab_strip) {
+  // May switch between cozy and tall MD avatar button here.
+  DCHECK_EQ(tab_strip, tab_strip_);
+  ChildPreferredSizeChanged(profile_switcher_.view());

[Evan Stade, 2017/05/02 15:49:20]

can you call this conditionally? Only if the button's height doesn't match what
it should be with the new max_x. Currently this is causing a lot of unnecessary
extra layouts.

Also technically the child's preferred size hasn't changed, so I wouldn't re-use
this function. Instead call frame()->GetRootView()->Layout(); directly (if
necessary).

[emx, 2017/05/03 17:10:59]

I think calling this conditionally would be unsafe - I'm not sure the result of
estimating whether the button size needs to change would always be right. Note
this comment in Layout():

    // The profile switcher button depends on the caption button layout, so this
    // must be called prior to LayoutProfileSwitcher().
    LayoutCaptionButtons();

I also think it's unlikely to have a noticeable impact on performance. In my
testing, TabStripMaxXChanged is only called when the window width is being
changed while the tabstrip is full. Layout is already called much more often
than that via View::BoundsChanged(). At most a third of the calls to Layout()
are via TabStripMaxXChanged().

Changed to call frame()->GetRootView()->Layout() directly. Note that the
ChildPreferredSizeChanged override is still needed - I've added a comment
explaining why.

[Evan Stade, 2017/05/03 23:18:08]

But the caption button layout will never change based on max x, will it? So all
you really need to do is call LayoutProfileSwitcher. I think just calling that
should be fine because if the calculated bounds are the same, SetBounds will be
a no-op.

[emx, 2017/05/04 13:48:33]

OK, LayoutProfileSwitcher() seems to work.

+}
+
+void GlassBrowserFrameView::TabStripRemovedTabAt(TabStrip* tab_strip,
+                                                 int index) {
+  // May switch between cozy and tall button here, too. TabStripMaxXChanged
+  // is not enough when a tab other than the last tab is closed.
+  DCHECK_EQ(tab_strip, tab_strip_);
+  ChildPreferredSizeChanged(profile_switcher_.view());
+}
+
+void GlassBrowserFrameView::TabStripDeleted(TabStrip* tab_strip) {
+  DCHECK_EQ(tab_strip, tab_strip_);
+  DCHECK(tab_strip_);
+  tab_strip_->RemoveObserver(this);
+  tab_strip_ = nullptr;
+}
+
 void GlassBrowserFrameView::LayoutProfileSwitcher() {
   DCHECK(browser_view()->IsRegularOrGuestSession());
-  if (!profile_switcher_.view())
+
+  View* button_view = GetProfileSwitcherView();
+  if (!button_view)
     return;
 
-  gfx::Size label_size = profile_switcher_.view()->GetPreferredSize();
+  gfx::Size button_size = button_view->GetPreferredSize();
+  int button_width = button_size.width();
+  int button_height = button_size.height();
 
   int button_x;
   if (CaptionButtonsOnLeadingEdge()) {
     button_x = width() - frame()->GetMinimizeButtonOffset() +
                kProfileSwitcherButtonOffset;
   } else {
-    button_x =
-        MinimizeButtonX() - kProfileSwitcherButtonOffset - label_size.width();
+    button_x = MinimizeButtonX() - kProfileSwitcherButtonOffset - button_width;
   }
 
   int button_y = WindowTopY();
   if (IsMaximized()) {
     // In maximized mode the caption buttons appear only 19 pixels high, but
     // their contents are aligned as if they were 20 pixels high and extended
     // 1 pixel off the top of the screen. We position the profile switcher
     // button the same way to match.
     button_y -= 1;
   }
-  profile_switcher_.view()->SetBounds(button_x, button_y, label_size.width(),
-                                      kProfileSwitcherButtonHeight);
+
+  // Does the button need to slide over the tabstrip? If so, use its minimum
+  // height. In RTL the new tab button is on the left, so it can never slide
+  // under the avatar button (which is still on the right).
+  if (tab_strip_ && !base::i18n::IsRTL() && tab_strip_->max_x() >= button_x) {

[Evan Stade, 2017/05/02 15:49:20]

nit: no curlies

[emx, 2017/05/03 17:10:59]

Coding standard says "In general, curly braces are not required for single-line
statements, but they are allowed if you like them". I like them and it's also
consistent with the rest of the file.

[Evan Stade, 2017/05/03 23:18:08]

It doesn't seem to be consistent with the file. I see no braces on L466, L496,
L502, L519, L652, L811, L816 etc.

[emx, 2017/05/04 13:48:33]

OK, I've removed the braces.

+    button_height = button_view->GetMinimumSize().height();
+  }
+
+  button_view->SetBounds(button_x, button_y, button_width, button_height);
 }
 
 void GlassBrowserFrameView::LayoutIncognitoIcon() {
   const gfx::Size size(GetIncognitoAvatarIcon().size());
   int x = ClientBorderThickness(false);
   // In RTL, the icon needs to start after the caption buttons.
   if (CaptionButtonsOnLeadingEdge()) {
     x = width() - frame()->GetMinimizeButtonOffset() +
-        (profile_switcher_.view() ? (profile_switcher_.view()->width() +
+        (GetProfileSwitcherView() ? (GetProfileSwitcherView()->width() +
                                      kProfileSwitcherButtonOffset)
                                   : 0);
   }
   const int bottom = GetTopInset(false) + browser_view()->GetTabStripHeight() -
                      kAvatarIconPadding;
   incognito_bounds_.SetRect(
       x + (profile_indicator_icon() ? kAvatarIconPadding : 0),
       bottom - size.height(), profile_indicator_icon() ? size.width() : 0,
       size.height());
   if (profile_indicator_icon())
@@ skipping 152 matching lines @@
   static bool initialized = false;
   if (!initialized) {
     for (int i = 0; i < kThrobberIconCount; ++i) {
       throbber_icons_[i] =
           ui::LoadThemeIconFromResourcesDataDLL(IDI_THROBBER_01 + i);
       DCHECK(throbber_icons_[i]);
     }
     initialized = true;
   }
 }