Update avatar button to MD (part 1)

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

Index: chrome/browser/ui/views/frame/glass_browser_frame_view.cc
diff --git a/chrome/browser/ui/views/frame/glass_browser_frame_view.cc b/chrome/browser/ui/views/frame/glass_browser_frame_view.cc
index 8f049ca733af21bec5a84a35c0459c14150e997d..60dc6fcdbef39969bf3301beba38fb412d04f0d5 100644
--- a/chrome/browser/ui/views/frame/glass_browser_frame_view.cc
+++ b/chrome/browser/ui/views/frame/glass_browser_frame_view.cc
@@ -49,7 +49,7 @@ const int kNonClientRestoredExtraThickness = 11;
 // 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
@@ -60,11 +60,6 @@ const int kNewTabCaptionRestoredSpacing = 5;
 // 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;
@@ -99,7 +94,8 @@ GlassBrowserFrameView::GlassBrowserFrameView(BrowserFrame* frame,
       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()
@@ -132,6 +128,10 @@ GlassBrowserFrameView::GlassBrowserFrameView(BrowserFrame* frame,
 }
 
 GlassBrowserFrameView::~GlassBrowserFrameView() {
+  if (tab_strip_) {
+    tab_strip_->RemoveObserver(this);
+    tab_strip_ = nullptr;

[Evan Stade, 2017/05/04 15:56:28]

not sure I understand why nulling this out is necessary. We don't usually null
out weak refs in destructors.

[Peter Kasting, 2017/05/06 02:23:08]

This; also, ScopedObserver (as Evan wisely suggests) would eliminate the need
for this code in the destructor anyway (and could replace the |tab_strip_|
member).

[emx, 2017/05/09 16:26:51]

OK, now using ScopedObserver, so the destructor is gone.

+  }
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -148,12 +148,14 @@ gfx::Rect GlassBrowserFrameView::GetBoundsForTabStrip(
 
     // The profile switcher button is optionally displayed to the left of the
     // minimize button.
-    if (profile_switcher_.view()) {
+    views::View* button_view = GetProfileSwitcherView();

[Peter Kasting, 2017/05/06 02:23:08]

Nit: |profile_switcher| seems like a better name. (2 places)

[emx, 2017/05/09 16:26:51]

Done.

+    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.

[Peter Kasting, 2017/05/06 02:23:08]

What does "cozy" mean, and why would it being or not being "an MD button"
matter?  This feels like a comment written by/for someone with lots of context
of avatar button mocks, which is probably not what a reader of this code would
have.

[emx, 2017/05/09 16:26:51]

Removed the line.

       if (!IsMaximized()) {
         end_x = std::min(end_x + GetLayoutSize(NEW_TAB_BUTTON).width() +
                              kNewTabCaptionRestoredSpacing,
@@ -217,6 +219,13 @@ views::View* GlassBrowserFrameView::GetProfileSwitcherView() const {
   return profile_switcher_.view();
 }
 
+void GlassBrowserFrameView::OnBrowserViewInitViewsComplete() {
+  DCHECK(browser_view()->tabstrip());

[Peter Kasting, 2017/05/06 02:23:08]

Are you sure you always get a tabstrip?  What about, say, popup windows?
(popuptest.com)

[emx, 2017/05/09 16:26:51]

I got a tabstrip even in a pop-up on that page, but to be safe I've changed the
DCHECK to an if anyway.

+  DCHECK(!tab_strip_);
+  tab_strip_ = browser_view()->tabstrip();

[Evan Stade, 2017/05/04 15:56:28]

why do you need this instead of just referencing browser_view()->tabstrip()? You
also might find ScopedObserver to be useful.

[emx, 2017/05/09 16:26:51]

Done.

+  tab_strip_->AddObserver(this);
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // GlassBrowserFrameView, views::NonClientFrameView implementation:
 
@@ -270,8 +279,8 @@ int GlassBrowserFrameView::NonClientHitTest(const gfx::Point& point) {
   // 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() &&

[Peter Kasting, 2017/05/06 02:23:08]

Nit: I'm not a huge fan of changes like this in this file, because they now call
an expensive (by its name) function repeatedly, which we'd normally stuff in a
temp.

I'd just leave these like they were before, unless you envision the
functionality of GetProfileSwitcherView() becoming more complicated -- in which
case we might want to refactor this code (and similar elsewhere) to use temps to
avoid repeated calls. (many places)

[emx, 2017/05/09 16:26:51]

Reverted.

+       GetProfileSwitcherView()->GetMirroredBounds().Contains(point)))
     return HTCLIENT;
 
   int frame_component = frame()->client_view()->NonClientHitTest(point);
@@ -406,6 +415,19 @@ void GlassBrowserFrameView::Layout() {
   LayoutClientView();
 }
 
+void GlassBrowserFrameView::ChildPreferredSizeChanged(views::View* child) {
+  if (child == GetProfileSwitcherView()) {
+    // Need to layout the root view here, too, as the avatar button may change
+    // between the text and the icon when a profile is added or removed, which
+    // may in turn cause it to change between "tall" and "cozy" if it's right
+    // near the end of the tabstrip. LayoutProfileSwitcher() is not enough
+    // here - it does not re-draw the line below the tabstrip properly when a
+    // profile is added or removed and the avatar button changes between tall
+    // and cozy as a result.

[Peter Kasting, 2017/05/06 02:23:08]

This comment helps, but I'm still a bit confused by usage of words like "tall"
and "cozy", and by being uncertain about what happens to the tabstrip as the
avatar button changes shape here.  Is the core issue that the tabstrip bounds
can change?  If so, I would say that more directly.

[emx, 2017/05/09 16:26:51]

No, in this case the tabstrip bounds don't change, but the width of the button
changes, causing it to overlap the tabstrip. I've updated the comment to
hopefully make this clear.

+    frame()->GetRootView()->Layout();

[Peter Kasting, 2017/05/06 02:23:08]

Do we really need to layout the root view?  If the issue is that the tabstrip
needs relayout, isn't just asking the avatar button and the tabstrip to layout
enough?  If the problem is the portion of the toolbar top stroke drawn by the
frame view not repainting, are we missing a SchedulePaint()?

[emx, 2017/05/09 16:26:51]

I don't know enough to answer that. What I can say is that calling
  LayoutProfileSwitcher();
  browser_view()->tabstrip()->Layout();
is not enough, but calling
  frame()->GetRootView()->Layout();
is enough.

[Peter Kasting, 2017/05/09 17:40:28]

Then while I don't want to burden you, I think you need to dig more deeply to
understand why the first doesn't work and the second does, so you can do the
most efficient thing possible here and explain clearly to readers why anything
less doesn't suffice.  Also ensure that just adding SchedulePaint() after the
layout calls doesn't help.

[emx, 2017/05/10 17:38:48]

I tried adding SchedulePaint(); and that's still not enough.

While you say you don't WANT to burden me, trying to figure this out certainly
would. It could take me a long time to understand how Chrome views are painted
and I'm likely to need someone's help. At this point I don't even know where to
begin troubleshooting this. I'm generally all for understanding how things work,
rather than just doing something that seems to work by magic, but given the size
and complexity of the code base, there are plenty of cases where I simply follow
the pattern I find elsewhere in the code (in this case -
BrowserNonClientFrameViewAsh::ChildPreferredSizeChanged). I don't think it's
realistic to block this CL on every single thing I don't understand.

Do you really think that there is a performance issue here, considering that
this code only gets called when a user adds or removes a profile? I don't notice
any performance issue in my testing, either (delay or anything "blinking",
etc.), even in the debug build.

[Peter Kasting, 2017/05/11 01:03:42]

OK.  Let's leave it doing a Layout on the root view as you have it, with a TODO
for estade to dig deeper.

[emx, 2017/05/11 12:56:10]

Done.

[Evan Stade, 2017/05/11 14:29:09]

I'm not the right one to dig deeper on this Windows only code.

[Peter Kasting, 2017/05/11 15:04:34]

OK, for now have bsep own this.

+  }
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // GlassBrowserFrameView, protected:
 
@@ -434,8 +456,8 @@ bool GlassBrowserFrameView::DoesIntersectRect(const views::View* target,
       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);
 }
@@ -715,20 +737,44 @@ void GlassBrowserFrameView::FillClientEdgeRects(int x,
   canvas->FillRect(side, color);
 }
 
+void GlassBrowserFrameView::TabStripMaxXChanged(TabStrip* tab_strip) {
+  // May switch between cozy and tall MD avatar button here.

[Peter Kasting, 2017/05/06 02:23:08]

Nit: Avoid referring to "MD" in code ever, since it can mean many different
things.  Also, once again, terms like "cozy" are context-free here.  Just saying
"The profile switcher button's height depends on the position of the new tab
button." would be clear and unambiguous.

[emx, 2017/05/09 16:26:51]

Done.

+  DCHECK_EQ(tab_strip, tab_strip_);

[Peter Kasting, 2017/05/06 02:23:08]

Nit: I'd remove this DCHECK and the similar ones below, as it doesn't seem like
they verify something any maintainer would ever question or violate.

[emx, 2017/05/09 16:26:51]

Done.

+  LayoutProfileSwitcher();
+}
+
+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.

[Peter Kasting, 2017/05/06 02:23:08]

Hrm.  This seems like it means that closing the last tab will snap the button
larger, but then when the mouse moves away and the strip animates back to the
right, the button will snap smaller again.

I wonder if instead the button should stay the size it is until the tabstrip
begins to resize, and at that point, if it's going to get bigger, it should do
so.  This also suggests that any time the button changes size, the size change
should be animated.  That may be beyond the scope of this CL, but consider it as
part of the interaction design here.

[emx, 2017/05/09 16:26:51]

Acknowledged.

+  DCHECK_EQ(tab_strip, tab_strip_);
+  LayoutProfileSwitcher();
+}
+
+void GlassBrowserFrameView::TabStripDeleted(TabStrip* tab_strip) {

[Peter Kasting, 2017/05/06 02:23:08]

Does this happen before frame destruction?

[emx, 2017/05/09 16:26:51]

I've never seen this method called at all, but I copied it from
BrowserNonClientFrameViewMus::TabStripDeleted (along with the rest of the
TabStripObserver-related code).

[Peter Kasting, 2017/05/09 17:40:28]

Hmm.  I think you should ask the author/OWNERs of that code what's going on and
whether this is truly necessary.  I'm willing to believe it is, but I don't want
to cargo-cult copy and paste.

[emx, 2017/05/10 17:38:48]

OK, I've asked him (it was sky@) and he said that it's not necessary and a
DCHECK in TabStripDeleted should be enough.

[emx, 2017/05/11 12:56:10]

I've replaced the contents with a NOTREACHED().

+  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();
@@ -739,8 +785,14 @@ void GlassBrowserFrameView::LayoutProfileSwitcher() {
     // 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

[Peter Kasting, 2017/05/06 02:23:08]

Nit: First two sentences could just be "Shrink the button height when it's atop
part of the tabstrip."

[emx, 2017/05/09 16:26:51]

Done.

+  // under the avatar button (which is still on the right).

[Peter Kasting, 2017/05/06 02:23:08]

Nit: You might want to note that caption buttons being on the right in RTL is
bug 560619.

You might also want to consider placing the profile switcher on the left in RTL,
away from the caption buttons.  The fact that they're on the right is
nonstandard Windows behavior and a bug in Chrome; we don't need to perpetuate it
with our custom buttons.

[emx, 2017/05/09 16:26:51]

I've added a comment about that bug, which already mentions that the avatar
button "probably" should be on the left in RTL. That's a can of worms I really
don't want to open in this CL.

[Peter Kasting, 2017/05/09 17:40:28]

Oh?  What's particularly daunting about it?  It seems like a pretty small change
that I'd think would make the code simpler overall.  I didn't expect any worms
to ooze out.

[emx, 2017/05/10 17:38:48]

For one, I'm not sure that there is agreement on where the button should be in
RTL. I don't even know who makes that decision. So just to confirm the
requirement can take a while. Then there is implementing the change. Maybe it's
simple for you, but I haven't looked into how to change the position of the
button, since it has very little to do with this CL. Then there is testing it -
even if it looks OK at first glance, there may be edge cases where it breaks. If
it does break, and it's part of this CL, then this entire CL may get reverted
when it didn't need to be.

[Peter Kasting, 2017/05/11 01:03:42]

Yes, your objections are fair.  If this were to be done, it would make much more
sense to do as a separate CL.

+  if (tab_strip_ && !base::i18n::IsRTL() && tab_strip_->max_x() >= button_x)
+    button_height = button_view->GetMinimumSize().height();
+
+  button_view->SetBounds(button_x, button_y, button_width, button_height);
 }
 
 void GlassBrowserFrameView::LayoutIncognitoIcon() {
@@ -749,7 +801,7 @@ void GlassBrowserFrameView::LayoutIncognitoIcon() {
   // 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);
   }