Refactor HarfBuzzShaper to not retain font data

Refactor HarfBuzzShaper to not retain font data

First part in a suite of changes to allow shaping of individual segments
in a TextRun using a single HarfBuzzShaper instance with the prospective
objective to support continuous text shaping across DOM node boundaries.

This change removes the Font argument from the constructor and moves all
font setup plus associated feature detection to the shapeResults method.

By moving the font and font feature settings from the constructor to the
shapeResults call each segment may specify a different font and features
while sharing the same normalized text buffer and thus allowing segments
to use the surrounding text as context while shaping.

Additionally it moves ownership of the holes queue and font feature list
from the class itself to the shapeResults method thereby simplifying the
life cycle management for those collections and ensures that the feature
settings list and the holes queue are both reset between each segment.

TEST=HarfBuzzShaperTest.cpp
BUG=666758
R=drott@chromium.org

Committed: https://crrev.com/185f262ceb59340af223f08a89491beb71f30a2c
Cr-Commit-Position: refs/heads/master@{#433222}

[eae, 2016-11-17 18:35:12]

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

[eae, 2016-11-17 18:35:24]

Description was changed from

==========
Refactor HarfBuzzShaper to not retain font data

First part in a suite of changes to allow shaping of individual segments
in a TextRun using a single HarfBuzzShaper instance with the prospective
objective to support continuous text shaping across DOM node boundaries.

This change removes the Font argument from the constructor and moves all
font setup plus associated feature detection to the shapeResults method.

Additionally it moves ownership of the holes queue and font feature list
from the class itself to the shapeResults method thereby simplifying the
life cycle management for those collections and ensuring call isolation.

BUG=6122
R=drott@chromium.org
==========

to

==========
Refactor HarfBuzzShaper to not retain font data

First part in a suite of changes to allow shaping of individual segments
in a TextRun using a single HarfBuzzShaper instance with the prospective
objective to support continuous text shaping across DOM node boundaries.

This change removes the Font argument from the constructor and moves all
font setup plus associated feature detection to the shapeResults method.

Additionally it moves ownership of the holes queue and font feature list
from the class itself to the shapeResults method thereby simplifying the
life cycle management for those collections and ensuring call isolation.

TEST=HarfBuzzShaperTest.cpp
BUG=6122
R=drott@chromium.org
==========

[commit-bot: I haz the power, 2016-11-17 18:36:01]

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, 2016-11-17 21:16:53]

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

[commit-bot: I haz the power, 2016-11-17 21:16:54]

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

[drott, 2016-11-18 16:14:28]

So you're going for 

  TextRun paragraphText;
  HarfBuzzShaper shaper(paragraphText);
  RefPtr<ShapeResult> result = shaper.shapeRange(font, startIndex, endIndex);

of the examples we discussed?

Could you add a little more reasoning why this change is necessary in the CL
description? I am suspecting it's because we need to get to a world where two
shaping calls issued by layout on the same buffer can specify a different
font/feature set, but keep context, right? Perhaps you could explain that there?
By call isolation I guess you mean that we wouldn't need to take care of
resetting font, holesqueue and features for the second and additional calls on
the shaper? Perhaps it would make sense to describe that a bit more verbosely,
but that's personal taste.

Since there is going to be a series of CLs as you say I believe it would also
make sense to file a separate bug for LayoutNG work on context-preserving
shaping and block 6122 on this one, in order to keep the bug readable and have a
closer connection between bug title and CLs, if that makes sense?

With those remarks, LGTM. By the way, I am also very open to renaming holesQueue
to something better if you have suggestions, perhaps unshapedGapQueue might be a
candidate, but we can leave it as is as well.

[eae, 2016-11-18 16:23:02]

On 2016/11/18 16:14:28, drott wrote:
> So you're going for 
> 
>   TextRun paragraphText;
>   HarfBuzzShaper shaper(paragraphText);
>   RefPtr<ShapeResult> result = shaper.shapeRange(font, startIndex, endIndex);
> 
> of the examples we discussed?
> 
> Could you add a little more reasoning why this change is necessary in the CL
> description? I am suspecting it's because we need to get to a world where two
> shaping calls issued by layout on the same buffer can specify a different
> font/feature set, but keep context, right? Perhaps you could explain that
there?
> By call isolation I guess you mean that we wouldn't need to take care of
> resetting font, holesqueue and features for the second and additional calls on
> the shaper? Perhaps it would make sense to describe that a bit more verbosely,
> but that's personal taste.

Indeed, it's about preserving the normalized text buffer across calls and
allowing fonts/features to be specified for each sub-run. I'll expand on the
rationale behind it in the CL description.



> Since there is going to be a series of CLs as you say I believe it would also
> make sense to file a separate bug for LayoutNG work on context-preserving
> shaping and block 6122 on this one, in order to keep the bug readable and have
a
> closer connection between bug title and CLs, if that makes sense?

Good idea, will do!


> With those remarks, LGTM. By the way, I am also very open to renaming
holesQueue
> to something better if you have suggestions, perhaps unshapedGapQueue might be
a
> candidate, but we can leave it as is as well.

Hmm, unshapedGapQueue is a better name. We might also want to rename
shapeResults to something like shapeText or shapeSegment and shapeRange to
shapeGap or something? Let's leave all of that for a separate name-only change
though.

Thanks as always for a through review!

[eae, 2016-11-18 16:38:53]

Description was changed from

==========
Refactor HarfBuzzShaper to not retain font data

First part in a suite of changes to allow shaping of individual segments
in a TextRun using a single HarfBuzzShaper instance with the prospective
objective to support continuous text shaping across DOM node boundaries.

This change removes the Font argument from the constructor and moves all
font setup plus associated feature detection to the shapeResults method.

Additionally it moves ownership of the holes queue and font feature list
from the class itself to the shapeResults method thereby simplifying the
life cycle management for those collections and ensuring call isolation.

TEST=HarfBuzzShaperTest.cpp
BUG=6122
R=drott@chromium.org
==========

to

==========
Refactor HarfBuzzShaper to not retain font data

First part in a suite of changes to allow shaping of individual segments
in a TextRun using a single HarfBuzzShaper instance with the prospective
objective to support continuous text shaping across DOM node boundaries.

This change removes the Font argument from the constructor and moves all
font setup plus associated feature detection to the shapeResults method.

Additionally it moves ownership of the holes queue and font feature list
from the class itself to the shapeResults method thereby simplifying the
life cycle management for those collections and ensuring call isolation.

By moving the font and font feature settings from the constructor to the
shapeResults call each segment may specify a different font and features
while sharing the same normalized text buffer and thus allowing segments
to use the surrounding text as context while shaping.

TEST=HarfBuzzShaperTest.cpp
BUG=666758
R=drott@chromium.org
==========

[eae, 2016-11-18 16:43:54]

Description was changed from

==========
Refactor HarfBuzzShaper to not retain font data

First part in a suite of changes to allow shaping of individual segments
in a TextRun using a single HarfBuzzShaper instance with the prospective
objective to support continuous text shaping across DOM node boundaries.

This change removes the Font argument from the constructor and moves all
font setup plus associated feature detection to the shapeResults method.

Additionally it moves ownership of the holes queue and font feature list
from the class itself to the shapeResults method thereby simplifying the
life cycle management for those collections and ensuring call isolation.

By moving the font and font feature settings from the constructor to the
shapeResults call each segment may specify a different font and features
while sharing the same normalized text buffer and thus allowing segments
to use the surrounding text as context while shaping.

TEST=HarfBuzzShaperTest.cpp
BUG=666758
R=drott@chromium.org
==========

to

==========
Refactor HarfBuzzShaper to not retain font data

First part in a suite of changes to allow shaping of individual segments
in a TextRun using a single HarfBuzzShaper instance with the prospective
objective to support continuous text shaping across DOM node boundaries.

This change removes the Font argument from the constructor and moves all
font setup plus associated feature detection to the shapeResults method.

By moving the font and font feature settings from the constructor to the
shapeResults call each segment may specify a different font and features
while sharing the same normalized text buffer and thus allowing segments
to use the surrounding text as context while shaping.

Additionally it moves ownership of the holes queue and font feature list
from the class itself to the shapeResults method thereby simplifying the
life cycle management for those collections and ensures that the feature
settings list and the holes queue are both reset between each segment.

TEST=HarfBuzzShaperTest.cpp
BUG=666758
R=drott@chromium.org
==========

[eae, 2016-11-18 16:44:12]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2016-11-18 16:44:55]

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, 2016-11-18 17:05:59]

Description was changed from

==========
Refactor HarfBuzzShaper to not retain font data

First part in a suite of changes to allow shaping of individual segments
in a TextRun using a single HarfBuzzShaper instance with the prospective
objective to support continuous text shaping across DOM node boundaries.

This change removes the Font argument from the constructor and moves all
font setup plus associated feature detection to the shapeResults method.

By moving the font and font feature settings from the constructor to the
shapeResults call each segment may specify a different font and features
while sharing the same normalized text buffer and thus allowing segments
to use the surrounding text as context while shaping.

Additionally it moves ownership of the holes queue and font feature list
from the class itself to the shapeResults method thereby simplifying the
life cycle management for those collections and ensures that the feature
settings list and the holes queue are both reset between each segment.

TEST=HarfBuzzShaperTest.cpp
BUG=666758
R=drott@chromium.org
==========

to

==========
Refactor HarfBuzzShaper to not retain font data

First part in a suite of changes to allow shaping of individual segments
in a TextRun using a single HarfBuzzShaper instance with the prospective
objective to support continuous text shaping across DOM node boundaries.

This change removes the Font argument from the constructor and moves all
font setup plus associated feature detection to the shapeResults method.

By moving the font and font feature settings from the constructor to the
shapeResults call each segment may specify a different font and features
while sharing the same normalized text buffer and thus allowing segments
to use the surrounding text as context while shaping.

Additionally it moves ownership of the holes queue and font feature list
from the class itself to the shapeResults method thereby simplifying the
life cycle management for those collections and ensures that the feature
settings list and the holes queue are both reset between each segment.

TEST=HarfBuzzShaperTest.cpp
BUG=666758
R=drott@chromium.org
==========

[commit-bot: I haz the power, 2016-11-18 17:06:00]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-11-18 17:08:21]

Description was changed from

==========
Refactor HarfBuzzShaper to not retain font data

First part in a suite of changes to allow shaping of individual segments
in a TextRun using a single HarfBuzzShaper instance with the prospective
objective to support continuous text shaping across DOM node boundaries.

This change removes the Font argument from the constructor and moves all
font setup plus associated feature detection to the shapeResults method.

By moving the font and font feature settings from the constructor to the
shapeResults call each segment may specify a different font and features
while sharing the same normalized text buffer and thus allowing segments
to use the surrounding text as context while shaping.

Additionally it moves ownership of the holes queue and font feature list
from the class itself to the shapeResults method thereby simplifying the
life cycle management for those collections and ensures that the feature
settings list and the holes queue are both reset between each segment.

TEST=HarfBuzzShaperTest.cpp
BUG=666758
R=drott@chromium.org
==========

to

==========
Refactor HarfBuzzShaper to not retain font data

First part in a suite of changes to allow shaping of individual segments
in a TextRun using a single HarfBuzzShaper instance with the prospective
objective to support continuous text shaping across DOM node boundaries.

This change removes the Font argument from the constructor and moves all
font setup plus associated feature detection to the shapeResults method.

By moving the font and font feature settings from the constructor to the
shapeResults call each segment may specify a different font and features
while sharing the same normalized text buffer and thus allowing segments
to use the surrounding text as context while shaping.

Additionally it moves ownership of the holes queue and font feature list
from the class itself to the shapeResults method thereby simplifying the
life cycle management for those collections and ensures that the feature
settings list and the holes queue are both reset between each segment.

TEST=HarfBuzzShaperTest.cpp
BUG=666758
R=drott@chromium.org

Committed: https://crrev.com/185f262ceb59340af223f08a89491beb71f30a2c
Cr-Commit-Position: refs/heads/master@{#433222}
==========

[commit-bot: I haz the power, 2016-11-18 17:08:23]

Patchset 1 (id:??) landed as
https://crrev.com/185f262ceb59340af223f08a89491beb71f30a2c
Cr-Commit-Position: refs/heads/master@{#433222}