Remove GlyphBuffer

Remove GlyphBuffer

After dropping the simple shaper, GlyphBuffer is just an intermediate/temporary data structure
we create of the fly at paint time in order to convert the shape results to SkTextBlobs:


  ShapeResults -> GlyphBuffer -> GlyphBufferBloberizer -> SkTextBlobs

This CL introduces a ShapeResultBloberizer, and removes the GlyphBuffer intermediate:

  ShapeResults -> ShapeResultBloberizer -> SkTextBlobs

Shape results are fed to a ShapeResultBloberizer one glyph at a time, and are greedily combined
into SkTextBlobs:

  * glyphs sharing the same SimpleFontData are merged into the same run
  * runs sharing the same rotation are merged into the same blob

A detail worth mentioning: while previously vertical text required an additional pass over offsets
to perform verticalBaselineXOffset adjustments, the new approach applies the transformation on the
fly, as the glyphs are appended.

Refactor only, no functional changes expected.

BUG=574136
Review-Url: https://codereview.chromium.org/2714413003
Cr-Commit-Position: refs/heads/master@{#459229}
Committed: https://chromium.googlesource.com/chromium/src/+/123744c5850b84cf7f3ef9228b6d64a281732443

[f(malita), 2017-02-27 14:55:07]

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

[commit-bot: I haz the power, 2017-02-27 14:55:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2017-02-27 16:02:09]

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

[commit-bot: I haz the power, 2017-02-27 16:02:55]

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-02-27 18:04:55]

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

[commit-bot: I haz the power, 2017-02-27 18:04:56]

Dry run: This issue passed the CQ dry run.

[f(malita), 2017-02-27 21:39:46]

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

[commit-bot: I haz the power, 2017-02-27 21:40:21]

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-02-28 00:04:11]

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

[commit-bot: I haz the power, 2017-02-28 00:04:11]

Dry run: This issue passed the CQ dry run.

[f(malita), 2017-02-28 14:12:51]

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

[commit-bot: I haz the power, 2017-02-28 14:13:06]

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-02-28 16:44:22]

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

[commit-bot: I haz the power, 2017-02-28 16:44:22]

Dry run: This issue passed the CQ dry run.

[f(malita), 2017-03-04 14:40:01]

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

[commit-bot: I haz the power, 2017-03-04 14:40:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2017-03-04 15:55:52]

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

[commit-bot: I haz the power, 2017-03-04 15:56:04]

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-03-04 16:06:03]

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

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

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

[f(malita), 2017-03-04 16:09:06]

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

[commit-bot: I haz the power, 2017-03-04 16:09: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-03-04 17:35:04]

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

[commit-bot: I haz the power, 2017-03-04 17:35:05]

Dry run: This issue passed the CQ dry run.

[f(malita), 2017-03-08 18:47:06]

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

[commit-bot: I haz the power, 2017-03-08 18:47:45]

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-03-08 19:58:31]

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

[commit-bot: I haz the power, 2017-03-08 19:58:32]

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

[f(malita), 2017-03-10 20:27:34]

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

[commit-bot: I haz the power, 2017-03-10 20:28:12]

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-03-10 21:22:35]

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

[commit-bot: I haz the power, 2017-03-10 21:22:36]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[f(malita), 2017-03-22 13:49:09]

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

[commit-bot: I haz the power, 2017-03-22 13:49:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2017-03-22 14:48:29]

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

[commit-bot: I haz the power, 2017-03-22 14:48:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2017-03-22 15:42:26]

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

[commit-bot: I haz the power, 2017-03-22 15:42:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2017-03-22 16:02:59]

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

[commit-bot: I haz the power, 2017-03-22 16:03:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[f(malita), 2017-03-22 16:51:25]

Description was changed from

==========
Remove GlyphBuffer

WIP

BUG=
==========

to

==========
Remove GlyphBuffer

After dropping the simple shaper, GlyphBuffer is just an intermediate/temporary
data structure
we create of the fly at paint time in order to convert the shape results to
SkTextBlobs:


  ShapeResults -> GlyphBuffer -> GlyphBufferBloberizer -> SkTextBlobs

This CL introduces a ShapeResultBloberizer, and removes the GlyphBuffer
intermediate:

  ShapeResults -> ShapeResultBloberizer -> SkTextBlobs

Shape results are fed to a ShapeResultBloberizer one glyph at a time, and are
greedily combined
into SkTextBlobs:

  * glyphs sharing the same SimpleFontData are merged into the same run
  * runs sharing the same rotation are merged into the same blob

A detail worth mentioning: while previously vertical text required an additional
pass over offsets
to perform verticalBaselineXOffset adjustments, the new approach applies the
transformation on the
fly, as the glyphs are appended.

Refactor only, no functional changes expected.

BUG=574136
==========

[commit-bot: I haz the power, 2017-03-22 17:37:18]

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

[commit-bot: I haz the power, 2017-03-22 17:37:19]

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

[f(malita), 2017-03-23 13:18:53]

fmalita@chromium.org changed reviewers:
+ drott@chromium.org, eae@chromium.org

[f(malita), 2017-03-23 13:18:54]

Final nail in GlyphBuffer's coffin.

Cluster Telemetry shows a minor (1-2.5%) improvement for the caching-disabled
tests.

https://storage.cloud.google.com/cluster-telemetry/tasks/chromium_perf_runs/f...

[drott, 2017-03-23 15:36:53]

> Refactor only, no functional changes expected.

And what a refactor!

> Final nail in GlyphBuffer's coffin.

I think you nailed it here, excellent work. (And I am happy we have one more
person who know's Blink's text painting/shaperesult code in and out now! ;-))

LGTM, with a suggestion to add a test case for checking blob split caused by
different orientation. If I have understood the test cases correctly, this case
was not yet covered.

https://codereview.chromium.org/2714413003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/fonts/shaping/ShapeResultBloberizerTest.cpp
(right):

https://codereview.chromium.org/2714413003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/shaping/ShapeResultBloberizerTest.cpp:173:
}  // namespace blink
Could you perhaps add a test case which modifies the TestSimpleFontData so that
it would have the characteristic of having a different blobRotation() and check
that we see two blobs with one run each created? Could be a follow-up CL.

[eae, 2017-03-23 17:48:00]

This is awesome, thank you!

LGTM

[f(malita), 2017-03-23 19:30:18]

Thanks.

https://codereview.chromium.org/2714413003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/fonts/shaping/ShapeResultBloberizerTest.cpp
(right):

https://codereview.chromium.org/2714413003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/shaping/ShapeResultBloberizerTest.cpp:173:
}  // namespace blink
On 2017/03/23 15:36:52, drott wrote:
> Could you perhaps add a test case which modifies the TestSimpleFontData so
that
> it would have the characteristic of having a different blobRotation() and
check
> that we see two blobs with one run each created? Could be a follow-up CL.

I tried and initially got stuck because OpenTypeVerticalData is pulled from
FontCache and I couldn't find a good way to mock it.  I think I figured a way to
do it though, I'll upload a follow-up (requires some SimpleFontData fiddling).

[f(malita), 2017-03-23 19:30:24]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2017-03-23 19:31:00]

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-03-23 21:30:21]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1490297424776180,
"parent_rev": "3540cc14cd0b86a200aaedb2ef7fe024978aa98d", "commit_rev":
"123744c5850b84cf7f3ef9228b6d64a281732443"}

[commit-bot: I haz the power, 2017-03-23 21:31:08]

Description was changed from

==========
Remove GlyphBuffer

After dropping the simple shaper, GlyphBuffer is just an intermediate/temporary
data structure
we create of the fly at paint time in order to convert the shape results to
SkTextBlobs:


  ShapeResults -> GlyphBuffer -> GlyphBufferBloberizer -> SkTextBlobs

This CL introduces a ShapeResultBloberizer, and removes the GlyphBuffer
intermediate:

  ShapeResults -> ShapeResultBloberizer -> SkTextBlobs

Shape results are fed to a ShapeResultBloberizer one glyph at a time, and are
greedily combined
into SkTextBlobs:

  * glyphs sharing the same SimpleFontData are merged into the same run
  * runs sharing the same rotation are merged into the same blob

A detail worth mentioning: while previously vertical text required an additional
pass over offsets
to perform verticalBaselineXOffset adjustments, the new approach applies the
transformation on the
fly, as the glyphs are appended.

Refactor only, no functional changes expected.

BUG=574136
==========

to

==========
Remove GlyphBuffer

After dropping the simple shaper, GlyphBuffer is just an intermediate/temporary
data structure
we create of the fly at paint time in order to convert the shape results to
SkTextBlobs:


  ShapeResults -> GlyphBuffer -> GlyphBufferBloberizer -> SkTextBlobs

This CL introduces a ShapeResultBloberizer, and removes the GlyphBuffer
intermediate:

  ShapeResults -> ShapeResultBloberizer -> SkTextBlobs

Shape results are fed to a ShapeResultBloberizer one glyph at a time, and are
greedily combined
into SkTextBlobs:

  * glyphs sharing the same SimpleFontData are merged into the same run
  * runs sharing the same rotation are merged into the same blob

A detail worth mentioning: while previously vertical text required an additional
pass over offsets
to perform verticalBaselineXOffset adjustments, the new approach applies the
transformation on the
fly, as the glyphs are appended.

Refactor only, no functional changes expected.

BUG=574136

Review-Url: https://codereview.chromium.org/2714413003
Cr-Commit-Position: refs/heads/master@{#459229}
Committed:
https://chromium.googlesource.com/chromium/src/+/123744c5850b84cf7f3ef9228b6d...
==========

[commit-bot: I haz the power, 2017-03-23 21:31:10]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/123744c5850b84cf7f3ef9228b6d...