Move gfx::ElideText functionality to RenderText.

Move gfx::ElideText functionality to RenderText.

This is a prerequisite for http://crrev.com/23228004
(RenderText must elide correctly for direct Label use)

Use RenderText in gfx::ElideText on Win, Linux, Mac.
(old impl still needed for iOS and Android, for now)

Support additional eliding types in RenderText.
(matches behavior of gfx::ElideText, see TextEliderTest)
(still fixes the directionality of trailing ellipses)
(respect head and middle eliding when truncating)

Disambiguate gfx::NO_ELIDE from gfx::TRUNCATE.
Make the ElideEmail helper a private RenderText function.
Disable tests and no-op gfx::ElideText on iOS/Android.
Improve ElideTextSurrogatePairs perf: 7561 ms -> 3196 ms.

TODO: Fix RenderText::ElideEmail GetStringWidthF calls.
TODO: Support eliding filenames, like gfx::ElideFilename.

BUG=249938, 327846, 240037, 125348, 338784
R=asvitkine@chromium.org,sky@chromium.org
TEST=No observable text eliding behavior changes.

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

[msw, 2014-06-27 21:49:09]

Hey Alexei, please take a look; thanks!

[Alexei Svitkine (slow), 2014-06-27 21:50:26]

Sorry, I won't be able to get to this till Wednesday next week, since I'm about
to go now and we have a 4-day weekend here.

[Alexei Svitkine (slow), 2014-06-27 21:54:34]

https://codereview.chromium.org/354963003/diff/40001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/354963003/diff/40001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:424: base::string16 Elide(const base::string16& text,
It seems more natural to me to be able to set ElideBehavior via a setter on
RenderText and have it elide internally to fit the width of its rect, rather
than this standalone function.

[msw, 2014-06-27 23:51:49]

I addressed your comment and refined some behavior. Scott, please take a look if
you have a chance, otherwise I'll wait for Alexei to return (no real rush).

https://codereview.chromium.org/354963003/diff/40001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/354963003/diff/40001/ui/gfx/render_text.h#new...
ui/gfx/render_text.h:424: base::string16 Elide(const base::string16& text,
On 2014/06/27 21:54:33, Alexei Svitkine wrote:
> It seems more natural to me to be able to set ElideBehavior via a setter on
> RenderText and have it elide internally to fit the width of its rect, rather
> than this standalone function.

Done; this required disambiguating a new ElideBehavior gfx::NO_ELIDE (do not
modify the string, let it overflow the bounds) from the existing gfx::TRUNCATE
(trim the string to fit the bounds without appending ellipses), since
gfx::ElideText and RenderText/views::Label had different notions of what
gfx::TRUNCATE meant. This was a good suggestion.

[Alexei Svitkine (slow), 2014-07-03 20:12:34]

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

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1161: // Respect ELIDE_HEAD and ELIDE_MIDDLE preferences
during truncation.
Can you add test coverage for this?

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/render_text.h#ne...
ui/gfx/render_text.h:260: base::string16 layout_text() const { return
layout_text_; }
Return const base::string16&.

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/render_text_pang...
File ui/gfx/render_text_pango.cc (right):

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/render_text_pang...
ui/gfx/render_text_pango.cc:89: // while detecting that isn't worthwhile, this
handles the 0 width cases.
Did we file a bug against pango for this? iirc Behdad is a maintainer of Pango,
so he may be able to fix this (we can still have the workaround here, just
better to fix the Pango bug too for the future).

Is this already covered by a test? If not, please add one.

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

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/text_elider.cc#n...
ui/gfx/text_elider.cc:205: #if defined(OS_WIN) || defined(OS_LINUX)
I understand why it wouldn't work on platforms that don't have RenderText (e.g.
iOS), but why not Mac? afaik, eliding shouldn't be using any things
unimplemented by RenderTextMac.

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/text_elider.cc#n...
ui/gfx/text_elider.cc:220: GetStringWidthF(text, font_list) <=
available_pixel_width)
Nit: {}'s

[msw, 2014-07-08 19:07:36]

Please take another look; thanks!

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

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1161: // Respect ELIDE_HEAD and ELIDE_MIDDLE preferences
during truncation.
On 2014/07/03 20:12:33, asvitkine (OOO back July 14th) wrote:
> Can you add test coverage for this?

I added this code because otherwise TextEliderTest.ElideTextLongStrings fails,
so this already has test coverage, lmk if you'd like additional cases.

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/render_text.h
File ui/gfx/render_text.h (right):

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/render_text.h#ne...
ui/gfx/render_text.h:260: base::string16 layout_text() const { return
layout_text_; }
On 2014/07/03 20:12:33, asvitkine (OOO back July 14th) wrote:
> Return const base::string16&.

Done.

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/render_text_pang...
File ui/gfx/render_text_pango.cc (right):

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/render_text_pang...
ui/gfx/render_text_pango.cc:89: // while detecting that isn't worthwhile, this
handles the 0 width cases.
On 2014/07/03 20:12:33, asvitkine (OOO back July 14th) wrote:
> Did we file a bug against pango for this? iirc Behdad is a maintainer of
Pango,
> so he may be able to fix this (we can still have the workaround here, just
> better to fix the Pango bug too for the future).

I'm not sure and can't find any relevant open bugs:
https://bugzilla.gnome.org/buglist.cgi?quicksearch=pango_layout_get_pixel_size
https://bugzilla.gnome.org/buglist.cgi?quicksearch=pango_glyph_string_extents...
+CC estade per http://crrev.com/32572 for http://crbug.com/15435
+CC Behdad for help finding or filing a bug.

> Is this already covered by a test? If not, please add one.

Done; added RenderTextTest.StringSizeLongStrings.

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

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/text_elider.cc#n...
ui/gfx/text_elider.cc:205: #if defined(OS_WIN) || defined(OS_LINUX)
On 2014/07/03 20:12:34, asvitkine (OOO back July 14th) wrote:
> I understand why it wouldn't work on platforms that don't have RenderText
(e.g.
> iOS), but why not Mac? afaik, eliding shouldn't be using any things
> unimplemented by RenderTextMac.

RenderText doesn't support RectF values for the display rect and failed some
unit tests. I ceil'ed the available  width and made GetContentWidth return float
values. That gets the tests passing, but might cause a non-integral available
width elision to assume use of the full pixel. Do you think I should attempt to
support RectF display rects in this CL (or as a prerequisite CL)?

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/text_elider.cc#n...
ui/gfx/text_elider.cc:220: GetStringWidthF(text, font_list) <=
available_pixel_width)
On 2014/07/03 20:12:34, asvitkine (OOO back July 14th) wrote:
> Nit: {}'s

Done.

[Evan Stade, 2014-07-08 21:07:19]

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/render_text_pang...
File ui/gfx/render_text_pango.cc (right):

https://codereview.chromium.org/354963003/diff/220001/ui/gfx/render_text_pang...
ui/gfx/render_text_pango.cc:89: // while detecting that isn't worthwhile, this
handles the 0 width cases.
On 2014/07/08 19:07:36, msw wrote:
> On 2014/07/03 20:12:33, asvitkine (OOO back July 14th) wrote:
> > Did we file a bug against pango for this? iirc Behdad is a maintainer of
> Pango,
> > so he may be able to fix this (we can still have the workaround here, just
> > better to fix the Pango bug too for the future).
> 
> I'm not sure and can't find any relevant open bugs:
> https://bugzilla.gnome.org/buglist.cgi?quicksearch=pango_layout_get_pixel_size
>
https://bugzilla.gnome.org/buglist.cgi?quicksearch=pango_glyph_string_extents...
> +CC estade per http://crrev.com/32572 for http://crbug.com/15435

Unfortunately, my memory lasts exactly 5 years, so I've forgotten if I filed a
bug :)

I think the answer is "no".

> +CC Behdad for help finding or filing a bug.
> 
> > Is this already covered by a test? If not, please add one.
> 
> Done; added RenderTextTest.StringSizeLongStrings.

[behdad_google, 2014-07-08 21:22:04]

On 2014/07/08 21:07:19, Evan Stade wrote:
>
https://codereview.chromium.org/354963003/diff/220001/ui/gfx/render_text_pang...
> File ui/gfx/render_text_pango.cc (right):
> 
>
https://codereview.chromium.org/354963003/diff/220001/ui/gfx/render_text_pang...
> ui/gfx/render_text_pango.cc:89: // while detecting that isn't worthwhile, this
> handles the 0 width cases.
> On 2014/07/08 19:07:36, msw wrote:
> > On 2014/07/03 20:12:33, asvitkine (OOO back July 14th) wrote:
> > > Did we file a bug against pango for this? iirc Behdad is a maintainer of
> > Pango,
> > > so he may be able to fix this (we can still have the workaround here, just
> > > better to fix the Pango bug too for the future).
> > 
> > I'm not sure and can't find any relevant open bugs:
> >
https://bugzilla.gnome.org/buglist.cgi?quicksearch=pango_layout_get_pixel_size
> >
>
https://bugzilla.gnome.org/buglist.cgi?quicksearch=pango_glyph_string_extents...
> > +CC estade per http://crrev.com/32572 for http://crbug.com/15435
> 
> Unfortunately, my memory lasts exactly 5 years, so I've forgotten if I filed a
> bug :)
> 
> I think the answer is "no".

The root cause seems to be the same as:

  https://bugzilla.gnome.org/show_bug.cgi?id=722409

There's not much we can do at this point given that the 22.10 float presentation
is part of Pango API.  I recommend capping the input length before calling
Pango.

[sky, 2014-07-09 18:19:39]

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

https://codereview.chromium.org/354963003/diff/320001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1163: iter.setIndex32(text.length() - truncate_length_ +
1);
Is it possible for truncate_length_ to equal text.length()? Would that cause
problems here with the +1?

https://codereview.chromium.org/354963003/diff/320001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1171: text.substr(ellipsis_end));
Might it be possible for ellipsis_start to be > ellipsis_end?

[msw, 2014-07-09 19:16:08]

Comments addressed and I sped up a test; PTAL.

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

https://codereview.chromium.org/354963003/diff/320001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1163: iter.setIndex32(text.length() - truncate_length_ +
1);
On 2014/07/09 18:19:38, sky wrote:
> Is it possible for truncate_length_ to equal text.length()? Would that cause
> problems here with the +1?

The condition at line 1158 ensures |truncate_length_ < text.length()|.

https://codereview.chromium.org/354963003/diff/320001/ui/gfx/render_text.cc#n...
ui/gfx/render_text.cc:1171: text.substr(ellipsis_end));
On 2014/07/09 18:19:38, sky wrote:
> Might it be possible for ellipsis_start to be > ellipsis_end?

Afaict, that would only happen if |truncate_length_ >= text.length()| (not
possible as per the condition on line 1158), or if the iterator somehow yielded
character break positions in the reverse order of the input indices, which also
seems impossible. Just to make sure, I added a DCHECK that |ellipsis_start <=
ellipsis_end|.

[sky, 2014-07-10 15:05:30]

LGTM

[msw, 2014-07-10 16:01:55]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-07-10 16:04:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/354963003/340001

[commit-bot: I haz the power, 2014-07-10 16:09:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[commit-bot: I haz the power, 2014-07-10 16:14:44]

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

[commit-bot: I haz the power, 2014-07-10 16:14:45]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[msw, 2014-07-10 16:55:00]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-07-10 16:57:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/354963003/360001

[commit-bot: I haz the power, 2014-07-10 21:41:02]

Change committed as 282433