Elide long emails in the wrench and profile menus.

Elide long emails in the wrench and profile menus.

NOTE: This CL now only adds eliding functionality, but doesn't use it in the UI just yet.

Refer to bug 113133 for details (refer to comment 22 for a screenshot of the result: http://code.google.com/p/chromium/issues/detail?id=113133#c22).

BUG=113133
TEST=Make sure ui_unittests (ElideEmail and ElideEmailMoreSpace) pass.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126445

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127175

[gab, 2012-02-28 15:08:16]

PTAL

[gab, 2012-02-28 15:38:30]

Need OWNERS approval.
sail: chrome/browser/profiles/*
ben: chrome/browser/ui/*, ui/base/*

jeffreyc: Can you take a look at the changes to
chrome/app/generated_resources.grd

Thanks all,
Gab

[Alexei Svitkine (slow), 2012-02-28 15:46:22]

https://chromiumcodereview.appspot.com/9489011/diff/4001/ui/base/text/text_el...
File ui/base/text/text_elider.cc (right):

https://chromiumcodereview.appspot.com/9489011/diff/4001/ui/base/text/text_el...
ui/base/text/text_elider.cc:164: // This function assumes max_email_length is >=
5.
It is sufficient to only include the comment in the header declaration of the
function.

https://chromiumcodereview.appspot.com/9489011/diff/4001/ui/base/text/text_el...
ui/base/text/text_elider.cc:173: DCHECK_EQ(email_split.size(), size_t(2));
We don't use constructor-style casts in the Chromium codebase. Generally, this
would be static_cast<size_t>(2), but in this case you can use 2U to indicate
that it's unsigned.

https://chromiumcodereview.appspot.com/9489011/diff/4001/ui/base/text/text_el...
ui/base/text/text_elider.cc:175: string16& domain = email_split.back();
Non-const references are discouraged - remove the &'s here.

https://chromiumcodereview.appspot.com/9489011/diff/4001/ui/base/text/text_el...
ui/base/text/text_elider.cc:182: const size_t kAvailableDomainChars =
max_email_length - kEllipsisLength - 2;
Rename to |available_domain_chars| since this is a computed constant that
depends on function parameters.

https://chromiumcodereview.appspot.com/9489011/diff/4001/ui/base/text/text_el...
ui/base/text/text_elider.cc:185: const size_t n = max_email_length / 2 -
kEllipsisLength;
Nit: Rename |n| to |length|. Same thing in the block below.

https://chromiumcodereview.appspot.com/9489011/diff/4001/ui/base/text/text_el...
ui/base/text/text_elider.cc:189: const size_t kFinalDomainLength =
domain.length();
Same comment as for kAvailableDomainChars above. Although, in this case, I
suggest just using |domain.length()| directly below, since it's actually
shorter.

https://chromiumcodereview.appspot.com/9489011/diff/4001/ui/base/text/text_el...
File ui/base/text/text_elider.h (right):

https://chromiumcodereview.appspot.com/9489011/diff/4001/ui/base/text/text_el...
ui/base/text/text_elider.h:27: UI_EXPORT extern const size_t
kMaxProfileUsernameLength;
This is the wrong place for such a constant. ui/base/... shouldn't know anything
about profiles.

https://chromiumcodereview.appspot.com/9489011/diff/4001/ui/base/text/text_el...
ui/base/text/text_elider.h:48: // overall email is at most max_email_length
characters (e.g.
Please variables parameter names in comments with |'s. i.e. |max_email_length|

https://chromiumcodereview.appspot.com/9489011/diff/4001/ui/base/text/text_el...
ui/base/text/text_elider.h:51: // we elide the domain name in the middle so that
only max_email_length / 2
Nit: Please rephrase in a way that avoids using "we". For example:

If the domain name already has more characters than available, it is elided in
the middle ...

https://chromiumcodereview.appspot.com/9489011/diff/4001/ui/base/text/text_el...
File ui/base/text/text_elider_unittest.cc (right):

https://chromiumcodereview.appspot.com/9489011/diff/4001/ui/base/text/text_el...
ui/base/text/text_elider_unittest.cc:56: void RunEmailTest(Testcase* testcases,
size_t num_testcases,
Nit: Put "num_testcases" on its own line.

https://chromiumcodereview.appspot.com/9489011/diff/4001/ui/base/text/text_el...
ui/base/text/text_elider_unittest.cc:78: {"short@small.com", "s" + kEllipsisStr
+ "@s" +kEllipsisStr},
Add a space between + and kEllipsisStr.

https://chromiumcodereview.appspot.com/9489011/diff/4001/ui/base/text/text_el...
ui/base/text/text_elider_unittest.cc:87: // all expected values in this test
assume the UTF16 ellipsis is a single char
Comments should be complete sentences: starting with a capital and ending with a
period.

[Alexei Svitkine (slow), 2012-02-28 15:50:51]

> Please variables parameter names in comments with |'s. i.e.
> |max_email_length|

Sorry, this should have read:

Please *surround* variable names in comments with ...

[sail, 2012-02-28 18:37:19]

+rsesek since he wrote AvatarMenuModel

http://codereview.chromium.org/9489011/diff/4001/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/9489011/diff/4001/ui/base/text/text_elider.h#n...
ui/base/text/text_elider.h:59: size_t max_email_length);
Eliding using character length is incorrect. Eliding should be based on pixel
width instead. See: 
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/cocoa/boo...

If you make this change then I think you should move the eliding out of
AvatarMenuModel and into the various platform specific functions.

[Robert Sesek, 2012-02-28 18:55:06]

On 2012/02/28 18:37:19, sail wrote:
> +rsesek since he wrote AvatarMenuModel
> 
> http://codereview.chromium.org/9489011/diff/4001/ui/base/text/text_elider.h
> File ui/base/text/text_elider.h (right):
> 
>
http://codereview.chromium.org/9489011/diff/4001/ui/base/text/text_elider.h#n...
> ui/base/text/text_elider.h:59: size_t max_email_length);
> Eliding using character length is incorrect. Eliding should be based on pixel
> width instead. See: 
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/cocoa/boo...
> 
> If you make this change then I think you should move the eliding out of
> AvatarMenuModel and into the various platform specific functions.

I agree with Sailesh that this isn't something that belongs in the model, which
is the canonical source for data. Eliding is done something at the
view/controller layer.

[gab, 2012-02-28 19:20:11]

Fixed most of the points addressed by Alexei (see replies to inline comments)

http://codereview.chromium.org/9489011/diff/4001/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/9489011/diff/4001/ui/base/text/text_elider.cc#...
ui/base/text/text_elider.cc:164: // This function assumes max_email_length is >=
5.
Ok, was mimicking ElideUrl which has the comment in both places (should I
refactor that at the same time?)

On 2012/02/28 15:46:22, Alexei Svitkine wrote:
> It is sufficient to only include the comment in the header declaration of the
> function.

http://codereview.chromium.org/9489011/diff/4001/ui/base/text/text_elider.cc#...
ui/base/text/text_elider.cc:175: string16& domain = email_split.back();
Ok, but this will force an implicit copy of the string which is unnecessary, are
we ok with that?

On 2012/02/28 15:46:22, Alexei Svitkine wrote:
> Non-const references are discouraged - remove the &'s here.

http://codereview.chromium.org/9489011/diff/4001/ui/base/text/text_elider.cc#...
ui/base/text/text_elider.cc:189: const size_t kFinalDomainLength =
domain.length();
|domain.length()| is shorter, but is a function (hence why I made a variable) as
the value is potentially needed twice; I'll only rename it, let me know if you
want me to proceed otherwise.

On 2012/02/28 15:46:22, Alexei Svitkine wrote:
> Same comment as for kAvailableDomainChars above. Although, in this case, I
> suggest just using |domain.length()| directly below, since it's actually
> shorter.

http://codereview.chromium.org/9489011/diff/4001/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/9489011/diff/4001/ui/base/text/text_elider.h#n...
ui/base/text/text_elider.h:27: UI_EXPORT extern const size_t
kMaxProfileUsernameLength;
Right, I didn't know where to put this as the reverse is also true: profiles
shouldn't know anything about ui.

Any suggestions on where to put it?

On 2012/02/28 15:46:22, Alexei Svitkine wrote:
> This is the wrong place for such a constant. ui/base/... shouldn't know
anything
> about profiles.

[gab, 2012-02-28 19:20:36]

The reason I did it based on # characters is that the menu already stretches to
take the full width of the string (email) we put in it. As it is now it is ugly
for very long emails so we are only trying to get rid of this problem by
deciding on an arbitrary 20 characters max length (as suggested by Glen here:
http://code.google.com/p/chromium/issues/detail?id=113133#c14)

Doing it by pixel width only makes sense in my opinion if we decide on a maximum
width for each menu (and we could then determine in the views how much space we
have left for the string and elide based on the font). Since the menus are
currently stretching as wide as their content, eliding on width could only be
done based on an arbitrary width (say 20*font.GetAverageCharacterWidth(), but
this is redundant...).

I agree the eliding call itself could be done outside of the model, but this
would duplicate code and that duplication is unnecessary when the eliding is
solely based on the number of characters. The model is placing the strings in
the menu model, so unless we need font information, it makes sense to me to
elide the string (which we want displayed equally in each view) before inserting
it in the model.

[gab, 2012-02-28 19:23:01]

http://codereview.chromium.org/9489011/diff/10001/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/9489011/diff/10001/ui/base/text/text_elider.h#...
ui/base/text/text_elider.h:42: size_t max_email_length);
I moved this here as ElideEmail is above ElideUrl in the .cc file

[sail, 2012-02-28 19:30:28]

> eliding on width could only be
> done based on an arbitrary width (say 20*font.GetAverageCharacterWidth(), but
> this is redundant...).

This is not true. Eliding to a fixed pixel width (say 300) is better than
character count. Here's an example, say you change the system font size to 30.
Your elided email address won't fit the wrench menu and will be elided a second
time (which is incorrect).

Also, the wrench menu already has a maximum pixel width, see:
http://code.google.com/codesearch#OAMlx_jo-ck/src/ui/views/controls/menu/menu...

[gab, 2012-02-28 20:20:18]

> This is not true. Eliding to a fixed pixel width (say 300) is better than
> character count. Here's an example, say you change the system font size to 30.
> Your elided email address won't fit the wrench menu and will be elided a
second
> time (which is incorrect).

Ok, but we still want the UI to look nice under "regular" font size right?

I did a quick estimate that currently with normal size font on Ubuntu, the 20
characters elided email is about 170 pixels in the wrench menu and 140 pixels in
the avatar menu.

Should we take those numbers and declare it a fixed width? People with font 30,
might get something like "Signed in as ...@..." (which might also still be too
big in the limit and require extra eliding). 

The problem is we are eliding each side of an email (we want to keep the @ sign)
within a longer string, I don't think we can prevent extra eliding of the end
result in extreme cases.

[sail, 2012-02-28 20:30:04]

On 2012/02/28 20:20:18, gab wrote:
> > This is not true. Eliding to a fixed pixel width (say 300) is better than
> > character count. Here's an example, say you change the system font size to
30.
> > Your elided email address won't fit the wrench menu and will be elided a
> second
> > time (which is incorrect).
> 
> Ok, but we still want the UI to look nice under "regular" font size right?
> 
> I did a quick estimate that currently with normal size font on Ubuntu, the 20
> characters elided email is about 170 pixels in the wrench menu and 140 pixels
in
> the avatar menu.
> 
> Should we take those numbers and declare it a fixed width? People with font
30,
> might get something like "Signed in as ...@..." (which might also still be too
> big in the limit and require extra eliding). 
> 
> The problem is we are eliding each side of an email (we want to keep the @
sign)
> within a longer string, I don't think we can prevent extra eliding of the end
> result in extreme cases.

Ideally we would elide the full menu title (Signed in as ..). On my Mac 300
pixels seems like a good maximum for the full menu title.

Also, if you get to a point where the email address looks weird you can always
fallback to normal string eliding.

[gab, 2012-02-28 21:54:24]

> 
> Ideally we would elide the full menu title (Signed in as ..). On my Mac 300
> pixels seems like a good maximum for the full menu title.

Ok I will do that.

> Also, if you get to a point where the email address looks weird you can always
> fallback to normal string eliding.

Definitely.


FYI, I found some current UI code which uses character base eliding:
http://code.google.com/codesearch#search/&exact_package=chromium&q=ElideStrin...

[gab, 2012-02-28 21:55:52]

> FYI, I found some current UI code which uses character base eliding:
>
http://code.google.com/codesearch#search/&exact_package=chromium&q=ElideStrin...

Should we open a bug for this? or is it acceptable in some scenarios?

[sail, 2012-02-28 21:59:40]

On 2012/02/28 21:55:52, gab wrote:
> > FYI, I found some current UI code which uses character base eliding:
> >
>
http://code.google.com/codesearch#search/&exact_package=chromium&q=ElideStrin...
> 
> Should we open a bug for this? or is it acceptable in some scenarios?

All those look wrong. I'm not sure filing bugs would be useful though.

[gab, 2012-03-01 19:37:25]

PTAL

@sail: no longer need OWNER approval in chrome/browser/profiles/*
@ben: still need OWNER approval in ui/base/*, but not in chrome/browser/ui/*

--------------------------------

Rewrote the eliding code to elide based on pixel width, not character count.

Removed eliding code intrusion in the model (another CL will follow to actually
use the new eliding code in the UI code; as it it now, nothing was changed in
the UI).

Refactored comments in ui/base/test/text_elider.[cc|h] (removed method comments
from the .cc and detailed some comments in the .h file as some had diverged from
their actual implementation).

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:162: string16 domain = email_split.back();
@asvitkine: I remember you said you prefer string16 to string16& here, but that
forces an unnecessary string copy. Are we okay with that tiny performance loss?

[asvitkine_google, 2012-03-01 20:13:29]

Some initial comments. I'll give you more once you address these.

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:162: string16 domain = email_split.back();
On 2012/03/01 19:37:25, gab wrote:
> @asvitkine: I remember you said you prefer string16 to string16& here, but
that
> forces an unnecessary string copy. Are we okay with that tiny performance
loss?

This is fine. The run time will be dominated by calls to GetStringWidth(),
rather than the cost of copying a few dozen characters.

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:165: const int kEllipsisSize =
font.GetStringWidth(kEllipsisUTF16);
Please rename all the variables that end in Size or _size to use "width"
instead, so that it's clearer that they refer to string widths.

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:172: // one character remaining in the username.
I would comment this slightly differently - have the comment mention why it's
doing this, i.e. to explain you're checking whether the domain name needs to be
elided, which is true only if it doesn't fit when the username is fully elided.

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:175: font.GetStringWidth(username.substr(0, 1));
Please use font.GetStringWidth(username.substr(0, 1) + kEllipsisUTF16), since
it's not guaranteed that it equals the sum of the individual widths.

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:179: std::max(available_pixel_width -
full_username_size,
Please comment the logic behind this.

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider.h#...
ui/base/text/text_elider.h:29: // returned email is at most
|available_pixel_width| wide under |font|.
I would suggest having the first sentence describe the overall behavior of the
function. "Elides only the username..." is not the overall behavior - since it
can also elide the domain.

So please change the first sentence to something like "Elides an email address
to fit into |available_pixel_width| using the specified |font|." Then, you can
go into more details about how it's done. (That it prioritizes eliding the
username first before trying to elide the domain name...)

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider.h#...
ui/base/text/text_elider.h:70: // |text| and adds an ellipsis where specified by
|elide_behavior|.
I like the previous wording better. If you want, you can still add the second
clarification sentence, but please don't use "we".

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider_un...
File ui/base/text/text_elider_unittest.cc (right):

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider_un...
ui/base/text/text_elider_unittest.cc:61: Testcase testcases[] = {
Please add some tests that check no eliding takes place if available width is >
than email width.

[gab, 2012-03-01 21:48:17]

Addressed all of asvitkine's comments on patch set 6.

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:179: std::max(available_pixel_width -
full_username_size,
On 2012/03/01 20:13:29, asvitkine wrote:
> Please comment the logic behind this.

I was trying to be less verbose by leaving all the description in the method
header comment, this addresses this part of the method spec: "(should the
username be short enough that it doesn't require half the available width: the
domain will take that extra width)."

I added a comment inline that explains this in more details. Let me know what
you prefer.

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/9489011/diff/19004/ui/base/text/text_elider.h#...
ui/base/text/text_elider.h:70: // |text| and adds an ellipsis where specified by
|elide_behavior|.
On 2012/03/01 20:13:29, asvitkine wrote:
> I like the previous wording better. If you want, you can still add the second
> clarification sentence, but please don't use "we".

Right, one of the versions of the code I tried yesterday needed to assume that
an empty string was returned, so I added this to the spec to make sure it wasn't
just a side-effect of the current implementation.

Double-checking now, this version doesn't need to make this assumption anymore;
hence I removed this change.

[asvitkine_google, 2012-03-01 22:10:56]

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:166: // Ignore the @ symbol and add it back at the
end.
Nit: Can you reword this better? The second part of the comment isn't relevant
here since this deals with measurement of the widths rather than the
construction of the final string.

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:179: // let the domain take that extra space.
If the username is short, shouldn't you elide the domain to the maximum width it
can take in that case?

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:188: const int final_domain_width =
font.GetStringWidth(domain);
Nit: How about:

available_pixel_width -= font.GetStringWidth(domain);

and then not needing to use final_domain_width below.

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider.h#...
ui/base/text/text_elider.h:31: // character, other than the ellipsis-es, on
either side of the '@'. If it is
ellipsis-es => ellipses

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider_un...
File ui/base/text/text_elider_unittest.cc (right):

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider_un...
ui/base/text/text_elider_unittest.cc:84: const size_t num_testcases =
arraysize(testcases);
Inline this into the loop.

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider_un...
ui/base/text/text_elider_unittest.cc:109: const size_t num_test_emails =
arraysize(test_emails);
Don't need these as separate variables, just use arraysize() directly. (It's
basically a constant once the compiler is done with it).

[gab, 2012-03-01 23:03:46]

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:179: // let the domain take that extra space.
On 2012/03/01 22:10:56, asvitkine wrote:
> If the username is short, shouldn't you elide the domain to the maximum width
it
> can take in that case?

Yes, this is exactly what this does. I reworded the comment to make this more
obvious, let me know if it's better.

Verbosely: if the domain must be elided: give half the width to the domain and
the other half to the username. If the username doesn't need all of its half
(i.e. the non-elided version is not even that wide), the domain takes the extra
width by being elided to fit a bigger width.

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:188: const int final_domain_width =
font.GetStringWidth(domain);
On 2012/03/01 22:10:56, asvitkine wrote:
> Nit: How about:
> 
> available_pixel_width -= font.GetStringWidth(domain);
> 
> and then not needing to use final_domain_width below.

I like that :)!

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider.h#...
ui/base/text/text_elider.h:31: // character, other than the ellipsis-es, on
either side of the '@'. If it is
On 2012/03/01 22:10:56, asvitkine wrote:
> ellipsis-es => ellipses

Ya, saw this form around the web to indicate the potential singular/plural form
of "ellipsis", but I agree it's kind of ugly and not so necessary...

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider_un...
File ui/base/text/text_elider_unittest.cc (right):

http://codereview.chromium.org/9489011/diff/23005/ui/base/text/text_elider_un...
ui/base/text/text_elider_unittest.cc:109: const size_t num_test_emails =
arraysize(test_emails);
On 2012/03/01 22:10:56, asvitkine wrote:
> Don't need these as separate variables, just use arraysize() directly. (It's
> basically a constant once the compiler is done with it).

Ah ok good to know, thought there would be some redundant computation involved!

Another thing that annoyed me was having to declare a gfx::Font in every method
(as everyone else has done). I tried to refactor it out as a global in the
anonymous namespace, but it seems to need more context and won't allow me to put
it there...

[Alexei Svitkine (slow), 2012-03-02 20:20:44]

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:182: available_pixel_width / 2);
I think the "available_pixel_width / 2" is not strictly correct here. You're
trying to elide the domain to satisfy the if statement condition, such as the
post condition should be that font.GetStringWidth(domain) <=
available_domain_width.

Suppose the username is long. so available_pixel_width - full_username_width is
e.g. 0 or negative. Then domain will be elided to available_pixel_width/2.

Now, suppose that available_pixel_width/2 is >= available_domain_width. This
would only happen if available_pixel_width is enough to fit only a few
characters or so, so granted it's unlikely. Still, in that case you might elide
domain to a width that won't fit the "r..." name.

I think you can fix this by adding a std::min(available_domain_width,
available_pixel_width / 2) here (while still keeping the outer std::max()).

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:191: username = ElideText(username,
I think this can fit on one line now.

[SteveT, 2012-03-02 20:26:35]

Quick drive by comment.

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.h#...
ui/base/text/text_elider.h:37: // extra width).
Do you assume that |email| is well formed when passed in? If you, add a comment
about that.

Or even better - write a helper to verify that |email| is well formed and DCHECK
it when passed in.

Or if you want to change ElideEMail to return an error when it's not well
formed, make sure you test for that.

[gab, 2012-03-02 21:47:50]

On 2012/03/02 20:26:35, SteveT wrote:
> Quick drive by comment.
> 
> http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.h
> File ui/base/text/text_elider.h (right):
> 
>
http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.h#...
> ui/base/text/text_elider.h:37: // extra width).
> Do you assume that |email| is well formed when passed in? If you, add a
comment
> about that.
> 
> Or even better - write a helper to verify that |email| is well formed and
DCHECK
> it when passed in.
> 
> Or if you want to change ElideEMail to return an error when it's not well
> formed, make sure you test for that.

There is a DCHECK that makes sure we get 2 and only 2 strings when splitting the
string at the @ symbol. If there are some illegal characters beyond that on
either side of the @ symbol, the result would simply be an elided string with
potential illegal characters carrying over (say spaces)...

Should I add to the spec of the function that the email is expected to be well
formed (in fact technically as it is now I only expect to be passing it Google
profile usernames from the profile_info_cache, but the function is not
restricted to this sort input)?

[gab, 2012-03-02 21:57:37]

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:182: available_pixel_width / 2);
On 2012/03/02 20:20:44, Alexei Svitkine wrote:
> I think the "available_pixel_width / 2" is not strictly correct here. You're
> trying to elide the domain to satisfy the if statement condition, such as the
> post condition should be that font.GetStringWidth(domain) <=
> available_domain_width.
> 
> Suppose the username is long. so available_pixel_width - full_username_width
is
> e.g. 0 or negative. Then domain will be elided to available_pixel_width/2.
> 
> Now, suppose that available_pixel_width/2 is >= available_domain_width. This
> would only happen if available_pixel_width is enough to fit only a few
> characters or so, so granted it's unlikely. Still, in that case you might
elide
> domain to a width that won't fit the "r..." name.
> 
> I think you can fix this by adding a std::min(available_domain_width,
> available_pixel_width / 2) here (while still keeping the outer std::max()).

Actually, if available_pixel_width/2 > available_domain_width (note: == is
fine), then later the username will elide to "..." which will cause "..." to be
returned as a whole (as specified by the spec which doesn't allow either side to
be "character-less").

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:191: username = ElideText(username,
On 2012/03/02 20:20:44, Alexei Svitkine wrote:
> I think this can fit on one line now.

Yay :)!

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.h#...
ui/base/text/text_elider.h:37: // extra width).
On 2012/03/02 20:26:36, SteveT wrote:
> Do you assume that |email| is well formed when passed in? If you, add a
comment
> about that.
> 
> Or even better - write a helper to verify that |email| is well formed and
DCHECK
> it when passed in.
> 
> Or if you want to change ElideEMail to return an error when it's not well
> formed, make sure you test for that.

There is a DCHECK that makes sure we get 2 and only 2 strings when splitting the
string at the @ symbol. If there are some illegal characters beyond that on
either side of the @ symbol, the result would simply be an elided string with
potential illegal characters carrying over (say spaces)...

Should I add to the spec of the function that the email is expected to be well
formed (in fact technically as it is now I only expect to be passing it Google
profile usernames from the profile_info_cache, but the function is not
restricted to this sort input)?

[SteveT, 2012-03-02 22:03:56]

lgtm

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.h#...
ui/base/text/text_elider.h:37: // extra width).
Sounds good. Just add the comment to specify that it must be well formed and
your DCHECKs should do the rest :)

[asvitkine_google, 2012-03-02 22:08:40]

LGTM

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:182: available_pixel_width / 2);
> Actually, if available_pixel_width/2 > available_domain_width (note: == is
> fine), then later the username will elide to "..." which will cause "..." to
be
> returned as a whole (as specified by the spec which doesn't allow either side
to
> be "character-less").

Okay. Please add a comment here explaining this.

[gab, 2012-03-02 23:37:21]

@asvitkine: I found some very picky corner cases and changed the logic slightly
can you have a second look? Thanks!

Ready for OWNER approval, thanks!

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.h
File ui/base/text/text_elider.h (right):

http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.h#...
ui/base/text/text_elider.h:37: // extra width).
On 2012/03/02 22:03:56, SteveT wrote:
> Sounds good. Just add the comment to specify that it must be well formed and
> your DCHECKs should do the rest :)

Perfect, done!

http://codereview.chromium.org/9489011/diff/30007/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/9489011/diff/30007/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:175: std::min(full_username_width,
I just realized that if the full username is smaller than its first letter +
ellipsis, we could be forcing the domain to elide when it didn't need to.

The new test: {"l@lllllllll.com", "l@lllll" + kEllipsisStr + ".com"}
confirms this (i.e. it fails if you remove this min calculation and passes now)

http://codereview.chromium.org/9489011/diff/30007/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:187: available_pixel_width / 2));
@asvitkine re-thinking about it: you were right :)!

I decided to do all the logic here so that if we do make it to the username
eliding we are 100% sure there is enough space to respect the spec and don't
need to further check the result of the 2nd elide.

I was able to construct a test: {"mmm@llllll", "m" + kEllipsisStr + "@l" +
kEllipsisStr + "l"}
 in which the |desired_domain_width| without the min would result in a number
larger than the |available_domain_width| (which means we could potentially leave
the domain too big and be left with not enough room for the username when a
resolution was actually possible).
Under my fonts in Ubuntu however this test didn't fail without the min as
unluckily it turned out that (desired_domain_width == 23, available_domain_width
== 21, but unluckily the elided string turned out to be 21 as well (as elision
doesn't guarantee an exact fit obviously)...)

I tested/observed this by adding:
    if(std::max(available_pixel_width - full_username_width,
                available_pixel_width / 2) > available_domain_width){
      LOG(INFO) << "HEEERRRRRRRRRRRRRRRREEEEEEEEEEEEEEEEEEEEEEEE";

      LOG(INFO) << "desired: " << desired_domain_width;
      LOG(INFO) << "avail_dom: " << available_domain_width;
      LOG(INFO) << "avail_pix: " << available_pixel_width;
      LOG(INFO) << "full_username_width:" << full_username_width;
      LOG(INFO) << "final_dom:" << font.GetStringWidth(domain);
    }
to the end of the domain eliding if block and seeing that in the previous
version without the min: the desired width would come out bigger than the
available width. This new min fixes this problem.

[asvitkine_google, 2012-03-03 00:37:20]

I'll take a look at the latest changes later.

Could you update the CL description to mention that it's adding a
utility eliding function that will be used later? (Since you're not
making the other changes in this CL anymore.) Thanks!

On Fri, Mar 2, 2012 at 6:37 PM,  <gab@chromium.org> wrote:
> @asvitkine: I found some very picky corner cases and changed the logic
> slightly
> can you have a second look? Thanks!
>
> Ready for OWNER approval, thanks!
>
>
>
> http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.h
> File ui/base/text/text_elider.h (right):
>
>
http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.h#...
> ui/base/text/text_elider.h:37: // extra width).
> On 2012/03/02 22:03:56, SteveT wrote:
>>
>> Sounds good. Just add the comment to specify that it must be well
>
> formed and
>>
>> your DCHECKs should do the rest :)
>
>
> Perfect, done!
>
> http://codereview.chromium.org/9489011/diff/30007/ui/base/text/text_elider.cc
> File ui/base/text/text_elider.cc (right):
>
>
http://codereview.chromium.org/9489011/diff/30007/ui/base/text/text_elider.cc...
> ui/base/text/text_elider.cc:175: std::min(full_username_width,
> I just realized that if the full username is smaller than its first
> letter + ellipsis, we could be forcing the domain to elide when it
> didn't need to.
>
> The new test: {"l@lllllllll.com", "l@lllll" + kEllipsisStr + ".com"}
> confirms this (i.e. it fails if you remove this min calculation and
> passes now)
>
>
http://codereview.chromium.org/9489011/diff/30007/ui/base/text/text_elider.cc...
> ui/base/text/text_elider.cc:187: available_pixel_width / 2));
> @asvitkine re-thinking about it: you were right :)!
>
> I decided to do all the logic here so that if we do make it to the
> username eliding we are 100% sure there is enough space to respect the
> spec and don't need to further check the result of the 2nd elide.
>
> I was able to construct a test: {"mmm@llllll", "m" + kEllipsisStr + "@l"
> + kEllipsisStr + "l"}
>  in which the |desired_domain_width| without the min would result in a
> number larger than the |available_domain_width| (which means we could
> potentially leave the domain too big and be left with not enough room
> for the username when a resolution was actually possible).
> Under my fonts in Ubuntu however this test didn't fail without the min
> as unluckily it turned out that (desired_domain_width == 23,
> available_domain_width == 21, but unluckily the elided string turned out
> to be 21 as well (as elision doesn't guarantee an exact fit
> obviously)...)
>
> I tested/observed this by adding:
>    if(std::max(available_pixel_width - full_username_width,
>                available_pixel_width / 2) > available_domain_width){
>      LOG(INFO) << "HEEERRRRRRRRRRRRRRRREEEEEEEEEEEEEEEEEEEEEEEE";
>
>      LOG(INFO) << "desired: " << desired_domain_width;
>      LOG(INFO) << "avail_dom: " << available_domain_width;
>      LOG(INFO) << "avail_pix: " << available_pixel_width;
>      LOG(INFO) << "full_username_width:" << full_username_width;
>      LOG(INFO) << "final_dom:" << font.GetStringWidth(domain);
>    }
> to the end of the domain eliding if block and seeing that in the
> previous version without the min: the desired width would come out
> bigger than the available width. This new min fixes this problem.
>
> http://codereview.chromium.org/9489011/

[gab1, 2012-03-03 12:26:15]

On Fri, Mar 2, 2012 at 7:36 PM, Alexei Svitkine <asvitkine@google.com>wrote:

> I'll take a look at the latest changes later.
>

Thanks


>
> Could you update the CL description to mention that it's adding a
> utility eliding function that will be used later? (Since you're not
> making the other changes in this CL anymore.) Thanks!
>

Yep, I'd already changed the CL description (I didn't change the title
however as I felt that could lead to confusion; in particular with the
email threads).


>
> On Fri, Mar 2, 2012 at 6:37 PM,  <gab@chromium.org> wrote:
> > @asvitkine: I found some very picky corner cases and changed the logic
> > slightly
> > can you have a second look? Thanks!
> >
> > Ready for OWNER approval, thanks!
> >
> >
> >
> >
> http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.h
> > File ui/base/text/text_elider.h (right):
> >
> >
>
http://codereview.chromium.org/9489011/diff/20008/ui/base/text/text_elider.h#...
> > ui/base/text/text_elider.h:37: // extra width).
> > On 2012/03/02 22:03:56, SteveT wrote:
> >>
> >> Sounds good. Just add the comment to specify that it must be well
> >
> > formed and
> >>
> >> your DCHECKs should do the rest :)
> >
> >
> > Perfect, done!
> >
> >
> http://codereview.chromium.org/9489011/diff/30007/ui/base/text/text_elider.cc
> > File ui/base/text/text_elider.cc (right):
> >
> >
>
http://codereview.chromium.org/9489011/diff/30007/ui/base/text/text_elider.cc...
> > ui/base/text/text_elider.cc:175: std::min(full_username_width,
> > I just realized that if the full username is smaller than its first
> > letter + ellipsis, we could be forcing the domain to elide when it
> > didn't need to.
> >
> > The new test: {"l@lllllllll.com", "l@lllll" + kEllipsisStr + ".com"}
> > confirms this (i.e. it fails if you remove this min calculation and
> > passes now)
> >
> >
>
http://codereview.chromium.org/9489011/diff/30007/ui/base/text/text_elider.cc...
> > ui/base/text/text_elider.cc:187: available_pixel_width / 2));
> > @asvitkine re-thinking about it: you were right :)!
> >
> > I decided to do all the logic here so that if we do make it to the
> > username eliding we are 100% sure there is enough space to respect the
> > spec and don't need to further check the result of the 2nd elide.
> >
> > I was able to construct a test: {"mmm@llllll", "m" + kEllipsisStr + "@l"
> > + kEllipsisStr + "l"}
> >  in which the |desired_domain_width| without the min would result in a
> > number larger than the |available_domain_width| (which means we could
> > potentially leave the domain too big and be left with not enough room
> > for the username when a resolution was actually possible).
> > Under my fonts in Ubuntu however this test didn't fail without the min
> > as unluckily it turned out that (desired_domain_width == 23,
> > available_domain_width == 21, but unluckily the elided string turned out
> > to be 21 as well (as elision doesn't guarantee an exact fit
> > obviously)...)
> >
> > I tested/observed this by adding:
> >    if(std::max(available_pixel_width - full_username_width,
> >                available_pixel_width / 2) > available_domain_width){
> >      LOG(INFO) << "HEEERRRRRRRRRRRRRRRREEEEEEEEEEEEEEEEEEEEEEEE";
> >
> >      LOG(INFO) << "desired: " << desired_domain_width;
> >      LOG(INFO) << "avail_dom: " << available_domain_width;
> >      LOG(INFO) << "avail_pix: " << available_pixel_width;
> >      LOG(INFO) << "full_username_width:" << full_username_width;
> >      LOG(INFO) << "final_dom:" << font.GetStringWidth(domain);
> >    }
> > to the end of the domain eliding if block and seeing that in the
> > previous version without the min: the desired width would come out
> > bigger than the available width. This new min fixes this problem.
> >
> > http://codereview.chromium.org/9489011/
>

[asvitkine_google, 2012-03-05 17:14:23]

Still LGTM

[gab, 2012-03-05 22:13:16]

ben, can you have a second look and give owner approval?

Thanks,
Gab

[sky, 2012-03-06 16:56:16]

http://codereview.chromium.org/9489011/diff/30007/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/9489011/diff/30007/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:159: base::SplitString(email, '@', &email_split);
Are you sure this is right? The spec seems to indicate the local-part can have
embedded @s in it.

[gab, 2012-03-06 22:06:21]

http://codereview.chromium.org/9489011/diff/30007/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/9489011/diff/30007/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:159: base::SplitString(email, '@', &email_split);
On 2012/03/06 16:56:16, sky wrote:
> Are you sure this is right? The spec seems to indicate the local-part can have
> embedded @s in it.

I spent a while today looking through code search trying to find an existing
method to validate emails (in all possible forms as described here:
http://en.wikipedia.org/wiki/Email_address#Local_part).

There is nothing in current Chromium code that validates email addresses
correctly: the only thing I could find is a regex in depot_tools that doesn't
encompass all possibilities in the email spec: http://goo.gl/UR7X5)

I also found that ChromeOS's email canonicalizer assumes the same that I did
(about a single @ symbol per email address):
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/log...

As it is now, this function is only to be used to elide emails returned from
logged in GAIA profiles (does GAIA support the full email spectrum? If not, I
could change the name of my function to ElideGAIAEmail or simply add a comment
to make it clear it doesn't support the full email spectrum).

[sky, 2012-03-06 22:48:27]

I would feel more comfortable if it weren't a DCHECK, but instead
searched for the @ from the end.

  -Scott

On Tue, Mar 6, 2012 at 2:06 PM,  <gab@chromium.org> wrote:
>
> http://codereview.chromium.org/9489011/diff/30007/ui/base/text/text_elider.cc
> File ui/base/text/text_elider.cc (right):
>
>
http://codereview.chromium.org/9489011/diff/30007/ui/base/text/text_elider.cc...
> ui/base/text/text_elider.cc:159: base::SplitString(email, '@',
> &email_split);
> On 2012/03/06 16:56:16, sky wrote:
>>
>> Are you sure this is right? The spec seems to indicate the local-part
>
> can have
>>
>> embedded @s in it.
>
>
> I spent a while today looking through code search trying to find an
> existing method to validate emails (in all possible forms as described
> here: http://en.wikipedia.org/wiki/Email_address#Local_part).
>
> There is nothing in current Chromium code that validates email addresses
> correctly: the only thing I could find is a regex in depot_tools that
> doesn't encompass all possibilities in the email spec:
> http://goo.gl/UR7X5)
>
> I also found that ChromeOS's email canonicalizer assumes the same that I
> did (about a single @ symbol per email address):
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chromeos/log...
>
> As it is now, this function is only to be used to elide emails returned
> from logged in GAIA profiles (does GAIA support the full email spectrum?
> If not, I could change the name of my function to ElideGAIAEmail or
> simply add a comment to make it clear it doesn't support the full email
> spectrum).
>
> http://codereview.chromium.org/9489011/

[gab, 2012-03-07 19:01:39]

On 2012/03/06 22:48:27, sky wrote:
> I would feel more comfortable if it weren't a DCHECK, but instead
> searched for the @ from the end.

Ok, makes sense, please have a look at the new patch.

Thanks,
Gab

[Alexei Svitkine (slow), 2012-03-07 19:07:35]

http://codereview.chromium.org/9489011/diff/40001/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/9489011/diff/40001/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:163: const size_t splitIndex =
email.find_last_of('@');
Rename to split_index.

http://codereview.chromium.org/9489011/diff/40001/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:164: DCHECK_LT(splitIndex, email.length());
DCHECK that it's not string16::npos.

[sky, 2012-03-07 19:18:55]

LGTM

[gab, 2012-03-07 19:36:15]

http://codereview.chromium.org/9489011/diff/40001/ui/base/text/text_elider.cc
File ui/base/text/text_elider.cc (right):

http://codereview.chromium.org/9489011/diff/40001/ui/base/text/text_elider.cc...
ui/base/text/text_elider.cc:164: DCHECK_LT(splitIndex, email.length());
On 2012/03/07 19:07:35, Alexei Svitkine wrote:
> DCHECK that it's not string16::npos.

Sure, I felt DCHECK_LT length was stronger, but DCHECK_NE string16::npos is more
explicit.

[Alexei Svitkine (slow), 2012-03-07 20:23:44]

LGTM

[commit-bot: I haz the power, 2012-03-07 20:57:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/9489011/42002

[commit-bot: I haz the power, 2012-03-07 22:43:53]

Try job failure for 9489011-42002 (retry) on mac_rel for step "gfx_unittests".
It's a second try, previously, step "gfx_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[gab, 2012-03-12 19:33:00]

The 't' and the 'm' don't have the same relative width in all fonts and having
an expectation of "notgoogley.com" --> "not...om" would not be elided the same
way under all fonts.

Fixed by replacing the 'm' by an 'a' and not leaving room for the 't':
"notgoogley.ca" --> "no...ca" works under all OS's.

All other tests were already passing on all OS's.
Tested on Lion, Windows 7, and Ubuntu.

Please lgtm and I'll put it through the CQ again.

Thanks,
Gab

[asvitkine_google, 2012-03-12 20:04:13]

Is there a way to make the test not fragile to these issues?

For example, just because it happens to work right now, doesn't mean
that will be the cases with different fonts, etc. Ideally, the test
should not be fragile to this sort of thing.

On Mon, Mar 12, 2012 at 12:33 PM,  <gab@chromium.org> wrote:
> The 't' and the 'm' don't have the same relative width in all fonts and
> having
> an expectation of "notgoogley.com" --> "not...om" would not be elided the
> same
> way under all fonts.
>
> Fixed by replacing the 'm' by an 'a' and not leaving room for the 't':
> "notgoogley.ca" --> "no...ca" works under all OS's.
>
> All other tests were already passing on all OS's.
> Tested on Lion, Windows 7, and Ubuntu.
>
> Please lgtm and I'll put it through the CQ again.
>
> Thanks,
> Gab
>
> https://chromiumcodereview.appspot.com/9489011/

[gab, 2012-03-12 20:33:30]

On 2012/03/12 20:04:13, asvitkine wrote:
> Is there a way to make the test not fragile to these issues?

Well, if I want to achieve 100% branch coverage of my code, I don't think I can
make tests that avoid this. The reason being that in the case which tests that
half of the space is given to each of username/hostname s.t. we get
"mess...@no...ca" (i.e. 4 characters each), if 'n' happens to be much bigger
than the other characters in one font then maybe "mess...@(big n)..." will be
the result.

I can see two ways to make the test robust:
1) Test with a hard-coded font which is not OS dependent (does that even
exists?).
2) Only use a single character per test (i.e. "ooooooo@oooooooooooo" -->
"oooo...@oo...oo") where no matter what the font size of 'o' is doesn't matter.
But then I feel like we are focusing the test on a single character (although
I'm not sure what we could actually catch with multiple characters that we
wouldn't with a single repeated char).

Furthermore this test: "mmm@llllll" --> "m...@l...l", which is meant to test the
need for std::min(available_domain_width, ...) only does so because 'l' is
typically smaller than 'm'; this is definitely not robust in ALL possible fonts
(and can't be tested with a single repeated character as suggested above).

[asvitkine_google, 2012-03-13 00:36:05]

On 2012/03/12 20:33:30, gab wrote:
> On 2012/03/12 20:04:13, asvitkine wrote:
> > Is there a way to make the test not fragile to these issues?
> 
> Well, if I want to achieve 100% branch coverage of my code, I don't think I
can
> make tests that avoid this. The reason being that in the case which tests that
> half of the space is given to each of username/hostname s.t. we get
> "mess...@no...ca" (i.e. 4 characters each), if 'n' happens to be much bigger
> than the other characters in one font then maybe "mess...@(big n)..." will be
> the result.
> 
> I can see two ways to make the test robust:
> 1) Test with a hard-coded font which is not OS dependent (does that even
> exists?).
> 2) Only use a single character per test (i.e. "ooooooo@oooooooooooo" -->
> "oooo...@oo...oo") where no matter what the font size of 'o' is doesn't
matter.
> But then I feel like we are focusing the test on a single character (although
> I'm not sure what we could actually catch with multiple characters that we
> wouldn't with a single repeated char).
> 
> Furthermore this test: "mmm@llllll" --> "m...@l...l", which is meant to test
the
> need for std::min(available_domain_width, ...) only does so because 'l' is
> typically smaller than 'm'; this is definitely not robust in ALL possible
fonts
> (and can't be tested with a single repeated character as suggested above).

I think it's okay to assume the width('m') > width('l') in the test, but not
e.g. width of 'm' vs. width of 'a' and such.

Perhaps TextEliderTest.ElideEmail is not the appropriate way to test eliding
that touches both username and domain? (e.g. there could be different ways to
elide the two parts to achieve the same width)

Can you think of a different way to test those cases? Maybe the single character
tests are okay. Or alternatively, have a different test target that checks for
widths differently..

[gab, 2012-03-13 15:09:03]

Here is a modified set of tests which I think are robust under all fonts.

(I made a few corrections please diff patch 18 against 15).

The only case where fonts can be a problem is when elision of both username and
domain is required as this is when character sizes under different fonts can
play a role as to which side gets how many characters displayed.
To avoid this I made sure that in all such tests: the username is prefixed with
the expected remaining characters in the domain, such that the characters
remaining on both sides are identical and fonts can't interfere with that.

I also managed to make a test that tests for the need of
for:std::min(available_domain_width, ...):
"mmmmm@llllllllll" --> "m...@l...". I commented this one as it's not obvious at
first sight why it's different from the other tests. Removing the min (by
hard-coding a big number instead of |available_domain_width| in the min makes
this test fail on Ubuntu which shows its purpose). Furthermore, this test is not
font dependent (although this test might NOT fail without the required min on
all platforms, it can't result in something different as the space is so tight
that no matter how the eliding is derived there is only enough space for
"m...@l...").

The only assumption here I guess is that the ellipsis character itself is not
ridiculously long (i.e. width ("m...") < width("mmmmm"), but I guess this is
fair as there would be no point eliding if the ellipsis is huge... (I just added
more characters to the original test to make sure of this; i.e. it used to be
"mmm@llllll")

[Alexei Svitkine (slow), 2012-03-13 15:36:54]

LGTM. Thanks!

http://codereview.chromium.org/9489011/diff/52004/ui/base/text/text_elider_un...
File ui/base/text/text_elider_unittest.cc (right):

http://codereview.chromium.org/9489011/diff/52004/ui/base/text/text_elider_un...
ui/base/text/text_elider_unittest.cc:61: // Test emails and their expected
elided forms (which the available widths
Nit: "from which the available widths will be derived"

[gab, 2012-03-13 15:49:53]

Great, pushing to CQ.

http://codereview.chromium.org/9489011/diff/52004/ui/base/text/text_elider_un...
File ui/base/text/text_elider_unittest.cc (right):

http://codereview.chromium.org/9489011/diff/52004/ui/base/text/text_elider_un...
ui/base/text/text_elider_unittest.cc:61: // Test emails and their expected
elided forms (which the available widths
On 2012/03/13 15:36:54, Alexei Svitkine wrote:
> Nit: "from which the available widths will be derived"

Fixed

[commit-bot: I haz the power, 2012-03-13 15:50:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/9489011/49003

[commit-bot: I haz the power, 2012-03-13 20:04:37]

Change committed as 126445

[gab, 2012-03-13 20:40:34]

Fixed, can someone launch a try job for me (I should have provisional committer
access soon, but until then...) thanks!

Failure on: Chromium ChromiumOS on Linux ChromiumOS Tester (1), revision 126446

Failure output:
ui/base/text/text_elider_unittest.cc:109: Failure
Value of: ElideEmail(UTF8ToUTF16(testcases[i].input), font,
font.GetStringWidth(expected_output))
  Actual: l@lllllllll.com
Expected: expected_output
Which is: l@lllll….com

http://codereview.chromium.org/9489011/diff/49005/ui/base/text/text_elider_un...
File ui/base/text/text_elider_unittest.cc (right):

http://codereview.chromium.org/9489011/diff/49005/ui/base/text/text_elider_un...
ui/base/text/text_elider_unittest.cc:87: {"l@llllllllllllllllllllllll.com",
"l@lllll" + kEllipsisStr + ".com"},
It failed on Chromium OS as it seems width("...") >= width("llll"), which means
it didn't need to elide in this test... adding more l's, there is no way
width("...") >= width("lllllllllllllllllll")... otherwise this is a Chromium OS
bug...

[asvitkine_google, 2012-03-13 20:58:24]

Sent try jobs.

[gab, 2012-03-13 21:03:08]

On 2012/03/13 20:58:24, asvitkine wrote:
> Sent try jobs.

Looks like patching failed, I uploaded from svn r126443 if that helps..?

Thanks again.

[gab, 2012-03-15 22:29:56]

All relevant bots passed.

linux_aura && mac_aura are not ready for public use (as maruel@ told me)

Ok to commit?

[asvitkine_google, 2012-03-15 22:34:34]

Sounds good to me.

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/9489011/49005

[commit-bot: I haz the power, 2012-03-16 13:11:55]

Try job failure for 9489011-49005 on mac_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-03-16 13:17:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/9489011/49005

[commit-bot: I haz the power, 2012-03-16 16:25:05]

Change committed as 127175