New avatar button: Consolidate text elision between Mac and Win/Linux

New avatar button: Consolidate text elision between Mac and Win/Linux

This is a cleanup, as discussed in the comments of https://codereview.chromium.org/287103004/

This turned out to require extending gfx::TruncateString with an extra BreakType parameter; it now supports breaking within words as well as at word boundaries. All call sites have been updated accordingly.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284933

[Marc Treib, 2014-07-11 10:55:27]

Somewhat delayed, but I didn't forget! ;)
PTAL!

[noms (inactive), 2014-07-11 13:21:57]

Would you mind attaching some screenshots? I remember that when I looked into
this, the mac button would elide much sooner than the windows button (eg. on
windows we would display slightly longer strings than we did on the mac).

https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profiles_state.cc:88: const int kMaxCharactersToDisplay
= 15;
I think msw had suggested we use a max size, and not a max character to display,
as GetExpectedTextWidth is a little expensive.

[Marc Treib, 2014-07-11 13:45:44]

On 2014/07/11 13:21:57, Monica Dinculescu wrote:
> Would you mind attaching some screenshots? I remember that when I looked into
> this, the mac button would elide much sooner than the windows button (eg. on
> windows we would display slightly longer strings than we did on the mac).

At the moment it's actually the other way around, Win elides at 12ish characters
already (before as well as after this CL).

>
https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
> File chrome/browser/profiles/profiles_state.cc (right):
> 
>
https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
> chrome/browser/profiles/profiles_state.cc:88: const int
kMaxCharactersToDisplay
> = 15;
> I think msw had suggested we use a max size, and not a max character to
display,
> as GetExpectedTextWidth is a little expensive.

Really? The various implementations all look rather simple, i.e. don't actually
render any text but just do something like "num_chars * average_width". Am I
missing something?

Once we've decided on max chars vs pixels, I'll add screenshots!

[msw, 2014-07-11 22:17:06]

https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profiles_state.cc:88: const int kMaxCharactersToDisplay
= 15;
On 2014/07/11 13:21:57, Monica Dinculescu wrote:
> I think msw had suggested we use a max size, and not a max character to
display,
> as GetExpectedTextWidth is a little expensive.

My goal is to remove the explicit call to ElideText here, and let the button
elide the text as needed. You can use GetExpectedTextWidth to help with setting
a maximum button width if you wish.

You should sync past r282433 and you may want to try this with my (hopefully
innocuous) WIP CL <https://codereview.chromium.org/23228004/>.

[Marc Treib, 2014-07-14 08:54:43]

On 2014/07/11 22:17:06, msw wrote:
>
https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
> File chrome/browser/profiles/profiles_state.cc (right):
> 
>
https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
> chrome/browser/profiles/profiles_state.cc:88: const int
kMaxCharactersToDisplay
> = 15;
> On 2014/07/11 13:21:57, Monica Dinculescu wrote:
> > I think msw had suggested we use a max size, and not a max character to
> display,
> > as GetExpectedTextWidth is a little expensive.
> 
> My goal is to remove the explicit call to ElideText here, and let the button
> elide the text as needed. You can use GetExpectedTextWidth to help with
setting
> a maximum button width if you wish.

Hm, I don't think it will be easily possible here to have the button elide
automatically, since in the case of a supervised user we don't want to elide at
the end, but rather like: "LongElidedUserNa... (Supervised)", i.e. elide the
name, but not the "supervised" label. Do you see any way to still get around an
explicit ElideText call in this case?

> You should sync past r282433 and you may want to try this with my (hopefully
> innocuous) WIP CL <https://codereview.chromium.org/23228004/>.

[msw, 2014-07-14 17:02:48]

On 2014/07/14 08:54:43, treib wrote:
> On 2014/07/11 22:17:06, msw wrote:
> >
>
https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
> > File chrome/browser/profiles/profiles_state.cc (right):
> > 
> >
>
https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
> > chrome/browser/profiles/profiles_state.cc:88: const int
> kMaxCharactersToDisplay
> > = 15;
> > On 2014/07/11 13:21:57, Monica Dinculescu wrote:
> > > I think msw had suggested we use a max size, and not a max character to
> > display,
> > > as GetExpectedTextWidth is a little expensive.
> > 
> > My goal is to remove the explicit call to ElideText here, and let the button
> > elide the text as needed. You can use GetExpectedTextWidth to help with
> setting
> > a maximum button width if you wish.
> 
> Hm, I don't think it will be easily possible here to have the button elide
> automatically, since in the case of a supervised user we don't want to elide
at
> the end, but rather like: "LongElidedUserNa... (Supervised)", i.e. elide the
> name, but not the "supervised" label. Do you see any way to still get around
an
> explicit ElideText call in this case?

Sigh, no.

> > You should sync past r282433 and you may want to try this with my (hopefully
> > innocuous) WIP CL <https://codereview.chromium.org/23228004/>.

[Marc Treib, 2014-07-16 09:57:39]

On 2014/07/14 17:02:48, msw wrote:
> On 2014/07/14 08:54:43, treib wrote:
> > On 2014/07/11 22:17:06, msw wrote:
> > >
> >
>
https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
> > > File chrome/browser/profiles/profiles_state.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
> > > chrome/browser/profiles/profiles_state.cc:88: const int
> > kMaxCharactersToDisplay
> > > = 15;
> > > On 2014/07/11 13:21:57, Monica Dinculescu wrote:
> > > > I think msw had suggested we use a max size, and not a max character to
> > > display,
> > > > as GetExpectedTextWidth is a little expensive.
> > > 
> > > My goal is to remove the explicit call to ElideText here, and let the
button
> > > elide the text as needed. You can use GetExpectedTextWidth to help with
> > setting
> > > a maximum button width if you wish.
> > 
> > Hm, I don't think it will be easily possible here to have the button elide
> > automatically, since in the case of a supervised user we don't want to elide
> at
> > the end, but rather like: "LongElidedUserNa... (Supervised)", i.e. elide the
> > name, but not the "supervised" label. Do you see any way to still get around
> an
> > explicit ElideText call in this case?
> 
> Sigh, no.
> 
> > > You should sync past r282433 and you may want to try this with my
(hopefully
> > > innocuous) WIP CL <https://codereview.chromium.org/23228004/>.

Screenshots are here:
https://drive.google.com/a/google.com/folderview?id=0BxTU2YPRG-lRM0xZeW5QWFJq...
(For Win and Linux there are no separate before/after images because they're
identical.)

[msw, 2014-07-16 16:58:28]

https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profiles_state.cc:92:
font_list.GetExpectedTextWidth(kMaxCharactersToDisplay),
Could you use gfx::TruncateString instead, since you more or less intend to cut
the string after x characters? That'd avoid the more expensive eliding and width
variation by font, etc. You may need to add support for breaking mid-word.

[Marc Treib, 2014-07-17 08:43:00]

https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/381953002/diff/1/chrome/browser/profiles/prof...
chrome/browser/profiles/profiles_state.cc:92:
font_list.GetExpectedTextWidth(kMaxCharactersToDisplay),
On 2014/07/16 16:58:28, msw wrote:
> Could you use gfx::TruncateString instead, since you more or less intend to
cut
> the string after x characters? That'd avoid the more expensive eliding and
width
> variation by font, etc. You may need to add support for breaking mid-word.

Well, that depends on what exactly the intended behavior is, but given that Mac
and Win behaved differently so far, I'm guessing we don't really care. So unless
Monica has objections, I'll extend TruncateString and use that.

[Marc Treib, 2014-07-17 15:15:24]

I've extended gfx::TruncateString to optionally cut anywhere (instead of only at
word boundaries), and now use that in GetAvatarButtonTextForProfile. PTAL again!

[msw, 2014-07-17 17:18:35]

Nice work, but I wonder if we ought to modify TruncateString at all, since it
seems that it would already break mid-word if no word breaks were found
preceding the 15 character limit. I suppose it's a design question:
For "Bland Profile Name" do we want: "Bland Profile N..." or "Bland Profile..."
For "My Longprofilename" do we want: "My Longprofilen..." or "My..."
Whereas for "Superlongprofilename", both return "Superlongprofil...", right?
It seems the original behavior has worked decently elsewhere.

https://codereview.chromium.org/381953002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right):

https://codereview.chromium.org/381953002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:224: return
base::SysUTF16ToNSString(
nit: maybe inline this below.

https://codereview.chromium.org/381953002/diff/20001/ui/gfx/text_elider.h
File ui/gfx/text_elider.h (right):

https://codereview.chromium.org/381953002/diff/20001/ui/gfx/text_elider.h#new...
ui/gfx/text_elider.h:162: bool only_at_word_breaks);
nit: maybe use gfx::BreakType or base::i18n::BreakIterator::BreakType?

[noms (inactive), 2014-07-17 17:33:36]

I don't think we were enforcing breaking after a word before, nor is it a big
deal if we don't, so from my point of view, that doesn't need to be a
constraint. :)

[Marc Treib, 2014-07-18 10:05:09]

Right, the TruncateString changes will only make a difference if there is a word
break before the 15 char limit.
I'd (slightly) prefer also breaking within words, to maximize the amount of
information (e.g. so we can distinguish "My Longprofilename" from "My
Otherlongprofilename").

https://codereview.chromium.org/381953002/diff/20001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right):

https://codereview.chromium.org/381953002/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:224: return
base::SysUTF16ToNSString(
On 2014/07/17 17:18:35, msw wrote:
> nit: maybe inline this below.

Done.

https://codereview.chromium.org/381953002/diff/20001/ui/gfx/text_elider.h
File ui/gfx/text_elider.h (right):

https://codereview.chromium.org/381953002/diff/20001/ui/gfx/text_elider.h#new...
ui/gfx/text_elider.h:162: bool only_at_word_breaks);
On 2014/07/17 17:18:35, msw wrote:
> nit: maybe use gfx::BreakType or base::i18n::BreakIterator::BreakType?

Hm, base::i18n::BreakIterator::BreakType has lots of cases that aren't handled
separately here, and it might be confusing that the behavior here doesn't quite
match that of a corresponding BreakIterator.
gfx::BreakType has WORD_BREAK and LINE_BREAK which would probably mean the same
thing here? But there's a TODO to merge it with
base::i18n::BreakIterator::BreakType, so.. hm.
I agree that an enum would be better here than a bool, but neither of these seem
to really fit, and I wouldn't like to introduce yet another one.

[msw, 2014-07-18 19:21:48]

https://codereview.chromium.org/381953002/diff/20001/ui/gfx/text_elider.h
File ui/gfx/text_elider.h (right):

https://codereview.chromium.org/381953002/diff/20001/ui/gfx/text_elider.h#new...
ui/gfx/text_elider.h:162: bool only_at_word_breaks);
On 2014/07/18 10:05:09, treib wrote:
> On 2014/07/17 17:18:35, msw wrote:
> > nit: maybe use gfx::BreakType or base::i18n::BreakIterator::BreakType?
> 
> Hm, base::i18n::BreakIterator::BreakType has lots of cases that aren't handled
> separately here, and it might be confusing that the behavior here doesn't
quite
> match that of a corresponding BreakIterator.
> gfx::BreakType has WORD_BREAK and LINE_BREAK which would probably mean the
same
> thing here? But there's a TODO to merge it with
> base::i18n::BreakIterator::BreakType, so.. hm.
> I agree that an enum would be better here than a bool, but neither of these
seem
> to really fit, and I wouldn't like to introduce yet another one.

gfx::BreakType seems appropriate for now. When my TODO is addressed, it can be
replaced with a [likely leaner] base::i18n::BreakIterator::BreakType. It already
supported WORD_BREAK and now supports CHARACTER_BREAK, and could later support
LINE_BREAK (start by finding the first newline before the argument length). If
we are adding an argument to support some addition breaking type, it should be
an enum, not a bool.

[Marc Treib, 2014-07-21 09:03:00]

https://codereview.chromium.org/381953002/diff/20001/ui/gfx/text_elider.h
File ui/gfx/text_elider.h (right):

https://codereview.chromium.org/381953002/diff/20001/ui/gfx/text_elider.h#new...
ui/gfx/text_elider.h:162: bool only_at_word_breaks);
On 2014/07/18 19:21:48, msw wrote:
> On 2014/07/18 10:05:09, treib wrote:
> > On 2014/07/17 17:18:35, msw wrote:
> > > nit: maybe use gfx::BreakType or base::i18n::BreakIterator::BreakType?
> > 
> > Hm, base::i18n::BreakIterator::BreakType has lots of cases that aren't
handled
> > separately here, and it might be confusing that the behavior here doesn't
> quite
> > match that of a corresponding BreakIterator.
> > gfx::BreakType has WORD_BREAK and LINE_BREAK which would probably mean the
> same
> > thing here? But there's a TODO to merge it with
> > base::i18n::BreakIterator::BreakType, so.. hm.
> > I agree that an enum would be better here than a bool, but neither of these
> seem
> > to really fit, and I wouldn't like to introduce yet another one.
> 
> gfx::BreakType seems appropriate for now. When my TODO is addressed, it can be
> replaced with a [likely leaner] base::i18n::BreakIterator::BreakType. It
already
> supported WORD_BREAK and now supports CHARACTER_BREAK, and could later support
> LINE_BREAK (start by finding the first newline before the argument length). If
> we are adding an argument to support some addition breaking type, it should be
> an enum, not a bool.

Done.

[msw, 2014-07-21 15:26:08]

Thanks, just some unused headers/decls and nits remain.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right):

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:16: #include
"grit/generated_resources.h"
Remove this.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:20: #include
"ui/base/l10n/l10n_util_mac.h"
Remove this.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:25: #include
"ui/gfx/text_elider.h"
Remove this.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:150: -
(base::string16)getElidedAvatarName;
Remove this.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:7: #include
"base/strings/utf_string_conversions.h"
Remove this.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:8: #include
"base/win/windows_version.h"
I think you can remove this.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:12: #include
"chrome/browser/signin/profile_oauth2_token_service_factory.h"
Can you remove this?

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:14: #include
"components/signin/core/browser/profile_oauth2_token_service.h"
Can you remove this?

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:15: #include
"grit/generated_resources.h"
Remove this.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:17: #include
"ui/base/l10n/l10n_util.h"
Remove this.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:20: #include
"ui/gfx/color_utils.h"
I think you can remove this.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:21: #include
"ui/gfx/font_list.h"
Remove this.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:22: #include
"ui/gfx/text_constants.h"
I think you can remove this.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:23: #include
"ui/gfx/text_elider.h"
Remove this.

https://codereview.chromium.org/381953002/diff/60001/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/381953002/diff/60001/ui/gfx/text_elider.cc#ne...
ui/gfx/text_elider.cc:759: const base::string16& string, size_t length,
BreakType break_type) {
nit: one parameter per line

https://codereview.chromium.org/381953002/diff/60001/ui/gfx/text_elider.h
File ui/gfx/text_elider.h (right):

https://codereview.chromium.org/381953002/diff/60001/ui/gfx/text_elider.h#new...
ui/gfx/text_elider.h:158: // character 0x2026) to render ...
nit: put ... in quotation marks and start the next sentence on this line.

https://codereview.chromium.org/381953002/diff/60001/ui/message_center/views/...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/381953002/diff/60001/ui/message_center/views/...
ui/message_center/views/notification_view.cc:535:
gfx::TruncateString(notification.title(),
nit: init a shared local |title| string outside the if/else block.

https://codereview.chromium.org/381953002/diff/60001/ui/message_center/views/...
ui/message_center/views/notification_view.cc:568: message_view_ = new
BoundedLabel(gfx::TruncateString(notification.message(),
ditto

https://codereview.chromium.org/381953002/diff/60001/ui/message_center/views/...
ui/message_center/views/notification_view.cc:601: new
BoundedLabel(gfx::TruncateString(notification.context_message(),
ditto

[Marc Treib, 2014-07-21 16:43:52]

Thanks for the detailed comments! All done.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm (right):

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:16: #include
"grit/generated_resources.h"
On 2014/07/21 15:26:06, msw wrote:
> Remove this.

Done.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:20: #include
"ui/base/l10n/l10n_util_mac.h"
On 2014/07/21 15:26:07, msw wrote:
> Remove this.

Done.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:25: #include
"ui/gfx/text_elider.h"
On 2014/07/21 15:26:06, msw wrote:
> Remove this.

Done.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:150: -
(base::string16)getElidedAvatarName;
On 2014/07/21 15:26:07, msw wrote:
> Remove this.

Done.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:7: #include
"base/strings/utf_string_conversions.h"
On 2014/07/21 15:26:07, msw wrote:
> Remove this.

Done.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:8: #include
"base/win/windows_version.h"
On 2014/07/21 15:26:07, msw wrote:
> I think you can remove this.

Done.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:12: #include
"chrome/browser/signin/profile_oauth2_token_service_factory.h"
On 2014/07/21 15:26:07, msw wrote:
> Can you remove this?

Done.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:14: #include
"components/signin/core/browser/profile_oauth2_token_service.h"
On 2014/07/21 15:26:07, msw wrote:
> Can you remove this?

Done.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:15: #include
"grit/generated_resources.h"
On 2014/07/21 15:26:07, msw wrote:
> Remove this.

Done.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:17: #include
"ui/base/l10n/l10n_util.h"
On 2014/07/21 15:26:07, msw wrote:
> Remove this.

Done.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:20: #include
"ui/gfx/color_utils.h"
On 2014/07/21 15:26:07, msw wrote:
> I think you can remove this.

Done.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:21: #include
"ui/gfx/font_list.h"
On 2014/07/21 15:26:07, msw wrote:
> Remove this.

Done.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:22: #include
"ui/gfx/text_constants.h"
On 2014/07/21 15:26:07, msw wrote:
> I think you can remove this.

Done.

https://codereview.chromium.org/381953002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:23: #include
"ui/gfx/text_elider.h"
On 2014/07/21 15:26:07, msw wrote:
> Remove this.

Done.

https://codereview.chromium.org/381953002/diff/60001/ui/gfx/text_elider.cc
File ui/gfx/text_elider.cc (right):

https://codereview.chromium.org/381953002/diff/60001/ui/gfx/text_elider.cc#ne...
ui/gfx/text_elider.cc:759: const base::string16& string, size_t length,
BreakType break_type) {
On 2014/07/21 15:26:07, msw wrote:
> nit: one parameter per line

Done.

https://codereview.chromium.org/381953002/diff/60001/ui/gfx/text_elider.h
File ui/gfx/text_elider.h (right):

https://codereview.chromium.org/381953002/diff/60001/ui/gfx/text_elider.h#new...
ui/gfx/text_elider.h:158: // character 0x2026) to render ...
On 2014/07/21 15:26:07, msw wrote:
> nit: put ... in quotation marks and start the next sentence on this line.

Done.

https://codereview.chromium.org/381953002/diff/60001/ui/message_center/views/...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/381953002/diff/60001/ui/message_center/views/...
ui/message_center/views/notification_view.cc:535:
gfx::TruncateString(notification.title(),
On 2014/07/21 15:26:07, msw wrote:
> nit: init a shared local |title| string outside the if/else block.

Done.

https://codereview.chromium.org/381953002/diff/60001/ui/message_center/views/...
ui/message_center/views/notification_view.cc:568: message_view_ = new
BoundedLabel(gfx::TruncateString(notification.message(),
On 2014/07/21 15:26:07, msw wrote:
> ditto

Done.

https://codereview.chromium.org/381953002/diff/60001/ui/message_center/views/...
ui/message_center/views/notification_view.cc:601: new
BoundedLabel(gfx::TruncateString(notification.context_message(),
On 2014/07/21 15:26:07, msw wrote:
> ditto

Done.

[msw, 2014-07-21 16:47:29]

lgtm with nits

https://codereview.chromium.org/381953002/diff/80001/ui/message_center/views/...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/381953002/diff/80001/ui/message_center/views/...
ui/message_center/views/notification_view.cc:562: base::string16 title =
gfx::TruncateString(notification.message(),
nit: rename this to |text|, since this isn't a |title|

https://codereview.chromium.org/381953002/diff/80001/ui/message_center/views/...
ui/message_center/views/notification_view.cc:593: base::string16 title =
gfx::TruncateString(notification.context_message(),
ditto

[Marc Treib, 2014-07-22 08:12:48]

https://codereview.chromium.org/381953002/diff/80001/ui/message_center/views/...
File ui/message_center/views/notification_view.cc (right):

https://codereview.chromium.org/381953002/diff/80001/ui/message_center/views/...
ui/message_center/views/notification_view.cc:562: base::string16 title =
gfx::TruncateString(notification.message(),
On 2014/07/21 16:47:29, msw wrote:
> nit: rename this to |text|, since this isn't a |title|

Done.

https://codereview.chromium.org/381953002/diff/80001/ui/message_center/views/...
ui/message_center/views/notification_view.cc:593: base::string16 title =
gfx::TruncateString(notification.context_message(),
On 2014/07/21 16:47:29, msw wrote:
> ditto

Done.

[Marc Treib, 2014-07-22 08:15:38]

Adding reviewers for the remaining parts. It's all simple stuff that doesn't
change behavior.

lazyboy@chromium.org: Please review changes in c/b/renderer_context_menu.

rdevlin.cronin@chromium.org: Please review changes in c/b/extensions.

mukai@chromium.org: Please review changes in ui/message_center.

sadrul@chromium.org: Please review changes in ui/views.

Also ping @ noms for c/b/profiles!

[noms (inactive), 2014-07-22 13:40:59]

c/b/profiles lgtm (with a small nit)

https://codereview.chromium.org/381953002/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/381953002/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.cc:92: name =
l10n_util::GetStringFUTF16(IDS_SUPERVISED_USER_NEW_AVATAR_LABEL,
nit: I think this may need { } because the statement is on 2 lines.

[Marc Treib, 2014-07-22 14:12:44]

https://codereview.chromium.org/381953002/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/381953002/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.cc:92: name =
l10n_util::GetStringFUTF16(IDS_SUPERVISED_USER_NEW_AVATAR_LABEL,
On 2014/07/22 13:40:59, Monica Dinculescu wrote:
> nit: I think this may need { } because the statement is on 2 lines.

Done.

[lazyboy, 2014-07-22 16:14:27]

render_view_context_menu LGTM

[Devlin, 2014-07-22 16:31:13]

extensions lgtm

[Jun Mukai, 2014-07-22 16:44:26]

message_center lgtm

[sadrul, 2014-07-22 16:51:11]

lgtm

Please update the CL description to include some details about the CL (e.g. the
change made to the gfx API)

[Marc Treib, 2014-07-23 08:27:57]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2014-07-23 08:28:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/treib@chromium.org/381953002/120001

[commit-bot: I haz the power, 2014-07-23 09:39:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/31982)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/36723)

[commit-bot: I haz the power, 2014-07-23 09:45:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-23 09:45:51]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[Marc Treib, 2014-07-23 10:51:50]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2014-07-23 10:53:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/treib@chromium.org/381953002/160001

[commit-bot: I haz the power, 2014-07-23 14:06:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-23 15:53:06]

Change committed as 284933