Break runs by clusters rather than iteration over code points

Break runs by clusters rather than iteration over code points

In some languages, the current logic (iteration over code
points and then get the glyphs by CharRangeToGlyphRange)
makes wrong effect, because both a character and its
diacritic marks can point to the same glyph range and
it misunderstands the width of the word for wrapping.

This CL changes the iteration logic for clusters so that
it skips the code points for the diacritic marks.

BUG=470073
R=msw@chromium.org, ckocagil@chromium.org
TEST=the new test case covers

Committed: https://crrev.com/043e7d282111711173853dcf36b258553352488b
Cr-Commit-Position: refs/heads/master@{#322316}

[msw, 2015-03-24 22:52:46]

Can you add a test for the fixed behavior?

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

https://codereview.chromium.org/1036663003/diff/1/ui/gfx/render_text_harfbuzz...
ui/gfx/render_text_harfbuzz.cc:331: // Sometimes multiple code points can point
to the same glyph (e.g.
This fix seems odd/wrong. Shouldn't the line above just get the glyph range from
|i| to IndexOfAdjacentGrapheme(i, CURSOR_FORWARD)?

[Jun Mukai, 2015-03-25 00:23:15]

Added the test case. PTAL.

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

https://codereview.chromium.org/1036663003/diff/1/ui/gfx/render_text_harfbuzz...
ui/gfx/render_text_harfbuzz.cc:331: // Sometimes multiple code points can point
to the same glyph (e.g.
On 2015/03/24 22:52:45, msw wrote:
> This fix seems odd/wrong. Shouldn't the line above just get the glyph range
from
> |i| to IndexOfAdjacentGrapheme(i, CURSOR_FORWARD)?

This is inside of EnsureLayout(), which is called from
IndexOfAdjacentGrapheme(), so cannot invoke that method.
Also, as far as I tested, IsGraphemeBoundary() returns true on this specific
example (see the new test case).

Considering about that, however, I think the right way is to use GetClusterAt()
rather than CharRangeToGlyphRange() and run over the clusters. PTAL.

[msw, 2015-03-25 20:25:17]

That's a much cleaner and clearer fix, just nits left.

https://codereview.chromium.org/1036663003/diff/40001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1036663003/diff/40001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:8: #include <set>
nit: remove

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

https://codereview.chromium.org/1036663003/diff/40001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2586: gfx::SizeF text_size =
render_text.GetStringSizeF();
nit: const

https://codereview.chromium.org/1036663003/diff/40001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2588: render_text.SetText(kWord + kWord);
nit: after this, but before SetDisplayRect, can you test something like:
  EXPECT_EQ(text_size.width() * 2, render_text.GetStringSizeF().width());

https://codereview.chromium.org/1036663003/diff/40001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2590: render_text.SetDisplayRect(gfx::Rect(0, 0,
text_size.width(), 0));
nit: instead of setting the display rect, can you add a '\n' char?

[Jun Mukai, 2015-03-25 21:50:05]

https://codereview.chromium.org/1036663003/diff/40001/ui/gfx/render_text_harf...
File ui/gfx/render_text_harfbuzz.cc (right):

https://codereview.chromium.org/1036663003/diff/40001/ui/gfx/render_text_harf...
ui/gfx/render_text_harfbuzz.cc:8: #include <set>
On 2015/03/25 20:25:16, msw wrote:
> nit: remove

Done.

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

https://codereview.chromium.org/1036663003/diff/40001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2586: gfx::SizeF text_size =
render_text.GetStringSizeF();
On 2015/03/25 20:25:16, msw wrote:
> nit: const

Done.

https://codereview.chromium.org/1036663003/diff/40001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2588: render_text.SetText(kWord + kWord);
On 2015/03/25 20:25:16, msw wrote:
> nit: after this, but before SetDisplayRect, can you test something like:
>   EXPECT_EQ(text_size.width() * 2, render_text.GetStringSizeF().width());

Done.

https://codereview.chromium.org/1036663003/diff/40001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2590: render_text.SetDisplayRect(gfx::Rect(0, 0,
text_size.width(), 0));
On 2015/03/25 20:25:16, msw wrote:
> nit: instead of setting the display rect, can you add a '\n' char?

'\n' doesn't work well in this case, since RTHB splits the text into different
runs, this CL wants to verify the line breaks in a run.

[msw, 2015-03-25 22:51:44]

lgtm

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

https://codereview.chromium.org/1036663003/diff/40001/ui/gfx/render_text_unit...
ui/gfx/render_text_unittest.cc:2590: render_text.SetDisplayRect(gfx::Rect(0, 0,
text_size.width(), 0));
On 2015/03/25 21:50:05, Jun Mukai wrote:
> On 2015/03/25 20:25:16, msw wrote:
> > nit: instead of setting the display rect, can you add a '\n' char?
> 
> '\n' doesn't work well in this case, since RTHB splits the text into different
> runs, this CL wants to verify the line breaks in a run.

Acknowledged.

[Jun Mukai, 2015-03-25 23:03:33]

updated the CL description, it described the old approach.

[Jun Mukai, 2015-03-25 23:03:38]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2015-03-25 23:04:35]

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

[commit-bot: I haz the power, 2015-03-26 01:46:56]

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

[commit-bot: I haz the power, 2015-03-26 01:46:57]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Jun Mukai, 2015-03-26 02:59:02]

The CQ bit was checked by mukai@chromium.org

[Jun Mukai, 2015-03-26 02:59:02]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1036663003/#ps80001
(title: "win/mac test fix")

[commit-bot: I haz the power, 2015-03-26 02:59:28]

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

[commit-bot: I haz the power, 2015-03-26 04:27:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-26 04:28:14]

Patchset 5 (id:??) landed as
https://crrev.com/043e7d282111711173853dcf36b258553352488b
Cr-Commit-Position: refs/heads/master@{#322316}