Use precomputed text metrics instead of recomputing them in SVGTextQuery

Use precomputed text metrics instead of recomputing them in SVGTextQuery

This patch changes SVGTextQuery to use precomputed text metrics instead
of recomputing them on every text query. This will result in a small
performance improvement for text queries.

The purpose of this patch is to unify the SVG text metrics calculations
so they are only computed in one place and one time. A followup patch
will change how metrics are calculated and now only needs to change
SVGTextMetricsBuilder.

There is a small test expectations change in lengthAdjust-text-metrics
due to how the 'T' in 'Test' overhangs 'est'. When measured in
isolation using SVGTextContentElement.getSubStringLength, the width
of 'e' is different compared to when it is measured in the string. The
precise values used in the test are somewhat arbitrary and will be
changing in a followup but, aesthetically, http://jsbin.com/fayimoz is
improved with this patch.

BUG=589525
Committed: https://crrev.com/931bd4a856cfe2a08f33b8be8cc405bd616873b5
Cr-Commit-Position: refs/heads/master@{#380243}

[pdr., 2016-03-09 02:45:22]

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

[pdr., 2016-03-09 02:45:29]

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

[commit-bot: I haz the power, 2016-03-09 02:45:44]

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

[commit-bot: I haz the power, 2016-03-09 04:19:23]

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

[commit-bot: I haz the power, 2016-03-09 04:19:24]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-03-09 05:38:40]

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

[commit-bot: I haz the power, 2016-03-09 05:38:59]

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

[commit-bot: I haz the power, 2016-03-09 07:16:22]

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

[commit-bot: I haz the power, 2016-03-09 07:16:23]

Dry run: This issue passed the CQ dry run.

[fs, 2016-03-09 09:55:42]

I have something similar (getting rid of calls to measureCharacterRange) stashed
away somewhere , so this naturally LGTM. (I did have a less squarish approach to
range computation though - which I found to be a bit convoluted in its linear
form - but we can iterate on that.)

Nits to follow.

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

https://codereview.chromium.org/1773353003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGTextQuery.cpp:277: if (end <=
start)
Nit: I believe this should be uncommon enough here (this is usually dealt with
via mapStart...) to not warrant a special case, and just let it fall out
naturally in the loop below - i.e zero trips round the loop.

https://codereview.chromium.org/1773353003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGTextQuery.cpp:282: SVGTextMetrics
metrics = findMetricsForCharacter(textMetricsValues, fragment, character);
Nit: const SVGTextMetrics&

[pdr., 2016-03-09 19:13:50]

The CQ bit was checked by pdr@chromium.org

[pdr., 2016-03-09 19:13:51]

Thanks! Off to the CQ.

Can you expand a bit on how to improve the range computation?

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

https://codereview.chromium.org/1773353003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGTextQuery.cpp:277: if (end <=
start)
On 2016/03/09 at 09:55:42, fs wrote:
> Nit: I believe this should be uncommon enough here (this is usually dealt with
via mapStart...) to not warrant a special case, and just let it fall out
naturally in the loop below - i.e zero trips round the loop.

SGTM, done.

https://codereview.chromium.org/1773353003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/svg/SVGTextQuery.cpp:282: SVGTextMetrics
metrics = findMetricsForCharacter(textMetricsValues, fragment, character);
On 2016/03/09 at 09:55:42, fs wrote:
> Nit: const SVGTextMetrics&

Done.

[pdr., 2016-03-09 19:13:51]

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/1773353003/#ps40001
(title: "Land rover: address reviewer comments and drive over to the CQ")

[commit-bot: I haz the power, 2016-03-09 19:14:09]

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

[fs, 2016-03-09 19:54:54]

On 2016/03/09 at 19:13:51, pdr wrote:
> Thanks! Off to the CQ.
> 
> Can you expand a bit on how to improve the range computation?

Very sketchy, but roughly:

current metric = find metrics list offset for |start| character
while (true) {
  glyphRange += current metric 'advance'
  if (current metric start < |end| character)
    break;
  current metric = next metrics cell
}

or some such. (Could probably also make findMetricsForCharacter - or a suitable
rename/rework - return an iterator to the Vector and then iterate the range
summing the advances.)

[fs, 2016-03-09 20:01:14]

On 2016/03/09 at 19:54:54, fs wrote:
> On 2016/03/09 at 19:13:51, pdr wrote:
> > Thanks! Off to the CQ.
> > 
> > Can you expand a bit on how to improve the range computation?
> 
> Very sketchy, but roughly:
> 
> current metric = find metrics list offset for |start| character
> while (true) {
>   glyphRange += current metric 'advance'
>   if (current metric start < |end| character)

Uhm, not this exact condition, but probably more like |end| <= current metric
end. ("Cell contains current character.")

>     break;
>   current metric = next metrics cell
> }
> 
> or some such. (Could probably also make findMetricsForCharacter - or a
suitable rename/rework - return an iterator to the Vector and then iterate the
range summing the advances.)

[commit-bot: I haz the power, 2016-03-09 22:29:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-09 22:48:01]

Description was changed from

==========
Use precomputed text metrics instead of recomputing them in SVGTextQuery

This patch changes SVGTextQuery to use precomputed text metrics instead
of recomputing them on every text query. This will result in a small
performance improvement for text queries.

The purpose of this patch is to unify the SVG text metrics calculations
so they are only computed in one place and one time. A followup patch
will change how metrics are calculated and now only needs to change
SVGTextMetricsBuilder.

There is a small test expectations change in lengthAdjust-text-metrics
due to how the 'T' in 'Test' overhangs 'est'. When measured in
isolation using SVGTextContentElement.getSubStringLength, the width
of 'e' is different compared to when it is measured in the string. The
precise values used in the test are somewhat arbitrary and will be
changing in a followup but, aesthetically, http://jsbin.com/fayimoz is
improved with this patch.

BUG=589525
==========

to

==========
Use precomputed text metrics instead of recomputing them in SVGTextQuery

This patch changes SVGTextQuery to use precomputed text metrics instead
of recomputing them on every text query. This will result in a small
performance improvement for text queries.

The purpose of this patch is to unify the SVG text metrics calculations
so they are only computed in one place and one time. A followup patch
will change how metrics are calculated and now only needs to change
SVGTextMetricsBuilder.

There is a small test expectations change in lengthAdjust-text-metrics
due to how the 'T' in 'Test' overhangs 'est'. When measured in
isolation using SVGTextContentElement.getSubStringLength, the width
of 'e' is different compared to when it is measured in the string. The
precise values used in the test are somewhat arbitrary and will be
changing in a followup but, aesthetically, http://jsbin.com/fayimoz is
improved with this patch.

BUG=589525

Committed: https://crrev.com/931bd4a856cfe2a08f33b8be8cc405bd616873b5
Cr-Commit-Position: refs/heads/master@{#380243}
==========

[commit-bot: I haz the power, 2016-03-09 22:48:02]

Patchset 3 (id:??) landed as
https://crrev.com/931bd4a856cfe2a08f33b8be8cc405bd616873b5
Cr-Commit-Position: refs/heads/master@{#380243}