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 nece
Peter Kasting
2017/05/06 02:23:08
This; also, ScopedObserver (as Evan wisely suggest
emx
2017/05/09 16:26:51
OK, now using ScopedObserver, so the destructor is
|
+ } |
} |
/////////////////////////////////////////////////////////////////////////////// |
@@ -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.
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 n
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 abou
emx
2017/05/09 16:26:51
I got a tabstrip even in a pop-up on that page, bu
|
+ 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 b
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 th
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 b
emx
2017/05/09 16:26:51
No, in this case the tabstrip bounds don't change,
|
+ frame()->GetRootView()->Layout(); |
Peter Kasting
2017/05/06 02:23:08
Do we really need to layout the root view? If the
emx
2017/05/09 16:26:51
I don't know enough to answer that. What I can say
Peter Kasting
2017/05/09 17:40:28
Then while I don't want to burden you, I think you
emx
2017/05/10 17:38:48
I tried adding SchedulePaint(); and that's still n
Peter Kasting
2017/05/11 01:03:42
OK. Let's leave it doing a Layout on the root vie
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 Window
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 i
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 b
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 la
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 c
Peter Kasting
2017/05/09 17:40:28
Hmm. I think you should ask the author/OWNERs of
emx
2017/05/10 17:38:48
OK, I've asked him (it was sky@) and he said that
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
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 b
emx
2017/05/09 16:26:51
I've added a comment about that bug, which already
Peter Kasting
2017/05/09 17:40:28
Oh? What's particularly daunting about it? It se
emx
2017/05/10 17:38:48
For one, I'm not sure that there is agreement on w
Peter Kasting
2017/05/11 01:03:42
Yes, your objections are fair. If this were to be
|
+ 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); |
} |