Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2208973003:
Add some extra height to bookmark bar if the font wants to be larger (Closed)
|
Created:
4 years, 4 months ago by kylix_rd Modified:
4 years, 3 months ago CC:
chromium-reviews, tfarina, noyau+watch_chromium.org, Evan Stade, Peter Kasting Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSometimes the normal UI font is larger (eg. Hindi text). This code will ensure the Bookmark bar takes this into account and makes the height accommodate it. This eliminates painting artifacts when the Omnibox suggestions dropdown is displayed.
BUG=622678
Committed: https://crrev.com/510b6ef721d3b695d5b11dac659d09e5cbc13a5f
Cr-Commit-Position: refs/heads/master@{#417113}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Refinements to ensure consistent detection of larger-than-normal fonts #Patch Set 3 : Take into account the case where only the instructions view is what is visible #
Total comments: 9
Patch Set 4 : Set a border on the BookmarkInstructionsView instance and update GetPreferredSize() to respect it. #
Total comments: 9
Patch Set 5 : Some minor updates and tweaks #Patch Set 6 : Only call GetPreferredHeight() once in Layout() #
Total comments: 10
Patch Set 7 : Make sure the inset border is taken into account for both width and height #Patch Set 8 : Renamed kBookmarBarHeight & kNTPBookmarkBarHeight to kMinimumBookmarkBarHeight & kNTPMinimumBookmar… #Patch Set 9 : Fixed a Mac compile error #Patch Set 10 : More build fixes for MacOS #Patch Set 11 : Call GetPreferredHeight() only once in GetPreferredSize() #
Total comments: 8
Patch Set 12 : Removed #if in header not used for iOS & Android #
Total comments: 2
Patch Set 13 : Rename kNTPMinimumBookmarkBarHeight back to kNTPBookmarkBarheight #Patch Set 14 : Update comments regarding kMinimumBookmarkBarHeight #
Total comments: 5
Patch Set 15 : Address various nits from code review comments #Messages
Total messages: 82 (33 generated)
Description was changed from ========== Add some extra height to bookmark bar if the font wants to be larger BUG= ========== to ========== Sometimes the normal UI font is larger. This code will ensure the Bookmark bar takes this into account and makes the height accommodate it. This eliminates painting artifacts when the Omnibox suggestions dropdown is displayed. BUG=622678 ==========
kylixrd@chromium.org changed reviewers: + estade@chromium.org, pkasting@google.com, sky@chromium.org
Description was changed from ========== Sometimes the normal UI font is larger. This code will ensure the Bookmark bar takes this into account and makes the height accommodate it. This eliminates painting artifacts when the Omnibox suggestions dropdown is displayed. BUG=622678 ========== to ========== Sometimes the normal UI font is larger (eg. Hindi text). This code will ensure the Bookmark bar takes this into account and makes the height accommodate it. This eliminates painting artifacts when the Omnibox suggestions dropdown is displayed. BUG=622678 ==========
sky@, although you're the owner of these files, I've also been working with pkasting@ & estade@ on this fix. The referenced bug has screen shots showing the behavior this is trying to fix. I've also tested this code with a theme set and it seems to still behave as intended.
https://codereview.chromium.org/2208973003/diff/1/chrome/browser/ui/views/boo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/1/chrome/browser/ui/views/boo... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:870: static_cast<int>((chrome::kBookmarkBarHeight + GetExtraFontPadding()) * this should probably just take the max of its children's preferred heights and add some padding. What you have now doesn't actually respond to changes in children since it only calculates the extra padding the first time.
https://codereview.chromium.org/2208973003/diff/1/chrome/browser/ui/views/boo... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/1/chrome/browser/ui/views/boo... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:870: static_cast<int>((chrome::kBookmarkBarHeight + GetExtraFontPadding()) * On 2016/08/04 16:05:40, Evan Stade (sorta ooo) wrote: > this should probably just take the max of its children's preferred heights and > add some padding. What you have now doesn't actually respond to changes in > children since it only calculates the extra padding the first time. One of the issues I noticed is if the calculation is done every time, for some reason the children start returning kBookmarkBarHeight (28) and no padding is added. This renders this code ineffective and the BookmarkBarView doesn't grow. By caching the value early in the layout process, the BookmarkBarView remains the right height.
On 2016/08/04 16:42:10, kylix_rd wrote: > https://codereview.chromium.org/2208973003/diff/1/chrome/browser/ui/views/boo... > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): > > https://codereview.chromium.org/2208973003/diff/1/chrome/browser/ui/views/boo... > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:870: > static_cast<int>((chrome::kBookmarkBarHeight + GetExtraFontPadding()) * > On 2016/08/04 16:05:40, Evan Stade (sorta ooo) wrote: > > this should probably just take the max of its children's preferred heights and > > add some padding. What you have now doesn't actually respond to changes in > > children since it only calculates the extra padding the first time. > > One of the issues I noticed is if the calculation is done every time, for some > reason the children start returning kBookmarkBarHeight (28) and no padding is > added. This renders this code ineffective and the BookmarkBarView doesn't grow. > > By caching the value early in the layout process, the BookmarkBarView remains > the right height. Then I'd recommend investigating why this is happening instead of using a workaround that's not fully understood and might not handle all cases properly.
On 2016/08/04 18:01:59, Evan Stade (sorta ooo) wrote: > On 2016/08/04 16:42:10, kylix_rd wrote: > > > https://codereview.chromium.org/2208973003/diff/1/chrome/browser/ui/views/boo... > > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): > > > > > https://codereview.chromium.org/2208973003/diff/1/chrome/browser/ui/views/boo... > > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:870: > > static_cast<int>((chrome::kBookmarkBarHeight + GetExtraFontPadding()) * > > On 2016/08/04 16:05:40, Evan Stade (sorta ooo) wrote: > > > this should probably just take the max of its children's preferred heights > and > > > add some padding. What you have now doesn't actually respond to changes in > > > children since it only calculates the extra padding the first time. > > > > One of the issues I noticed is if the calculation is done every time, for some > > reason the children start returning kBookmarkBarHeight (28) and no padding is > > added. This renders this code ineffective and the BookmarkBarView doesn't > grow. > > > > By caching the value early in the layout process, the BookmarkBarView remains > > the right height. > > Then I'd recommend investigating why this is happening instead of using a > workaround that's not fully understood and might not handle all cases properly. Well, I was hoping someone might have remembered something and had some insight into why it was happening. I'll dig into it some more and see what I can find out.
OK. Round 2. GetPreferredSize() and Layout() are called a *lot* during startup. Eventually it settles down and the various views placed onto the bookmark bar are made invisible. The "Apps" button would report a different (smaller) size than the other buttons due to an inconsistency with the insets used. By changing the insets to be consistent and artificially enlarging the height of the instructions view by the insets amount kept things matched up. The code could then easily detect when to grow the bookmark bar if a larger font was selected. https://codereview.chromium.org/2208973003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:175: static const int kButtonPaddingVertical = 5; This value is inconsistent with the other buttons' padding and was causing flaky behavior of the code which tries to expand the size of the bookmark bar to accommodate larger fonts.
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2208973003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:852: // If this is the instructions view, enlarge it's height by the same Nit: its https://codereview.chromium.org/2208973003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:854: // view is the only visible view. This comment confuses me a bit. Are we basically just doing this because of this? (If so, feel free to change to this comment:) We want as much padding around the instructions label as we have around the labels in the buttons. https://codereview.chromium.org/2208973003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:856: pref.Enlarge(0, kButtonPaddingVertical * 2); What if we just set this as an empty border on the instructions view? Would that be good enough to avoid this GetPreferredHeight() method? (Perhaps we also need a layout manager? That can autosize the height to the max of the preferred child heights.) https://codereview.chromium.org/2208973003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:872: prefsize.set_height(static_cast<int>((GetPreferredHeight()) * Nit: No need for extra parens
Rather than special case the instructions_ instance, this patch sets an empty border and then updates GetPreferredSize() to respect it. https://codereview.chromium.org/2208973003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:852: // If this is the instructions view, enlarge it's height by the same On 2016/08/05 21:23:00, Peter Kasting wrote: > Nit: its Done. https://codereview.chromium.org/2208973003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:856: pref.Enlarge(0, kButtonPaddingVertical * 2); On 2016/08/05 21:23:00, Peter Kasting wrote: > What if we just set this as an empty border on the instructions view? Would The instructions view doesn't respect the set borders. GetPreferredSize() ignores them. An empty border could be set on the instructions_ view in Init and BookmarkBarInstructionsView::GetPreferredSize() modified to respect them. > that be good enough to avoid this GetPreferredHeight() method? (Perhaps we also > need a layout manager? That can autosize the height to the max of the preferred > child heights.) We need this method to determine whether the preferred height is larger than chrome::kBookmarkBarHeight. IOW, kBookmarkBarHeight is the minimum height and should only grow if any child view wants to be larger. https://codereview.chromium.org/2208973003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:872: prefsize.set_height(static_cast<int>((GetPreferredHeight()) * On 2016/08/05 21:23:00, Peter Kasting wrote: > Nit: No need for extra parens Done.
https://codereview.chromium.org/2208973003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:855: return std::max(height, chrome::kBookmarkBarHeight); seems like this constant should be renamed to kBookmarkBarMinimumHeight https://codereview.chromium.org/2208973003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:946: y += (View::height() - chrome::kBookmarkBarHeight) / 2; is this still correct? Why isn't it using kNTPBookmarkBarHeight? (All this manual layout without the use of LayoutManagers seems very brittle to me...) https://codereview.chromium.org/2208973003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right): https://codereview.chromium.org/2208973003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:389: // the preferred height if the font is taller than normal. (eg. Hindi text). I can't parse this sentence, and I'm not sure it's totally necessary to describe the implementation from here
https://codereview.chromium.org/2208973003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:855: return std::max(height, chrome::kBookmarkBarHeight); On 2016/08/08 16:10:57, Evan Stade wrote: > seems like this constant should be renamed to kBookmarkBarMinimumHeight That should be done in a separate CL. This constant spills into some MacOS specific code. https://codereview.chromium.org/2208973003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:946: y += (View::height() - chrome::kBookmarkBarHeight) / 2; On 2016/08/08 16:10:57, Evan Stade wrote: > is this still correct? Why isn't it using kNTPBookmarkBarHeight? (All this > manual layout without the use of LayoutManagers seems very brittle to me...) It should, at least, be calling GetPreferredHeight(). Looking closer at this code is's odd. Why is it animating the horizontal dimensions and not the vertical? Transitioning between attached and detached, should really only animate in the vertical direction with the content remaining horizontally fixed. At least that is how it *appears*. I'll change chrome::kBookmarkBarHeight to GetPreferredHeight() for now. https://codereview.chromium.org/2208973003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right): https://codereview.chromium.org/2208973003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:389: // the preferred height if the font is taller than normal. (eg. Hindi text). On 2016/08/08 16:10:57, Evan Stade wrote: > I can't parse this sentence, and I'm not sure it's totally necessary to describe > the implementation from here Should there be no comment? I suppose the name is fairly clear.
The CQ bit was checked by kylixrd@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/2208973003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:856: pref.Enlarge(0, kButtonPaddingVertical * 2); On 2016/08/08 15:33:21, kylix_rd wrote: > On 2016/08/05 21:23:00, Peter Kasting wrote: > > What if we just set this as an empty border on the instructions view? Would > > The instructions view doesn't respect the set borders. GetPreferredSize() > ignores them. An empty border could be set on the instructions_ view in Init and > BookmarkBarInstructionsView::GetPreferredSize() modified to respect them. It seems offhand like part of the problem there is that the instructions view does manual sizing and layout instead of just using a layout manager, which would presumably pay attention to this value by default. I know you tweaked it to respect your new border, but only in GetPreferredSize(), and only the height (and not width) of the border. A layout manager seems much more robust. Is there a reason using one won't work? (In this case, I'd think we'd need a BoxLayout.) > > that be good enough to avoid this GetPreferredHeight() method? (Perhaps we > also > > need a layout manager? That can autosize the height to the max of the > preferred > > child heights.) > > We need this method to determine whether the preferred height is larger than > chrome::kBookmarkBarHeight. IOW, kBookmarkBarHeight is the minimum height and > should only grow if any child view wants to be larger. Sure, but that could be done without the loop here (and only the max() call) if the bookmark bar view was using a layout manager to compute the preferred size. Or it could probably be done without overriding this method at all by adding a dummy child of kBookmarkBarHeight.
https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc (right): https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:72: height += GetInsets().height(); If you're going to accommodate insets for the height, why not the width too? https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:848: for (int i = 0; i < child_count(); ++i) { Really you only care about the special views here, e.g. instructions_view_. For the bookmark buttons they are all positioned assuming they have the same preferred size (line 1026 new ish). https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1001: x + kInstructionsPadding, y, Is there a reason you don't want to accommodate the additional size here? Seems better when the layout is manual already.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:848: for (int i = 0; i < child_count(); ++i) { On 2016/08/08 20:10:40, sky wrote: > Really you only care about the special views here, e.g. instructions_view_. For > the bookmark buttons they are all positioned assuming they have the same > preferred size (line 1026 new ish). The code cares about all views. Not all views are visible all the time, but the labels (embedded within the other views/buttons/menu buttons) still need more space. This is reported by the GetPreferredSize() method from each view type. kBookmarkBarHeight is the minimum height of the bookmark bar in attached mode. The size is only expanded if some embedded view needs the space to better fit it's text. https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1001: x + kInstructionsPadding, y, On 2016/08/08 20:10:40, sky wrote: > Is there a reason you don't want to accommodate the additional size here? Seems > better when the layout is manual already. It does take it into account. 'height' is set above. Maybe I don't understand what you mean here?
https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:848: for (int i = 0; i < child_count(); ++i) { On 2016/08/08 21:23:54, kylix_rd wrote: > On 2016/08/08 20:10:40, sky wrote: > > Really you only care about the special views here, e.g. instructions_view_. > For > > the bookmark buttons they are all positioned assuming they have the same > > preferred size (line 1026 new ish). > > The code cares about all views. Not all views are visible all the time, but the > labels (embedded within the other views/buttons/menu buttons) still need more > space. This is reported by the GetPreferredSize() method from each view type. What I was saying is that all the bookmark buttons are assumed to be the same height, so it's overkill to iterate over those. > kBookmarkBarHeight is the minimum height of the bookmark bar in attached mode. > The size is only expanded if some embedded view needs the space to better fit > it's text. https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1001: x + kInstructionsPadding, y, On 2016/08/08 21:23:54, kylix_rd wrote: > On 2016/08/08 20:10:40, sky wrote: > > Is there a reason you don't want to accommodate the additional size here? > Seems > > better when the layout is manual already. > > It does take it into account. 'height' is set above. Maybe I don't understand > what you mean here? I was suggesting you accommodate the extra height in layout rather than setting the border. But what you have is fine.
https://codereview.chromium.org/2208973003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:855: return std::max(height, chrome::kBookmarkBarHeight); On 2016/08/08 17:05:23, kylix_rd wrote: > On 2016/08/08 16:10:57, Evan Stade wrote: > > seems like this constant should be renamed to kBookmarkBarMinimumHeight > > That should be done in a separate CL. This constant spills into some MacOS > specific code. I don't really see why that should split off? It's not accurate to imply that this is the height of the bookmark bar when it's only the minimum. It would make more sense to me if Mac used kMinimum[...] even though it never grew, because that's not actually a contradiction. https://codereview.chromium.org/2208973003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:946: y += (View::height() - chrome::kBookmarkBarHeight) / 2; On 2016/08/08 17:05:23, kylix_rd wrote: > On 2016/08/08 16:10:57, Evan Stade wrote: > > is this still correct? Why isn't it using kNTPBookmarkBarHeight? (All this > > manual layout without the use of LayoutManagers seems very brittle to me...) > > It should, at least, be calling GetPreferredHeight(). Looking closer at this > code is's odd. Why is it animating the horizontal dimensions and not the > vertical? It is animating the vertical because height() changes based on the animation state. > Transitioning between attached and detached, should really only > animate in the vertical direction with the content remaining horizontally fixed. It seems that we intentionally have a different horizontal offset in the detached and attached states, so I believe the horizontal animation is correct. > At least that is how it *appears*. > > I'll change chrome::kBookmarkBarHeight to GetPreferredHeight() for now.
https://codereview.chromium.org/2208973003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:855: return std::max(height, chrome::kBookmarkBarHeight); On 2016/08/08 22:48:06, Evan Stade wrote: > On 2016/08/08 17:05:23, kylix_rd wrote: > > On 2016/08/08 16:10:57, Evan Stade wrote: > > > seems like this constant should be renamed to kBookmarkBarMinimumHeight > > > > That should be done in a separate CL. This constant spills into some MacOS > > specific code. > > I don't really see why that should split off? It's not accurate to imply that > this is the height of the bookmark bar when it's only the minimum. It would make > more sense to me if Mac used kMinimum[...] even though it never grew, because > that's not actually a contradiction. OK. I can add it to this review and then include the MacOS files & reviewers as well. https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:848: for (int i = 0; i < child_count(); ++i) { On 2016/08/08 22:25:35, sky wrote: > On 2016/08/08 21:23:54, kylix_rd wrote: > > On 2016/08/08 20:10:40, sky wrote: > > > Really you only care about the special views here, e.g. instructions_view_. > > For > > > the bookmark buttons they are all positioned assuming they have the same > > > preferred size (line 1026 new ish). > > > > The code cares about all views. Not all views are visible all the time, but > the > > labels (embedded within the other views/buttons/menu buttons) still need more > > space. This is reported by the GetPreferredSize() method from each view type. > > What I was saying is that all the bookmark buttons are assumed to be the same > height, so it's overkill to iterate over those. > > > kBookmarkBarHeight is the minimum height of the bookmark bar in attached mode. > > The size is only expanded if some embedded view needs the space to better fit > > it's text. > Are you saying the code could just check one of the bookmark buttons and the instructions view? I could unroll the loop and check each instance separately because different ones are visible at different times, unless visiblity is irrelevant here?
https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:848: for (int i = 0; i < child_count(); ++i) { On 2016/08/09 21:09:13, kylix_rd wrote: > On 2016/08/08 22:25:35, sky wrote: > > On 2016/08/08 21:23:54, kylix_rd wrote: > > > On 2016/08/08 20:10:40, sky wrote: > > > > Really you only care about the special views here, e.g. > instructions_view_. > > > For > > > > the bookmark buttons they are all positioned assuming they have the same > > > > preferred size (line 1026 new ish). > > > > > > The code cares about all views. Not all views are visible all the time, but > > the > > > labels (embedded within the other views/buttons/menu buttons) still need > more > > > space. This is reported by the GetPreferredSize() method from each view > type. > > > > What I was saying is that all the bookmark buttons are assumed to be the same > > height, so it's overkill to iterate over those. > > > > > kBookmarkBarHeight is the minimum height of the bookmark bar in attached > mode. > > > The size is only expanded if some embedded view needs the space to better > fit > > > it's text. > > > > Are you saying the code could just check one of the bookmark buttons and the > instructions view? I could unroll the loop and check each instance separately > because different ones are visible at different times, unless visiblity is > irrelevant here? Keep what you have. What I'm suggesting, while more efficient, would be more fragile.
On 2016/08/09 21:20:32, sky wrote: > https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... > File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): > > https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... > chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:848: for (int i = 0; i < > child_count(); ++i) { > On 2016/08/09 21:09:13, kylix_rd wrote: > > On 2016/08/08 22:25:35, sky wrote: > > > On 2016/08/08 21:23:54, kylix_rd wrote: > > > > On 2016/08/08 20:10:40, sky wrote: > > > > > Really you only care about the special views here, e.g. > > instructions_view_. > > > > For > > > > > the bookmark buttons they are all positioned assuming they have the same > > > > > preferred size (line 1026 new ish). > > > > > > > > The code cares about all views. Not all views are visible all the time, > but > > > the > > > > labels (embedded within the other views/buttons/menu buttons) still need > > more > > > > space. This is reported by the GetPreferredSize() method from each view > > type. > > > > > > What I was saying is that all the bookmark buttons are assumed to be the > same > > > height, so it's overkill to iterate over those. > > > > > > > kBookmarkBarHeight is the minimum height of the bookmark bar in attached > > mode. > > > > The size is only expanded if some embedded view needs the space to better > > fit > > > > it's text. > > > > > > > Are you saying the code could just check one of the bookmark buttons and the > > instructions view? I could unroll the loop and check each instance separately > > because different ones are visible at different times, unless visiblity is > > irrelevant here? > > Keep what you have. What I'm suggesting, while more efficient, would be more > fragile. And sorry for the noise.
I feel like a broken record here, but I still feel like all this would wind up cleaner and safer if both these classes used layout managers. I think the reason they don't is that they predate extensive use of layout managers (we didn't have them for a while, and even after we did, I know I for one didn't really understand or use them for some time). I would be willing to accept a "let's land this for now to fix the bug, and I'm looking at the layout manager change in parallel", but unless I'm wrong and a layout manager ends up being a loss here, it seems like now (while you've looked into how layout is working) is the ideal time to go ahead and investigate that.
If you look at BookmarkBarView::Layout you'll see it does a number of interesting things that would require a very tightly coupled LayoutManager. At that point I don't think the LayoutManager buys us anything. The only value I could see in separating the layout is if we needed to swap different layouts. Is that the case for md work? -Scott On Tue, Aug 9, 2016 at 3:42 PM, <pkasting@chromium.org> wrote: > I feel like a broken record here, but I still feel like all this would wind > up > cleaner and safer if both these classes used layout managers. I think the > reason they don't is that they predate extensive use of layout managers (we > didn't have them for a while, and even after we did, I know I for one didn't > really understand or use them for some time). > > I would be willing to accept a "let's land this for now to fix the bug, and > I'm > looking at the layout manager change in parallel", but unless I'm wrong and > a > layout manager ends up being a loss here, it seems like now (while you've > looked > into how layout is working) is the ideal time to go ahead and investigate > that. > > https://codereview.chromium.org/2208973003/ -- 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.
On 2016/08/10 02:48:23, sky wrote: > If you look at BookmarkBarView::Layout you'll see it does a number of > interesting things that would require a very tightly coupled > LayoutManager. At that point I don't think the LayoutManager buys us > anything. The only value I could see in separating the layout is if we > needed to swap different layouts. Is that the case for md work? I don't think so. Perhaps I am wrong about a LayoutManager being a win for the bookmark bar, though just skimming Layout(), it seems like a reasonably straightforward function made more complicated by a desire to prioritize which elements would hide first -- which I'd think we could do with some LayoutManager? Maybe I'm overlooking some complexity. The instructions view certainly looks like a simple BoxLayout would suffice though.
On 2016/08/09 22:42:47, Peter Kasting wrote: > I feel like a broken record here, but I still feel like all this would wind up > cleaner and safer if both these classes used layout managers. I think the > reason they don't is that they predate extensive use of layout managers (we > didn't have them for a while, and even after we did, I know I for one didn't > really understand or use them for some time). > > I would be willing to accept a "let's land this for now to fix the bug, and I'm > looking at the layout manager change in parallel", but unless I'm wrong and a > layout manager ends up being a loss here, it seems like now (while you've looked > into how layout is working) is the ideal time to go ahead and investigate that. As a matter of fact, I am looking into doing some layout improvements in conjunction with LayoutManagers. It's in the early investigation/feasibility phase right now. It's something I've worked on in other frameworks.
https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc (right): https://codereview.chromium.org/2208973003/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:72: height += GetInsets().height(); On 2016/08/08 20:10:40, sky wrote: > If you're going to accommodate insets for the height, why not the width too? Done.
The CQ bit was checked by kylixrd@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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
On Tue, Aug 9, 2016 at 10:31 PM, <pkasting@chromium.org> wrote: > On 2016/08/10 02:48:23, sky wrote: >> If you look at BookmarkBarView::Layout you'll see it does a number of >> interesting things that would require a very tightly coupled >> LayoutManager. At that point I don't think the LayoutManager buys us >> anything. The only value I could see in separating the layout is if we >> needed to swap different layouts. Is that the case for md work? > > I don't think so. Perhaps I am wrong about a LayoutManager being a win for > the > bookmark bar, though just skimming Layout(), it seems like a reasonably > straightforward function made more complicated by a desire to prioritize > which > elements would hide first -- which I'd think we could do with some > LayoutManager? I don't believe any of the existing layout managers offer the level of customization needed. I'm not against converting BookmarkBarView to using a layout manager, my gut tells me it won't really help things, but the proof is in the patch;) That said, I don't think we should hold up this review for converting to a layoutmanager. -Scott > Maybe I'm overlooking some complexity. > > The instructions view certainly looks like a simple BoxLayout would suffice > though. > > https://codereview.chromium.org/2208973003/ -- 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.
re: layout managers, fwiw I was able to convert download items to use them and the code got a bit simpler. This may be because download items used to be more complicated or because the stock layout managers used to be less powerful. At first glance it seems like the bookmark bar should be able to use layout manager(s) because it just sticks a bunch of buttons in a row, but I'd be wary of trusting first impressions in this case.
On 2016/08/10 15:48:09, Evan Stade wrote: > re: layout managers, fwiw I was able to convert download items to use them and > the code got a bit simpler. This may be because download items used to be more > complicated or because the stock layout managers used to be less powerful. At > first glance it seems like the bookmark bar should be able to use layout > manager(s) because it just sticks a bunch of buttons in a row, but I'd be wary > of trusting first impressions in this case. It would be nice if the layout managers were able to use "hints" or some kind of meta-data attached (aka. attributes or properties) to each view that assist in the proper layout. Rather than rely solely on child ordering or some other metric, each view could request from the layout manager where and how it would like to be placed. This extra information would be attached to the view as it is created rather than just being baked in. This would allow more complicated layouts to be described without resorting to writing one-off layout managers or have a bunch of layout code (in Layout() or similar) speckled throughout the code base. I think this would lead to an overall reduction in the amount of unique layout code.
My worry with meta data that is used for ordering is that it may get out of sync with the actual children. But I may be misunderstanding what you are suggesting. Also, I'm not sure why you would need meta data rather than telling the LayoutManager directly what you want, similar to how GridLayout is configured. -Scott On Wed, Aug 10, 2016 at 9:10 AM, <kylixrd@chromium.org> wrote: > On 2016/08/10 15:48:09, Evan Stade wrote: >> re: layout managers, fwiw I was able to convert download items to use them >> and >> the code got a bit simpler. This may be because download items used to be >> more >> complicated or because the stock layout managers used to be less powerful. >> At >> first glance it seems like the bookmark bar should be able to use layout >> manager(s) because it just sticks a bunch of buttons in a row, but I'd be >> wary >> of trusting first impressions in this case. > > It would be nice if the layout managers were able to use "hints" or some > kind of > meta-data attached (aka. attributes or properties) to each view that assist > in > the proper layout. Rather than rely solely on child ordering or some other > metric, each view could request from the layout manager where and how it > would > like to be placed. This extra information would be attached to the view as > it is > created rather than just being baked in. This would allow more complicated > layouts to be described without resorting to writing one-off layout managers > or > have a bunch of layout code (in Layout() or similar) speckled throughout the > code base. I think this would lead to an overall reduction in the amount of > unique layout code. > > > https://codereview.chromium.org/2208973003/ -- 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.
On 2016/08/10 18:14:31, sky wrote: > My worry with meta data that is used for ordering is that it may get > out of sync with the actual children. But I may be misunderstanding > what you are suggesting. Also, I'm not sure why you would need meta > data rather than telling the LayoutManager directly what you want, > similar to how GridLayout is configured. > This information is added as the views are instantiated and inserted into the child list. It is attached to the child view itself, so it cannot easily get out of sync.
That doesn't sound like meta data then. I'm happy to look at any patches that improve things. -Scott On Wed, Aug 10, 2016 at 11:24 AM, <kylixrd@chromium.org> wrote: > On 2016/08/10 18:14:31, sky wrote: >> My worry with meta data that is used for ordering is that it may get >> out of sync with the actual children. But I may be misunderstanding >> what you are suggesting. Also, I'm not sure why you would need meta >> data rather than telling the LayoutManager directly what you want, >> similar to how GridLayout is configured. >> > > This information is added as the views are instantiated and inserted into > the > child list. It is attached to the child view itself, so it cannot easily get > out > of sync. > > > https://codereview.chromium.org/2208973003/ -- 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.
On 2016/08/10 20:03:00, sky wrote: > That doesn't sound like meta data then. I'm happy to look at any > patches that improve things. Admittedly, I'm using all those terms interchangeably. More appropriate terms would be properties or attributes. These can be assigned dynamically at run-time on a per-instance basis. > -- > 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 mailto:chromium-reviews+unsubscribe@chromium.org. >
kylixrd@chromium.org changed reviewers: + asvitkine@chromium.org
Renamed kBookmarkBarHeight & kNTPBookmarkBarHeight to kMinimumBookmarkBarHeight & kNTPMinimumBookmarkBarHeight, respecitively. asvtikine@ please review the MacOS BookmarkBar related files. I work mainly on Windows.
The CQ bit was checked by kylixrd@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Some more minor code tweaks
There are now a lot of us on the review line. I'm not sure if this is "whichever of you gets to this first review" or if you're hoping for specific feedback/looks from one or more of us in particular. Want to say briefly what your current review expectation is?
kylixrd@chromium.org changed reviewers: - pkasting@google.com
On 2016/08/16 20:05:41, Peter Kasting wrote: > There are now a lot of us on the review line. I'm not sure if this is > "whichever of you gets to this first review" or if you're hoping for specific > feedback/looks from one or more of us in particular. > > Want to say briefly what your current review expectation is? asvitkine@ - MacOS/Cocoa related files. estade@ - ntp_resource_cache.cc sky@ - views/bookmarks/bookmark_bar pkasting@ - Expressed some opinions on the solution in the related bug.
On 2016/08/16 20:36:25, kylix_rd wrote: > On 2016/08/16 20:05:41, Peter Kasting wrote: > > There are now a lot of us on the review line. I'm not sure if this is > > "whichever of you gets to this first review" or if you're hoping for specific > > feedback/looks from one or more of us in particular. > > > > Want to say briefly what your current review expectation is? > > asvitkine@ - MacOS/Cocoa related files. > estade@ - ntp_resource_cache.cc > sky@ - views/bookmarks/bookmark_bar > pkasting@ - Expressed some opinions on the solution in the related bug. Thanks. I'm cool with whatever Evan/Scott approve of.
https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/book... File chrome/browser/ui/bookmarks/bookmark_bar_constants.h (right): https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_bar_constants.h:13: #if defined(TOOLKIT_VIEWS) || defined(OS_MACOSX) This leaves ios and android out, which I don't believe are using this file. I think you can remove this ifdef entirely. https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:865: (chrome::kNTPMinimumBookmarkBarHeight - preferred_height) * Might this line and line 899 go negative now. I guess that's ok, just weird.
cocoa lgtm assuming other reviewers are good with the overall change
https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:142: // if it's a top-aligned bar. ruh roh. This is now broken since we aren't hard-coding the bar size.
https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/book... File chrome/browser/ui/bookmarks/bookmark_bar_constants.h (right): https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_bar_constants.h:13: #if defined(TOOLKIT_VIEWS) || defined(OS_MACOSX) On 2016/08/16 21:56:13, sky wrote: > This leaves ios and android out, which I don't believe are using this file. I > think you can remove this ifdef entirely. Done. https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:865: (chrome::kNTPMinimumBookmarkBarHeight - preferred_height) * On 2016/08/16 21:56:13, sky wrote: > Might this line and line 899 go negative now. I guess that's ok, just weird. Unless the font pushes preferred_height > kNTPMinimumBookmarkBarHeight, it should remain >= 0. It's possible, yet, bit not very probable. kNTPMinimumBookmarkBarHeight has more "headroom" than kMinimumBookmarkBarHeight. https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:142: // if it's a top-aligned bar. On 2016/08/17 06:36:35, Evan Stade wrote: > ruh roh. This is now broken since we aren't hard-coding the bar size. It is only broken if kNTPMinimumBookmarkBarHeight isn't sufficiently tall to to display the fonts. Possible, but not likely, IMO. It doesn't look like this function has enough information to determine the proper offset if the NTP bookmark bar is taller than kNTPMinimumBookmarkBarHeight. I can investigate.
https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:142: // if it's a top-aligned bar. On 2016/08/17 15:39:02, kylix_rd wrote: > On 2016/08/17 06:36:35, Evan Stade wrote: > > ruh roh. This is now broken since we aren't hard-coding the bar size. > > It is only broken if kNTPMinimumBookmarkBarHeight isn't sufficiently tall to to > display the fonts. Possible, but not likely, IMO. > > It doesn't look like this function has enough information to determine the > proper offset if the NTP bookmark bar is taller than > kNTPMinimumBookmarkBarHeight. > > I can investigate. oh wait, that's right, the ntp version is taller and shouldn't have a problem even with Hindi. Forgive me if this was covered up thread, but why do we need to apply the same logic to the NTP bookmark bar then, instead of leaving its size hardcoded (to simplify stuff like this)? now that I look in the other files, it seems like you haven't changed how this is used, so why the name change? https://codereview.chromium.org/2208973003/diff/220001/chrome/browser/ui/book... File chrome/browser/ui/bookmarks/bookmark_bar_constants.h (right): https://codereview.chromium.org/2208973003/diff/220001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_bar_constants.h:15: // The height of Bookmarks Bar, when attached to the toolbar. nit: update comments
Changed kNTPMinimumBookmarkBarHeight back to kNTPBookmarkBarHeight. This removed the need to change ntp_resource_cache.cc https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/2208973003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:142: // if it's a top-aligned bar. On 2016/08/17 15:47:29, Evan Stade wrote: > On 2016/08/17 15:39:02, kylix_rd wrote: > > On 2016/08/17 06:36:35, Evan Stade wrote: > > > ruh roh. This is now broken since we aren't hard-coding the bar size. > > > > It is only broken if kNTPMinimumBookmarkBarHeight isn't sufficiently tall to > to > > display the fonts. Possible, but not likely, IMO. > > > > It doesn't look like this function has enough information to determine the > > proper offset if the NTP bookmark bar is taller than > > kNTPMinimumBookmarkBarHeight. > > > > I can investigate. > > oh wait, that's right, the ntp version is taller and shouldn't have a problem > even with Hindi. Forgive me if this was covered up thread, but why do we need to > apply the same logic to the NTP bookmark bar then, instead of leaving its size > hardcoded (to simplify stuff like this)? > > now that I look in the other files, it seems like you haven't changed how this > is used, so why the name change? Consistency? A little overzealous? Good question. I'll change it back.
The CQ bit was checked by kylixrd@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.
kylixrd@chromium.org changed reviewers: - estade@chromium.org
https://codereview.chromium.org/2208973003/diff/220001/chrome/browser/ui/book... File chrome/browser/ui/bookmarks/bookmark_bar_constants.h (right): https://codereview.chromium.org/2208973003/diff/220001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_bar_constants.h:15: // The height of Bookmarks Bar, when attached to the toolbar. On 2016/08/17 15:47:29, Evan Stade wrote: > nit: update comments Done.
I think I've addressed all the comments. Are there any that I've missed?
LGTM
estade@chromium.org changed reviewers: + estade@chromium.org
lgtm https://codereview.chromium.org/2208973003/diff/260001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/bookmarks/bookmark_bar_constants.h (right): https://codereview.chromium.org/2208973003/diff/260001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/bookmarks/bookmark_bar_constants.h:31: chrome::kMinimumBookmarkBarHeight + kVisualHeightOffset; I've no idea how this makes sense. The buttons are taller than the bar? oh well, Cocoa https://codereview.chromium.org/2208973003/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:851: gfx::Size pref = view->GetPreferredSize(); nit: inline pref instead of making it a local variable? https://codereview.chromium.org/2208973003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:855: return std::max(height, chrome::kMinimumBookmarkBarHeight); nit: you could put kMinimumBookmarkBarHeight as the initial value for height
The CQ bit was checked by kylixrd@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/2208973003/diff/260001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right): https://codereview.chromium.org/2208973003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:851: gfx::Size pref = view->GetPreferredSize(); On 2016/08/31 21:27:18, Evan Stade wrote: > nit: inline pref instead of making it a local variable? Done. https://codereview.chromium.org/2208973003/diff/260001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:855: return std::max(height, chrome::kMinimumBookmarkBarHeight); On 2016/08/31 21:27:18, Evan Stade wrote: > nit: you could put kMinimumBookmarkBarHeight as the initial value for height Done.
kylixrd@chromium.org changed reviewers: - pkasting@chromium.org
The CQ bit was checked by kylixrd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, sky@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2208973003/#ps280001 (title: "Address various nits from code review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Sometimes the normal UI font is larger (eg. Hindi text). This code will ensure the Bookmark bar takes this into account and makes the height accommodate it. This eliminates painting artifacts when the Omnibox suggestions dropdown is displayed. BUG=622678 ========== to ========== Sometimes the normal UI font is larger (eg. Hindi text). This code will ensure the Bookmark bar takes this into account and makes the height accommodate it. This eliminates painting artifacts when the Omnibox suggestions dropdown is displayed. BUG=622678 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Sometimes the normal UI font is larger (eg. Hindi text). This code will ensure the Bookmark bar takes this into account and makes the height accommodate it. This eliminates painting artifacts when the Omnibox suggestions dropdown is displayed. BUG=622678 ========== to ========== Sometimes the normal UI font is larger (eg. Hindi text). This code will ensure the Bookmark bar takes this into account and makes the height accommodate it. This eliminates painting artifacts when the Omnibox suggestions dropdown is displayed. BUG=622678 Committed: https://crrev.com/510b6ef721d3b695d5b11dac659d09e5cbc13a5f Cr-Commit-Position: refs/heads/master@{#417113} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/510b6ef721d3b695d5b11dac659d09e5cbc13a5f Cr-Commit-Position: refs/heads/master@{#417113} |
