Customize user details in ash system bubble for public account mode

Customize user details in ash system bubble for public account mode

This CL customizes the user details shown at the top of the ash system
bubble when logged into a public account. The regular display of user name
and e-mail is replaced by a text that informs the user this is a public
account and names the domain managing the account. The implementation
follows the UI mock in the linked bug.

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

[bartfab (slow), 2012-11-13 15:26:16]

Hi Scott,

Could you review the ash_strings.grd, chrome/browser/ui and ui/views changes?

Hi Steven,

Could you review the system tray changes?

[sky, 2012-11-13 17:47:00]

https://chromiumcodereview.appspot.com/11377133/diff/1/ui/views/view_text_uti...
File ui/views/view_text_utils.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/1/ui/views/view_text_uti...
ui/views/view_text_utils.cc:20: void DrawTextAndPositionUrl(gfx::Canvas* canvas,
A method named DrawText should get a non-null canvas. Why the change?

[bartfab (slow), 2012-11-13 17:51:24]

https://chromiumcodereview.appspot.com/11377133/diff/1/ui/views/view_text_uti...
File ui/views/view_text_utils.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/1/ui/views/view_text_uti...
ui/views/view_text_utils.cc:20: void DrawTextAndPositionUrl(gfx::Canvas* canvas,
Because I need a way to measure the text size without drawing it (so that the
View that uses this can compute its preferred size). The calculations for
measuring and drawing are absolutely identical and I wanted to avoid
copy&pasting the entire method.

[sky, 2012-11-13 17:56:50]

Ok, you need to update the comments accordingly then.

[stevenjb, 2012-11-13 20:54:22]

http://codereview.chromium.org/11377133/diff/1/ash/system/user/tray_user.cc
File ash/system/user/tray_user.cc (right):

http://codereview.chromium.org/11377133/diff/1/ash/system/user/tray_user.cc#n...
ash/system/user/tray_user.cc:186: kPublicAccountUserCardTextColor, false));
This seems like something that ought to be generalized. Could we use
GetStringFUTF16 and embed colorization tabs in the replacement text, then split
that into colorized chunks?

At minimum I would like to see this split into one or more smaller methods; it
is very challenging to follow right now.

http://codereview.chromium.org/11377133/diff/1/ash/system/user/tray_user.cc#n...
ash/system/user/tray_user.cc:188: kPublicAccountUserCardTextColor, false));
This bit seems strange to me. Is this the spacing between the text and the link?
Please comment.

http://codereview.chromium.org/11377133/diff/1/ash/system/user/tray_user.cc#n...
ash/system/user/tray_user.cc:253: }
So, I don't want to overcomplicate this CL too much, but you might want to look
at gfx::RenderText and TextfieldViewsModel / NativeTextfieldViews. If we can
leverage RenderText here instead of writing a custom text chunk renderer, that
would be great.

If that seems like too much effort for this CL, I would suggest at least
breaking up the text stying bit of this into a separate method (maybe of
TextChunk?) e.g. something like:
text = (*it)->GetWrappedText(font, width);

[bartfab (slow), 2012-11-16 19:59:15]

https://chromiumcodereview.appspot.com/11377133/diff/1/ash/system/user/tray_u...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/1/ash/system/user/tray_u...
ash/system/user/tray_user.cc:186: kPublicAccountUserCardTextColor, false));
On 2012/11/13 20:54:22, stevenjb (chromium) wrote:
> This seems like something that ought to be generalized. Could we use
> GetStringFUTF16 and embed colorization tabs in the replacement text, then
split
> that into colorized chunks?
> 
> At minimum I would like to see this split into one or more smaller methods; it
> is very challenging to follow right now.

Done.

https://chromiumcodereview.appspot.com/11377133/diff/1/ash/system/user/tray_u...
ash/system/user/tray_user.cc:188: kPublicAccountUserCardTextColor, false));
On 2012/11/13 20:54:22, stevenjb (chromium) wrote:
> This bit seems strange to me. Is this the spacing between the text and the
link?
> Please comment.

Done.

https://chromiumcodereview.appspot.com/11377133/diff/1/ash/system/user/tray_u...
ash/system/user/tray_user.cc:253: }
I went for the bonus points: I replaced the views_text_utils with RenderText
completely :).

On 2012/11/13 20:54:22, stevenjb (chromium) wrote:
> So, I don't want to overcomplicate this CL too much, but you might want to
look
> at gfx::RenderText and TextfieldViewsModel / NativeTextfieldViews. If we can
> leverage RenderText here instead of writing a custom text chunk renderer, that
> would be great.
> 
> If that seems like too much effort for this CL, I would suggest at least
> breaking up the text stying bit of this into a separate method (maybe of
> TextChunk?) e.g. something like:
> text = (*it)->GetWrappedText(font, width);

[bartfab (slow), 2012-11-16 20:01:32]

Hi Mike,

I added you as a reviewer for the tray_bubble_view.* changes.

Hi everyone else,

I redid the entire implementation. Please take a look.

[stevenjb, 2012-11-16 21:13:52]

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:214: display_name_markers.push(0);
This is confusing. It looks like this means that we may end up with an odd
number of entries in the queue, so the 0 entry will persist for the next line? I
think this might be more clear if we set a bool here then pushed an entry at 0
into the queue at the beginning of this section if the bool is true. At minimum
the comment needs to be more clear.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:261: // link do not wrap over more than three lines
in total for esthetic reasons.
nit: align, aesthetic (more common)

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:267: space_width + link_size.width();
We should probably limit max width here to a reasonable percentage of the screen
width (or a reasonable fixed size like 400px; on a large display a bubble half
its width would look odd. The SystemBubble should probably define the maximum
width. In practice this shouldn't happen of course...

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:278: (width - font_.GetStringWidth(lines.back()) <=
Avoid this (expensive) calculation if truncated?

[msw, 2012-11-16 22:29:32]

I suggest pushing back on the UI here, it's a lot of complicated impl that's
difficult to test and prone to very nuanced errors. I suspect that a simple
"This is a shared user account. [Learn more]" would probably suffice. You have
the power!

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/ash_strings.gr...
ash/ash_strings.grd:238: <message name="IDS_ASH_STATUS_TRAY_PUBLIC_LEARN_MORE"
desc="Text of the Learn more link shown in the ash system bubble's user card
when the current session is a public account session.">
nit: "Learn more" is a common string. Unfortunately all the other uses are in
chrome/app/...grd[p]. It might be worthwhile to anticipate other users and make
this IDS_ASH_LEARN_MORE or move it to ui/base/strings/ui_strings.grd and
potentially use this instead of other IDS_*LEARN_MORE.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:129: class PublicAccountUserDetails : public
views::View,
nit: separate declaration and definition.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:175: if (contents_area.IsEmpty())
nit: does this happen?

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:180: ui::ElideRectangleText(text_, font_,
contents_area.width(),
Does ElideRectangleText do the right thing if the UI directionality (say LTR for
the "is a shared account...") differs from the domain/user names (which could be
RTL)? Does your text styling also get this right? (I think that probably should
be okay, as it's all done in logical character indicies).

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:187: it != lines.end(); ++it) {
nit: indent one more space (just my preference to wrap within parens here,
etc.).

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:202: // Locate any markers indicating the start and
end of the user's display
nit: you should be able to safely assume that there will only be a single name
in the block of text, so there will be two markers to find. Use a ui::Range to
track their locations per line, and a flag to persist the outside-/inside-
flag-pair state across lines. Then you should only need to apply a single style
range if a marker was found or the flag is on.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:221: while (display_name_markers.size() >= 2) {
there should only be one non-default style range per line, afaict.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:232: // Position link after the label text,
separated by a space. If it does not
This should be tested for RTL UI; I suspect that it may be incorrect.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:261: // link do not wrap over more than three lines
in total for esthetic reasons.
What's done for arbitrarily long doman/user names? You can't expect them to
always fit on three lines if there's a hard maximum width (which we should have,
as Steven suggested).

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:265: int min_width = std::max(1,
link_size.width());
nit: it should be safe to assume the link width is larger.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:272: while (min_width < max_width) {
I fear this will be the performance bottleneck for showing the system tray menu,
although I lack data to substantiate that concern and don't have great
alternative suggestions beyond picking a reasonable width and letting the height
increase while potentially eliding/truncating obscenely long user/domain names.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:274: std::vector<string16> lines;
nit: declare |lines| outside this loop and use it below too (line 297).

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:279: space_width + link_size.width());
nit: make this a ternary * ? 0 : 1; to explicitly specify the value added.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:292: const int available_width =
The initial min_width value above (line 265) should be the maximum of this
allocated width and the width of "Learn More".

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:300: const int line_count = lines.size() +
ditto nit: make this a ternary * ? 0 : 1; to explicitly specify the value added.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:310: // If the system tray bubble currently is too
narrow to provide the
nit: "is currently"

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:312: if (min_width > available_width)
nit: can't you just call bubble_view->SizeToContents()?

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:325: class UserView : public views::View,
nit: separate declaration and definition.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:459: views::Label* email =
nit: you might want to SetElideBehavior for this label to ELIDE_AS_EMAIL.

[bartfab (slow), 2012-11-19 17:33:15]

On 2012/11/16 22:29:32, msw wrote:
> I suggest pushing back on the UI here, it's a lot of complicated impl that's
> difficult to test and prone to very nuanced errors. I suspect that a simple
> "This is a shared user account. [Learn more]" would probably suffice. You have
> the power!

I agree that we need to balance design requirements vs. code complexity. I have
pushed back on some of the design: The "End public session" button height was
supposed to be computed from the label text height, subtracting anything above
the cap height on the first row and below the baseline on the last row. These
are complex font calculations that Views does not support and that I do not
think are that essential here.

The account name and the managing domain name are essential pieces of
information though. The mocks evolved as the public accounts feature was being
developed. We are preparing for multiple public accounts on the same device -
without a visible account name, it would be impossible to tell which account you
are logged into. We want to make sure users are fully informed about what is
going on - without a visible domain name, it would be unclear who is in charge
of the account, making it impossible for users to make an informed choice about
whether to trust that entity or not.

Some of the code deals with styling the text (gray vs. black color). This
enhances readability a lot. Most of the complexity is simply down to having text
followed by an inline link flow together over multiple lines. This is something
that neither Label nor RenderText can do natively. Thus, the complexity of
custom layouting and rendering cannot be avoided here. Once RenderText supports
multiline text, it should be possible to simplify the custom code.

> 
> https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/ash_strings.grd
> File ash/ash_strings.grd (right):
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/ash_strings.gr...
> ash/ash_strings.grd:238: <message name="IDS_ASH_STATUS_TRAY_PUBLIC_LEARN_MORE"
> desc="Text of the Learn more link shown in the ash system bubble's user card
> when the current session is a public account session.">
> nit: "Learn more" is a common string. Unfortunately all the other uses are in
> chrome/app/...grd[p]. It might be worthwhile to anticipate other users and
make
> this IDS_ASH_LEARN_MORE or move it to ui/base/strings/ui_strings.grd and
> potentially use this instead of other IDS_*LEARN_MORE.
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> File ash/system/user/tray_user.cc (right):
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:129: class PublicAccountUserDetails : public
> views::View,
> nit: separate declaration and definition.
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:175: if (contents_area.IsEmpty())
> nit: does this happen?
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:180: ui::ElideRectangleText(text_, font_,
> contents_area.width(),
> Does ElideRectangleText do the right thing if the UI directionality (say LTR
for
> the "is a shared account...") differs from the domain/user names (which could
be
> RTL)? Does your text styling also get this right? (I think that probably
should
> be okay, as it's all done in logical character indicies).
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:187: it != lines.end(); ++it) {
> nit: indent one more space (just my preference to wrap within parens here,
> etc.).
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:202: // Locate any markers indicating the start
and
> end of the user's display
> nit: you should be able to safely assume that there will only be a single name
> in the block of text, so there will be two markers to find. Use a ui::Range to
> track their locations per line, and a flag to persist the outside-/inside-
> flag-pair state across lines. Then you should only need to apply a single
style
> range if a marker was found or the flag is on.
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:221: while (display_name_markers.size() >= 2) {
> there should only be one non-default style range per line, afaict.
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:232: // Position link after the label text,
> separated by a space. If it does not
> This should be tested for RTL UI; I suspect that it may be incorrect.
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:261: // link do not wrap over more than three
lines
> in total for esthetic reasons.
> What's done for arbitrarily long doman/user names? You can't expect them to
> always fit on three lines if there's a hard maximum width (which we should
have,
> as Steven suggested).
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:265: int min_width = std::max(1,
> link_size.width());
> nit: it should be safe to assume the link width is larger.
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:272: while (min_width < max_width) {
> I fear this will be the performance bottleneck for showing the system tray
menu,
> although I lack data to substantiate that concern and don't have great
> alternative suggestions beyond picking a reasonable width and letting the
height
> increase while potentially eliding/truncating obscenely long user/domain
names.
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:274: std::vector<string16> lines;
> nit: declare |lines| outside this loop and use it below too (line 297).
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:279: space_width + link_size.width());
> nit: make this a ternary * ? 0 : 1; to explicitly specify the value added.
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:292: const int available_width =
> The initial min_width value above (line 265) should be the maximum of this
> allocated width and the width of "Learn More".
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:300: const int line_count = lines.size() +
> ditto nit: make this a ternary * ? 0 : 1; to explicitly specify the value
added.
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:310: // If the system tray bubble currently is
too
> narrow to provide the
> nit: "is currently"
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:312: if (min_width > available_width)
> nit: can't you just call bubble_view->SizeToContents()?
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:325: class UserView : public views::View,
> nit: separate declaration and definition.
> 
>
https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
> ash/system/user/tray_user.cc:459: views::Label* email =
> nit: you might want to SetElideBehavior for this label to ELIDE_AS_EMAIL.

[bartfab (slow), 2012-11-19 17:34:06]

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/ash_strings.grd
File ash/ash_strings.grd (right):

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/ash_strings.gr...
ash/ash_strings.grd:238: <message name="IDS_ASH_STATUS_TRAY_PUBLIC_LEARN_MORE"
desc="Text of the Learn more link shown in the ash system bubble's user card
when the current session is a public account session.">
I renamed the string to IDS_ASH_LEARN_MORE. I agree that consolidating it with
all the other "Learn more" strings in Chrome would be even better but there are
so many of them that it would be a huge undertaking - one better left for a
separate clean-up CL.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:129: class PublicAccountUserDetails : public
views::View,
Done, although I was just following the style used in this file already.

On 2012/11/16 22:29:32, msw wrote:
> nit: separate declaration and definition.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:175: if (contents_area.IsEmpty())
Yes, it happens during bubble construction:

CreateBubbleWidget() ->
 Widget::Init() ->
  Widget::SetContentsView() ->
   RootView::SetContentsView() ->
    forces a Layout() call without any bounds set yet

On 2012/11/16 22:29:32, msw wrote:
> nit: does this happen?

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:180: ui::ElideRectangleText(text_, font_,
contents_area.width(),
Yes, I verified that all conceivable LTR/RTL combinations work correctly,
including corner cases such as an RTL UI, an LTR user name with a line break
occurring inside the LTR bit.

On 2012/11/16 22:29:32, msw wrote:
> Does ElideRectangleText do the right thing if the UI directionality (say LTR
for
> the "is a shared account...") differs from the domain/user names (which could
be
> RTL)? Does your text styling also get this right? (I think that probably
should
> be okay, as it's all done in logical character indicies).

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:187: it != lines.end(); ++it) {
Yes, this is my preference as well. The missing space was an oversight. Thanks
for pointing it out.

On 2012/11/16 22:29:32, msw wrote:
> nit: indent one more space (just my preference to wrap within parens here,
> etc.).

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:202: // Locate any markers indicating the start and
end of the user's display
On 2012/11/16 22:29:32, msw wrote:
> nit: you should be able to safely assume that there will only be a single name
> in the block of text, so there will be two markers to find. Use a ui::Range to
> track their locations per line, and a flag to persist the outside-/inside-
> flag-pair state across lines. Then you should only need to apply a single
style
> range if a marker was found or the flag is on.

Done.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:214: display_name_markers.push(0);
I reworked the code as per Mike's suggestion. I am using a ui::Range now to
track the markers on a line and also to persist the
inside-display-name/outside-display-name state across lines.

On 2012/11/16 21:13:52, stevenjb (chromium) wrote:
> This is confusing. It looks like this means that we may end up with an odd
> number of entries in the queue, so the 0 entry will persist for the next line?
I
> think this might be more clear if we set a bool here then pushed an entry at 0
> into the queue at the beginning of this section if the bool is true. At
minimum
> the comment needs to be more clear.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:221: while (display_name_markers.size() >= 2) {
Correct. I simplified the code, removing the loop.

On 2012/11/16 22:29:32, msw wrote:
> there should only be one non-default style range per line, afaict.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:232: // Position link after the label text,
separated by a space. If it does not
I heavily tested this for RTL and did not encounter any bugs.

On 2012/11/16 22:29:32, msw wrote:
> This should be tested for RTL UI; I suspect that it may be incorrect.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:261: // link do not wrap over more than three lines
in total for esthetic reasons.
On 2012/11/16 21:13:52, stevenjb (chromium) wrote:
> nit: align, aesthetic (more common)

Done.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:261: // link do not wrap over more than three lines
in total for esthetic reasons.
I added a maximum width. The text can now wrap to > 3 lines in extreme cases
(very, very, very long user name) and/or may be elided (very, very, very long
domain name).

On 2012/11/16 22:29:32, msw wrote:
> What's done for arbitrarily long doman/user names? You can't expect them to
> always fit on three lines if there's a hard maximum width (which we should
have,
> as Steven suggested).

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:265: int min_width = std::max(1,
link_size.width());
On 2012/11/16 22:29:32, msw wrote:
> nit: it should be safe to assume the link width is larger.

Done.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:267: space_width + link_size.width();
On 2012/11/16 21:13:52, stevenjb (chromium) wrote:
> We should probably limit max width here to a reasonable percentage of the
screen
> width (or a reasonable fixed size like 400px; on a large display a bubble half
> its width would look odd. The SystemBubble should probably define the maximum
> width. In practice this shouldn't happen of course...

Done.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:272: while (min_width < max_width) {
Since this is a binary search, it only takes a handful of iterations, even if
the search range happens to be very long. I very much want to use this form of
wrapping as a fixed width produces ugly results for many cases I tested,
including the simplest thing that came to my mind first, "Internet kiosk" and
"google.com".

Note also that this entire code runs *only* in public account mode. Regular
users will not be affected by it in any way.

On 2012/11/16 22:29:32, msw wrote:
> I fear this will be the performance bottleneck for showing the system tray
menu,
> although I lack data to substantiate that concern and don't have great
> alternative suggestions beyond picking a reasonable width and letting the
height
> increase while potentially eliding/truncating obscenely long user/domain
names.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:274: std::vector<string16> lines;
On 2012/11/16 22:29:32, msw wrote:
> nit: declare |lines| outside this loop and use it below too (line 297).

Done.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:278: (width - font_.GetStringWidth(lines.back()) <=
On 2012/11/16 21:13:52, stevenjb (chromium) wrote:
> Avoid this (expensive) calculation if truncated?

Done.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:279: space_width + link_size.width());
I reordered the code to avoid this implicit bool-to-int conversion and avoid any
unnecessary computations.

On 2012/11/16 22:29:32, msw wrote:
> nit: make this a ternary * ? 0 : 1; to explicitly specify the value added.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:292: const int available_width =
On 2012/11/16 22:29:32, msw wrote:
> The initial min_width value above (line 265) should be the maximum of this
> allocated width and the width of "Learn More".

Done.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:300: const int line_count = lines.size() +
On 2012/11/16 22:29:32, msw wrote:
> ditto nit: make this a ternary * ? 0 : 1; to explicitly specify the value
added.

Done.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:310: // If the system tray bubble currently is too
narrow to provide the
On 2012/11/16 22:29:32, msw wrote:
> nit: "is currently"

Done.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:312: if (min_width > available_width)
Unfortunately, no:

SizeToContents() takes all child Views' preferred sizes into account. I would
have to sabotage all other childrens' GetPreferredSize() to ensure that they
never request a width larger than the current bubble width (because the bubble
is supposed to expand only for the user card and only in public account mode).

On 2012/11/16 22:29:32, msw wrote:
> nit: can't you just call bubble_view->SizeToContents()?

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:325: class UserView : public views::View,
On 2012/11/16 22:29:32, msw wrote:
> nit: separate declaration and definition. 

Done.

https://chromiumcodereview.appspot.com/11377133/diff/10001/ash/system/user/tr...
ash/system/user/tray_user.cc:459: views::Label* email =
On 2012/11/16 22:29:32, msw wrote:
> nit: you might want to SetElideBehavior for this label to ELIDE_AS_EMAIL.

Done.

[bartfab (slow), 2012-11-19 18:18:05]

https://chromiumcodereview.appspot.com/11377133/diff/11015/ash/system/user/tr...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/11015/ash/system/user/tr...
ash/system/user/tray_user.cc:503:
email->SetElideBehavior(views::Label::ELIDE_AS_EMAIL);
I changed this back because it introduced a rather visible change that I am not
sure is right. Many (most?) e-mail addresses will be too long to fit into the
bubble. I expect the user name part before the @ to be the important bit,
letting you see at a glance e.g. which one of the users at your organization is
logged in. This is what you currently see. With ELIDE_AS_EMAIL, priority is
given to the domain name instead (which would likely be the same for all users
at an organization and @gmail.com for consumers), eliding the user name
instead... I would not want to make that change without a UI designer asking for
it.

[stevenjb, 2012-11-19 18:36:53]

https://chromiumcodereview.appspot.com/11377133/diff/19001/ash/system/user/tr...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/19001/ash/system/user/tr...
ash/system/user/tray_user.cc:221: ReplaceChars(display_name, kDisplayNameMark,
string16(), &display_name);
Wouldn't RemoveChars work here and be mroe clear? Same below.

https://chromiumcodereview.appspot.com/11377133/diff/19001/ash/system/user/tr...
ash/system/user/tray_user.cc:223: display_name.push_back(kDisplayNameMark[0]);
I think the above two lines would be easier to read as:
display_name = kDisplayNameMark + display_name + kDisplayNameMark;

[bartfab (slow), 2012-11-19 18:44:08]

https://chromiumcodereview.appspot.com/11377133/diff/19001/ash/system/user/tr...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/19001/ash/system/user/tr...
ash/system/user/tray_user.cc:221: ReplaceChars(display_name, kDisplayNameMark,
string16(), &display_name);
On 2012/11/19 18:36:53, stevenjb (chromium) wrote:
> Wouldn't RemoveChars work here and be mroe clear? Same below.

Done.

https://chromiumcodereview.appspot.com/11377133/diff/19001/ash/system/user/tr...
ash/system/user/tray_user.cc:223: display_name.push_back(kDisplayNameMark[0]);
On 2012/11/19 18:36:53, stevenjb (chromium) wrote:
> I think the above two lines would be easier to read as:
> display_name = kDisplayNameMark + display_name + kDisplayNameMark;

Done.

[stevenjb, 2012-11-27 01:23:10]

lgtm

[bartfab (slow), 2012-11-28 19:24:50]

Hi Scott,

Review ping for ash/ash_strings.grd and chrome/browser/ui/chrome_pages.*

Hi Mike,

Review ping for ui/views/bubble/tray_bubble_view.*

[sky, 2012-11-28 22:08:03]

LGTM

[msw, 2012-11-29 01:14:10]

If you don't mind, please don't delete the original inline comments from your
inline responses, that makes it harder for me to review.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/tray/te...
File ash/system/tray/test_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/tray/te...
ash/system/tray/test_system_tray_delegate.cc:96: return "";
nit: return std::string();

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/tray/tr...
File ash/system/tray/tray_constants.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/tray/tr...
ash/system/tray/tray_constants.cc:54: const int kTrayNotificationContentsWidth =
kTrayPopupMinWidth -
nit: does newly variable bubble width break this const? consider TODO/followup.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/tray/tr...
ash/system/tray/tray_constants.cc:55: (kNotificationIconWidth +
kTrayPopupPaddingHorizontal) * 2;
nit: you didn't change this, but why does this calculation multiply
|kNotificationIconWidth| by two, whereas the similar |msg_width| calculation in
ash/system/chromeos/network/tray_sms.cc does not? I suspect this is wrong.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:77: const int
kPublicAccountLogoutButtonBorderImagesHovered[] = {
nit: add a blank line above.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:104: // Set the image that should be displayed from
a pointer. The pointer
nit: update comment to remove inaccurate mention of pointer.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:131: // Overridden from views::LinkListener.
nit: Match LinkListener/View OVERRIDE and base class list ordering.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:136: virtual gfx::Size GetPreferredSize() OVERRIDE
{ return preferred_size_; };
The style guide strongly discourages inline virtual function definitions,
otherwise there shouldn't be a semicolon trailing the definition.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:219:
font_(ResourceBundle::GetSharedInstance().GetFont(
nit: You should be able to omit this, as the default gfx::Font ctor should be
sufficient. In fact, you should be able to nix |font_| altogether and just use
Font() and function locals as needed.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:308: // Position link after the label text,
separated by a space. If it does not
nit: "the link"

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:333: views::View::OnPaint(canvas);
nit: call this first, to avoid anything painting over the text.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:338: 
nit: remove blank line

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:395: CHECK(login != ash::user::LOGGED_IN_NONE);
nit: CHECK_NE

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:410: // Make sure the default user default view
item is at least as tall as the
nit: Is "default user default view item" correct?

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:440: DCHECK(sender == logout_button_);
nit: DCHECK_EQ

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:445: // The logout button must be added before the
user card.
Why? Also, this is trivially enforced by the order of the calls in the ctor, so
it's probably not worth checking.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:453: TrayPopupLabelButton* logout_button = new
TrayPopupLabelButton(
nit: just assign to |logout_button_| directly.

[bartfab (slow), 2012-11-29 16:28:58]

Sorry if I managed to delete the quote when replying to a comment somewhere -
this was not intentional.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/tray/te...
File ash/system/tray/test_system_tray_delegate.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/tray/te...
ash/system/tray/test_system_tray_delegate.cc:96: return "";
On 2012/11/29 01:14:10, msw wrote:
> nit: return std::string();

Done.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/tray/tr...
File ash/system/tray/tray_constants.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/tray/tr...
ash/system/tray/tray_constants.cc:54: const int kTrayNotificationContentsWidth =
kTrayPopupMinWidth -
On 2012/11/29 01:14:10, msw wrote:
> nit: does newly variable bubble width break this const? consider
TODO/followup.

No, the new variable bubble width does not break this const. The variable width
is used in the main system tray bubble only and this const is for notification
bubbles.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/tray/tr...
ash/system/tray/tray_constants.cc:55: (kNotificationIconWidth +
kTrayPopupPaddingHorizontal) * 2;
On 2012/11/29 01:14:10, msw wrote:
> nit: you didn't change this, but why does this calculation multiply
> |kNotificationIconWidth| by two, whereas the similar |msg_width| calculation
in
> ash/system/chromeos/network/tray_sms.cc does not? I suspect this is wrong.

From my reading of TrayNotificationView::InitView(), something is definitely off
here. A notification view has a grid layout with the following column widths:

* kTrayPopupPaddingHorizontal / 2
* kNotificationIconWidth
* kTrayPopupPaddingHorizontal / 2
* kTrayNotificationContentsWidth
* kTrayPopupPaddingHorizontal / 2
* kNotificationButtonWidth

There are a number of things wrong:

1) There is no padding column after the close button
2) As you pointed out and I actually noticed myself before, the icon width is
counted twice in |kTrayNotificationContentsWidth|
3) The width of the close button is not counted at all in
|kTrayNotificationContentsWidth|.

Since I did not look into this code in more detail, I do not want to just change
the constant and hope for the best. Should I add a TODO(msw) to look into this?

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:77: const int
kPublicAccountLogoutButtonBorderImagesHovered[] = {
On 2012/11/29 01:14:10, msw wrote:
> nit: add a blank line above.

Done.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:104: // Set the image that should be displayed from
a pointer. The pointer
On 2012/11/29 01:14:10, msw wrote:
> nit: update comment to remove inaccurate mention of pointer.

Done.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:131: // Overridden from views::LinkListener.
On 2012/11/29 01:14:10, msw wrote:
> nit: Match LinkListener/View OVERRIDE and base class list ordering.

Done.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:136: virtual gfx::Size GetPreferredSize() OVERRIDE
{ return preferred_size_; };
On 2012/11/29 01:14:10, msw wrote:
> The style guide strongly discourages inline virtual function definitions,
> otherwise there shouldn't be a semicolon trailing the definition.

Done.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:219:
font_(ResourceBundle::GetSharedInstance().GetFont(
On 2012/11/29 01:14:10, msw wrote:
> nit: You should be able to omit this, as the default gfx::Font ctor should be
> sufficient. In fact, you should be able to nix |font_| altogether and just use
> Font() and function locals as needed.

Done.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:308: // Position link after the label text,
separated by a space. If it does not
On 2012/11/29 01:14:10, msw wrote:
> nit: "the link"

Done.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:333: views::View::OnPaint(canvas);
On 2012/11/29 01:14:10, msw wrote:
> nit: call this first, to avoid anything painting over the text.

This is actually on purpose. In line 318, the link is offset so that its border
impinges onto the text line above it by 1px. When the link is focused and its
border shows, the border should be painted over the text. This is necessary
because, due to its border, a link is 1px taller than regular text and its
baseline would be off by 1px otherwise.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:338: 
On 2012/11/29 01:14:10, msw wrote:
> nit: remove blank line

Done.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:395: CHECK(login != ash::user::LOGGED_IN_NONE);
On 2012/11/29 01:14:10, msw wrote:
> nit: CHECK_NE

Done.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:410: // Make sure the default user default view
item is at least as tall as the
On 2012/11/29 01:14:10, msw wrote:
> nit: Is "default user default view item" correct?

Typo. Thanks for catching it.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:440: DCHECK(sender == logout_button_);
On 2012/11/29 01:14:10, msw wrote:
> nit: DCHECK_EQ

Done.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:445: // The logout button must be added before the
user card.
On 2012/11/29 01:14:10, msw wrote:
> Why? Also, this is trivially enforced by the order of the calls in the ctor,
so
> it's probably not worth checking.

The reason is that for the user card to calculate available space, it must be
able to query the button's width. I had the DCHECK() in there in case someone in
the future refactors the constructor and swaps around the order. I removed the
DCHECK() now and added a comment in the constructor instead.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/user/tr...
ash/system/user/tray_user.cc:453: TrayPopupLabelButton* logout_button = new
TrayPopupLabelButton(
On 2012/11/29 01:14:10, msw wrote:
> nit: just assign to |logout_button_| directly.

Done.

[msw, 2012-11-29 18:37:46]

LGTM with nits. Tests for the text layout would be nice, as I still think it's
prone to subtle issues with long and mixed directional strings, but I don't want
to hold you up more than necessary.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/tray/tr...
File ash/system/tray/tray_constants.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/tray/tr...
ash/system/tray/tray_constants.cc:55: (kNotificationIconWidth +
kTrayPopupPaddingHorizontal) * 2;
On 2012/11/29 16:28:59, bartfab wrote:
> On 2012/11/29 01:14:10, msw wrote:
> > nit: you didn't change this, but why does this calculation multiply
> > |kNotificationIconWidth| by two, whereas the similar |msg_width| calculation
> in
> > ash/system/chromeos/network/tray_sms.cc does not? I suspect this is wrong.
> 
> From my reading of TrayNotificationView::InitView(), something is definitely
off
> here. A notification view has a grid layout with the following column widths:
> 
> * kTrayPopupPaddingHorizontal / 2
> * kNotificationIconWidth
> * kTrayPopupPaddingHorizontal / 2
> * kTrayNotificationContentsWidth
> * kTrayPopupPaddingHorizontal / 2
> * kNotificationButtonWidth
> 
> There are a number of things wrong:
> 
> 1) There is no padding column after the close button
> 2) As you pointed out and I actually noticed myself before, the icon width is
> counted twice in |kTrayNotificationContentsWidth|
> 3) The width of the close button is not counted at all in
> |kTrayNotificationContentsWidth|.
> 
> Since I did not look into this code in more detail, I do not want to just
change
> the constant and hope for the best. Should I add a TODO(msw) to look into
this?

You can add a TODO and file an issue for the relevant author, which is not me.

https://chromiumcodereview.appspot.com/11377133/diff/23006/ash/system/user/tr...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/23006/ash/system/user/tr...
ash/system/user/tray_user.cc:218: : learn_more_(NULL) {
nit: outdent two spaces.

[bartfab (slow), 2012-11-29 19:15:01]

Ah yes, the external struggle between feature work and proper testing.

I need to land this CL quickly, so with your blessing, I will go ahead and land
it as-is. But I will take a TODO to write some tests for the custom wrapping
code.

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/tray/tr...
File ash/system/tray/tray_constants.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/12021/ash/system/tray/tr...
ash/system/tray/tray_constants.cc:55: (kNotificationIconWidth +
kTrayPopupPaddingHorizontal) * 2;
On 2012/11/29 18:37:46, msw wrote:
> On 2012/11/29 16:28:59, bartfab wrote:
> > On 2012/11/29 01:14:10, msw wrote:
> > > nit: you didn't change this, but why does this calculation multiply
> > > |kNotificationIconWidth| by two, whereas the similar |msg_width|
calculation
> > in
> > > ash/system/chromeos/network/tray_sms.cc does not? I suspect this is wrong.
> > 
> > From my reading of TrayNotificationView::InitView(), something is definitely
> off
> > here. A notification view has a grid layout with the following column
widths:
> > 
> > * kTrayPopupPaddingHorizontal / 2
> > * kNotificationIconWidth
> > * kTrayPopupPaddingHorizontal / 2
> > * kTrayNotificationContentsWidth
> > * kTrayPopupPaddingHorizontal / 2
> > * kNotificationButtonWidth
> > 
> > There are a number of things wrong:
> > 
> > 1) There is no padding column after the close button
> > 2) As you pointed out and I actually noticed myself before, the icon width
is
> > counted twice in |kTrayNotificationContentsWidth|
> > 3) The width of the close button is not counted at all in
> > |kTrayNotificationContentsWidth|.
> > 
> > Since I did not look into this code in more detail, I do not want to just
> change
> > the constant and hope for the best. Should I add a TODO(msw) to look into
> this?
> 
> You can add a TODO and file an issue for the relevant author, which is not me.

Done.

https://chromiumcodereview.appspot.com/11377133/diff/23006/ash/system/user/tr...
File ash/system/user/tray_user.cc (right):

https://chromiumcodereview.appspot.com/11377133/diff/23006/ash/system/user/tr...
ash/system/user/tray_user.cc:218: : learn_more_(NULL) {
On 2012/11/29 18:37:47, msw wrote:
> nit: outdent two spaces.

Done.

[commit-bot: I haz the power, 2012-11-29 19:17:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11377133/13008

[commit-bot: I haz the power, 2012-11-29 19:17:44]

Failed to apply patch for ash/system/tray/test_system_tray_delegate.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ash/system/tray/test_system_tray_delegate.h
  Hunk #2 FAILED at 42.
  1 out of 2 hunks FAILED -- saving rejects to file
ash/system/tray/test_system_tray_delegate.h.rej

Patch:       ash/system/tray/test_system_tray_delegate.h
Index: ash/system/tray/test_system_tray_delegate.h
diff --git a/ash/system/tray/test_system_tray_delegate.h
b/ash/system/tray/test_system_tray_delegate.h
index
a49b3076e3e45f904346e4ae0ea5a190bd0334a6..a3f9920cde5671f791cc8116fc4f0b5a65bb1789
100644
--- a/ash/system/tray/test_system_tray_delegate.h
+++ b/ash/system/tray/test_system_tray_delegate.h
@@ -29,6 +29,7 @@ class TestSystemTrayDelegate : public SystemTrayDelegate {
   virtual const std::string GetUserEmail() const OVERRIDE;
   virtual const gfx::ImageSkia& GetUserImage() const OVERRIDE;
   virtual user::LoginStatus GetUserLoginStatus() const OVERRIDE;
+  virtual const std::string GetEnterpriseDomain() const OVERRIDE;
   virtual bool SystemShouldUpgrade() const OVERRIDE;
   virtual base::HourClockType GetHourClockType() const OVERRIDE;
   virtual PowerSupplyStatus GetPowerSupplyStatus() const OVERRIDE;
@@ -41,6 +42,7 @@ class TestSystemTrayDelegate : public SystemTrayDelegate {
   virtual void ShowDriveSettings() OVERRIDE;
   virtual void ShowIMESettings() OVERRIDE;
   virtual void ShowHelp() OVERRIDE;
+  virtual void ShowPublicAccountInfo() OVERRIDE;
   virtual void ShutDown() OVERRIDE;
   virtual void SignOut() OVERRIDE;
   virtual void RequestLockScreen() OVERRIDE;

[commit-bot: I haz the power, 2012-11-29 19:34:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11377133/10028

[commit-bot: I haz the power, 2012-11-29 20:18:05]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-29 22:38:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11377133/10028

[commit-bot: I haz the power, 2012-11-29 23:15:13]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_aura&n...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-30 09:32:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11377133/10030

[commit-bot: I haz the power, 2012-12-03 09:20:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11377133/7009

[commit-bot: I haz the power, 2012-12-03 11:41:35]

Retried try job too often on win_rel for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2012-12-03 11:50:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11377133/7009

[commit-bot: I haz the power, 2012-12-03 12:27:31]

Retried try job too often on linux_rel for step(s) ash_unittests

[commit-bot: I haz the power, 2012-12-03 13:35:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11377133/7009

[commit-bot: I haz the power, 2012-12-03 14:12:43]

Retried try job too often on linux_rel for step(s) ash_unittests

[commit-bot: I haz the power, 2012-12-03 14:58:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11377133/7009

[commit-bot: I haz the power, 2012-12-03 15:46:52]

Retried try job too often on linux_rel for step(s) ash_unittests

[commit-bot: I haz the power, 2012-12-03 16:16:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11377133/24019

[commit-bot: I haz the power, 2012-12-03 17:39:18]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-03 17:54:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11377133/24019

[commit-bot: I haz the power, 2012-12-03 18:49:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartfab@chromium.org/11377133/24019

[commit-bot: I haz the power, 2012-12-03 20:31:10]

Change committed as 170802