RenderText: always break runs at whitespace

RenderText: always break runs at whitespace

RenderTextHarfBuzz is currently inconsistent with where it chooses to
break runs around whitespace whenever non-ascii characters are involved.
Currently it attaches runs of whitespace _after_ characters to the
preceding run (ignoring whether there is a script/ascii/non-ascii
mismatch between the run and the kind of whitespace). However, it does
not always attach runs of whitespace that occur _before_ runs (even if
they are both ascii).

Attaching whitespace while ignoring what script that whitespace is from
can cause problems. A font is not obligated to provide whitespace glyphs
for all scripts.

To fix this, always break at whitespace. This is closer to Blink
behaviour - shorter runs without whitespace makes better use of the
cache of text runs kept in blink's Font class; indexed by string
(typically, a dictionary word).

BUG=731563
Review-Url: https://codereview.chromium.org/2942843002
Cr-Commit-Position: refs/heads/master@{#480350}
Committed: https://chromium.googlesource.com/chromium/src/+/141af5c41b4e6a777e5c6d53575f124bc838865e

[tapted, 2017-06-15 08:53:00]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 08:53:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 09:31:12]

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

[commit-bot: I haz the power, 2017-06-15 09:31:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-06-16 00:59:25]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 00:59:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-06-16 01:46:30]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 01:46:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-06-16 01:58:49]

Description was changed from

==========
RenderText: always break runs at whitespace

BUG=731563
==========

to

==========
RenderText: always break runs at whitespace

RenderTextHarfBuzz is currently inconsistent with where it chooses to
break runs around whitespace whenever non-ascii characters are involved.
Currently it attaches runs of whitespace _after_ characters to the
preceding run (ignoring whether there is a script/ascii/non-ascii
mismatch between the run and the kind of whitespace). However, it does
not always attach runs of whitespace that occur _before_ runs (even if
they are both ascii).

Attaching whitespace while ignoring what script that whitespace is from
can cause problems. A font is not obligated to provide whitespace glyphs
for all scripts.

To fix this, always break at whitespace. This is closer to Blink
behaviour - shorter runs without whitespace makes better use of the
cache of text runs kept in blink's Font class, indexed by string
(typically, a dictionary word).

BUG=731563
==========

[tapted, 2017-06-16 01:58:49]

tapted@chromium.org changed reviewers:
+ asvitkine@chromium.org

[tapted, 2017-06-16 01:59:44]

Description was changed from

==========
RenderText: always break runs at whitespace

RenderTextHarfBuzz is currently inconsistent with where it chooses to
break runs around whitespace whenever non-ascii characters are involved.
Currently it attaches runs of whitespace _after_ characters to the
preceding run (ignoring whether there is a script/ascii/non-ascii
mismatch between the run and the kind of whitespace). However, it does
not always attach runs of whitespace that occur _before_ runs (even if
they are both ascii).

Attaching whitespace while ignoring what script that whitespace is from
can cause problems. A font is not obligated to provide whitespace glyphs
for all scripts.

To fix this, always break at whitespace. This is closer to Blink
behaviour - shorter runs without whitespace makes better use of the
cache of text runs kept in blink's Font class, indexed by string
(typically, a dictionary word).

BUG=731563
==========

to

==========
RenderText: always break runs at whitespace

RenderTextHarfBuzz is currently inconsistent with where it chooses to
break runs around whitespace whenever non-ascii characters are involved.
Currently it attaches runs of whitespace _after_ characters to the
preceding run (ignoring whether there is a script/ascii/non-ascii
mismatch between the run and the kind of whitespace). However, it does
not always attach runs of whitespace that occur _before_ runs (even if
they are both ascii).

Attaching whitespace while ignoring what script that whitespace is from
can cause problems. A font is not obligated to provide whitespace glyphs
for all scripts.

To fix this, always break at whitespace. This is closer to Blink
behaviour - shorter runs without whitespace makes better use of the
cache of text runs kept in blink's Font class; indexed by string
(typically, a dictionary word).

BUG=731563
==========

[commit-bot: I haz the power, 2017-06-16 02:03:13]

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

[commit-bot: I haz the power, 2017-06-16 02:03:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tapted, 2017-06-16 02:41:10]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 02:41:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-06-16 03:41:00]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 03:41:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 04:34:46]

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

[commit-bot: I haz the power, 2017-06-16 04:34:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tapted, 2017-06-16 05:52:34]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 05:52:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-16 06:13:07]

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

[commit-bot: I haz the power, 2017-06-16 06:13:08]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[tapted, 2017-06-16 07:12:36]

https://codereview.chromium.org/2942843002/diff/100001/ui/gfx/render_text_uni...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2942843002/diff/100001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:3727: EXPECT_EQ(ToString16Vec({"x", " ", "▶", "
", "y"}), GetRuns());
this test actually captures the old inconsistency. This line would have been
written

EXPECT_EQ(ToString16Vec({"x ", "▶", " ", "y"}), GetRuns());

i.e. 4 breaks, with whitespace attached to the 'x' but not attached to the 'y'.

[tapted, 2017-06-16 07:12:59]

Hi Alexei, WDYT? E.g. is there a downside to always breaking on whitespace that
I've missed?. I think that's what happens in Blink.

[Alexei Svitkine (slow), 2017-06-16 14:45:33]

lgtm

https://codereview.chromium.org/2942843002/diff/100001/ui/gfx/render_text_uni...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2942843002/diff/100001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:308: std::vector<base::string16> ToString16Vec(
Nit: Add 1-line comments for the new helper methods you're adding.

[tapted, 2017-06-19 03:53:51]

The CQ bit was checked by tapted@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 03:54:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tapted, 2017-06-19 04:10:50]

https://codereview.chromium.org/2942843002/diff/100001/ui/gfx/render_text_uni...
File ui/gfx/render_text_unittest.cc (right):

https://codereview.chromium.org/2942843002/diff/100001/ui/gfx/render_text_uni...
ui/gfx/render_text_unittest.cc:308: std::vector<base::string16> ToString16Vec(
On 2017/06/16 14:45:33, Alexei Svitkine (slow) wrote:
> Nit: Add 1-line comments for the new helper methods you're adding.

Done.

[tapted, 2017-06-19 04:10:57]

The CQ bit was unchecked by tapted@chromium.org

[tapted, 2017-06-19 04:11:03]

The CQ bit was checked by tapted@chromium.org

[tapted, 2017-06-19 04:11:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2942843002/#ps120001
(title: "Helper method comments")

[commit-bot: I haz the power, 2017-06-19 04:11:14]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 05:07:25]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497845463210670,
"parent_rev": "df1dced217b98dc93c6744ed4df94c7a456dd803", "commit_rev":
"141af5c41b4e6a777e5c6d53575f124bc838865e"}

[commit-bot: I haz the power, 2017-06-19 05:07:35]

Description was changed from

==========
RenderText: always break runs at whitespace

RenderTextHarfBuzz is currently inconsistent with where it chooses to
break runs around whitespace whenever non-ascii characters are involved.
Currently it attaches runs of whitespace _after_ characters to the
preceding run (ignoring whether there is a script/ascii/non-ascii
mismatch between the run and the kind of whitespace). However, it does
not always attach runs of whitespace that occur _before_ runs (even if
they are both ascii).

Attaching whitespace while ignoring what script that whitespace is from
can cause problems. A font is not obligated to provide whitespace glyphs
for all scripts.

To fix this, always break at whitespace. This is closer to Blink
behaviour - shorter runs without whitespace makes better use of the
cache of text runs kept in blink's Font class; indexed by string
(typically, a dictionary word).

BUG=731563
==========

to

==========
RenderText: always break runs at whitespace

RenderTextHarfBuzz is currently inconsistent with where it chooses to
break runs around whitespace whenever non-ascii characters are involved.
Currently it attaches runs of whitespace _after_ characters to the
preceding run (ignoring whether there is a script/ascii/non-ascii
mismatch between the run and the kind of whitespace). However, it does
not always attach runs of whitespace that occur _before_ runs (even if
they are both ascii).

Attaching whitespace while ignoring what script that whitespace is from
can cause problems. A font is not obligated to provide whitespace glyphs
for all scripts.

To fix this, always break at whitespace. This is closer to Blink
behaviour - shorter runs without whitespace makes better use of the
cache of text runs kept in blink's Font class; indexed by string
(typically, a dictionary word).

BUG=731563

Review-Url: https://codereview.chromium.org/2942843002
Cr-Commit-Position: refs/heads/master@{#480350}
Committed:
https://chromium.googlesource.com/chromium/src/+/141af5c41b4e6a777e5c6d53575f...
==========

[commit-bot: I haz the power, 2017-06-19 05:07:36]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/141af5c41b4e6a777e5c6d53575f...