Implement eliding/truncating at end in RenderText

Implement eliding/truncating at end in RenderText
patch from issue 112063003

The eliding is determined using binary search similar to the ElideText
in ui/gfx/text_elider.cc.

The mixed LTR-RTL handling for ellipsis is done using LTR/RTL markers.
There is no other way of rendering the ellipsis with the directionality
of preceding strong-directional characters. Previous scheme worked
because it rendered LTR and RTL sub-strings as different RenderText
instances.

Added helper method to be able to determine directionality of the
trailing text.

Made StringSlicer used by text_elider.cc public to be shared by RenderText.

Additional Fix:
- Call UpdateLayoutText from SetDisplayRect.
- Disable erratic tests on versions of windows older than Vista.

BUG=327833
TEST=ui_unittests,base_unittests
R=msw@chromium.org
TBR=jshin@chromium.org,pkasting@chromium.org

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

[Anuj, 2013-12-14 02:41:14]

PTAL.

[msw, 2013-12-14 03:11:41]

LGTM with a couple comments.

https://codereview.chromium.org/107513011/diff/120001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/107513011/diff/120001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:448: UpdateLayoutText();
Just do this if the ElideBehavior is not NO_ELIDE (that's the only case where
the layouttext actually depends on the display rect).

https://codereview.chromium.org/107513011/diff/120001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/107513011/diff/120001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:430: // RLM marker added as "ab" have string LTR
directionality.
nice, good comments!

https://codereview.chromium.org/107513011/diff/120001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:455: int original_width =
render_text->GetContentWidth();
optional nit: you can inline this in the check below.

https://codereview.chromium.org/107513011/diff/120001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:457: (expected_width >= original_width))
Ha! Was this the issue? the ellipsis character was wider than the text being
elided? Could this be corrected by adding more characters to the end of the
original string, so that the ellipsis is needed regardless, and the intended
codepoints will actually be elided? You don't *need* to fix this now, but I
would recommend it, since I could foresee the same issue recurring on other
platforms/versions/fonts. At least add a TODO comment about what's going on and
how it might be fixed.

[Anuj, 2013-12-14 03:35:03]

FYI.

https://codereview.chromium.org/107513011/diff/120001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/107513011/diff/120001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:448: UpdateLayoutText();
On 2013/12/14 03:11:41, msw wrote:
> Just do this if the ElideBehavior is not NO_ELIDE (that's the only case where
> the layouttext actually depends on the display rect).

Done.

https://codereview.chromium.org/107513011/diff/120001/ui/gfx/render_text_unit...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/107513011/diff/120001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:430: // RLM marker added as "ab" have string LTR
directionality.
Fixed spelling of "strong" :)

https://codereview.chromium.org/107513011/diff/120001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:455: int original_width =
render_text->GetContentWidth();
I will let this be.

https://codereview.chromium.org/107513011/diff/120001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:457: (expected_width >= original_width))
I am guessing so. Added the TODO.

[commit-bot: I haz the power, 2013-12-14 03:35:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/107513011/140001

[commit-bot: I haz the power, 2013-12-14 04:44:45]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-14 05:38:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/107513011/140001

[commit-bot: I haz the power, 2013-12-14 06:16:34]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-14 09:23:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/107513011/140001

[commit-bot: I haz the power, 2013-12-14 10:33:41]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-12-14 10:46:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/107513011/140001

[commit-bot: I haz the power, 2013-12-14 13:56:14]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-15 01:21:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/107513011/140001

[commit-bot: I haz the power, 2013-12-15 04:13:01]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-16 01:51:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/107513011/140001

[commit-bot: I haz the power, 2013-12-16 04:45:44]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-12-16 18:44:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skanuj@chromium.org/107513011/140001

[commit-bot: I haz the power, 2013-12-16 20:19:07]

Change committed as 241000

[szym, 2013-12-16 22:05:50]

On 2013/12/16 20:19:07, I haz the power (commit-bot) wrote:
> Change committed as 241000

Fails on Win 7 Tests x64:
http://build.chromium.org/p/chromium.win/builders/Win%207%20Tests%20x64%20%28...

[szym, 2013-12-16 22:11:16]

A revert of this CL has been created in
https://codereview.chromium.org/103053014/ by szym@chromium.org.

The reason for reverting is: Test fails on main waterfall..

[msw, 2013-12-17 02:14:32]

https://codereview.chromium.org/107513011/diff/140001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/107513011/diff/140001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1220: hi = guess - 1;
I hit a weird bug while testing your code locally on string kWeak " . " with a
width of 9px, just by removing the ElideText test's calls to SetFontList(...),
SetText(string16()), and by forcing expected_width = 9; can you repro? I worry
that there's a numeric underflow here:
LOW:0 HI:2
LOW:0 HI:2 GUESS:1
LOW:0 HI:0 GUESS:0
LOW:0 HI:4294967295 GUESS:2147483647
FATAL:text_elider.cc(118): index <= text_.length() (2147483647 vs. 3)

[Peter Kasting, 2013-12-17 21:30:55]

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:376:
std::min(classifications[i + 1].offset, text_length) : text_length;
Nit: Indent 4, not 2

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:383: ColorKind colorKind
= TEXT;
Nit: colorKind -> color_kind

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:387:
(classifications[i].style & ACMatchClassification::DIM))
Nit: I would probably add {} to all these conditional arms at this point; I
don't know that it's mandated, but it seems less potentially-confusing.

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:396: (remaining_width <
render_text->GetContentWidth())) {
Nit: This {} isn't necessary, though.

Do we actually need this conditional?  Seems like running through the code below
unconditionally ought to work OK.

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:400: if
(render_text->GetContentWidth() > remaining_width)
Do we need this conditional?  Can't we SetElideBehavior() unconditionally (and
probably do it at the time we create the RenderText?)

[Anuj, 2013-12-20 07:02:41]

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:376:
std::min(classifications[i + 1].offset, text_length) : text_length;
On 2013/12/17 21:30:56, Peter Kasting wrote:
> Nit: Indent 4, not 2

Done.

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:383: ColorKind colorKind
= TEXT;
On 2013/12/17 21:30:56, Peter Kasting wrote:
> Nit: colorKind -> color_kind

Done.

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:387:
(classifications[i].style & ACMatchClassification::DIM))
On 2013/12/17 21:30:56, Peter Kasting wrote:
> Nit: I would probably add {} to all these conditional arms at this point; I
> don't know that it's mandated, but it seems less potentially-confusing.

Done.

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:396: (remaining_width <
render_text->GetContentWidth())) {
No. But I feel better with this check :)

Also, I thought {} is required when "if conditional has a line-break"

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:400: if
(render_text->GetContentWidth() > remaining_width)
On 2013/12/17 21:30:56, Peter Kasting wrote:
> Do we need this conditional?  Can't we SetElideBehavior() unconditionally (and
> probably do it at the time we create the RenderText?)

Done.

https://codereview.chromium.org/107513011/diff/140001/ui/gfx/render_text.cc
File ui/gfx/render_text.cc (right):

https://codereview.chromium.org/107513011/diff/140001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1220: hi = guess - 1;
On 2013/12/17 02:14:33, msw wrote:
> I hit a weird bug while testing your code locally on string kWeak " . " with a
> width of 9px, just by removing the ElideText test's calls to SetFontList(...),
> SetText(string16()), and by forcing expected_width = 9; can you repro? I worry
> that there's a numeric underflow here:
> LOW:0 HI:2
> LOW:0 HI:2 GUESS:1
> LOW:0 HI:0 GUESS:0
> LOW:0 HI:4294967295 GUESS:2147483647
> FATAL:text_elider.cc(118): index <= text_.length() (2147483647 vs. 3)

Done.

[Peter Kasting, 2013-12-20 07:32:30]

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:396: (remaining_width <
render_text->GetContentWidth())) {
On 2013/12/20 07:02:42, Anuj wrote:
> No. But I feel better with this check :)

I find it misleading because it implies the code below won't work correctly, or
will be too slow, or something.  When in fact this doesn't really have any
useful effect at all AFAICT.  I'm not a fan of pointless code.  (If this isn't
pointless, the comment should say why we should be doing it.)

> Also, I thought {} is required when "if conditional has a line-break"

No, it's a matter of when the body takes up more than one physical line.

[Anuj, 2013-12-20 08:21:55]

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_result_view.cc (right):

https://codereview.chromium.org/107513011/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_result_view.cc:396: (remaining_width <
render_text->GetContentWidth())) {
> > Also, I thought {} is required when "if conditional has a line-break"
> 
> No, it's a matter of when the body takes up more than one physical line.

See comment from Scott Hess  on line 118.
https://codereview.chromium.org/98463012/diff/120001/chrome/browser/autocompl...