Fix multiline behaviors for RTL text.

Fix multiline behaviors for RTL text.

- Usually, logical first segment should appear in the first line
  when multi-lined. This is the reported bug as crbug.com/458219
  Therefore, TextRunHarfBuzz should be added to the line breaker
  in the logical order.
- The segment order in a line is assumed as visual order,
  therefore it needs to be sorted.
- Modified the test expectations which support this behavior.
- Added new test cases to verify where exactly the segment is
  drawn.

BUG=458219
R=msw@chromium.org, ckocagil@chromium.org
TEST=gfx_unittests

Committed: https://crrev.com/41d94b4931866d75bdbe9b75377b7608bba63d3a
Cr-Commit-Position: refs/heads/master@{#316954}

[ckocagil, 2015-02-13 19:05:25]

Optional: Can we also test LineSegment::x_range?

https://codereview.chromium.org/924773002/diff/1/ui/gfx/render_text_harfbuzz.cc
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/924773002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:270: struct SortByVisualPosition {
Make this comparator a lambda expression and define it in the caller function.

[Jun Mukai, 2015-02-13 21:57:59]

which test should I verify x_range?

Also -- I've noticed my mistake, the fractional glyph-width might not be the
case of crbug.com/456692. I'll make another CL for my case, but let me remove
the test case for now.

https://codereview.chromium.org/924773002/diff/1/ui/gfx/render_text_harfbuzz.cc
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/924773002/diff/1/ui/gfx/render_text_harfbuzz....
ui/gfx/render_text_harfbuzz.cc:270: struct SortByVisualPosition {
On 2015/02/13 19:05:25, ckocagil wrote:
> Make this comparator a lambda expression and define it in the caller function.

Done.

[msw, 2015-02-14 03:03:10]

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:93: class TestSkiaTextRenderer : public
internal::SkiaTextRenderer {
nit: add a comment.

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:135: for (size_t i = 0u; i < glyph_count; ++i) {
nit: remove curly braces.

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:136:
log_entry.bounds.Union(gfx::RectF(pos[i].x(), pos[i].y(), 1, 1));
q: are the 1x1 bounds really faithful to what's happening here?

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:150: std::vector<TextLog> draw_text_log_;
nit: rename |text_log_| to parallel |decoration_log_|?
(update GetDrawTextLogAndReset and param too, if so)

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2022:
EXPECT_EQ(kTestStrings[i].first_line_char_range.length(),
nit: comparing the char ranges to the glyph counts isn't always reliable, but
will work in simple cases... perhaps just add a NOTE: comment to that effect.

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2110: TEST_F(RenderTextTest,
HarfBuzz_RTLDrawingPositions) {
What value does this add over your new test cases above?

[Jun Mukai, 2015-02-18 00:24:14]

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:93: class TestSkiaTextRenderer : public
internal::SkiaTextRenderer {
On 2015/02/14 03:03:09, msw wrote:
> nit: add a comment.

Done.

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:135: for (size_t i = 0u; i < glyph_count; ++i) {
On 2015/02/14 03:03:10, msw wrote:
> nit: remove curly braces.

Modified the code a bit and now it needs curly braces.

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:136:
log_entry.bounds.Union(gfx::RectF(pos[i].x(), pos[i].y(), 1, 1));
On 2015/02/14 03:03:10, msw wrote:
> q: are the 1x1 bounds really faithful to what's happening here?

It is unknown the actual width and height for the glyphs in this method.
But actually the test cases are interested in where it will be drawn, which
means only the origin (positions) matter.  Modified |bounds| to |origin| so that
we don't have to care about the size (for now).

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:150: std::vector<TextLog> draw_text_log_;
On 2015/02/14 03:03:10, msw wrote:
> nit: rename |text_log_| to parallel |decoration_log_|?
> (update GetDrawTextLogAndReset and param too, if so)

Done.

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2022:
EXPECT_EQ(kTestStrings[i].first_line_char_range.length(),
On 2015/02/14 03:03:10, msw wrote:
> nit: comparing the char ranges to the glyph counts isn't always reliable, but
> will work in simple cases... perhaps just add a NOTE: comment to that effect.

That's true, added.

[msw, 2015-02-18 18:58:37]

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2110: TEST_F(RenderTextTest,
HarfBuzz_RTLDrawingPositions) {
On 2015/02/14 03:03:10, msw wrote:
> What value does this add over your new test cases above?

Ping! Why not just add this as a test case of Multiline_NormalWidth?

https://codereview.chromium.org/924773002/diff/60001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/924773002/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:159: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/924773002/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2028: // which doesn't always equal. This is okay
only because the all test
nit: s/doesn't/isn't/ and s/the all test/all the test/

[Jun Mukai, 2015-02-18 19:55:56]

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/924773002/diff/20001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2110: TEST_F(RenderTextTest,
HarfBuzz_RTLDrawingPositions) {
On 2015/02/18 18:58:37, msw wrote:
> On 2015/02/14 03:03:10, msw wrote:
> > What value does this add over your new test cases above?
> 
> Ping! Why not just add this as a test case of Multiline_NormalWidth?

This verifies the visual position in a line, while Multiline_NormalWidth
verifies the visual position among lines.
In other words, Mulitline_NormalWidth verifies the logically first segment
appears above if they are split into two lines, this verifies the logically
first segment appears at right if they are in a single line.

Actually, at first I forgot the std::sort in LineBreaker, and my code still
passed the all tests, and then I realized the necessity to verify the horizontal
positions.

Revised the test cases and it now verifies ltr languages as well.

https://codereview.chromium.org/924773002/diff/60001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/924773002/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:159: };
On 2015/02/18 18:58:37, msw wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/924773002/diff/60001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2028: // which doesn't always equal. This is okay
only because the all test
On 2015/02/18 18:58:37, msw wrote:
> nit: s/doesn't/isn't/ and s/the all test/all the test/

Done.

[msw, 2015-02-18 20:08:52]

lgtm with nits, thanks.

https://codereview.chromium.org/924773002/diff/80001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/924773002/diff/80001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2139: // Make sure two RTL runs are drawn in RTL
order (logically first run appears
nit: update this comment as the test now covers LTR too.

https://codereview.chromium.org/924773002/diff/80001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2165: if (run_list->runs().size() != 2U)
nit: make the above an ASSERT_EQ and remove this (like Multiline_NormalWidth).

[Jun Mukai, 2015-02-18 23:53:31]

New patchsets have been uploaded after l-g-t-m from msw@chromium.org

[Jun Mukai, 2015-02-18 23:53:37]

https://codereview.chromium.org/924773002/diff/80001/ui/gfx/render_text_unitt...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/924773002/diff/80001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2139: // Make sure two RTL runs are drawn in RTL
order (logically first run appears
On 2015/02/18 20:08:52, msw wrote:
> nit: update this comment as the test now covers LTR too.

Done.

https://codereview.chromium.org/924773002/diff/80001/ui/gfx/render_text_unitt...
ui/gfx/render_text_unittest.cc:2165: if (run_list->runs().size() != 2U)
On 2015/02/18 20:08:52, msw wrote:
> nit: make the above an ASSERT_EQ and remove this (like Multiline_NormalWidth).

Done.

[Jun Mukai, 2015-02-18 23:53:44]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2015-02-18 23:54:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/924773002/100001

[commit-bot: I haz the power, 2015-02-19 01:26:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-19 01:27:17]

Patchset 6 (id:??) landed as
https://crrev.com/41d94b4931866d75bdbe9b75377b7608bba63d3a
Cr-Commit-Position: refs/heads/master@{#316954}