Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2851543002:
Update avatar button to MD (part 1) (Closed)
DescriptionUpdate avatar button to MD (part 1)
Update avatar button to MD
Summary of changes made [ see screenshots in https://bugs.chromium.org/p/chromium/issues/detail?id=635699&desc=2#c36 ]:
1) Show new MD button only on Windows 10 and only if not using a theme
2) Make MD button "cozy" (20px) when tabstrip is full and not maximised and text direction is LTR (RTL can be tested with --force-ui-direction=rtl)
3) Make "tall" (non-cozy) MD button the same height as caption bar buttons - it's not pixel-perfect (see bug 716365)
4) Change icon and highlighting for MD button
5) Show "ink drop" animation when MD button is clicked, like for bookmarks bar and other menu buttons
6) Make menu appear on press, not on release (for all platforms, not just Windows 10)
BUG=635699
Review-Url: https://codereview.chromium.org/2851543002
Cr-Commit-Position: refs/heads/master@{#471279}
Committed: https://chromium.googlesource.com/chromium/src/+/da9bbac7c610430c9b94142d83ce56ee1d4f2f87
Patch Set 1 #
Total comments: 9
Patch Set 2 : Renamed NewAvatarButton->ThemedAvatarButton and MaterialDesignAvatarButton->Win10NativeAvatarButton… #
Total comments: 12
Patch Set 3 : More review changes #
Total comments: 21
Patch Set 4 : Further review changes #Patch Set 5 : Made AvatarButton inherit from LabelButton once more (instead of MenuButton) #Patch Set 6 : Merged ThemedAvatarButton and Win10NativeAvatarButton into the base AvatarButton class #
Total comments: 107
Patch Set 7 : Review changes and MinimizeButtonMetrics for avatar button width/height #Patch Set 8 : Rebased on CL 2868293002 [Rename new_avatar_button.* to avatar_button] #
Total comments: 14
Patch Set 9 : Using a hardcoded minimum width again + other fixes #Patch Set 10 : Fixed TODO comments #Messages
Total messages: 67 (39 generated)
Description was changed from ========== Update avatar button to MD (part 1) Update avatar button to MD (part 1) This includes most of the changes from issue 2832823002, except for caption button height (issue 2833363002) and keeping the avatar button pressed while the menu is shown. BUG=635699 ========== to ========== Update avatar button to MD (part 1) This includes most of the changes from issue 2832823002, except for caption button height (issue 2833363002) and keeping the avatar button pressed while the menu is shown. BUG=635699 ==========
emx@chromium.org changed reviewers: + estade@chromium.org
Hi Evan, I've made the changes from your review of CL 2832823002 here (except renaming the classes - waiting to confirm the names).
thanks for splitting this out! Could you make the CL description stand on its own, i.e. copy over the other description less the part that's not handled. https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/avatar_button_manager.h (right): https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/avatar_button_manager.h:37: AvatarButton* button_; // Owned by views hierarchy. Is changing the type necessary? https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/avatar_button_delegate.h (right): https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/avatar_button_delegate.h:14: virtual void ButtonPreferredSizeChanged() = 0; this is not your doing, but it seems like we shouldn't need this but could instead use View::PreferredSizeChanged() and View::ChildPreferredSizeChanged(View* child). The appropriate view that holds the button can check if the child matches the avatar button and act accordingly. This would eliminate this class and simplify AvatarButtonManager, and delete this code which seems out of place in that class: frame_view_->InvalidateLayout(); frame_view_->frame()->GetRootView()->Layout(); https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.h:19: class AvatarButton : public views::MenuButton, does this really need to be a menu button? what is the advantage?
Description was changed from ========== Update avatar button to MD (part 1) This includes most of the changes from issue 2832823002, except for caption button height (issue 2833363002) and keeping the avatar button pressed while the menu is shown. BUG=635699 ========== to ========== Update avatar button to MD (part 1) Update avatar button to MD Summary of changes made [ see screenshots in https://bugs.chromium.org/p/chromium/issues/detail?id=635699&desc=2#c36 ]: 1) Show new MD button only on Windows 10 and only if not using a theme 2) Make MD button "cozy" (20px) when tabstrip is full and not maximised and text direction is LTR (RTL can be tested with --force-ui-direction=rtl) 3) Make "tall" (non-cozy) MD button the same height as caption bar buttons - it's not pixel-perfect (see bug 716365) 4) Change icon and highlighting for MD button 5) Show "ink drop" animation when MD button is clicked, like for bookmarks bar and other menu buttons 6) Make menu appear on press, not on release (for all platforms, not just Windows 10) BUG=635699 ==========
I've renamed and moved the derived AvatarButton classes as requested in the other CL and removed AvatarButtonDelegate. https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/fra... File chrome/browser/ui/views/frame/avatar_button_manager.h (right): https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/fra... chrome/browser/ui/views/frame/avatar_button_manager.h:37: AvatarButton* button_; // Owned by views hierarchy. On 2017/04/27 17:15:02, Evan Stade wrote: > Is changing the type necessary? I think so, because I need to call AvatarButton::UpdateButtonHeightForPosition() in GlassBrowserFrameView::LayoutProfileSwitcher(). https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/avatar_button_delegate.h (right): https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/avatar_button_delegate.h:14: virtual void ButtonPreferredSizeChanged() = 0; On 2017/04/27 17:15:02, Evan Stade wrote: > this is not your doing, but it seems like we shouldn't need this but could > instead use View::PreferredSizeChanged() and > View::ChildPreferredSizeChanged(View* child). The appropriate view that holds > the button can check if the child matches the avatar button and act accordingly. > This would eliminate this class and simplify AvatarButtonManager, and delete > this code which seems out of place in that class: > > frame_view_->InvalidateLayout(); > frame_view_->frame()->GetRootView()->Layout(); Thanks, this simplifies things quite a bit! https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.h:19: class AvatarButton : public views::MenuButton, On 2017/04/27 17:15:02, Evan Stade wrote: > does this really need to be a menu button? what is the advantage? MenuButton takes care of the ink drop animation on click, highlighting the button on hover and on press and makes it easy to keep the button in the "pressed" state while the menu is showing (though I've removed that part from this CL, as requested).
The CQ bit was checked by emx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.h:19: class AvatarButton : public views::MenuButton, On 2017/04/28 12:39:40, emx wrote: > On 2017/04/27 17:15:02, Evan Stade wrote: > > does this really need to be a menu button? what is the advantage? > > MenuButton takes care of the ink drop animation on click, highlighting the > button on hover and on press and makes it easy to keep the button in the > "pressed" state while the menu is showing (though I've removed that part from > this CL, as requested). I believe all of the things you've described are also easy with a LabelButton, except the keep-it-pressed part, which won't be added back in the form you had it before. SetInkDropMode etc. all work on LabelButtons. https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/avatar_button_manager.cc (right): https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/avatar_button_manager.cc:17: #include "chrome/browser/ui/views/profiles/win10_native_avatar_button.h" nit: only include on windows. https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:785: avatar_button->UpdateButtonHeightForPosition(button_x, &button_height); I would rearrange these calculations slightly. You can compare the tabstrip max x to button_x here, and you can use GetMinimumSize to get the cozy height from the button. The buttons that don't have a cozy height will naturally return their normal preferred height. That way you don't have to introduce a new method that's sometimes a no op or change the button types around. This also means you needn't pass a BrowserView into AvatarButton. https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/win10_native_avatar_button.cc (right): https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/win10_native_avatar_button.cc:21: const SkColor kButtonIconColor = SkColorSetARGB(138, 0, 0, 0); // Alpha 54% I believe this is more or less equivalent to gfx::kChromeIconGrey. If it's really important to have a non-opaque version of that color, I'm still pretty sure this has to be used somewhere else as well and should be shared. https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/win10_native_avatar_button.cc:23: const float kHighlightInkDropOpacity = 0.08f; nit: InkDropHighlight, InkDropRipple https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/win10_native_avatar_button.cc:74: auto center = gfx::RectF(gfx::Rect(size())).CenterPoint(); nit: s/gfx::Rect(size())/GetLocalBounds() https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/win10_native_avatar_button.cc:82: Win10NativeAvatarButton::CreateInkDropRipple() const { this looks like it's copied pretty much verbatim from LabelButton. Could we just make it possible to override UseFloodFill instead?
The CQ bit was checked by emx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.h:19: class AvatarButton : public views::MenuButton, On 2017/04/28 18:38:29, Evan Stade wrote: > On 2017/04/28 12:39:40, emx wrote: > > On 2017/04/27 17:15:02, Evan Stade wrote: > > > does this really need to be a menu button? what is the advantage? > > > > MenuButton takes care of the ink drop animation on click, highlighting the > > button on hover and on press and makes it easy to keep the button in the > > "pressed" state while the menu is showing (though I've removed that part from > > this CL, as requested). > > I believe all of the things you've described are also easy with a LabelButton, > except the keep-it-pressed part, which won't be added back in the form you had > it before. SetInkDropMode etc. all work on LabelButtons. I forgot to say: AvatarButton used to override OnMousePressed, OnMouseReleased and OnGestureEvent to make sure that we don't show the bubble again if the user clicks the button when it's already showing. MenuButton does this. It seems like exactly what the avatar button is - a button that shows a menu. Is there a downside to using it? https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/avatar_button_manager.cc (right): https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/avatar_button_manager.cc:17: #include "chrome/browser/ui/views/profiles/win10_native_avatar_button.h" On 2017/04/28 18:38:29, Evan Stade wrote: > nit: only include on windows. Done. https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:785: avatar_button->UpdateButtonHeightForPosition(button_x, &button_height); On 2017/04/28 18:38:29, Evan Stade wrote: > I would rearrange these calculations slightly. You can compare the tabstrip max > x to button_x here, and you can use GetMinimumSize to get the cozy height from > the button. The buttons that don't have a cozy height will naturally return > their normal preferred height. That way you don't have to introduce a new method > that's sometimes a no op or change the button types around. This also means you > needn't pass a BrowserView into AvatarButton. Yes, that simplifies things a lot and now I don't need to change view_ to AvatarButton anymore. Thanks! https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/win10_native_avatar_button.cc (right): https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/win10_native_avatar_button.cc:21: const SkColor kButtonIconColor = SkColorSetARGB(138, 0, 0, 0); // Alpha 54% On 2017/04/28 18:38:29, Evan Stade wrote: > I believe this is more or less equivalent to gfx::kChromeIconGrey. If it's > really important to have a non-opaque version of that color, I'm still pretty > sure this has to be used somewhere else as well and should be shared. It's noticeably lighter than kChromeIconGrey. As Alan wrote on 2017-04-12: "Evan is right that #5a5a5a is identical to #000 on grey, but #000 0.54a is preferred for its color blending properties.". I've changed the code to make it clearer that this is black at 54% opacity, but I could not find an existing constant for that. https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/win10_native_avatar_button.cc:23: const float kHighlightInkDropOpacity = 0.08f; On 2017/04/28 18:38:29, Evan Stade wrote: > nit: InkDropHighlight, InkDropRipple Done. https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/win10_native_avatar_button.cc:74: auto center = gfx::RectF(gfx::Rect(size())).CenterPoint(); On 2017/04/28 18:38:29, Evan Stade wrote: > nit: s/gfx::Rect(size())/GetLocalBounds() Done. https://codereview.chromium.org/2851543002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/win10_native_avatar_button.cc:82: Win10NativeAvatarButton::CreateInkDropRipple() const { On 2017/04/28 18:38:30, Evan Stade wrote: > this looks like it's copied pretty much verbatim from LabelButton. Could we just > make it possible to override UseFloodFill instead? Done.
thanks, patch is making great progress https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/new_avatar_button.h (right): https://codereview.chromium.org/2851543002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/new_avatar_button.h:19: class AvatarButton : public views::MenuButton, On 2017/05/02 15:17:11, emx wrote: > On 2017/04/28 18:38:29, Evan Stade wrote: > > On 2017/04/28 12:39:40, emx wrote: > > > On 2017/04/27 17:15:02, Evan Stade wrote: > > > > does this really need to be a menu button? what is the advantage? > > > > > > MenuButton takes care of the ink drop animation on click, highlighting the > > > button on hover and on press and makes it easy to keep the button in the > > > "pressed" state while the menu is showing (though I've removed that part > from > > > this CL, as requested). > > > > I believe all of the things you've described are also easy with a LabelButton, > > except the keep-it-pressed part, which won't be added back in the form you had > > it before. SetInkDropMode etc. all work on LabelButtons. > > I forgot to say: AvatarButton used to override OnMousePressed, OnMouseReleased > and OnGestureEvent to make sure that we don't show the bubble again if the user > clicks the button when it's already showing. MenuButton does this. It seems like > exactly what the avatar button is - a button that shows a menu. Is there a > downside to using it? I don't know if that's what the avatar button is or not. Which one is it most like? - bookmark star/bubble, page icon/OIB, etc. These show bubbles on mouse release. - bookmark bar folder. This shows the dropdown on mouse release. - app menu. This shows a menu on mouse press. I think you could make an argument either way. With this change, can you press the mouse, move the mouse down over a row, and release, and have it activate the row? To me that's a crucial interaction pattern for something that's considered a menu. https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:419: if (child == profile_switcher_.view()) { nit: GetProfileSwitcherView()? I don't have much of a preference but it would be good to be consistent, I guess. https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:739: ChildPreferredSizeChanged(profile_switcher_.view()); 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). https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:788: if (tab_strip_ && !base::i18n::IsRTL() && tab_strip_->max_x() >= button_x) { nit: no curlies https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/themed_avatar_button.cc (right): https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/themed_avatar_button.cc:55: std::unique_ptr<views::Border> ThemedAvatarButton::CreateBorder( can this go back to being a file-local static? https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/themed_avatar_button.h (right): https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/themed_avatar_button.h:12: class ThemedAvatarButton : public AvatarButton { so now that these two/three avatar button classes are more simplified, I wonder if we can re-combine them into a single class? Especially because the themed version is going to want many if not all of the same ink drop settings when I MD-ify it. https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/win10_native_avatar_button.cc (right): https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/win10_native_avatar_button.cc:65: std::unique_ptr<views::Border> Win10NativeAvatarButton::CreateBorder() const { file local static? https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/win10_native_avatar_button.h (right): https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/win10_native_avatar_button.h:19: gfx::Size GetMinimumSize() const override; nit: these are also overrides and I guess it's best to label them as such as you did with L24 below. I'd just lump them all together as // AvatarButton rather than calling out which exact super class declares each of them. https://codereview.chromium.org/2851543002/diff/40001/ui/views/controls/butto... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/2851543002/diff/40001/ui/views/controls/butto... ui/views/controls/button/label_button.h:163: virtual bool UseFloodFillInkDrop() const; nit: rearrange ordering in .cc file to match new order in header and while we're at it, can we rename it to ShouldUseFloodFillInkDrop? I think that's more idiomatic (doesn't sound like a command).
Thanks for the quick review! I really don't know whether the avatar button is like any of the examples you list, let alone which one it's more like. They all show their menu/bubble on mouse release for me. What do you mean by "row" in this context? The list of profiles? If so, no, it does not activate in that case, but I don't see how that prevents us from using the MenuButton class if it offers other useful functionality we'd otherwise have to re-implement (the double press detection). Is there a downside to using MenuButton? On 2 May 2017 at 17:49, <estade@chromium.org> wrote: > thanks, patch is making great progress > > > https://codereview.chromium.org/2851543002/diff/1/chrome/ > browser/ui/views/profiles/new_avatar_button.h > File chrome/browser/ui/views/profiles/new_avatar_button.h (right): > > https://codereview.chromium.org/2851543002/diff/1/chrome/ > browser/ui/views/profiles/new_avatar_button.h#newcode19 > chrome/browser/ui/views/profiles/new_avatar_button.h:19: class > AvatarButton : public views::MenuButton, > On 2017/05/02 15:17:11, emx wrote: > > On 2017/04/28 18:38:29, Evan Stade wrote: > > > On 2017/04/28 12:39:40, emx wrote: > > > > On 2017/04/27 17:15:02, Evan Stade wrote: > > > > > does this really need to be a menu button? what is the > advantage? > > > > > > > > MenuButton takes care of the ink drop animation on click, > highlighting the > > > > button on hover and on press and makes it easy to keep the button > in the > > > > "pressed" state while the menu is showing (though I've removed > that part > > from > > > > this CL, as requested). > > > > > > I believe all of the things you've described are also easy with a > LabelButton, > > > except the keep-it-pressed part, which won't be added back in the > form you had > > > it before. SetInkDropMode etc. all work on LabelButtons. > > > > I forgot to say: AvatarButton used to override OnMousePressed, > OnMouseReleased > > and OnGestureEvent to make sure that we don't show the bubble again if > the user > > clicks the button when it's already showing. MenuButton does this. It > seems like > > exactly what the avatar button is - a button that shows a menu. Is > there a > > downside to using it? > > I don't know if that's what the avatar button is or not. Which one is it > most like? > > - bookmark star/bubble, page icon/OIB, etc. These show bubbles on mouse > release. > - bookmark bar folder. This shows the dropdown on mouse release. > - app menu. This shows a menu on mouse press. > > I think you could make an argument either way. With this change, can you > press the mouse, move the mouse down over a row, and release, and have > it activate the row? To me that's a crucial interaction pattern for > something that's considered a menu. > > https://codereview.chromium.org/2851543002/diff/40001/ > chrome/browser/ui/views/frame/glass_browser_frame_view.cc > File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): > > https://codereview.chromium.org/2851543002/diff/40001/ > chrome/browser/ui/views/frame/glass_browser_frame_view.cc#newcode419 > chrome/browser/ui/views/frame/glass_browser_frame_view.cc:419: if (child > == profile_switcher_.view()) { > nit: GetProfileSwitcherView()? I don't have much of a preference but it > would be good to be consistent, I guess. > > https://codereview.chromium.org/2851543002/diff/40001/ > chrome/browser/ui/views/frame/glass_browser_frame_view.cc#newcode739 > chrome/browser/ui/views/frame/glass_browser_frame_view.cc:739: > ChildPreferredSizeChanged(profile_switcher_.view()); > 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). > > https://codereview.chromium.org/2851543002/diff/40001/ > chrome/browser/ui/views/frame/glass_browser_frame_view.cc#newcode788 > chrome/browser/ui/views/frame/glass_browser_frame_view.cc:788: if > (tab_strip_ && !base::i18n::IsRTL() && tab_strip_->max_x() >= button_x) > { > nit: no curlies > > https://codereview.chromium.org/2851543002/diff/40001/ > chrome/browser/ui/views/profiles/themed_avatar_button.cc > File chrome/browser/ui/views/profiles/themed_avatar_button.cc (right): > > https://codereview.chromium.org/2851543002/diff/40001/ > chrome/browser/ui/views/profiles/themed_avatar_button.cc#newcode55 > chrome/browser/ui/views/profiles/themed_avatar_button.cc:55: > std::unique_ptr<views::Border> ThemedAvatarButton::CreateBorder( > can this go back to being a file-local static? > > https://codereview.chromium.org/2851543002/diff/40001/ > chrome/browser/ui/views/profiles/themed_avatar_button.h > File chrome/browser/ui/views/profiles/themed_avatar_button.h (right): > > https://codereview.chromium.org/2851543002/diff/40001/ > chrome/browser/ui/views/profiles/themed_avatar_button.h#newcode12 > chrome/browser/ui/views/profiles/themed_avatar_button.h:12: class > ThemedAvatarButton : public AvatarButton { > so now that these two/three avatar button classes are more simplified, I > wonder if we can re-combine them into a single class? Especially because > the themed version is going to want many if not all of the same ink drop > settings when I MD-ify it. > > https://codereview.chromium.org/2851543002/diff/40001/ > chrome/browser/ui/views/profiles/win10_native_avatar_button.cc > File chrome/browser/ui/views/profiles/win10_native_avatar_button.cc > (right): > > https://codereview.chromium.org/2851543002/diff/40001/ > chrome/browser/ui/views/profiles/win10_native_avatar_button.cc#newcode65 > chrome/browser/ui/views/profiles/win10_native_avatar_button.cc:65: > std::unique_ptr<views::Border> Win10NativeAvatarButton::CreateBorder() > const { > file local static? > > https://codereview.chromium.org/2851543002/diff/40001/ > chrome/browser/ui/views/profiles/win10_native_avatar_button.h > File chrome/browser/ui/views/profiles/win10_native_avatar_button.h > (right): > > https://codereview.chromium.org/2851543002/diff/40001/ > chrome/browser/ui/views/profiles/win10_native_avatar_button.h#newcode19 > chrome/browser/ui/views/profiles/win10_native_avatar_button.h:19: > gfx::Size GetMinimumSize() const override; > nit: these are also overrides and I guess it's best to label them as > such as you did with L24 below. I'd just lump them all together as // > AvatarButton rather than calling out which exact super class declares > each of them. > > https://codereview.chromium.org/2851543002/diff/40001/ui/ > views/controls/button/label_button.h > File ui/views/controls/button/label_button.h (right): > > https://codereview.chromium.org/2851543002/diff/40001/ui/ > views/controls/button/label_button.h#newcode163 > ui/views/controls/button/label_button.h:163: virtual bool > UseFloodFillInkDrop() const; > nit: rearrange ordering in .cc file to match new order in header > > and while we're at it, can we rename it to ShouldUseFloodFillInkDrop? I > think that's more idiomatic (doesn't sound like a command). > > https://codereview.chromium.org/2851543002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/win10_native_avatar_button.cc (right): https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/win10_native_avatar_button.cc:15: const int kButtonMinWidth = 48; nit: unnamed namespace for these constants
This patch set addresses all comments except for these 3 outstanding issues: 1) Changing base class of AvatarButton back to LabelButton. 2) Merging the derived AvatarButton classes back into AvatarButton. 3) Conditionally calling Layout() in GlassBrowserFrameView::TabStripMaxXChanged - we didn't have time to discuss this one, so please see my comment below. https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:419: if (child == profile_switcher_.view()) { On 2017/05/02 15:49:20, Evan Stade wrote: > nit: GetProfileSwitcherView()? I don't have much of a preference but it would be > good to be consistent, I guess. Done. https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:739: ChildPreferredSizeChanged(profile_switcher_.view()); On 2017/05/02 15:49:20, Evan Stade wrote: > 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). 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. https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:788: if (tab_strip_ && !base::i18n::IsRTL() && tab_strip_->max_x() >= button_x) { On 2017/05/02 15:49:20, Evan Stade wrote: > nit: no curlies 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. https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/themed_avatar_button.cc (right): https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/themed_avatar_button.cc:55: std::unique_ptr<views::Border> ThemedAvatarButton::CreateBorder( On 2017/05/02 15:49:20, Evan Stade wrote: > can this go back to being a file-local static? Done. https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/win10_native_avatar_button.cc (right): https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/win10_native_avatar_button.cc:15: const int kButtonMinWidth = 48; On 2017/05/03 16:39:36, Evan Stade wrote: > nit: unnamed namespace for these constants Done. https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/win10_native_avatar_button.cc:65: std::unique_ptr<views::Border> Win10NativeAvatarButton::CreateBorder() const { On 2017/05/02 15:49:20, Evan Stade wrote: > file local static? Done. https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/win10_native_avatar_button.h (right): https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/win10_native_avatar_button.h:19: gfx::Size GetMinimumSize() const override; On 2017/05/02 15:49:20, Evan Stade wrote: > nit: these are also overrides and I guess it's best to label them as such as you > did with L24 below. I'd just lump them all together as // AvatarButton rather > than calling out which exact super class declares each of them. Done. https://codereview.chromium.org/2851543002/diff/40001/ui/views/controls/butto... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/2851543002/diff/40001/ui/views/controls/butto... ui/views/controls/button/label_button.h:163: virtual bool UseFloodFillInkDrop() const; On 2017/05/02 15:49:20, Evan Stade wrote: > nit: rearrange ordering in .cc file to match new order in header > > and while we're at it, can we rename it to ShouldUseFloodFillInkDrop? I think > that's more idiomatic (doesn't sound like a command). Done.
https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:739: ChildPreferredSizeChanged(profile_switcher_.view()); On 2017/05/03 17:10:59, emx wrote: > On 2017/05/02 15:49:20, Evan Stade wrote: > > 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). > > 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(); 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. https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:788: if (tab_strip_ && !base::i18n::IsRTL() && tab_strip_->max_x() >= button_x) { On 2017/05/03 17:10:59, emx wrote: > On 2017/05/02 15:49:20, Evan Stade wrote: > > nit: no curlies > > 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. It doesn't seem to be consistent with the file. I see no braces on L466, L496, L502, L519, L652, L811, L816 etc.
The CQ bit was checked by emx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #6 (id:100001) has been deleted
emx@chromium.org changed reviewers: + pkasting@chromium.org
This addresses all the outstanding changes requested by Evan. Peter, adding you back to this - could you take a look, please? https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:739: ChildPreferredSizeChanged(profile_switcher_.view()); On 2017/05/03 23:18:08, Evan Stade wrote: > On 2017/05/03 17:10:59, emx wrote: > > On 2017/05/02 15:49:20, Evan Stade wrote: > > > 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). > > > > 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(); > > 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. OK, LayoutProfileSwitcher() seems to work. https://codereview.chromium.org/2851543002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:788: if (tab_strip_ && !base::i18n::IsRTL() && tab_strip_->max_x() >= button_x) { On 2017/05/03 23:18:08, Evan Stade wrote: > On 2017/05/03 17:10:59, emx wrote: > > On 2017/05/02 15:49:20, Evan Stade wrote: > > > nit: no curlies > > > > 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. > > It doesn't seem to be consistent with the file. I see no braces on L466, L496, > L502, L519, L652, L811, L816 etc. OK, I've removed the braces.
thanks for bearing with throughout this review. lgtm pending Peter's review. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:133: tab_strip_ = nullptr; not sure I understand why nulling this out is necessary. We don't usually null out weak refs in destructors. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:225: tab_strip_ = browser_view()->tabstrip(); why do you need this instead of just referencing browser_view()->tabstrip()? You also might find ScopedObserver to be useful.
https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:133: tab_strip_ = nullptr; On 2017/05/04 15:56:28, Evan Stade wrote: > not sure I understand why nulling this out is necessary. We don't usually null > out weak refs in destructors. 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). https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:151: views::View* button_view = GetProfileSwitcherView(); Nit: |profile_switcher| seems like a better name. (2 places) https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:158: // Note that the button should be "cozy" then, even if it's an MD button. 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. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:223: DCHECK(browser_view()->tabstrip()); Are you sure you always get a tabstrip? What about, say, popup windows? (popuptest.com) https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:282: (GetProfileSwitcherView() && 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) https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:426: // and cozy as a result. 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. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:427: frame()->GetRootView()->Layout(); 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()? https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:741: // May switch between cozy and tall MD avatar button here. 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. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:742: DCHECK_EQ(tab_strip, tab_strip_); 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. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:749: // is not enough when a tab other than the last tab is closed. 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. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:754: void GlassBrowserFrameView::TabStripDeleted(TabStrip* tab_strip) { Does this happen before frame destruction? https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:790: // height. In RTL the new tab button is on the left, so it can never slide Nit: First two sentences could just be "Shrink the button height when it's atop part of the tabstrip." https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:791: // under the avatar button (which is still on the right). 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. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.h:81: // TabStripObserver: Per recent discussion on chromium-dev, overrides should match the visibility of their parent, so these functions should all be public. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.h:202: // Tab strip to watch for changes to resize the avatar button. Nit: "The window's tabstrip, if any. Observed so we know when to resize any avatar button." https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_button.cc (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. Any way to get this to appear as a moved file w/diff instead of a brand new one? That would make the review easier... it would also make it look less weird that you're adding a file with a 2014 copyright. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:31: const int kThemedButtonHeight = 20; Nit: Prefer constexpr to const for compile-time constants. Prefer specifying constants in the functions that use them, as close to first use as possible, for everything that's used in only one function. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:33: const int kMdButtonMinWidth = 48; Should these values be computed based on average character widths instead of direct pixel values? That seems like it might be more i18n-friendly. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:35: // Height of the "cozy" MD button that slides over the tabstrip Nit: Can avoid the term "Cozy" and just talk about the height when the button and tabstrip are colocated https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:38: const int kMdButtonIconHeight = 16; This height differs slightly from the 20 DIP height used elsewhere with this icon. That concerns me that the icon will have been optimized for a slightly different height, leading to appearance issues. I would imagine we either way to use 20 here, use 16 elsewhere, or we may actually want separate versions of the icon optimized for different sizes. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:39: const SkColor kMdButtonIconColor = SkColorSetA(SK_ColorBLACK, 0.54 * 0xFF); These kinds of one-off constants (for opacity here and below) worry me. We should have consistent appearance and behavior for all UI elements across Chrome, rather than defining things individually per-element. For example, ash/system/tray/tray_constants.cc also uses 0.08f for highlight opacity, but 0.06f for ripple opacity instead of 0.04. This kind of subtle difference seems unlikely to be intentional. It also worries me that this is hardcoded to black, instead of computing a contrasting color with the button background dynamically (see win 10 custom caption button code). It also seems different than the text color, which seems unlikely to look good. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:61: const int kBottomInset = 4; Where did these insets come from? They, and the Win10 ones below, feel completely magic. Why do they look good? How can we compute them dynamically (e.g. by just horizontally centering the contents of the button)? https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:68: std::unique_ptr<views::Border> CreateWin10NativeBorder() { Is this really Win10-specific? Or is this basically "the MD appearance" and supposed to be used by other platforms too? https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:106: // TODO: use MD button in other cases, too [http://crbug.com/591586] Nit: Which other cases? All of them? Only Windows, but for custom themes as well? And who owns this TODO? You? estade? https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:110: use_win10_native_button_ = true; Nit: Again, is "win10 native button" really the right name for this, or is this basically "use newer style button"? I'm mostly allergic to things like "win10_xxx" appearing in code that's not #ifdef OS_WIN. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:161: .RemoveObserver(this); Nit: Can we use ScopedObserver to avoid this explicit remove? https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:193: std::min(std::max(size.width(), kMdButtonMinWidth), kMdButtonMaxWidth)); Nit: MathUtil::ClampToRange()? https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:194: // TODO(emx): get the proper height here - see http://crrev/2833363002 You can mark this patchset as dependent on that one and then actually do this here, now. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:255: Nit: Blank line not necessary. Or, leave the function below inlined here. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_button.h (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.h:17: // Base class for avatar buttons that display the active profile's name in the Nit: If these buttons are primarily about profile names, maybe we should move away from the term "avatar" in class/filenames entirely and just say ProfileSwitcherButton or the like. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.h:28: // Views Nit: While here: Fix this to say "views::LabelButton:" https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.h:31: protected: Nit: Make all these public that were public in the base class. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.h:32: // View: Nit: You don't subclass View directly. Name this "views::LabelButton:" https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.h:57: void UpdateButton(bool use_generic_button); Nit: Function needs a comment, including an explanation of this param https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.h:68: bool use_win10_native_button_; Nit: Should be separated from the variable above by a blank line, since the comment above doesn't apply to this member. Add a comment here describing what this means. https://codereview.chromium.org/2851543002/diff/120001/ui/views/controls/butt... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/2851543002/diff/120001/ui/views/controls/butt... ui/views/controls/button/label_button.h:156: virtual bool ShouldUseFloodFillInkDrop() const; Nit: Prefer to group virtual functions together, and non-virtual functions together.
https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:133: tab_strip_ = nullptr; On 2017/05/06 02:23:08, Peter Kasting wrote: > On 2017/05/04 15:56:28, Evan Stade wrote: > > not sure I understand why nulling this out is necessary. We don't usually null > > out weak refs in destructors. > > 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). OK, now using ScopedObserver, so the destructor is gone. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:151: views::View* button_view = GetProfileSwitcherView(); On 2017/05/06 02:23:08, Peter Kasting wrote: > Nit: |profile_switcher| seems like a better name. (2 places) Done. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:158: // Note that the button should be "cozy" then, even if it's an MD button. On 2017/05/06 02:23:08, Peter Kasting wrote: > 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. Removed the line. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:223: DCHECK(browser_view()->tabstrip()); On 2017/05/06 02:23:08, Peter Kasting wrote: > Are you sure you always get a tabstrip? What about, say, popup windows? > (http://popuptest.com) 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. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:225: tab_strip_ = browser_view()->tabstrip(); On 2017/05/04 15:56:28, Evan Stade wrote: > why do you need this instead of just referencing browser_view()->tabstrip()? You > also might find ScopedObserver to be useful. Done. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:282: (GetProfileSwitcherView() && On 2017/05/06 02:23:08, Peter Kasting wrote: > 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) Reverted. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:426: // and cozy as a result. On 2017/05/06 02:23:08, Peter Kasting wrote: > 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. 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. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:427: frame()->GetRootView()->Layout(); On 2017/05/06 02:23:08, Peter Kasting wrote: > 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()? 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. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:741: // May switch between cozy and tall MD avatar button here. On 2017/05/06 02:23:08, Peter Kasting wrote: > 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. Done. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:742: DCHECK_EQ(tab_strip, tab_strip_); On 2017/05/06 02:23:08, Peter Kasting wrote: > 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. Done. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:749: // is not enough when a tab other than the last tab is closed. On 2017/05/06 02:23:08, Peter Kasting wrote: > 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. Acknowledged. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:754: void GlassBrowserFrameView::TabStripDeleted(TabStrip* tab_strip) { On 2017/05/06 02:23:08, Peter Kasting wrote: > Does this happen before frame destruction? 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). https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:790: // height. In RTL the new tab button is on the left, so it can never slide On 2017/05/06 02:23:08, Peter Kasting wrote: > Nit: First two sentences could just be "Shrink the button height when it's atop > part of the tabstrip." Done. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:791: // under the avatar button (which is still on the right). On 2017/05/06 02:23:08, Peter Kasting wrote: > 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. 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. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.h (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.h:81: // TabStripObserver: On 2017/05/06 02:23:08, Peter Kasting wrote: > Per recent discussion on chromium-dev, overrides should match the visibility of > their parent, so these functions should all be public. Done. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.h:202: // Tab strip to watch for changes to resize the avatar button. On 2017/05/06 02:23:08, Peter Kasting wrote: > Nit: "The window's tabstrip, if any. Observed so we know when to resize any > avatar button." Done. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_button.cc (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/05/06 02:23:09, Peter Kasting wrote: > Any way to get this to appear as a moved file w/diff instead of a brand new one? > That would make the review easier... it would also make it look less weird that > you're adding a file with a 2014 copyright. I don't believe so. Git decides automatically whether a file is "moved" or not and I cannot find a way to force it to recognise that it was. See http://stackoverflow.com/questions/2314652/is-it-possible-to-move-rename-file... and https://git.wiki.kernel.org/index.php/GitFaq#Why_does_Git_not_.22track.22_ren... Even "git log --follow --find-copies-harder src\chrome\browser\ui\views\profiles\avatar_button.cc" does not find the rename. I guess it still doesn't look hard enough! https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:31: const int kThemedButtonHeight = 20; On 2017/05/06 02:23:08, Peter Kasting wrote: > Nit: Prefer constexpr to const for compile-time constants. Prefer specifying > constants in the functions that use them, as close to first use as possible, for > everything that's used in only one function. Done. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:33: const int kMdButtonMinWidth = 48; On 2017/05/06 02:23:08, Peter Kasting wrote: > Should these values be computed based on average character widths instead of > direct pixel values? That seems like it might be more i18n-friendly. The minimum width should be the caption button width actually - I've now implemented this the same way as the height in MinimizeButtonMetrics. For the maximum height, I don't know if it would be better, but the spec gives a fixed maximum width of 98 pixels (I had 96, actually - corrected it). I think this should be a separate bug, if it's necessary. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:35: // Height of the "cozy" MD button that slides over the tabstrip On 2017/05/06 02:23:09, Peter Kasting wrote: > Nit: Can avoid the term "Cozy" and just talk about the height when the button > and tabstrip are colocated Done. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:38: const int kMdButtonIconHeight = 16; On 2017/05/06 02:23:09, Peter Kasting wrote: > This height differs slightly from the 20 DIP height used elsewhere with this > icon. That concerns me that the icon will have been optimized for a slightly > different height, leading to appearance issues. > > I would imagine we either way to use 20 here, use 16 elsewhere, or we may > actually want separate versions of the icon optimized for different sizes. The spec clearly says 16 and it looks fine to me. 20 would be a tight fit when the button is over the tabstrip. It's possible with the button set to 22 (instead of 20), but that still leaves very little space above and below the icon. Anyway, this is something for bettes@. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:39: const SkColor kMdButtonIconColor = SkColorSetA(SK_ColorBLACK, 0.54 * 0xFF); On 2017/05/06 02:23:09, Peter Kasting wrote: > These kinds of one-off constants (for opacity here and below) worry me. We > should have consistent appearance and behavior for all UI elements across > Chrome, rather than defining things individually per-element. > > For example, ash/system/tray/tray_constants.cc also uses 0.08f for highlight > opacity, but 0.06f for ripple opacity instead of 0.04. This kind of subtle > difference seems unlikely to be intentional. > > It also worries me that this is hardcoded to black, instead of computing a > contrasting color with the button background dynamically (see win 10 custom > caption button code). It also seems different than the text color, which seems > unlikely to look good. The highlight opacity of 0.08 is in the spec. Whether it should be the same as any other UI element is a question I cannot answer. The ripple opacity of 0.04 I think was a misinterpretation of the spec on my part. The spec says to "Follow Core UI single tap animation curve and visuals (bookmark bar)". The bookmark button ripple opacity is the default 0.175 (it doesn't call set_ink_drop_visible_opacity), so I've also removed that call. The colour is also in the spec and bettes@ said in an email "#5a5a5a is identical to #000 on grey, but #000 0.54a is preferred for its color blending properties." Again, I really don't know anywhere near enough to answer what we should do here. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:61: const int kBottomInset = 4; On 2017/05/06 02:23:09, Peter Kasting wrote: > Where did these insets come from? They, and the Win10 ones below, feel > completely magic. Why do they look good? How can we compute them dynamically > (e.g. by just horizontally centering the contents of the button)? I don't know - this is existing code and I haven't changed them. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:68: std::unique_ptr<views::Border> CreateWin10NativeBorder() { On 2017/05/06 02:23:09, Peter Kasting wrote: > Is this really Win10-specific? Or is this basically "the MD appearance" and > supposed to be used by other platforms too? I arrived at these insets by experimentation on Windows 10. These are the only values I could find that look OK at all the listed scaling settings. As for the name of the method - it's a border currently used only by the windows 10 button, which was called "Win10NativeAvatarButton" at Evan's request (before being merged back into the AvatarButton class). I don't know whether this would apply on other platforms. I suspect not, but we won't know for sure until MD is actually implemented for other platforms. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:106: // TODO: use MD button in other cases, too [http://crbug.com/591586] On 2017/05/06 02:23:09, Peter Kasting wrote: > Nit: Which other cases? All of them? Only Windows, but for custom themes as > well? And who owns this TODO? You? estade? I don't know which other cases. I added this TODO at your request, so maybe you can tell me? Or the reader can just look at the linked bug for details. I would assume the owner of this TODO is the owner of the bug it refers to, which can change from time to time. (Currently it's estade.) https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:110: use_win10_native_button_ = true; On 2017/05/06 02:23:08, Peter Kasting wrote: > Nit: Again, is "win10 native button" really the right name for this, or is this > basically "use newer style button"? I'm mostly allergic to things like > "win10_xxx" appearing in code that's not #ifdef OS_WIN. "Win10NativeAvatarButton" was the class name suggested by estade back when this was a separate class. (I had "MaterialDesignAvatarButton" before.) I don't know how much of this code he plans to re-use on Linux or themed Windows. Could you discuss with him, please, and come up with a name you're both happy with? https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:161: .RemoveObserver(this); On 2017/05/06 02:23:08, Peter Kasting wrote: > Nit: Can we use ScopedObserver to avoid this explicit remove? Done. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:193: std::min(std::max(size.width(), kMdButtonMinWidth), kMdButtonMaxWidth)); On 2017/05/06 02:23:09, Peter Kasting wrote: > Nit: MathUtil::ClampToRange()? I tried that, but "git cl upload" failed: ** Presubmit ERRORS ** You added one or more #includes that violate checkdeps rules. chrome\browser\ui\views\profiles\avatar_button.cc Illegal include: "cc/base/math_util.h" Because of no rule applying. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:194: // TODO(emx): get the proper height here - see http://crrev/2833363002 On 2017/05/06 02:23:08, Peter Kasting wrote: > You can mark this patchset as dependent on that one and then actually do this > here, now. That one has been committed now. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:255: On 2017/05/06 02:23:09, Peter Kasting wrote: > Nit: Blank line not necessary. Or, leave the function below inlined here. Inlined. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_button.h (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.h:17: // Base class for avatar buttons that display the active profile's name in the On 2017/05/06 02:23:09, Peter Kasting wrote: > Nit: If these buttons are primarily about profile names, maybe we should move > away from the term "avatar" in class/filenames entirely and just say > ProfileSwitcherButton or the like. I don't know the answer to that, but the strings "AvatarButton" and "avatar button" are used in many places throughout the code. I think changing that would be well outside the scope of this CL. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.h:28: // Views On 2017/05/06 02:23:09, Peter Kasting wrote: > Nit: While here: Fix this to say "views::LabelButton:" Done. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.h:31: protected: On 2017/05/06 02:23:09, Peter Kasting wrote: > Nit: Make all these public that were public in the base class. Done. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.h:32: // View: On 2017/05/06 02:23:09, Peter Kasting wrote: > Nit: You don't subclass View directly. Name this "views::LabelButton:" Done. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.h:57: void UpdateButton(bool use_generic_button); On 2017/05/06 02:23:09, Peter Kasting wrote: > Nit: Function needs a comment, including an explanation of this param I've inlined the function instead. It was only called in one place from Update(). https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.h:68: bool use_win10_native_button_; On 2017/05/06 02:23:09, Peter Kasting wrote: > Nit: Should be separated from the variable above by a blank line, since the > comment above doesn't apply to this member. > > Add a comment here describing what this means. Done. https://codereview.chromium.org/2851543002/diff/120001/ui/views/controls/butt... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/2851543002/diff/120001/ui/views/controls/butt... ui/views/controls/button/label_button.h:156: virtual bool ShouldUseFloodFillInkDrop() const; On 2017/05/06 02:23:09, Peter Kasting wrote: > Nit: Prefer to group virtual functions together, and non-virtual functions > together. Done.
Have not re-reviewed -- just responding to comments. Wanted to send a word of encouragement. This is a big change, and you've done good work here. I suspect it's been frustrating to go through lots of different people's reviews and have things drag out. We're getting there! There is light at the end of the tunnel, and it's not an oncoming train :) https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:427: frame()->GetRootView()->Layout(); On 2017/05/09 16:26:51, emx wrote: > On 2017/05/06 02:23:08, Peter Kasting wrote: > > 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()? > > 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. 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. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:754: void GlassBrowserFrameView::TabStripDeleted(TabStrip* tab_strip) { On 2017/05/09 16:26:51, emx wrote: > On 2017/05/06 02:23:08, Peter Kasting wrote: > > Does this happen before frame destruction? > > 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). 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. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:791: // under the avatar button (which is still on the right). On 2017/05/09 16:26:51, emx wrote: > On 2017/05/06 02:23:08, Peter Kasting wrote: > > 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. > > 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. 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. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_button.cc (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/05/09 16:26:52, emx wrote: > On 2017/05/06 02:23:09, Peter Kasting wrote: > > Any way to get this to appear as a moved file w/diff instead of a brand new > one? > > That would make the review easier... it would also make it look less weird > that > > you're adding a file with a 2014 copyright. > > I don't believe so. Git decides automatically whether a file is "moved" or not > and I cannot find a way to force it to recognise that it was. See > http://stackoverflow.com/questions/2314652/is-it-possible-to-move-rename-file... > and > https://git.wiki.kernel.org/index.php/GitFaq#Why_does_Git_not_.22track.22_ren... > Even "git log --follow --find-copies-harder > src\chrome\browser\ui\views\profiles\avatar_button.cc" does not find the rename. > I guess it still doesn't look hard enough! In that case I'd suggest doing the file rename as a separate CL that this one depends on, with no other changes (other than #include fixups or whatever), and then making this CL have just the changes to the file content. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:33: const int kMdButtonMinWidth = 48; On 2017/05/09 16:26:52, emx wrote: > On 2017/05/06 02:23:08, Peter Kasting wrote: > > Should these values be computed based on average character widths instead of > > direct pixel values? That seems like it might be more i18n-friendly. > > The minimum width should be the caption button width actually - I've now > implemented this the same way as the height in MinimizeButtonMetrics. > > For the maximum height, I don't know if it would be better, but the spec gives a > fixed maximum width of 98 pixels (I had 96, actually - corrected it). I think > this should be a separate bug, if it's necessary. Implementation-wise, the spec is a design suggestion, but the engineer responsible for implementing is also supposed to be responsible for ensuring we handle things like i18n and a11y correctly. Since you're newly-implementing this, there's no real reason in code to prefer 96 vs. some number of ave-char-widths vs. some other solution. So I'd ask the designer what the design intent of 96 was -- does it match some other width? Is it "wide enough to show this many characters"? Learning the design intent will inform what we should implement here. It's possible to punt this to a separate bug, but it will likely be more work in the end and you'll be on the hook for doing said work, so it might be easiest to just go ahead and resolve immediately. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:38: const int kMdButtonIconHeight = 16; On 2017/05/09 16:26:52, emx wrote: > On 2017/05/06 02:23:09, Peter Kasting wrote: > > This height differs slightly from the 20 DIP height used elsewhere with this > > icon. That concerns me that the icon will have been optimized for a slightly > > different height, leading to appearance issues. > > > > I would imagine we either way to use 20 here, use 16 elsewhere, or we may > > actually want separate versions of the icon optimized for different sizes. > > The spec clearly says 16 See comment earlier about specs being suggestions :) > and it looks fine to me. Right, I'm willing to believe using 16 elsewhere, rather than 20 here, is better. My suspicion is that Alan didn't even know we have this 20-px-optimized icon in the tree we're using, that we use in other places, and that learning what those places are would give more context for the design decisions about what both the sizes there and here should be. Definitely share this and see what he thinks. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:39: const SkColor kMdButtonIconColor = SkColorSetA(SK_ColorBLACK, 0.54 * 0xFF); On 2017/05/09 16:26:52, emx wrote: > On 2017/05/06 02:23:09, Peter Kasting wrote: > > These kinds of one-off constants (for opacity here and below) worry me. We > > should have consistent appearance and behavior for all UI elements across > > Chrome, rather than defining things individually per-element. > > > > For example, ash/system/tray/tray_constants.cc also uses 0.08f for highlight > > opacity, but 0.06f for ripple opacity instead of 0.04. This kind of subtle > > difference seems unlikely to be intentional. > > > > It also worries me that this is hardcoded to black, instead of computing a > > contrasting color with the button background dynamically (see win 10 custom > > caption button code). It also seems different than the text color, which > seems > > unlikely to look good. > > The highlight opacity of 0.08 is in the spec. Whether it should be the same as > any other UI element is a question I cannot answer. The way I would start is to come back to Alan and ask where the 0.08 came from to begin with, and how, if at all, it should relate to other UI elements in Chrome and Ash. You could note the place I brought up or other cases you find where we explicitly set highlight opacity, along with the default opacity value, and ask if we shouldn't be trying to make all these the same; and if not, what sort of general rules we should follow to understand where variance should come from. The reason my replies sound like a broken record about "can't just implement the spec, have to understand why and be consistent" is that Chrome has too many surfaces for any one designer to keep in their head, so while they'll do the best job they can locally, we engineers have the context in code to see when something is implemented consistently versus as a one-off; and there's a heavy push from Chrome leadership for consistency and coherency across all Chrome UI, so it's the sort of thing we need to uphold in our implementations. Think of yourself as the Last Line Of Defense for making any given feature not only locally good, but a unified part of the Chrome whole :) https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:61: const int kBottomInset = 4; On 2017/05/09 16:26:52, emx wrote: > On 2017/05/06 02:23:09, Peter Kasting wrote: > > Where did these insets come from? They, and the Win10 ones below, feel > > completely magic. Why do they look good? How can we compute them dynamically > > (e.g. by just horizontally centering the contents of the button)? > > I don't know - this is existing code and I haven't changed them. OK. We really shouldn't do this sort of thing, but it's out-of-scope for this CL to fix it, so don't worry about it here for now. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:68: std::unique_ptr<views::Border> CreateWin10NativeBorder() { On 2017/05/09 16:26:52, emx wrote: > On 2017/05/06 02:23:09, Peter Kasting wrote: > > Is this really Win10-specific? Or is this basically "the MD appearance" and > > supposed to be used by other platforms too? > > I arrived at these insets by experimentation on Windows 10. These are the only > values I could find that look OK at all the listed scaling settings. Can you say more about "look OK"? For example, what sorts of things break if one uses the cross-platform insets, or just centers everything vertically? Are there screenshots? As much as possible, we want to avoid magic hardcoded values, because it's not clear where they come from and why they work, in particular as we change button sizes, font sizes, layout alignments, etc. in the future. The ideal is no constants, and everything computed algorithmically (e.g. centering the text horizontally, which will Just Work even if we make the button wider or something). Less ideal but still possible is to have as few constants as we can, and then make it clear in comments what principles and properties one can use to derive new values in the future as the UI changes. Also, regarding things looking OK at different scales, if we find some values work well at certain scales and not others, that might indicate problems elsewhere in layout, e.g. we shouldn't fix by changing the constant but rather by changing flooring to rounding somewhere, or some other similar tweak. It's impossible to be more specific without understanding what kind of badness you were running into. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:106: // TODO: use MD button in other cases, too [http://crbug.com/591586] On 2017/05/09 16:26:52, emx wrote: > On 2017/05/06 02:23:09, Peter Kasting wrote: > > Nit: Which other cases? All of them? Only Windows, but for custom themes as > > well? And who owns this TODO? You? estade? > > I don't know which other cases. I added this TODO at your request, so maybe you > can tell me? Or the reader can just look at the linked bug for details. > > I would assume the owner of this TODO is the owner of the bug it refers to, > which can change from time to time. (Currently it's estade.) It seems like Evan is the likely person to do this and will hopefully be doing it soon, so maybe just make the TODO owned by him. Normally when adding TODOs we put in an owner, who's not necessarily the one responsible for implementing the work, but is the person who would understand what the TODO means and how it could be resolved. Hence my suggestion to have an owner. We also normally try to make these as specific and actionable as possible (hence my other questions), but it seems like if Evan will be doing this shortly that's not a big deal. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:110: use_win10_native_button_ = true; On 2017/05/09 16:26:52, emx wrote: > On 2017/05/06 02:23:08, Peter Kasting wrote: > > Nit: Again, is "win10 native button" really the right name for this, or is > this > > basically "use newer style button"? I'm mostly allergic to things like > > "win10_xxx" appearing in code that's not #ifdef OS_WIN. > > "Win10NativeAvatarButton" was the class name suggested by estade back when this > was a separate class. (I had "MaterialDesignAvatarButton" before.) I don't know > how much of this code he plans to re-use on Linux or themed Windows. Could you > discuss with him, please, and come up with a name you're both happy with? In the interest of not blocking you, go ahead and leave this as-is for now, and hopefully it will get updated by his changes. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_button.h (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.h:17: // Base class for avatar buttons that display the active profile's name in the On 2017/05/09 16:26:52, emx wrote: > On 2017/05/06 02:23:09, Peter Kasting wrote: > > Nit: If these buttons are primarily about profile names, maybe we should move > > away from the term "avatar" in class/filenames entirely and just say > > ProfileSwitcherButton or the like. > > I don't know the answer to that, but the strings "AvatarButton" and "avatar > button" are used in many places throughout the code. I think changing that would > be well outside the scope of this CL. That's fair. Consider whether it makes sense as a separate task.
Patchset #8 (id:160001) has been deleted
The CQ bit was checked by emx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> Wanted to send a word of encouragement. This is a big change, and you've done > good work here. I suspect it's been frustrating to go through lots of different > people's reviews and have things drag out. We're getting there! There is light > at the end of the tunnel, and it's not an oncoming train :) Thank you, appreciate the words of encouragement, but you're right - it is frustrating to have some of these issues (including questions about requirements) only raised now, when the reviews have been going for over a month. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:427: frame()->GetRootView()->Layout(); On 2017/05/09 17:40:28, Peter Kasting wrote: > On 2017/05/09 16:26:51, emx wrote: > > On 2017/05/06 02:23:08, Peter Kasting wrote: > > > 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()? > > > > 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. > > 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. 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. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:754: void GlassBrowserFrameView::TabStripDeleted(TabStrip* tab_strip) { On 2017/05/09 17:40:28, Peter Kasting wrote: > On 2017/05/09 16:26:51, emx wrote: > > On 2017/05/06 02:23:08, Peter Kasting wrote: > > > Does this happen before frame destruction? > > > > 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). > > 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. OK, I've asked him (it was sky@) and he said that it's not necessary and a DCHECK in TabStripDeleted should be enough. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:791: // under the avatar button (which is still on the right). On 2017/05/09 17:40:28, Peter Kasting wrote: > On 2017/05/09 16:26:51, emx wrote: > > On 2017/05/06 02:23:08, Peter Kasting wrote: > > > 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. > > > > 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. > > 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. 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. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_button.cc (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2017/05/09 17:40:28, Peter Kasting wrote: > On 2017/05/09 16:26:52, emx wrote: > > On 2017/05/06 02:23:09, Peter Kasting wrote: > > > Any way to get this to appear as a moved file w/diff instead of a brand new > > one? > > > That would make the review easier... it would also make it look less weird > > that > > > you're adding a file with a 2014 copyright. > > > > I don't believe so. Git decides automatically whether a file is "moved" or not > > and I cannot find a way to force it to recognise that it was. See > > > http://stackoverflow.com/questions/2314652/is-it-possible-to-move-rename-file... > > and > > > https://git.wiki.kernel.org/index.php/GitFaq#Why_does_Git_not_.22track.22_ren... > > Even "git log --follow --find-copies-harder > > src\chrome\browser\ui\views\profiles\avatar_button.cc" does not find the > rename. > > I guess it still doesn't look hard enough! > > In that case I'd suggest doing the file rename as a separate CL that this one > depends on, with no other changes (other than #include fixups or whatever), and > then making this CL have just the changes to the file content. OK, I've made it a separate CL. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:33: const int kMdButtonMinWidth = 48; On 2017/05/09 17:40:29, Peter Kasting wrote: > On 2017/05/09 16:26:52, emx wrote: > > On 2017/05/06 02:23:08, Peter Kasting wrote: > > > Should these values be computed based on average character widths instead of > > > direct pixel values? That seems like it might be more i18n-friendly. > > > > The minimum width should be the caption button width actually - I've now > > implemented this the same way as the height in MinimizeButtonMetrics. > > > > For the maximum height, I don't know if it would be better, but the spec gives > a > > fixed maximum width of 98 pixels (I had 96, actually - corrected it). I think > > this should be a separate bug, if it's necessary. > > Implementation-wise, the spec is a design suggestion, but the engineer > responsible for implementing is also supposed to be responsible for ensuring we > handle things like i18n and a11y correctly. Since you're newly-implementing > this, there's no real reason in code to prefer 96 vs. some number of > ave-char-widths vs. some other solution. So I'd ask the designer what the > design intent of 96 was -- does it match some other width? Is it "wide enough > to show this many characters"? Learning the design intent will inform what we > should implement here. > > It's possible to punt this to a separate bug, but it will likely be more work in > the end and you'll be on the hook for doing said work, so it might be easiest to > just go ahead and resolve immediately. OK, let's wait for Alan to reply to the email. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:38: const int kMdButtonIconHeight = 16; On 2017/05/09 17:40:29, Peter Kasting wrote: > On 2017/05/09 16:26:52, emx wrote: > > On 2017/05/06 02:23:09, Peter Kasting wrote: > > > This height differs slightly from the 20 DIP height used elsewhere with this > > > icon. That concerns me that the icon will have been optimized for a > slightly > > > different height, leading to appearance issues. > > > > > > I would imagine we either way to use 20 here, use 16 elsewhere, or we may > > > actually want separate versions of the icon optimized for different sizes. > > > > The spec clearly says 16 > > See comment earlier about specs being suggestions :) So if the spec is a suggestion, then who actually makes the final decision about how the product should look and function? > > and it looks fine to me. > > Right, I'm willing to believe using 16 elsewhere, rather than 20 here, is > better. My suspicion is that Alan didn't even know we have this 20-px-optimized > icon in the tree we're using, that we use in other places, and that learning > what those places are would give more context for the design decisions about > what both the sizes there and here should be. Definitely share this and see > what he thinks. I've replied to the email with a screenshot of both 16x16 and 20x20 icons. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:68: std::unique_ptr<views::Border> CreateWin10NativeBorder() { On 2017/05/09 17:40:28, Peter Kasting wrote: > On 2017/05/09 16:26:52, emx wrote: > > On 2017/05/06 02:23:09, Peter Kasting wrote: > > > Is this really Win10-specific? Or is this basically "the MD appearance" and > > > supposed to be used by other platforms too? > > > > I arrived at these insets by experimentation on Windows 10. These are the only > > values I could find that look OK at all the listed scaling settings. > > Can you say more about "look OK"? For example, what sorts of things break if > one uses the cross-platform insets, or just centers everything vertically? Are > there screenshots? > > As much as possible, we want to avoid magic hardcoded values, because it's not > clear where they come from and why they work, in particular as we change button > sizes, font sizes, layout alignments, etc. in the future. The ideal is no > constants, and everything computed algorithmically (e.g. centering the text > horizontally, which will Just Work even if we make the button wider or > something). Less ideal but still possible is to have as few constants as we > can, and then make it clear in comments what principles and properties one can > use to derive new values in the future as the UI changes. > > Also, regarding things looking OK at different scales, if we find some values > work well at certain scales and not others, that might indicate problems > elsewhere in layout, e.g. we shouldn't fix by changing the constant but rather > by changing flooring to rounding somewhere, or some other similar tweak. It's > impossible to be more specific without understanding what kind of badness you > were running into. Having tested the insets again, it actually looks good with the original insets (top 2, bottom 4, left/right 8), so I'm now using them for both the old ("themed") and new ("native Win10") buttons. I think I was using the wrong size icon before (18x18) and that required adjusting the insets.
LGTM https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:427: frame()->GetRootView()->Layout(); On 2017/05/10 17:38:48, emx wrote: > On 2017/05/09 17:40:28, Peter Kasting wrote: > > On 2017/05/09 16:26:51, emx wrote: > > > On 2017/05/06 02:23:08, Peter Kasting wrote: > > > > 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()? > > > > > > 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. > > > > 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. > > I tried adding SchedulePaint(); and that's still not enough. OK. Let's leave it doing a Layout on the root view as you have it, with a TODO for estade to dig deeper. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:791: // under the avatar button (which is still on the right). On 2017/05/10 17:38:48, emx wrote: > On 2017/05/09 17:40:28, Peter Kasting wrote: > > On 2017/05/09 16:26:51, emx wrote: > > > On 2017/05/06 02:23:08, Peter Kasting wrote: > > > > 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. > > > > > > 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. > > > > 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. > > 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. Yes, your objections are fair. If this were to be done, it would make much more sense to do as a separate CL. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_button.cc (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:33: const int kMdButtonMinWidth = 48; On 2017/05/10 17:38:49, emx wrote: > On 2017/05/09 17:40:29, Peter Kasting wrote: > > On 2017/05/09 16:26:52, emx wrote: > > > On 2017/05/06 02:23:08, Peter Kasting wrote: > > > > Should these values be computed based on average character widths instead > of > > > > direct pixel values? That seems like it might be more i18n-friendly. > > > > > > The minimum width should be the caption button width actually - I've now > > > implemented this the same way as the height in MinimizeButtonMetrics. > > > > > > For the maximum height, I don't know if it would be better, but the spec > gives > > a > > > fixed maximum width of 98 pixels (I had 96, actually - corrected it). I > think > > > this should be a separate bug, if it's necessary. > > > > Implementation-wise, the spec is a design suggestion, but the engineer > > responsible for implementing is also supposed to be responsible for ensuring > we > > handle things like i18n and a11y correctly. Since you're newly-implementing > > this, there's no real reason in code to prefer 96 vs. some number of > > ave-char-widths vs. some other solution. So I'd ask the designer what the > > design intent of 96 was -- does it match some other width? Is it "wide enough > > to show this many characters"? Learning the design intent will inform what we > > should implement here. > > > > It's possible to punt this to a separate bug, but it will likely be more work > in > > the end and you'll be on the hook for doing said work, so it might be easiest > to > > just go ahead and resolve immediately. > > OK, let's wait for Alan to reply to the email. If he doesn't get back with something quickly, I would go ahead and land as-is and leave a TODO about this and file it separately. While there is merit in the argument that "resolving now might be easier", there's also merit in "don't hang the CL up indefinitely". https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:38: const int kMdButtonIconHeight = 16; On 2017/05/10 17:38:48, emx wrote: > On 2017/05/09 17:40:29, Peter Kasting wrote: > > On 2017/05/09 16:26:52, emx wrote: > > > On 2017/05/06 02:23:09, Peter Kasting wrote: > > > > This height differs slightly from the 20 DIP height used elsewhere with > this > > > > icon. That concerns me that the icon will have been optimized for a > > slightly > > > > different height, leading to appearance issues. > > > > > > > > I would imagine we either way to use 20 here, use 16 elsewhere, or we may > > > > actually want separate versions of the icon optimized for different sizes. > > > > > > The spec clearly says 16 > > > > See comment earlier about specs being suggestions :) > > So if the spec is a suggestion, then who actually makes the final decision about > how the product should look and function? It's collaborative, ultimately -- UI Review has final authority but doesn't generally review everything, UI Design has the most design skill but generally less context + is spread thin, and engineering has the most context and focus. So I would generally say that while the final _decision_ isn't always made by the engineer doing the implementation, the primary responsibility to think through everything about the UX belongs to the engineer. In this case, you got tasked with a complicated set of changes that touch the browser top chrome, which is a fully-custom surface with lots of history and baggage, so you had a task that would normally be done by someone with far more background in similar changes (generally, the Windows-specific UI team, which would be people like me and bsep). Asking you to spin up all this background before landing your first project isn't fair of me. Maybe the best takeaways are: * Assume in future UI work (top chrome or not) that specs, existing practice, etc., may not actually be correct * Tackle complex projects in as small of pieces as possible (e.g. CLs that implement a single behavior aspect at a time) * Be quick to rope in OWNERS and folks experienced in the area However, given the task set you, you've done admirably at implementing this well, responding patiently to review feedback, and relaying stakeholder concerns from different directions. Bravo :) https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:68: std::unique_ptr<views::Border> CreateWin10NativeBorder() { On 2017/05/10 17:38:48, emx wrote: > On 2017/05/09 17:40:28, Peter Kasting wrote: > > On 2017/05/09 16:26:52, emx wrote: > > > On 2017/05/06 02:23:09, Peter Kasting wrote: > > > > Is this really Win10-specific? Or is this basically "the MD appearance" > and > > > > supposed to be used by other platforms too? > > > > > > I arrived at these insets by experimentation on Windows 10. These are the > only > > > values I could find that look OK at all the listed scaling settings. > > > > Can you say more about "look OK"? For example, what sorts of things break if > > one uses the cross-platform insets, or just centers everything vertically? > Are > > there screenshots? > > > > As much as possible, we want to avoid magic hardcoded values, because it's not > > clear where they come from and why they work, in particular as we change > button > > sizes, font sizes, layout alignments, etc. in the future. The ideal is no > > constants, and everything computed algorithmically (e.g. centering the text > > horizontally, which will Just Work even if we make the button wider or > > something). Less ideal but still possible is to have as few constants as we > > can, and then make it clear in comments what principles and properties one can > > use to derive new values in the future as the UI changes. > > > > Also, regarding things looking OK at different scales, if we find some values > > work well at certain scales and not others, that might indicate problems > > elsewhere in layout, e.g. we shouldn't fix by changing the constant but rather > > by changing flooring to rounding somewhere, or some other similar tweak. It's > > impossible to be more specific without understanding what kind of badness you > > were running into. > > Having tested the insets again, it actually looks good with the original insets > (top 2, bottom 4, left/right 8), so I'm now using them for both the old > ("themed") and new ("native Win10") buttons. I think I was using the wrong size > icon before (18x18) and that required adjusting the insets. Sounds good, but it doesn't look like those changes made it into the uploaded patch? https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:193: std::min(std::max(size.width(), kMdButtonMinWidth), kMdButtonMaxWidth)); On 2017/05/09 16:26:52, emx wrote: > On 2017/05/06 02:23:09, Peter Kasting wrote: > > Nit: MathUtil::ClampToRange()? > > I tried that, but "git cl upload" failed: > > ** Presubmit ERRORS ** > You added one or more #includes that violate checkdeps rules. > chrome\browser\ui\views\profiles\avatar_button.cc > Illegal include: "cc/base/math_util.h" > Because of no rule applying. Sadness. I'm dealing with this separately, don't worry about it. https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:58: return GetCaptionButtonDimensionInDIPs(SM_CXSIZE, SM_CXSIZEFRAME); It somewhat surprises me that the width would need to include CXSIZEFRAME, since the non-close caption buttons aren't next to the frame. But I believe you... https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/minimize_button_metrics_win.h (right): https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.h:21: // Return the width/height of the native caption buttons in DIPs. Nit: "Returns" was correct; see bit about "descriptive vs. imperative" in http://google.github.io/styleguide/cppguide.html#Function_Comments . https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.h:23: static int GetCaptionButtonHeightInDIPs(); Nit: Maybe static gfx::Size GetCaptionButtonSize(); ? (DIPs are implied with a gfx::Size, at least today.) https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_button.cc (right): https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:104: SkColorSetA(SK_ColorBLACK, 0.54 * 0xFF); Nit: static_cast second arg to SkAlpha https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:162: return gfx::Size(MinimizeButtonMetrics::GetCaptionButtonWidthInDIPs(), 20); Nit: Ultimately we'll probably want to calculate this dynamically based on the min of the normal height and the y position of the new tab button minus some padding; for now I'd just add a TODO to do that https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:184: size.set_height(20); Nit: Should this 20 and the 20 in GetMinimumSize() come from a common constant for now? I'm not actually 100% clear on whether those things should be linked. https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:195: // Opacity of the ink drop on hover Nit: Comment looks like it just restates the code, I'd remove.
The CQ bit was checked by emx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks a lot for that long-awaited LGTM! Submitting the remaining changes. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:427: frame()->GetRootView()->Layout(); On 2017/05/11 01:03:42, Peter Kasting wrote: > On 2017/05/10 17:38:48, emx wrote: > > On 2017/05/09 17:40:28, Peter Kasting wrote: > > > On 2017/05/09 16:26:51, emx wrote: > > > > On 2017/05/06 02:23:08, Peter Kasting wrote: > > > > > 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()? > > > > > > > > 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. > > > > > > 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. > > > > I tried adding SchedulePaint(); and that's still not enough. > > OK. Let's leave it doing a Layout on the root view as you have it, with a TODO > for estade to dig deeper. Done. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:754: void GlassBrowserFrameView::TabStripDeleted(TabStrip* tab_strip) { On 2017/05/10 17:38:48, emx wrote: > On 2017/05/09 17:40:28, Peter Kasting wrote: > > On 2017/05/09 16:26:51, emx wrote: > > > On 2017/05/06 02:23:08, Peter Kasting wrote: > > > > Does this happen before frame destruction? > > > > > > 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). > > > > 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. > > OK, I've asked him (it was sky@) and he said that it's not necessary and a > DCHECK in TabStripDeleted should be enough. I've replaced the contents with a NOTREACHED(). https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_button.cc (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:38: const int kMdButtonIconHeight = 16; On 2017/05/11 01:03:43, Peter Kasting wrote: > On 2017/05/10 17:38:48, emx wrote: > > On 2017/05/09 17:40:29, Peter Kasting wrote: > > > On 2017/05/09 16:26:52, emx wrote: > > > > On 2017/05/06 02:23:09, Peter Kasting wrote: > > > > > This height differs slightly from the 20 DIP height used elsewhere with > > this > > > > > icon. That concerns me that the icon will have been optimized for a > > > slightly > > > > > different height, leading to appearance issues. > > > > > > > > > > I would imagine we either way to use 20 here, use 16 elsewhere, or we > may > > > > > actually want separate versions of the icon optimized for different > sizes. > > > > > > > > The spec clearly says 16 > > > > > > See comment earlier about specs being suggestions :) > > > > So if the spec is a suggestion, then who actually makes the final decision > about > > how the product should look and function? > > It's collaborative, ultimately -- UI Review has final authority but doesn't > generally review everything, UI Design has the most design skill but generally > less context + is spread thin, and engineering has the most context and focus. > So I would generally say that while the final _decision_ isn't always made by > the engineer doing the implementation, the primary responsibility to think > through everything about the UX belongs to the engineer. > > In this case, you got tasked with a complicated set of changes that touch the > browser top chrome, which is a fully-custom surface with lots of history and > baggage, so you had a task that would normally be done by someone with far more > background in similar changes (generally, the Windows-specific UI team, which > would be people like me and bsep). Asking you to spin up all this background > before landing your first project isn't fair of me. > > Maybe the best takeaways are: > * Assume in future UI work (top chrome or not) that specs, existing practice, > etc., may not actually be correct > * Tackle complex projects in as small of pieces as possible (e.g. CLs that > implement a single behavior aspect at a time) > * Be quick to rope in OWNERS and folks experienced in the area > > However, given the task set you, you've done admirably at implementing this > well, responding patiently to review feedback, and relaying stakeholder concerns > from different directions. Bravo :) Thanks for the detailed explanation. I agree with your takeaways and was already thinking that, for the next such change, I will start by finding a code reviewer and ensuring that they agree with the spec first. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:68: std::unique_ptr<views::Border> CreateWin10NativeBorder() { On 2017/05/11 01:03:43, Peter Kasting wrote: > On 2017/05/10 17:38:48, emx wrote: > > On 2017/05/09 17:40:28, Peter Kasting wrote: > > > On 2017/05/09 16:26:52, emx wrote: > > > > On 2017/05/06 02:23:09, Peter Kasting wrote: > > > > > Is this really Win10-specific? Or is this basically "the MD appearance" > > and > > > > > supposed to be used by other platforms too? > > > > > > > > I arrived at these insets by experimentation on Windows 10. These are the > > only > > > > values I could find that look OK at all the listed scaling settings. > > > > > > Can you say more about "look OK"? For example, what sorts of things break > if > > > one uses the cross-platform insets, or just centers everything vertically? > > Are > > > there screenshots? > > > > > > As much as possible, we want to avoid magic hardcoded values, because it's > not > > > clear where they come from and why they work, in particular as we change > > button > > > sizes, font sizes, layout alignments, etc. in the future. The ideal is no > > > constants, and everything computed algorithmically (e.g. centering the text > > > horizontally, which will Just Work even if we make the button wider or > > > something). Less ideal but still possible is to have as few constants as we > > > can, and then make it clear in comments what principles and properties one > can > > > use to derive new values in the future as the UI changes. > > > > > > Also, regarding things looking OK at different scales, if we find some > values > > > work well at certain scales and not others, that might indicate problems > > > elsewhere in layout, e.g. we shouldn't fix by changing the constant but > rather > > > by changing flooring to rounding somewhere, or some other similar tweak. > It's > > > impossible to be more specific without understanding what kind of badness > you > > > were running into. > > > > Having tested the insets again, it actually looks good with the original > insets > > (top 2, bottom 4, left/right 8), so I'm now using them for both the old > > ("themed") and new ("native Win10") buttons. I think I was using the wrong > size > > icon before (18x18) and that required adjusting the insets. > > Sounds good, but it doesn't look like those changes made it into the uploaded > patch? Yes, sorry, I replied to the comments, but didn't upload any fixes yesterday (only rebased on the rename CL). Uploading today. https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:106: // TODO: use MD button in other cases, too [http://crbug.com/591586] On 2017/05/09 17:40:28, Peter Kasting wrote: > On 2017/05/09 16:26:52, emx wrote: > > On 2017/05/06 02:23:09, Peter Kasting wrote: > > > Nit: Which other cases? All of them? Only Windows, but for custom themes > as > > > well? And who owns this TODO? You? estade? > > > > I don't know which other cases. I added this TODO at your request, so maybe > you > > can tell me? Or the reader can just look at the linked bug for details. > > > > I would assume the owner of this TODO is the owner of the bug it refers to, > > which can change from time to time. (Currently it's estade.) > > It seems like Evan is the likely person to do this and will hopefully be doing > it soon, so maybe just make the TODO owned by him. > > Normally when adding TODOs we put in an owner, who's not necessarily the one > responsible for implementing the work, but is the person who would understand > what the TODO means and how it could be resolved. Hence my suggestion to have > an owner. > > We also normally try to make these as specific and actionable as possible (hence > my other questions), but it seems like if Evan will be doing this shortly that's > not a big deal. Makes sense, thanks for explaining. I didn't realise that the concept of "TODO owner" is a bit different to "bug owner". I've put in estade. https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/minimize_button_metrics_win.cc (right): https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.cc:58: return GetCaptionButtonDimensionInDIPs(SM_CXSIZE, SM_CXSIZEFRAME); On 2017/05/11 01:03:43, Peter Kasting wrote: > It somewhat surprises me that the width would need to include CXSIZEFRAME, since > the non-close caption buttons aren't next to the frame. But I believe you... Me too! In fact, I've measured it carefully again and found that even with CXSIZEFRAME the width is still too small. I've gone back to a hardcoded value for this (46), since I can't find a way to get the correct value from Windows, but tell me if you have any ideas! See https://docs.google.com/a/google.com/spreadsheets/d/1q4q8RWRp8K-S6fakxjg8tWdD... I think a 1 px mismatch here won't be as bad as for height where the user can see the difference due to the buttons being side by side. https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/minimize_button_metrics_win.h (right): https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.h:21: // Return the width/height of the native caption buttons in DIPs. On 2017/05/11 01:03:43, Peter Kasting wrote: > Nit: "Returns" was correct; see bit about "descriptive vs. imperative" in > http://google.github.io/styleguide/cppguide.html#Function_Comments . Oh, it was supposed to be plural to apply to both functions. :) But I've reverted it to one function. https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/frame/minimize_button_metrics_win.h:23: static int GetCaptionButtonHeightInDIPs(); On 2017/05/11 01:03:43, Peter Kasting wrote: > Nit: Maybe > > static gfx::Size GetCaptionButtonSize(); > > ? (DIPs are implied with a gfx::Size, at least today.) Good point, I'll use this if we ever figure out a way of getting the width! (See other comment.) https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/avatar_button.cc (right): https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:104: SkColorSetA(SK_ColorBLACK, 0.54 * 0xFF); On 2017/05/11 01:03:43, Peter Kasting wrote: > Nit: static_cast second arg to SkAlpha Done. https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:162: return gfx::Size(MinimizeButtonMetrics::GetCaptionButtonWidthInDIPs(), 20); On 2017/05/11 01:03:43, Peter Kasting wrote: > Nit: Ultimately we'll probably want to calculate this dynamically based on the > min of the normal height and the y position of the new tab button minus some > padding; for now I'd just add a TODO to do that Done. https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:184: size.set_height(20); On 2017/05/11 01:03:43, Peter Kasting wrote: > Nit: Should this 20 and the 20 in GetMinimumSize() come from a common constant > for now? I'm not actually 100% clear on whether those things should be linked. I've wondered that as well, but I don't think they should be linked, as the old and new buttons would likely change independently. For example, when I tried an 18x18 icon I made the new button height 22, but the old one remained 20. https://codereview.chromium.org/2851543002/diff/180001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/avatar_button.cc:195: // Opacity of the ink drop on hover On 2017/05/11 01:03:43, Peter Kasting wrote: > Nit: Comment looks like it just restates the code, I'd remove. Done.
emx@chromium.org changed reviewers: + msw@chromium.org
Hi Mike, could you review the label_button changes, please?
The CQ bit was unchecked by emx@chromium.org
The CQ bit was checked by emx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:200001) has been deleted
https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:427: frame()->GetRootView()->Layout(); On 2017/05/11 01:03:42, Peter Kasting wrote: > On 2017/05/10 17:38:48, emx wrote: > > On 2017/05/09 17:40:28, Peter Kasting wrote: > > > On 2017/05/09 16:26:51, emx wrote: > > > > On 2017/05/06 02:23:08, Peter Kasting wrote: > > > > > 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()? > > > > > > > > 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. > > > > > > 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. > > > > I tried adding SchedulePaint(); and that's still not enough. > > OK. Let's leave it doing a Layout on the root view as you have it, with a TODO > for estade to dig deeper. I'm not the right one to dig deeper on this Windows only code.
https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/glass_browser_frame_view.cc (right): https://codereview.chromium.org/2851543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/frame/glass_browser_frame_view.cc:427: frame()->GetRootView()->Layout(); On 2017/05/11 14:29:09, Evan Stade wrote: > On 2017/05/11 01:03:42, Peter Kasting wrote: > > On 2017/05/10 17:38:48, emx wrote: > > > On 2017/05/09 17:40:28, Peter Kasting wrote: > > > > On 2017/05/09 16:26:51, emx wrote: > > > > > On 2017/05/06 02:23:08, Peter Kasting wrote: > > > > > > 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()? > > > > > > > > > > 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. > > > > > > > > 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. > > > > > > I tried adding SchedulePaint(); and that's still not enough. > > > > OK. Let's leave it doing a Layout on the root view as you have it, with a > TODO > > for estade to dig deeper. > > I'm not the right one to dig deeper on this Windows only code. OK, for now have bsep own this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
label_button.* rubber stamp lgtm
The CQ bit was checked by emx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org, pkasting@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2851543002/#ps240001 (title: "Fixed TODO comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by emx@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by emx@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1494588857750160,
"parent_rev": "f2125fea63367c21af67cddccb5ba50d9309070d", "commit_rev":
"da9bbac7c610430c9b94142d83ce56ee1d4f2f87"}
Message was sent while issue was closed.
Description was changed from ========== Update avatar button to MD (part 1) Update avatar button to MD Summary of changes made [ see screenshots in https://bugs.chromium.org/p/chromium/issues/detail?id=635699&desc=2#c36 ]: 1) Show new MD button only on Windows 10 and only if not using a theme 2) Make MD button "cozy" (20px) when tabstrip is full and not maximised and text direction is LTR (RTL can be tested with --force-ui-direction=rtl) 3) Make "tall" (non-cozy) MD button the same height as caption bar buttons - it's not pixel-perfect (see bug 716365) 4) Change icon and highlighting for MD button 5) Show "ink drop" animation when MD button is clicked, like for bookmarks bar and other menu buttons 6) Make menu appear on press, not on release (for all platforms, not just Windows 10) BUG=635699 ========== to ========== Update avatar button to MD (part 1) Update avatar button to MD Summary of changes made [ see screenshots in https://bugs.chromium.org/p/chromium/issues/detail?id=635699&desc=2#c36 ]: 1) Show new MD button only on Windows 10 and only if not using a theme 2) Make MD button "cozy" (20px) when tabstrip is full and not maximised and text direction is LTR (RTL can be tested with --force-ui-direction=rtl) 3) Make "tall" (non-cozy) MD button the same height as caption bar buttons - it's not pixel-perfect (see bug 716365) 4) Change icon and highlighting for MD button 5) Show "ink drop" animation when MD button is clicked, like for bookmarks bar and other menu buttons 6) Make menu appear on press, not on release (for all platforms, not just Windows 10) BUG=635699 Review-Url: https://codereview.chromium.org/2851543002 Cr-Commit-Position: refs/heads/master@{#471279} Committed: https://chromium.googlesource.com/chromium/src/+/da9bbac7c610430c9b94142d83ce... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:240001) as https://chromium.googlesource.com/chromium/src/+/da9bbac7c610430c9b94142d83ce... |
