Drop SVGTextChunk::m_boxes

Drop SVGTextChunk::m_boxes

Since text chunks are now processed as they are formed, the m_boxes in
SVGTextChunk is of little use, and just means an additional copy is
needed. Drop it and use the range passed into handleTextChunk directly
instead.
This requires moving SVGTextChunk::calculateLength to
SVGTextChunkBuilder.cpp, so do that and put it in a helper class.
Also changing from passing <vector,startIndex,endIndex> to just passing
two (vector) iterators describing the start and end in the vector.
In SVGTextChunkBuilder::handleTextChunk, drop a redundant test
(!boxCount), and move the code that recalculates the chunk extent after
applying spacing adjustments for 'textLength' up into the relevant
branch.

BUG=486669
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196229

[fs, 2015-05-28 21:32:37]

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

[fs, 2015-05-28 21:32:38]

One more nail in the SVGTextChunk coffin...

[pdr., 2015-05-28 22:00:03]

I like where this is going!

LGTM, just minor stuff below.

https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGT...
File Source/core/layout/svg/SVGTextChunkBuilder.cpp (right):

https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextChunkBuilder.cpp:31: namespace {
Is this empty namespace needed?

https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextChunkBuilder.cpp:33: SVGTextChunk
createTextChunk(const SVGInlineTextBox* textBox)
Should this be static?

Can you change the textBox param to be a reference?

https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextChunkBuilder.cpp:158: for (; boxIter != endBox;
++boxIter) {
Not your fault, but can you switch this a fancy for loop?

https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextChunkBuilder.cpp:241: if (processTextAnchor) {
(Probably for another patch)
I think this additional walk can be avoided by using textLengthShift.

[fs, 2015-05-29 09:26:36]

https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGT...
File Source/core/layout/svg/SVGTextChunkBuilder.cpp (right):

https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextChunkBuilder.cpp:31: namespace {
On 2015/05/28 22:00:03, pdr wrote:
> Is this empty namespace needed?

Not strictly I suppose, but it provides a neat packaging mechanism for the
symbols that are intended to be CU-local.

https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextChunkBuilder.cpp:33: SVGTextChunk
createTextChunk(const SVGInlineTextBox* textBox)
On 2015/05/28 22:00:03, pdr wrote:
> Should this be static?

The unnamed namespace will have that effect - and also for
ChunkLengthAccumulator, which is why I chose this way rather than a simple
'static'.

> Can you change the textBox param to be a reference?

Done.

https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextChunkBuilder.cpp:158: for (; boxIter != endBox;
++boxIter) {
On 2015/05/28 22:00:03, pdr wrote:
> Not your fault, but can you switch this a fancy for loop?

I think that should be possible eventually, but for now we'll need access to the
iterators.

https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGT...
Source/core/layout/svg/SVGTextChunkBuilder.cpp:241: if (processTextAnchor) {
On 2015/05/28 22:00:03, pdr wrote:
> (Probably for another patch)
> I think this additional walk can be avoided by using textLengthShift.

Yepp, I've been thinking so too, but I'll leave that to another CL (need to
unravel this a bit first.)

[pdr., 2015-05-29 18:37:09]

(still LGTM)

On 2015/05/29 at 09:26:36, fs wrote:
>
https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGT...
> File Source/core/layout/svg/SVGTextChunkBuilder.cpp (right):
> 
>
https://codereview.chromium.org/1162653002/diff/1/Source/core/layout/svg/SVGT...
> Source/core/layout/svg/SVGTextChunkBuilder.cpp:31: namespace {
> On 2015/05/28 22:00:03, pdr wrote:
> > Is this empty namespace needed?
> 
> Not strictly I suppose, but it provides a neat packaging mechanism for the
symbols that are intended to be CU-local.

I don't yet understand the empty namespace, but this is really just a c++
question and not a comment about the patch. Because the class and function are
defined in the .cpp file, aren't they automatically CU local? Everything seems
to compile and work with the empty namespace removed, for example.

[f(malita), 2015-05-29 18:51:11]

lgtm++

On 2015/05/29 18:37:09, pdr wrote:
> I don't yet understand the empty namespace, but this is really just a c++
> question and not a comment about the patch. Because the class and function are
> defined in the .cpp file, aren't they automatically CU local? Everything seems
> to compile and work with the empty namespace removed, for example.

Like 'static', anonymous namespaces restrict *external* visibility.  They don't
fix/break CU-local users, but prevent symbol clashes with other CUs.

Anonymous namespaces are considered superior these days, and are recommended in
Chromium's coding style: 
https://www.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces

[fs, 2015-06-01 11:15:15]

On 2015/05/29 18:51:11, f(malita) wrote:
> lgtm++
> 
> On 2015/05/29 18:37:09, pdr wrote:
> > I don't yet understand the empty namespace, but this is really just a c++
> > question and not a comment about the patch. Because the class and function
are
> > defined in the .cpp file, aren't they automatically CU local? Everything
seems
> > to compile and work with the empty namespace removed, for example.
> 
> Like 'static', anonymous namespaces restrict *external* visibility.  They
don't
> fix/break CU-local users, but prevent symbol clashes with other CUs.

Yepp, however unlikely etc, etc.
 
> Anonymous namespaces are considered superior these days, and are recommended
in
> Chromium's coding style: 
> https://www.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces

...and that.

[fs, 2015-06-01 11:15:20]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-06-01 11:15:28]

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

[commit-bot: I haz the power, 2015-06-01 13:53:39]

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

[commit-bot: I haz the power, 2015-06-01 13:53:40]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/64568)

[fs, 2015-06-01 13:58:44]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2015-06-01 13:59:08]

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

[commit-bot: I haz the power, 2015-06-01 14:59:08]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196229