Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
470053004:
[Win, Linux] Always display the avatar button text at the same size. (Closed)
|
Created:
6 years, 4 months ago by noms (inactive) Modified:
6 years, 3 months ago CC:
chromium-reviews, tfarina, ckocagil Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Description[Win, Linux] Always display the avatar button text at the same size.
The problem is that some locales (Hindi) or platforms (Linux) have a
bigger base font size for the default font (crbug.com/405994). If this
is the case, we should decrease the font size to the one we expect,
so that the font fits in the button correctly.
Just adjusting the baseline is not always enough, as the new avatar
button has a fixed size, and the text might not fit correctly
regardless of the baseline adjustments.
Screenshots: https://drive.google.com/folderview?id=0B1B1Up4p2NRMRHc5WTBxZWNrRm8&usp=sharing
BUG=403466
TEST=Start Chrome with --enable-new-avatar-menu on Linux, or with --enable-new-avatar-menu and --lang-hi on Windows. The avatar button text should fit nicely in the button.
Committed: https://crrev.com/899ae44e331661f709f463d6861a1dbceb9194f2
Cr-Commit-Position: refs/heads/master@{#292715}
Patch Set 1 #
Total comments: 5
Patch Set 2 : pull out omnibox code to a common place #
Total comments: 9
Patch Set 3 : nits + added a test #Patch Set 4 : maybe made test better? #
Total comments: 8
Patch Set 5 : rebase + review comments and moar better test #
Total comments: 6
Patch Set 6 : nits #
Total comments: 4
Patch Set 7 : nits part 2 #
Messages
Total messages: 30 (1 generated)
https://codereview.chromium.org/470053004/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/470053004/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/new_avatar_button.cc:68: SetFontList(GetFontList().DeriveWithSizeDelta( I think we might need to use the font height instead of the size, similar to my work in <https://codereview.chromium.org/496873003>, since some fonts of the same size are taller than others. To do that, we'll need to generalize PlatformFontWin::DeriveFontWithHeight to other platform fonts and expose that method to the Font[List] class(es); that's needed here and probably for RenderTextHarfBuzz if my WIP CL is the right approach.
https://codereview.chromium.org/470053004/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/470053004/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/new_avatar_button.cc:68: SetFontList(GetFontList().DeriveWithSizeDelta( I originally had the font height, but it didn't seem to work as well (in particular) on Hindi. It worked fin on Linux, but in Hindi (when using english characters, not hindi ones), the font was way too small :/ I can give it a try again, if you thing that should work. Maybe I was doing something silly. On 2014/08/25 19:17:20, msw wrote: > I think we might need to use the font height instead of the size, similar to my > work in <https://codereview.chromium.org/496873003>, since some fonts of the > same size are taller than others. To do that, we'll need to generalize > PlatformFontWin::DeriveFontWithHeight to other platform fonts and expose that > method to the Font[List] class(es); that's needed here and probably for > RenderTextHarfBuzz if my WIP CL is the right approach.
https://codereview.chromium.org/470053004/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/470053004/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/new_avatar_button.cc:68: SetFontList(GetFontList().DeriveWithSizeDelta( I originally had the font height, but it didn't seem to work as well (in particular) on Hindi. It worked fin on Linux, but in Hindi (when using english characters, not hindi ones), the font was way too small :/ I can give it a try again, if you thing that should work. Maybe I was doing something silly. On 2014/08/25 19:17:20, msw wrote: > I think we might need to use the font height instead of the size, similar to my > work in <https://codereview.chromium.org/496873003>, since some fonts of the > same size are taller than others. To do that, we'll need to generalize > PlatformFontWin::DeriveFontWithHeight to other platform fonts and expose that > method to the Font[List] class(es); that's needed here and probably for > RenderTextHarfBuzz if my WIP CL is the right approach.
https://codereview.chromium.org/470053004/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/470053004/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/new_avatar_button.cc:68: SetFontList(GetFontList().DeriveWithSizeDelta( On 2014/08/25 19:19:36, Monica Dinculescu wrote: > I originally had the font height, but it didn't seem to work as well (in > particular) on Hindi. It worked fin on Linux, but in Hindi (when using english > characters, not hindi ones), the font was way too small :/ > > I can give it a try again, if you thing that should work. Maybe I was doing > something silly. > > On 2014/08/25 19:17:20, msw wrote: > > I think we might need to use the font height instead of the size, similar to > my > > work in <https://codereview.chromium.org/496873003>, since some fonts of the > > same size are taller than others. To do that, we'll need to generalize > > PlatformFontWin::DeriveFontWithHeight to other platform fonts and expose that > > method to the Font[List] class(es); that's needed here and probably for > > RenderTextHarfBuzz if my WIP CL is the right approach. > Unfortunately, what you had may have been "right". Matching sizes on my Win7 EN-US with "Segoe UI" (size 15, height 20) yields "Mangal" (size 15, height 28) for Hindi fallback. Any text using those some of those additional 8px of height could potentially be clipped. You might be able to use the full button height to bump up the font height a little, like I did for tab titles in r289073. I'd at least like to see how your height-based fix worked.
https://codereview.chromium.org/470053004/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/470053004/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/new_avatar_button.cc:68: SetFontList(GetFontList().DeriveWithSizeDelta( So what I had was: const int kDisplayFontSize = 15; int font_size = GetFontList().GetHeight(); if (font_size > kDisplayFontSize) { SetFontList(GetFontList().DeriveWithSizeDelta( kDisplayFontSize - font_size - 1)); } With the code above, the button looks like this: https://drive.google.com/open?id=0B1B1Up4p2NRMdjg0RTg0MnF0aDQ&authuser=1 I've tested my current patch (with size instead of height), and this is what it looks like when displaying the non-english characters in hindi: https://drive.google.com/open?id=0B1B1Up4p2NRMdDkxNVBrU3lKM3c&authuser=1, which is at the right size, i think. I think either works better than what we have now, so I'll go with whatever you think is correct. On 2014/08/25 19:43:48, msw wrote: > On 2014/08/25 19:19:36, Monica Dinculescu wrote: > > I originally had the font height, but it didn't seem to work as well (in > > particular) on Hindi. It worked fin on Linux, but in Hindi (when using english > > characters, not hindi ones), the font was way too small :/ > > > > I can give it a try again, if you thing that should work. Maybe I was doing > > something silly. > > > > On 2014/08/25 19:17:20, msw wrote: > > > I think we might need to use the font height instead of the size, similar to > > my > > > work in <https://codereview.chromium.org/496873003>, since some fonts of the > > > same size are taller than others. To do that, we'll need to generalize > > > PlatformFontWin::DeriveFontWithHeight to other platform fonts and expose > that > > > method to the Font[List] class(es); that's needed here and probably for > > > RenderTextHarfBuzz if my WIP CL is the right approach. > > > > Unfortunately, what you had may have been "right". Matching sizes on my Win7 > EN-US with "Segoe UI" (size 15, height 20) yields "Mangal" (size 15, height 28) > for Hindi fallback. Any text using those some of those additional 8px of height > could potentially be clipped. You might be able to use the full button height to > bump up the font height a little, like I did for tab titles in r289073. > > I'd at least like to see how your height-based fix worked.
https://codereview.chromium.org/470053004/diff/1/chrome/browser/ui/views/prof... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/470053004/diff/1/chrome/browser/ui/views/prof... chrome/browser/ui/views/profiles/new_avatar_button.cc:68: SetFontList(GetFontList().DeriveWithSizeDelta( On 2014/08/25 20:06:02, Monica Dinculescu wrote: > So what I had was: > const int kDisplayFontSize = 15; > > int font_size = GetFontList().GetHeight(); > if (font_size > kDisplayFontSize) { > SetFontList(GetFontList().DeriveWithSizeDelta( > kDisplayFontSize - font_size - 1)); > } > > With the code above, the button looks like this: > https://drive.google.com/open?id=0B1B1Up4p2NRMdjg0RTg0MnF0aDQ&authuser=1 > > I've tested my current patch (with size instead of height), and this is what it > looks like when displaying the non-english characters in hindi: > https://drive.google.com/open?id=0B1B1Up4p2NRMdDkxNVBrU3lKM3c&authuser=1, which > is at the right size, i think. > > I think either works better than what we have now, so I'll go with whatever you > think is correct. > > On 2014/08/25 19:43:48, msw wrote: > > On 2014/08/25 19:19:36, Monica Dinculescu wrote: > > > I originally had the font height, but it didn't seem to work as well (in > > > particular) on Hindi. It worked fin on Linux, but in Hindi (when using > english > > > characters, not hindi ones), the font was way too small :/ > > > > > > I can give it a try again, if you thing that should work. Maybe I was doing > > > something silly. > > > > > > On 2014/08/25 19:17:20, msw wrote: > > > > I think we might need to use the font height instead of the size, similar > to > > > my > > > > work in <https://codereview.chromium.org/496873003>, since some fonts of > the > > > > same size are taller than others. To do that, we'll need to generalize > > > > PlatformFontWin::DeriveFontWithHeight to other platform fonts and expose > > that > > > > method to the Font[List] class(es); that's needed here and probably for > > > > RenderTextHarfBuzz if my WIP CL is the right approach. > > > > > > > Unfortunately, what you had may have been "right". Matching sizes on my Win7 > > EN-US with "Segoe UI" (size 15, height 20) yields "Mangal" (size 15, height > 28) > > for Hindi fallback. Any text using those some of those additional 8px of > height > > could potentially be clipped. You might be able to use the full button height > to > > bump up the font height a little, like I did for tab titles in r289073. > > > > I'd at least like to see how your height-based fix worked. > No, you can't interchange font height and size values like that (and that could cause the text to shrink unreasonably as you noticed). Let's try to fix this as I described in my original "To do that..." comment, since jshin@'s latest comments at http://crbug.com/404999 seem to support my height-limiting approach for height-constrained UI scenarios.
I pulled out the font-shrinking function out of the omnibox, and I think it would look like this. I didn't have to change it at all, because it's using public methods on the FontList, so the platform implementation is taken care of there.
Screenshots with the patch applied: glass: https://drive.google.com/open?id=0B1B1Up4p2NRMejR1VXZMNkcxeHM&authuser=1 theme: https://drive.google.com/open?id=0B1B1Up4p2NRMZ19zRE1nMjRwRVE&authuser=1
Ping?
noms@chromium.org changed reviewers: + pkasting@chromium.org
+ Peter, as I think this is originally his code :)
Is there a way to unittest this, now that it's a public API? Otherwise, I'm fine with this if Mike is. https://codereview.chromium.org/470053004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/470053004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/new_avatar_button.cc:21: const int kDisplayFontHeight = 15; Where does this come from? Can we derive this from the heights of some image resources or other views? https://codereview.chromium.org/470053004/diff/20001/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/470053004/diff/20001/ui/gfx/font_list.h#newco... ui/gfx/font_list.h:120: // Given a containing |height| and a |base_font_list|, shrinks the font size This comment is out of date.
https://codereview.chromium.org/470053004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/470053004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/new_avatar_button.cc:21: const int kDisplayFontHeight = 15; Was this height derived visually from the button assets? It looks like the themed button internal area is 15 pixels, but it looks like Win Glass could potentially use 16 pixels. Even better if you could derive these values from the button height (and image set heights?). https://codereview.chromium.org/470053004/diff/20001/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/470053004/diff/20001/ui/gfx/font_list.h#newco... ui/gfx/font_list.h:143: gfx::FontList GetLargestFontListWithHeightBound(int height) const; Follow the Derive* pattern and reorder as (DeriveWithHeightUpperBound?).
I've added a test, but it doesn't look particularly inspiring to me. I wanted to somehow tie the font size and the height together. Should I expect them to decrease linearly? I.e. (initial_height - bound_desired_height) == (initial_font_size - calculated_font_size) for all fonts? https://codereview.chromium.org/470053004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/470053004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/new_avatar_button.cc:21: const int kDisplayFontHeight = 15; Hmm, it was, yes. Working on getting something better working (using the height and the insets, maybe?) :/ On 2014/08/28 19:40:03, msw wrote: > Was this height derived visually from the button assets? It looks like the > themed button internal area is 15 pixels, but it looks like Win Glass could > potentially use 16 pixels. Even better if you could derive these values from the > button height (and image set heights?). https://codereview.chromium.org/470053004/diff/20001/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/470053004/diff/20001/ui/gfx/font_list.h#newco... ui/gfx/font_list.h:120: // Given a containing |height| and a |base_font_list|, shrinks the font size On 2014/08/28 19:34:18, Peter Kasting wrote: > This comment is out of date. Done. https://codereview.chromium.org/470053004/diff/20001/ui/gfx/font_list.h#newco... ui/gfx/font_list.h:143: gfx::FontList GetLargestFontListWithHeightBound(int height) const; On 2014/08/28 19:40:03, msw wrote: > Follow the Derive* pattern and reorder as (DeriveWithHeightUpperBound?). Done.
Patch set 4 has a slightly less hacky test, though I'm not sure why the final font size is off by 1. I would expect decreasing the font height by n to decrease the size by n, but size seems to decrease by n-1.
On 2014/08/28 20:49:44, Monica Dinculescu wrote: > Patch set 4 has a slightly less hacky test, though I'm not sure why the final > font size is off by 1. I would expect decreasing the font height by n to > decrease the size by n, but size seems to decrease by n-1. Font size and font height are not linearly related, especially at small sizes, and even in an idealized linear world, the slope of the line is generally some other value than 1. This is part of why the method to find a font size that fits in the desired space doesn't just calculate the end size directly from the initial size, but steps down size-at-a-time.
https://codereview.chromium.org/470053004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/470053004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/new_avatar_button.cc:21: const int kDisplayFontHeight = 15; On 2014/08/28 20:36:25, Monica Dinculescu wrote: > Hmm, it was, yes. Working on getting something better working (using the height > and the insets, maybe?) :/ > > On 2014/08/28 19:40:03, msw wrote: > > Was this height derived visually from the button assets? It looks like the > > themed button internal area is 15 pixels, but it looks like Win Glass could > > potentially use 16 pixels. Even better if you could derive these values from > the > > button height (and image set heights?). > Give it a shot, I'd be okay with a literal (and a comment explaining its origin) iff there's no clear way to derive the value from the button height, the insets, and the image set heights. https://codereview.chromium.org/470053004/diff/60001/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/470053004/diff/60001/ui/gfx/font_list.h#newco... ui/gfx/font_list.h:124: // The expected layout: Perhaps the expected layout comment belongs in the function body as implementation details, unless it has value to FontList consumers; Peter? https://codereview.chromium.org/470053004/diff/60001/ui/gfx/font_list.h#newco... ui/gfx/font_list.h:143: gfx::FontList DeriveWithHeightUpperBound(int height) const; Reorder this (and the impl) with the other derive functions. https://codereview.chromium.org/470053004/diff/60001/ui/gfx/font_list_unittes... File ui/gfx/font_list_unittest.cc (right): https://codereview.chromium.org/470053004/diff/60001/ui/gfx/font_list_unittes... ui/gfx/font_list_unittest.cc:292: fonts.push_back(gfx::Font("Arial", 18).Derive(0, Font::NORMAL)); Why do you need to derive the normal style for these? https://codereview.chromium.org/470053004/diff/60001/ui/gfx/font_list_unittes... ui/gfx/font_list_unittest.cc:297: FontList derived = font_list.DeriveWithHeightUpperBound( Maybe just do something like: const int height = 14; FontList derived = font_list.DeriveWithHeightUpperBound(height); EXPECT_LE(height, derived.GetHeight()); EXPECT_LT(font_list.GetHeight(), derived.GetHeight()); Then do the same deriving another list with height of 22.
https://codereview.chromium.org/470053004/diff/60001/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/470053004/diff/60001/ui/gfx/font_list.h#newco... ui/gfx/font_list.h:124: // The expected layout: On 2014/08/28 21:43:50, msw wrote: > Perhaps the expected layout comment belongs in the function body as > implementation details, unless it has value to FontList consumers; Peter? I think the comment should either be left here or deleted entirely. I was never a huge fan of adding the comment (I didn't write it originally), as things seem clear enough to me from the paragraph above, but if they're not, the comment is useful to callers to explain what "cap height vertically centered" means.
https://codereview.chromium.org/470053004/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/470053004/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/new_avatar_button.cc:21: const int kDisplayFontHeight = 15; I went with adding a comment. Button height + insets alone weren't enough, and I'd like to land this as soon as possible, if it's not a deal breaker. On 2014/08/28 21:43:50, msw wrote: > On 2014/08/28 20:36:25, Monica Dinculescu wrote: > > Hmm, it was, yes. Working on getting something better working (using the > height > > and the insets, maybe?) :/ > > > > On 2014/08/28 19:40:03, msw wrote: > > > Was this height derived visually from the button assets? It looks like the > > > themed button internal area is 15 pixels, but it looks like Win Glass could > > > potentially use 16 pixels. Even better if you could derive these values from > > the > > > button height (and image set heights?). > > > > Give it a shot, I'd be okay with a literal (and a comment explaining its origin) > iff there's no clear way to derive the value from the button height, the insets, > and the image set heights. https://codereview.chromium.org/470053004/diff/60001/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/470053004/diff/60001/ui/gfx/font_list.h#newco... ui/gfx/font_list.h:143: gfx::FontList DeriveWithHeightUpperBound(int height) const; On 2014/08/28 21:43:50, msw wrote: > Reorder this (and the impl) with the other derive functions. Done. https://codereview.chromium.org/470053004/diff/60001/ui/gfx/font_list_unittes... File ui/gfx/font_list_unittest.cc (right): https://codereview.chromium.org/470053004/diff/60001/ui/gfx/font_list_unittes... ui/gfx/font_list_unittest.cc:292: fonts.push_back(gfx::Font("Arial", 18).Derive(0, Font::NORMAL)); On 2014/08/28 21:43:51, msw wrote: > Why do you need to derive the normal style for these? Done. https://codereview.chromium.org/470053004/diff/60001/ui/gfx/font_list_unittes... ui/gfx/font_list_unittest.cc:297: FontList derived = font_list.DeriveWithHeightUpperBound( I've done something along these lines, but without explicitly hard-coding the heights (turns out 22 wasn't large enough). On 2014/08/28 21:43:51, msw wrote: > Maybe just do something like: > > const int height = 14; > FontList derived = font_list.DeriveWithHeightUpperBound(height); > EXPECT_LE(height, derived.GetHeight()); > EXPECT_LT(font_list.GetHeight(), derived.GetHeight()); > > Then do the same deriving another list with height of 22.
LGTM with nits. https://codereview.chromium.org/470053004/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/470053004/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:24: const int kDisplayFontHeight = 15; nit: move this constant down to where its used (function-local), merge comments. https://codereview.chromium.org/470053004/diff/100001/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/470053004/diff/100001/ui/gfx/font_list.h#newc... ui/gfx/font_list.h:86: // Given a containing |height| shrinks the font size until the font list nit: Remove "Given a containing |height| " https://codereview.chromium.org/470053004/diff/100001/ui/gfx/font_list_unitte... File ui/gfx/font_list_unittest.cc (right): https://codereview.chromium.org/470053004/diff/100001/ui/gfx/font_list_unitte... ui/gfx/font_list_unittest.cc:307: EXPECT_EQ(font_list.GetHeight(), derived_2.GetHeight()); nit: can you also EXPECT_EQ their GetFontSize() values?
https://codereview.chromium.org/470053004/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/470053004/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:24: const int kDisplayFontHeight = 15; On 2014/08/29 17:25:58, msw wrote: > nit: move this constant down to where its used (function-local), merge comments. Done. https://codereview.chromium.org/470053004/diff/100001/ui/gfx/font_list.h File ui/gfx/font_list.h (right): https://codereview.chromium.org/470053004/diff/100001/ui/gfx/font_list.h#newc... ui/gfx/font_list.h:86: // Given a containing |height| shrinks the font size until the font list On 2014/08/29 17:25:59, msw wrote: > nit: Remove "Given a containing |height| " Done. Did some minor rewording to match the other comments. https://codereview.chromium.org/470053004/diff/100001/ui/gfx/font_list_unitte... File ui/gfx/font_list_unittest.cc (right): https://codereview.chromium.org/470053004/diff/100001/ui/gfx/font_list_unitte... ui/gfx/font_list_unittest.cc:307: EXPECT_EQ(font_list.GetHeight(), derived_2.GetHeight()); On 2014/08/29 17:25:59, msw wrote: > nit: can you also EXPECT_EQ their GetFontSize() values? Done.
LGTM https://codereview.chromium.org/470053004/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/470053004/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:65: // is larger than this, it will be shrunk to match it. Nit: Maybe a TODO about changing the constant here to be calculated algorithmically? I'm OK with landing this for now but I'd rather not have this stay long-term. https://codereview.chromium.org/470053004/diff/120001/ui/gfx/font_list_unitte... File ui/gfx/font_list_unittest.cc (right): https://codereview.chromium.org/470053004/diff/120001/ui/gfx/font_list_unitte... ui/gfx/font_list_unittest.cc:310: } Nit: Indentation
Yay, thanks everyone! https://codereview.chromium.org/470053004/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/new_avatar_button.cc (right): https://codereview.chromium.org/470053004/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/new_avatar_button.cc:65: // is larger than this, it will be shrunk to match it. On 2014/08/29 18:12:26, Peter Kasting wrote: > Nit: Maybe a TODO about changing the constant here to be calculated > algorithmically? I'm OK with landing this for now but I'd rather not have this > stay long-term. Done.
https://codereview.chromium.org/470053004/diff/120001/ui/gfx/font_list_unitte... File ui/gfx/font_list_unittest.cc (right): https://codereview.chromium.org/470053004/diff/120001/ui/gfx/font_list_unitte... ui/gfx/font_list_unittest.cc:310: } On 2014/08/29 18:12:26, Peter Kasting wrote: > Nit: Indentation Done.
The CQ bit was checked by noms@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/470053004/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as f4be6629e20a1164019b34e4fda4888324d6114b
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/899ae44e331661f709f463d6861a1dbceb9194f2 Cr-Commit-Position: refs/heads/master@{#292715} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
