Refactored GlassBrowserFrameView and BrowserDesktopTreeHostWin.

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

 // 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.
 
 #ifndef CHROME_BROWSER_UI_VIEWS_FRAME_GLASS_BROWSER_FRAME_VIEW_H_
 #define CHROME_BROWSER_UI_VIEWS_FRAME_GLASS_BROWSER_FRAME_VIEW_H_
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/win/scoped_gdi_object.h"
@@ skipping 34 matching lines @@
   void Layout() override;
 
   // BrowserNonClientFrameView:
   void UpdateAvatar() override;
 
  private:
   // views::NonClientFrameView:
   bool DoesIntersectRect(const views::View* target,
                          const gfx::Rect& rect) const override;
 
-  // Returns the thickness of the border that makes up the window left, right,
-  // and bottom frame edges.  This does not include any client edge.
-  int FrameBorderThickness() const;
+  // Returns the thickness of the window border for the left, right, and bottom
+  // edges of the frame. On Windows 10 this is a mostly-transparent handle that
+  // allows you to resize the window.
+  int NonClientBorderThickness() const;

[Peter Kasting, 2016/04/12 01:49:31]

Note: For all these functions you're renaming, consider whether we have the same
function names in OpaqueBrowserFrameView[Layout] and {Ash,Mus}BrowserFrameView. 
We probably want to rename such things in sync across all files to keep it as
clear as possible what the parallels are.

[Bret, 2016/04/12 23:19:38]

I took a look at these. It seems like Ash/Mus are architected a bit differently
so I didn't find any common functions. As for opaque, it has some commonly named
functions but they seem to actually be what they're named, which is probably why
glass has those names even though they ended up doing something else.

I did end up renaming OpaqueBrowserFrameViewLayout::NonClientTopBorderHeight to
NonClientTopBorderThickness to be consistent.

 
-  // Returns the height of the window top frame edge.  If |restored| is true,
-  // this is calculated as if the window was restored, regardless of its current
-  // state.
-  int FrameTopBorderHeight(bool restored) const;
+  // Returns the thickness of the window border for the top edge of the frame,
+  // which is sometimes different than NonClientBorderThickness(). Does not
+  // include the titlebar/tabstrip area. If |restored| is true, this is
+  // calculated as if the window was restored, regardless of its current state.
+  int NonClientTopBorderThickness(bool restored) const;
 
-  // Returns the thickness of the entire nonclient left, right, and bottom
-  // borders, including both the window frame and any client edge. If |restored|
-  // is true, this is calculated as if the window was restored, regardless of
-  // its current state.
-  int NonClientBorderThickness(bool restored) const;
+  // Returns the thickness of the border around the client area that separates
+  // it from the non-client area. If |restored| is true, this is calculated as
+  // if the window was restored, regardless of its current state.
+  int ClientBorderThickness(bool restored) const;

[Peter Kasting, 2016/04/12 01:49:31]

Nit: I might place this above the NonClientXXX() functions, so it's not in the
middle of them and so it's next to NonClientBorderThickness(), which it seems
related to.

[Bret, 2016/04/12 23:19:38]

ClientBorderThickness is only sort of related to NonClientBorderThickness...
you're right that it SOUNDS very related which means it probably needs a
different name. What do you think about "ContentBorderThickness"? Or maybe it
needs to be something very specific like "ClientNonClientBorderThickness"?
Opaque uses "ClientEdgeThickness," which I also kind of like, but it could get
confusing with kClientEdgeThickness from NonClientFrameView since it isn't the
same value.

At any rate I moved it up for now like you suggested.

[Peter Kasting, 2016/04/13 01:00:22]

Maybe it's the NonClient functions which have poor names.  Maybe
NonClientBorderThickness should be
NonClientThickness/NonClientAreaThickness/NonClientEdgeThickness, or perhaps
WindowBorderThickness.  Similarly for NonClientTopBorderThickness.

[Bret, 2016/04/13 22:19:47]

Before I spend a bunch of time thinking about these names see my comment in
glass_[etc].cc and let's figure out if we actually want to use Client/NonClient
terminology at all.

 
-  // Returns the height of the entire nonclient top border, including the window
-  // frame, any title area, and any connected client edge.  If |restored| is
-  // true, this is calculated as if the window was restored, regardless of its
-  // current state.
-  int NonClientTopBorderHeight(bool restored) const;
+  // Returns the height of the entire nonclient top border, from the edge of the
+  // window to the top of the tabs. If |restored| is true, this is calculated as
+  // if the window was restored, regardless of its current state.
+  int NonClientTopHeight(bool restored) const;

[Peter Kasting, 2016/04/12 01:49:31]

Nit: I might place this just below NonClientTopBorderThickness(), since it seems
related to that.

[Bret, 2016/04/12 23:19:38]

Done.

 
   // Returns whether the toolbar is currently visible.
   bool IsToolbarVisible() const;
 
+  // Returns whether we should use the special layout for when the caption
+  // buttons are drawn at the top-right corner in RTL mode.
+  bool RTLSpecialLayout() const;

[Peter Kasting, 2016/04/12 01:49:31]

"Special layout" is kind of vague.  If this is specifically about where the
caption buttons are, maybe call it CaptionButtonsOnLeft() or
UseRTLCaptionButtons() or something.

Perhaps better, since you indicated you intended to make this more complicated
later, might be to leave this out for now, having callers call the underlying
i18n function, and then add it in a subsequent patch set where its greater
functionality would make the correct name more obvious.

[Bret, 2016/04/12 23:19:38]

Hmm I like having a comment on it even if it's a simple delegation because it's
not obvious what the purpose of all those branches are otherwise. Renamed to
"RTLModeAndCaptionButtonsOnRight".

[Peter Kasting, 2016/04/13 01:00:22]

That implies we have two RTL mode states: caption buttons on right, and caption
buttons on left.  Is that true?  I thought today they were always on the right,
which was a bug, and we wanted them to be always on the left.

This means it's a bit of a mess to figure out what we want here because we
really want to change the behavior and make the buttons on the left, at which
point the functions we need, and where they're called from, will probably
differ.

For now, perhaps this should be CaptionButtonsOnLeadingEdge() or something.

[Bret, 2016/04/13 22:19:47]

You're correct that at this moment they're on always on the right on Windows,
but when I custom draw the titlebar I'll draw them on the left in RTL, so there
will be two modes. If someone does figure out how to make them always on the
left then all of this can just go away, but I don't foresee that happening soon.

Anyway that name seems fine.

[Peter Kasting, 2016/04/15 00:20:51]

Note that once you're custom-drawing them on the left, you actually probably
don't need any special logic to position them, as long as you use the views
auto-mirroring machinery and coordinate calculation stuff correctly, since
they'll logically always be on the trailing edge of the window.  We only need
two modes for now, since they're not always in the same logical position as long
as Windows is drawing them but we don't have the magic bit set that flips the
whole window (which would screw us over).

+
+  // Returns the y coordinate for an element that's aligned as far towards the
+  // top of the window as it should be allowed to draw.
+  int WindowTopY() const;

[Peter Kasting, 2016/04/12 01:49:31]

Nit: I would place this next to the other XXXTopXXX() functions above.

[Bret, 2016/04/12 23:19:38]

Done.

+
   // Paint various sub-components of this view.
   void PaintToolbarBackground(gfx::Canvas* canvas) const;
   void PaintClientEdge(gfx::Canvas* canvas) const;
   void FillClientEdgeRects(int x,
                            int y,
                            int right,
                            int bottom,
                            SkColor color,
                            gfx::Canvas* canvas) const;
 
   // Layout various sub-components of this view.
   void LayoutIncognitoIcon();
-  void LayoutNewStyleAvatar();
+  void LayoutAvatar();

[Peter Kasting, 2016/04/12 01:49:31]

Nit: If we're going to rename this, maybe we should rename it
LayoutProfileSwitcher(), since that's more accurate at this point (we no longer
show graphical avatars).

[Bret, 2016/04/12 23:19:38]

Done for references in this class. There are a bunch more "avatar" references
from other classes... it's inconsistent already so I'm a bit unsure how much
further I should go down the renaming rabbit hole.

   void LayoutClientView();
 
   // Returns the insets of the client area. If |restored| is true, this is
   // calculated as if the window was restored, regardless of its current state.
   gfx::Insets GetClientAreaInsets(bool restored) const;
 
   // Returns the bounds of the client area for the specified view size.
   gfx::Rect CalculateClientAreaBounds() const;
 
   // Starts/Stops the window throbber running.
@@ skipping 25 matching lines @@
   int throbber_frame_;
 
   static const int kThrobberIconCount = 24;
   static HICON throbber_icons_[kThrobberIconCount];
   static void InitThrobberIcons();
 
   DISALLOW_COPY_AND_ASSIGN(GlassBrowserFrameView);
 };
 
 #endif  // CHROME_BROWSER_UI_VIEWS_FRAME_GLASS_BROWSER_FRAME_VIEW_H_