Remove SVGPathElement.pathSegList and related interfaces

Remove SVGPathElement.pathSegList and related interfaces

This CL removes the SVGPathSegList collection of objects and interface:

  SVGPathSegList{,TearOff}
  SVGPathSeg*

and support classes such as SVGPathSegListBuilder and SVGPathSegListSource.

The SVGPathSegList class is renamed to SVGPath, and stripped of all it's
listiness - essentially reducing it to a SVGPathByteStream. SVGAnimatedPath
is updated to contain the new type.
A new specialization of SVGAnimatedProperty is added for properties that
don't have any associated tear-off type.

The SVGPathSeg.h header file which still contain various path segment
description data is renamed to SVGPathData.h.

All tests for SVGPathSeg* functionality are removed, interface tests are
updated to not include the removed interfaces and in one case the use of
the pathSegList is replaced with the string representation.

TBR=timvolodine@chromium.org
BUG=539385
Committed: https://crrev.com/240bb9316be42349d5185466ccf28f06f6eb8ee3
Cr-Commit-Position: refs/heads/master@{#356063}

[fs, 2015-10-22 11:21:51]

Description was changed from

==========
Remove SVGPathElement.pathSegList and related lists/properties

Rename SVGPathSegList to SVGPath

Rename SVGPathSegList -> SVGPath
Rename SVGPathSeg.h->SVGPathData.h

BUG=539385
==========

to

==========
Remove SVGPathElement.pathSegList and related interfaces

This CL removes the SVGPathSegList collection of objects and interface:

  SVGPathSegList{,TearOff
  SVGPathSeg*

and support classes such as SVGPathSegListBuilder and SVGPathSegListSource.

The SVGPathSegList class is renamed to SVGPath, and stripped of all it's
listiness - essentially reducing it to a SVGPathByteStream. SVGAnimatedPath
is updated to contain the new type.
A new specialization of SVGAnimatedProperty is added for properties that
don't have any associated tear-off type.

The SVGPathSeg.h header file which still contain various path segment
description data is renamed to SVGPathData.h.

All tests for SVGPathSeg* functionality are removed, interface tests are
updated to not include the removed interfaces and in one case the use of
the pathSegList is replaced with the string representation.

BUG=539385
==========

[fs, 2015-10-22 11:21:51]

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

[fs, 2015-10-22 11:43:50]

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

[fs, 2015-10-22 11:44:00]

Description was changed from

==========
Remove SVGPathElement.pathSegList and related interfaces

This CL removes the SVGPathSegList collection of objects and interface:

  SVGPathSegList{,TearOff
  SVGPathSeg*

and support classes such as SVGPathSegListBuilder and SVGPathSegListSource.

The SVGPathSegList class is renamed to SVGPath, and stripped of all it's
listiness - essentially reducing it to a SVGPathByteStream. SVGAnimatedPath
is updated to contain the new type.
A new specialization of SVGAnimatedProperty is added for properties that
don't have any associated tear-off type.

The SVGPathSeg.h header file which still contain various path segment
description data is renamed to SVGPathData.h.

All tests for SVGPathSeg* functionality are removed, interface tests are
updated to not include the removed interfaces and in one case the use of
the pathSegList is replaced with the string representation.

BUG=539385
==========

to

==========
Remove SVGPathElement.pathSegList and related interfaces

This CL removes the SVGPathSegList collection of objects and interface:

  SVGPathSegList{,TearOff}
  SVGPathSeg*

and support classes such as SVGPathSegListBuilder and SVGPathSegListSource.

The SVGPathSegList class is renamed to SVGPath, and stripped of all it's
listiness - essentially reducing it to a SVGPathByteStream. SVGAnimatedPath
is updated to contain the new type.
A new specialization of SVGAnimatedProperty is added for properties that
don't have any associated tear-off type.

The SVGPathSeg.h header file which still contain various path segment
description data is renamed to SVGPathData.h.

All tests for SVGPathSeg* functionality are removed, interface tests are
updated to not include the removed interfaces and in one case the use of
the pathSegList is replaced with the string representation.

BUG=539385
==========

[fs, 2015-10-22 20:21:07]

(Soundtrack: https://www.youtube.com/watch?v=u5FyRZbqfeM)

Figured I might as well put this out there now, even though we're still waiting
for the resolution of https://codereview.chromium.org/1408143004/ (included here
as well w/ incremental changes), so that any minor issues can be flushed out and
maybe allow us to conclude this before the week ends. There's not a whole lot of
changes or additions to review here though =)

@timvolodine: Could you RS the WebViewShell expectation updates?

[pdr., 2015-10-23 04:40:54]

pdr@chromium.org changed reviewers:
+ kouhei@chromium.org

[pdr., 2015-10-23 04:40:55]

This is pretty much the best patch \o/

-3kloc!

Just small nits below. LGTM

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/custom/js-svg-constructors.svg (right):

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/custom/js-svg-constructors.svg:125:
expect_failure("SVGPathSeg.PATHSEG_UNKNOWN", 0);
Just remove these? (similar for js-svg-constructors, global-constructors, etc)

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/PathSVGInterpolation.cpp (right):

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/PathSVGInterpolation.cpp:188:
segment.point2.setX(toInterpolableNumber(list.get(4))->value());
WDYT about making the re-use of point1 and point2 an implementation detail of
PathSegmentData? I.e., add a non-const FloatPoint PathSegmentData.arcRadii() and
similarly for arcAngle?

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGPath.h (right):

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGPath.h:42: typedef void TearOffType;
Is this needed?

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGPath.h:70: mutable
OwnPtr<SVGPathByteStream> m_byteStream;
Does this need to be mutable?

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGPathData.h (right):

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGPathData.h:91: SVGPathSegType command;
Could we get better packing by moving this next to arcSweep and arcLarge?

[fs, 2015-10-23 09:12:33]

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/custom/global-constructors-expected.txt
(right):

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/custom/global-constructors-expected.txt:50:
FAIL SVGPathSeg.toString() should be function SVGPathSeg() { [native code] }.
Threw exception ReferenceError: SVGPathSeg is not defined
Dropped these too per below.

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/custom/js-svg-constructors.svg (right):

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/custom/js-svg-constructors.svg:125:
expect_failure("SVGPathSeg.PATHSEG_UNKNOWN", 0);
On 2015/10/23 at 04:40:55, pdr wrote:
> Just remove these? (similar for js-svg-constructors, global-constructors, etc)

Sure. Done (w/ collateral damage in the form of removal of the other user of
expect_failure and expect_failure itself.)

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/PathSVGInterpolation.cpp (right):

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/PathSVGInterpolation.cpp:188:
segment.point2.setX(toInterpolableNumber(list.get(4))->value());
On 2015/10/23 at 04:40:55, pdr wrote:
> WDYT about making the re-use of point1 and point2 an implementation detail of
PathSegmentData? I.e., add a non-const FloatPoint PathSegmentData.arcRadii() and
similarly for arcAngle?

It has crossed my mind, but I think I'll punt on that to a follow-up (don't want
it to swim in all of these other changes.)

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGPath.h (right):

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGPath.h:42: typedef void TearOffType;
On 2015/10/23 at 04:40:55, pdr wrote:
> Is this needed?

Not required, but the alternative would be to have a lot of explicit <..., void>
specifications, so doing it this way is nicer I think.

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGPath.h:70: mutable
OwnPtr<SVGPathByteStream> m_byteStream;
On 2015/10/23 at 04:40:55, pdr wrote:
> Does this need to be mutable?

Should cancel out with the const_cast, so no. Dropped.

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGPathData.h (right):

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGPathData.h:91: SVGPathSegType command;
On 2015/10/23 at 04:40:55, pdr wrote:
> Could we get better packing by moving this next to arcSweep and arcLarge?

Just moving it will not gain us anything (FloatPoints should only require 4-byte
alignment). If we were to store these structures "more persistently", then
something like:

unsigned command : 8;
unsigned arcSweep : 1;
unsigned arcLarge : 1;

might be useful (to save 4 bytes; 32 -> 28; still assuming alignof(FloatPoint)
== 4).

[fs, 2015-10-23 11:40:55]

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/PathSVGInterpolation.cpp (right):

https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/PathSVGInterpolation.cpp:188:
segment.point2.setX(toInterpolableNumber(list.get(4))->value());
On 2015/10/23 at 09:12:33, fs wrote:
> On 2015/10/23 at 04:40:55, pdr wrote:
> > WDYT about making the re-use of point1 and point2 an implementation detail
of PathSegmentData? I.e., add a non-const FloatPoint PathSegmentData.arcRadii()
and similarly for arcAngle?
> 
> It has crossed my mind, but I think I'll punt on that to a follow-up (don't
want it to swim in all of these other changes.)

I ended up folding it in. Fun game: Find the 4 changes!

[fs, 2015-10-23 11:42:08]

@timvolodine: Ping.

[pdr., 2015-10-23 17:10:46]

On 2015/10/23 at 11:40:55, fs wrote:
>
https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/animation/PathSVGInterpolation.cpp
(right):
> 
>
https://codereview.chromium.org/1416273002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/animation/PathSVGInterpolation.cpp:188:
segment.point2.setX(toInterpolableNumber(list.get(4))->value());
> On 2015/10/23 at 09:12:33, fs wrote:
> > On 2015/10/23 at 04:40:55, pdr wrote:
> > > WDYT about making the re-use of point1 and point2 an implementation detail
of PathSegmentData? I.e., add a non-const FloatPoint PathSegmentData.arcRadii()
and similarly for arcAngle?
> > 
> > It has crossed my mind, but I think I'll punt on that to a follow-up (don't
want it to swim in all of these other changes.)
> 
> I ended up folding it in. Fun game: Find the 4 changes!

LGTM!

[Eric Willigers, 2015-10-23 18:49:39]

ericwilligers@chromium.org changed reviewers:
+ ericwilligers@chromium.org

[Eric Willigers, 2015-10-23 18:49:40]

lgtm

[fs, 2015-10-26 13:17:38]

On 2015/10/23 at 11:42:08, fs wrote:
> @timvolodine: Ping.

Ping^2 (Simple android_webview/tools/WebViewShell/test/ expectation update; not
scary - I promise!)

[pdr., 2015-10-26 16:15:34]

On 2015/10/26 at 13:17:38, fs wrote:
> On 2015/10/23 at 11:42:08, fs wrote:
> > @timvolodine: Ping.
> 
> Ping^2 (Simple android_webview/tools/WebViewShell/test/ expectation update;
not scary - I promise!)

Tim is out for either vacation or tpac. I think it would be okay to TBR this
change.

[fs, 2015-10-26 16:17:29]

Description was changed from

==========
Remove SVGPathElement.pathSegList and related interfaces

This CL removes the SVGPathSegList collection of objects and interface:

  SVGPathSegList{,TearOff}
  SVGPathSeg*

and support classes such as SVGPathSegListBuilder and SVGPathSegListSource.

The SVGPathSegList class is renamed to SVGPath, and stripped of all it's
listiness - essentially reducing it to a SVGPathByteStream. SVGAnimatedPath
is updated to contain the new type.
A new specialization of SVGAnimatedProperty is added for properties that
don't have any associated tear-off type.

The SVGPathSeg.h header file which still contain various path segment
description data is renamed to SVGPathData.h.

All tests for SVGPathSeg* functionality are removed, interface tests are
updated to not include the removed interfaces and in one case the use of
the pathSegList is replaced with the string representation.

BUG=539385
==========

to

==========
Remove SVGPathElement.pathSegList and related interfaces

This CL removes the SVGPathSegList collection of objects and interface:

  SVGPathSegList{,TearOff}
  SVGPathSeg*

and support classes such as SVGPathSegListBuilder and SVGPathSegListSource.

The SVGPathSegList class is renamed to SVGPath, and stripped of all it's
listiness - essentially reducing it to a SVGPathByteStream. SVGAnimatedPath
is updated to contain the new type.
A new specialization of SVGAnimatedProperty is added for properties that
don't have any associated tear-off type.

The SVGPathSeg.h header file which still contain various path segment
description data is renamed to SVGPathData.h.

All tests for SVGPathSeg* functionality are removed, interface tests are
updated to not include the removed interfaces and in one case the use of
the pathSegList is replaced with the string representation.

TBR=timvolodine@chromium.org
BUG=539385
==========

[fs, 2015-10-26 16:17:38]

On 2015/10/26 at 16:15:34, pdr wrote:
> On 2015/10/26 at 13:17:38, fs wrote:
> > On 2015/10/23 at 11:42:08, fs wrote:
> > > @timvolodine: Ping.
> > 
> > Ping^2 (Simple android_webview/tools/WebViewShell/test/ expectation update;
not scary - I promise!)
> 
> Tim is out for either vacation or tpac. I think it would be okay to TBR this
change.

TBRing...

[fs, 2015-10-26 16:17:46]

The CQ bit was checked by fs@opera.com

[fs, 2015-10-26 16:17:47]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
ericwilligers@chromium.org
Link to the patchset: https://codereview.chromium.org/1416273002/#ps60001
(title: "Rebase.")

[commit-bot: I haz the power, 2015-10-26 16:18:39]

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

[commit-bot: I haz the power, 2015-10-26 16:59:56]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-26 17:01:24]

Patchset 4 (id:??) landed as
https://crrev.com/240bb9316be42349d5185466ccf28f06f6eb8ee3
Cr-Commit-Position: refs/heads/master@{#356063}

[timvolodine, 2015-10-27 11:21:06]

wow what a mighty cleanup -7k lines!

yes was OOO recently sorry for delay.

android_webview/tools/WebViewShell/test/webexposed/global-interface-listing-expected.txt
-- LGTM