Separate metrics update and layout attribute resolving

Separate metrics update and layout attribute resolving

This splits the walkInlineText() function from SVGTextMetricsBuilder.cpp
into one function for computing the Vector of SVGTextMetrics (called
via updateTextMetrics in LayoutSVGInlineText) and one function for
computing the mapping of layout attributes (updateLayoutAttributes in
SVGTextLayoutAttributesBuilder.cpp).
This in turn mean that the UpdateAttribute helper struct is split and
done away with, similarly TreeWalkTextState.

BUG=594058
Committed: https://crrev.com/14177d51219e4ba4d1381eb6a4486ec5d71020c0
Cr-Commit-Position: refs/heads/master@{#386301}

[fs, 2016-04-06 15:11:31]

Description was changed from

==========
Separate metrics update and layout attribute resolving

Move layout attribute resolution to SVGTextLayoutAttributesBuilder.
Resolve text metrics via LayoutSVGInlineText.

BUG=594058
==========

to

==========
Separate metrics update and layout attribute resolving

This splits the walkInlineText() function from SVGTextMetricsBuilder.cpp
into one function for computing the Vector of SVGTextMetrics (called
via updateTextMetrics in LayoutSVGInlineText) and one function for
computing the mapping of layout attributes (updateLayoutAttributes in
SVGTextLayoutAttributesBuilder.cpp).
This in turn mean that the UpdateAttribute helper struct is split and
done away with, similarly TreeWalkTextState.

BUG=594058
==========

[fs, 2016-04-06 15:28:25]

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

[fs, 2016-04-06 15:28:26]

https://codereview.chromium.org/1861013003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp (right):

https://codereview.chromium.org/1861013003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp:210:
SVGTextMetricsBuilder::updateTextMetrics(*this, lastCharacterWasWhiteSpace);
My current thinking is that we should just move all of SVGTextMetricsBuilder
(i.e SVGTextMetricsCalculator) to LayoutSVGInlineText, if you have any other
preference, let me know. That's for another CL though (and should be a "plain"
move mostly.)

[pdr., 2016-04-06 23:07:34]

LGTM, much cleaner.

This patch makes it more obvious that tracking lastCharacterWasWhiteSpace is
pretty ugly. Not storing empty white space text metrics would probably be the
cleanest design, but it's not obvious how to get there.

https://codereview.chromium.org/1861013003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp (right):

https://codereview.chromium.org/1861013003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp:210:
SVGTextMetricsBuilder::updateTextMetrics(*this, lastCharacterWasWhiteSpace);
On 2016/04/06 at 15:28:25, fs wrote:
> My current thinking is that we should just move all of SVGTextMetricsBuilder
(i.e SVGTextMetricsCalculator) to LayoutSVGInlineText, if you have any other
preference, let me know. That's for another CL though (and should be a "plain"
move mostly.)

SGTM

https://codereview.chromium.org/1861013003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp
(right):

https://codereview.chromium.org/1861013003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp:244: //
TODO(pdr): This loop is too tightly coupled to SVGTextMetricsCalculator.
Is this comment still useful? If not, lets just remove it.

[fs, 2016-04-07 09:26:17]

On 2016/04/06 at 23:07:34, pdr wrote:
> LGTM, much cleaner.
> 
> This patch makes it more obvious that tracking lastCharacterWasWhiteSpace is
pretty ugly. Not storing empty white space text metrics would probably be the
cleanest design, but it's not obvious how to get there.

Well, the way things are now, we do need to compute the collapsing "adjustments"
somewhere, and this has the "pro", I think, of being an early enough place to do
it. Currently we don't use the "skip me"-markers for anything useful beyond,
well, skipping... but I've sort of imagined that we could make better use of
them to make our whitespace collapsing more generic (i.e actually be able to
handle other values of 'white-space' without falling over in a spectacular
fashion...)

https://codereview.chromium.org/1861013003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp
(right):

https://codereview.chromium.org/1861013003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp:244: //
TODO(pdr): This loop is too tightly coupled to SVGTextMetricsCalculator.
On 2016/04/06 at 23:07:33, pdr wrote:
> Is this comment still useful? If not, lets just remove it.

I think it might still be useful given that what it says can still be done
(reverse the iteration order) - and with this being moved to LayoutSVGInlineText
it might even be a good idea still. I guess we should ask this "pdr" guy though
- he might know what to do...

[pdr., 2016-04-07 18:34:05]

On 2016/04/07 at 09:26:17, fs wrote:
> On 2016/04/06 at 23:07:34, pdr wrote:
> > LGTM, much cleaner.
> > 
> > This patch makes it more obvious that tracking lastCharacterWasWhiteSpace is
pretty ugly. Not storing empty white space text metrics would probably be the
cleanest design, but it's not obvious how to get there.
> 
> Well, the way things are now, we do need to compute the collapsing
"adjustments" somewhere, and this has the "pro", I think, of being an early
enough place to do it. Currently we don't use the "skip me"-markers for anything
useful beyond, well, skipping... but I've sort of imagined that we could make
better use of them to make our whitespace collapsing more generic (i.e actually
be able to handle other values of 'white-space' without falling over in a
spectacular fashion...)
> 
>
https://codereview.chromium.org/1861013003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/1861013003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp:244: //
TODO(pdr): This loop is too tightly coupled to SVGTextMetricsCalculator.
> On 2016/04/06 at 23:07:33, pdr wrote:
> > Is this comment still useful? If not, lets just remove it.
> 
> I think it might still be useful given that what it says can still be done
(reverse the iteration order) - and with this being moved to LayoutSVGInlineText
it might even be a good idea still. I guess we should ask this "pdr" guy though
- he might know what to do...

Lets just ignore pdr for now.

This patch LGTM

[fs, 2016-04-09 16:59:24]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-04-09 16:59:27]

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

[commit-bot: I haz the power, 2016-04-09 18:02:25]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-09 18:03:44]

Description was changed from

==========
Separate metrics update and layout attribute resolving

This splits the walkInlineText() function from SVGTextMetricsBuilder.cpp
into one function for computing the Vector of SVGTextMetrics (called
via updateTextMetrics in LayoutSVGInlineText) and one function for
computing the mapping of layout attributes (updateLayoutAttributes in
SVGTextLayoutAttributesBuilder.cpp).
This in turn mean that the UpdateAttribute helper struct is split and
done away with, similarly TreeWalkTextState.

BUG=594058
==========

to

==========
Separate metrics update and layout attribute resolving

This splits the walkInlineText() function from SVGTextMetricsBuilder.cpp
into one function for computing the Vector of SVGTextMetrics (called
via updateTextMetrics in LayoutSVGInlineText) and one function for
computing the mapping of layout attributes (updateLayoutAttributes in
SVGTextLayoutAttributesBuilder.cpp).
This in turn mean that the UpdateAttribute helper struct is split and
done away with, similarly TreeWalkTextState.

BUG=594058

Committed: https://crrev.com/14177d51219e4ba4d1381eb6a4486ec5d71020c0
Cr-Commit-Position: refs/heads/master@{#386301}
==========

[commit-bot: I haz the power, 2016-04-09 18:03:45]

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