[SVG] SVGAnimatedPointList migration to new SVG property impl.

[SVG] SVGAnimatedPointList migration to new SVG property impl.

This CL replaces SVGAnimatedPointList class with NewSVGProperty implementation, and moves matrixTransform out of platform/FloatRect to SVGPoint.

For details, see design doc: https://docs.google.com/document/d/1bg7CUyUszqdwmENY3JX6_PoQD6uHRCNcRPJMlC4qlkw/edit?usp=sharing

BUG=302829
BUG=308818
TBR=pdr@chromium.org

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

[kouhei (in TOK), 2014-01-17 07:58:49]

pdr, haraken: Would you take a look?

[haraken, 2014-01-17 11:50:17]

LGTM with comments.

https://codereview.chromium.org/132233016/diff/1/Source/core/rendering/svg/SV...
File Source/core/rendering/svg/SVGPathData.cpp (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/rendering/svg/SV...
Source/core/rendering/svg/SVGPathData.cpp:88: path.addLineTo(it->value());

In the previous code, we didn't call addLineTo for the first element. However,
in the new code, we're calling addLineTo for the first element. Is it OK?

https://codereview.chromium.org/132233016/diff/1/Source/core/rendering/svg/SV...
Source/core/rendering/svg/SVGPathData.cpp:93:
updatePathFromPolylineElement(element, path);

What's the advantage of introducing this static method?

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGAnimatedN...
File Source/core/svg/SVGAnimatedNewPropertyAnimator.cpp (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGAnimatedN...
Source/core/svg/SVGAnimatedNewPropertyAnimator.cpp:87: case AnimatedRect:

Alphabetical order please.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPoint.cpp
File Source/core/svg/SVGPoint.cpp (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPoint.cpp...
Source/core/svg/SVGPoint.cpp:102: if (string.isNull() || string.isEmpty()) {

string.isEmpty() implies string.isNull(), so you don't need string.isNull().

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPoint.cpp...
Source/core/svg/SVGPoint.cpp:142: // SVGPoint is not animated by itself

Shouldn't this be a FIXME?

FIXME: Distance calculation is not possible for SVGPoint right now. We need the
distance for every single value.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPoint.h
File Source/core/svg/SVGPoint.h (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPoint.h#n...
Source/core/svg/SVGPoint.h:60: void setValue(const FloatPoint& v) { m_value = v;
}

v => value

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPoint.h#n...
Source/core/svg/SVGPoint.h:83: SVGPoint(const FloatPoint&);

Add explicit.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPointList...
File Source/core/svg/SVGPointList.cpp (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPointList...
Source/core/svg/SVGPointList.cpp:49: RefPtr<SVGPointList> ret =
SVGPointList::create();

ret => svgPointList

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPointList...
Source/core/svg/SVGPointList.cpp:56: RefPtr<SVGPointList> ret =
SVGPointList::create();

Ditto.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPointList...
Source/core/svg/SVGPointList.cpp:88: for (;;) {

You can change this to:

  while(ptr < end) { ... }

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPointList...
Source/core/svg/SVGPointList.cpp:98: if (ptr < end && *ptr == ',') {

The previous code has |delimParsed| but the new code does not have
|delimParsed|. Is it OK?

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPointList.h
File Source/core/svg/SVGPointList.h (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPointList...
Source/core/svg/SVGPointList.h:68: explicit SVGPointList();

Drop explicit.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGTextConte...
File Source/core/svg/SVGTextContentElement.cpp (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGTextConte...
Source/core/svg/SVGTextContentElement.cpp:166: // FIXME

Add some description.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGZoomEvent...
File Source/core/svg/SVGZoomEvent.idl (left):

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGZoomEvent...
Source/core/svg/SVGZoomEvent.idl:29: [Immutable] readonly attribute SVGPoint
previousTranslate;

I don't know what [Immutable] was for, but it is nice we can remove it :)

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/properties/N...
File Source/core/svg/properties/NewSVGPropertyTearOff.h (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/properties/N...
Source/core/svg/properties/NewSVGPropertyTearOff.h:84: virtual void
commitChange();

Add OVERRIDE.

[kouhei (in TOK), 2014-01-20 01:10:25]

https://codereview.chromium.org/132233016/diff/1/Source/core/rendering/svg/SV...
File Source/core/rendering/svg/SVGPathData.cpp (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/rendering/svg/SV...
Source/core/rendering/svg/SVGPathData.cpp:88: path.addLineTo(it->value());
On 2014/01/17 11:50:18, haraken wrote:
> 
> In the previous code, we didn't call addLineTo for the first element. However,
> in the new code, we're calling addLineTo for the first element. Is it OK?
We are not calling addLineTo for the first element. See line 85.

https://codereview.chromium.org/132233016/diff/1/Source/core/rendering/svg/SV...
Source/core/rendering/svg/SVGPathData.cpp:93:
updatePathFromPolylineElement(element, path);
On 2014/01/17 11:50:18, haraken wrote:
> 
> What's the advantage of introducing this static method?

This is for <polygon> element. The above is for <polyline> element.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGAnimatedN...
File Source/core/svg/SVGAnimatedNewPropertyAnimator.cpp (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGAnimatedN...
Source/core/svg/SVGAnimatedNewPropertyAnimator.cpp:87: case AnimatedRect:
On 2014/01/17 11:50:18, haraken wrote:
> 
> Alphabetical order please.

Done.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPoint.cpp
File Source/core/svg/SVGPoint.cpp (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPoint.cpp...
Source/core/svg/SVGPoint.cpp:102: if (string.isNull() || string.isEmpty()) {
On 2014/01/17 11:50:18, haraken wrote:
> 
> string.isEmpty() implies string.isNull(), so you don't need string.isNull().

Done.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPoint.cpp...
Source/core/svg/SVGPoint.cpp:142: // SVGPoint is not animated by itself
On 2014/01/17 11:50:18, haraken wrote:
> 
> Shouldn't this be a FIXME?
> 
> FIXME: Distance calculation is not possible for SVGPoint right now. We need
the
> distance for every single value.

This shouldn't be a FIXME. SVGPoint is never animated by itself (only its list
is animated), so calculateDistance is not needed. This is used for paced
animation.
http://www.w3.org/TR/SVG/animate.html#complexDistances

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPoint.h
File Source/core/svg/SVGPoint.h (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPoint.h#n...
Source/core/svg/SVGPoint.h:83: SVGPoint(const FloatPoint&);
On 2014/01/17 11:50:18, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPointList...
File Source/core/svg/SVGPointList.cpp (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPointList...
Source/core/svg/SVGPointList.cpp:49: RefPtr<SVGPointList> ret =
SVGPointList::create();
On 2014/01/17 11:50:18, haraken wrote:
> 
> ret => svgPointList

Done.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPointList...
Source/core/svg/SVGPointList.cpp:56: RefPtr<SVGPointList> ret =
SVGPointList::create();
On 2014/01/17 11:50:18, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPointList...
Source/core/svg/SVGPointList.cpp:88: for (;;) {
On 2014/01/17 11:50:18, haraken wrote:
> 
> You can change this to:
> 
>   while(ptr < end) { ... }

No.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPointList...
Source/core/svg/SVGPointList.cpp:98: if (ptr < end && *ptr == ',') {
On 2014/01/17 11:50:18, haraken wrote:
> 
> The previous code has |delimParsed| but the new code does not have
> |delimParsed|. Is it OK?

Yes. The logic we want to have here is to require list to have next point when
',' is detected.

Here, end of list check is only done when ',' isn't detected. When ',' is
detected, the execution will "continue" to next parseNumber, and it will fail
when it get end of list.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPointList.h
File Source/core/svg/SVGPointList.h (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGPointList...
Source/core/svg/SVGPointList.h:68: explicit SVGPointList();
On 2014/01/17 11:50:18, haraken wrote:
> 
> Drop explicit.

Done.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGTextConte...
File Source/core/svg/SVGTextContentElement.cpp (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/SVGTextConte...
Source/core/svg/SVGTextContentElement.cpp:166: // FIXME
On 2014/01/17 11:50:18, haraken wrote:
> 
> Add some description.
Removed. This was a temporary fix needed before nbarth's strict null checking
CL.

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/properties/N...
File Source/core/svg/properties/NewSVGPropertyTearOff.h (right):

https://codereview.chromium.org/132233016/diff/1/Source/core/svg/properties/N...
Source/core/svg/properties/NewSVGPropertyTearOff.h:84: virtual void
commitChange();
On 2014/01/17 11:50:18, haraken wrote:
> 
> Add OVERRIDE.
No this is the base class. Added OVERRIDE to SVGCurrentTranslateTearOff

[commit-bot: I haz the power, 2014-01-24 01:31:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/132233016/290001

[commit-bot: I haz the power, 2014-01-24 03:19:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/132233016/430001

[commit-bot: I haz the power, 2014-01-24 04:43:51]

Change committed as 165697