Cleanup SVGTextMetricsBuilder::measureTextLayoutObject

Cleanup SVGTextMetricsBuilder::measureTextLayoutObject

When working on SVGTextMetricsBuilder::measureTextLayoutObject I found
the MeasureTextData struct to be confusing because it is used for two
separate tasks. This patch is an attempt to separate attribute updating
from the text walk itself.

MeasureTextData has been split into two structs:
1) TreeWalkTextState
2) UpdateAttributes (optional)

Previously, attribute updating was gated on either "processLayoutObject"
or "data->allCharactersMap". This patch refactors both of these behind
an optional "UpdateAttributes" parameter.

Additionally, measureTextLayoutObject has been renamed walkInlineText
because measurement is only needed when updating attributes, otherwise
the walk is just used to update the tree walk text state.

Committed: https://crrev.com/1d6273f7b3bb80fa457e5b8dd682dba90bae5241
Cr-Commit-Position: refs/heads/master@{#382382}

[pdr., 2016-03-19 20:38:31]

pdr@chromium.org changed reviewers:
+ fs@opera.com

[pdr., 2016-03-19 20:38:43]

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

[commit-bot: I haz the power, 2016-03-19 20:38:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1822513002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1822513002/1

[pdr., 2016-03-19 22:14:31]

Bikeshedding welcome on this one.

[commit-bot: I haz the power, 2016-03-20 01:01:45]

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

[commit-bot: I haz the power, 2016-03-20 01:01:46]

Dry run: This issue passed the CQ dry run.

[fs, 2016-03-20 23:43:38]

lgtm

https://codereview.chromium.org/1822513002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp
(right):

https://codereview.chromium.org/1822513002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp:185: //
available, the attribute's character data map will also be updated.
Maybe mention the relation to valueListPosition too (since that part of
TreeWalkTextState is logically related to what's "going on" in this struct.)

https://codereview.chromium.org/1822513002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp:237:
ASSERT(!calculator.currentCharacterStartsSurrogatePair());
Maybe just assert calculator.currentCharacterMetrics().length() == 1 to cover
all bases?

https://codereview.chromium.org/1822513002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp:238:
ASSERT(textState.lastCharacterWasWhiteSpace);
This is part of the condition, so maybe a tad overzealous to assert.

[pdr., 2016-03-21 18:44:22]

The CQ bit was checked by pdr@chromium.org

[pdr., 2016-03-21 18:44:22]

Thanks for the review! Hopefully this code is quite a bit more understandable
now.

https://codereview.chromium.org/1822513002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp
(right):

https://codereview.chromium.org/1822513002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp:185: //
available, the attribute's character data map will also be updated.
On 2016/03/20 at 23:43:38, fs wrote:
> Maybe mention the relation to valueListPosition too (since that part of
TreeWalkTextState is logically related to what's "going on" in this struct.)

Good idea, done.

https://codereview.chromium.org/1822513002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp:237:
ASSERT(!calculator.currentCharacterStartsSurrogatePair());
On 2016/03/20 at 23:43:38, fs wrote:
> Maybe just assert calculator.currentCharacterMetrics().length() == 1 to cover
all bases?

Done

https://codereview.chromium.org/1822513002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp:238:
ASSERT(textState.lastCharacterWasWhiteSpace);
On 2016/03/20 at 23:43:38, fs wrote:
> This is part of the condition, so maybe a tad overzealous to assert.

Done

[pdr., 2016-03-21 18:44:23]

The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com
Link to the patchset: https://codereview.chromium.org/1822513002/#ps20001
(title: "Update per reviewer comments")

[commit-bot: I haz the power, 2016-03-21 18:44:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1822513002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1822513002/20001

[commit-bot: I haz the power, 2016-03-21 20:51:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-21 20:53:45]

Description was changed from

==========
Cleanup SVGTextMetricsBuilder::measureTextLayoutObject

When working on SVGTextMetricsBuilder::measureTextLayoutObject I found
the MeasureTextData struct to be confusing because it is used for two
separate tasks. This patch is an attempt to separate attribute updating
from the text walk itself.

MeasureTextData has been split into two structs:
1) TreeWalkTextState
2) UpdateAttributes (optional)

Previously, attribute updating was gated on either "processLayoutObject"
or "data->allCharactersMap". This patch refactors both of these behind
an optional "UpdateAttributes" parameter.

Additionally, measureTextLayoutObject has been renamed walkInlineText
because measurement is only needed when updating attributes, otherwise
the walk is just used to update the tree walk text state.
==========

to

==========
Cleanup SVGTextMetricsBuilder::measureTextLayoutObject

When working on SVGTextMetricsBuilder::measureTextLayoutObject I found
the MeasureTextData struct to be confusing because it is used for two
separate tasks. This patch is an attempt to separate attribute updating
from the text walk itself.

MeasureTextData has been split into two structs:
1) TreeWalkTextState
2) UpdateAttributes (optional)

Previously, attribute updating was gated on either "processLayoutObject"
or "data->allCharactersMap". This patch refactors both of these behind
an optional "UpdateAttributes" parameter.

Additionally, measureTextLayoutObject has been renamed walkInlineText
because measurement is only needed when updating attributes, otherwise
the walk is just used to update the tree walk text state.

Committed: https://crrev.com/1d6273f7b3bb80fa457e5b8dd682dba90bae5241
Cr-Commit-Position: refs/heads/master@{#382382}
==========

[commit-bot: I haz the power, 2016-03-21 20:53:46]

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