[SVG] SVGLength{,List} migration to new SVG property impl.

[SVG] SVGLength{,List} migration to new SVG property impl.

This CL replaces SVG{Animated,}Length{,List} classes with NewSVGProperty implementation.

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

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

[kouhei (in TOK), 2013-12-20 08:52:16]

haraken, pdr, nbarth: Here is a feature complete SVGLength{,List} migration
patch. I'd appreciate it if you would give a look.

Also, this patch would break some SVGLengthList tests from === incompatibility.
Any thoughts how we should handle this?

[pdr., 2014-01-07 06:42:52]

Overall this looks fantastic.

For the tests that now fail completely, you may be able to just add them to
LayoutTests/NeverFixTests. If any of the === tests still have meaningful
coverage I would recommend commenting out the === parts (maybe with a comment
linking to your bug) and updating the expectations accordingly.

This patch has some tricky performance areas that are easy to overlook in
review. Could you run the SVG performance tests with and without the patch to
verify that we haven't overlooked anything major?

https://codereview.chromium.org/112003003/diff/180001/Source/core/rendering/s...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.h:1378: SVGLength* strokeWidth() const {
return svgStyle()->strokeWidth(); }
Could this be introducing UAFs if the existing code gets the strokeWidth then
changes the renderstyle?

https://codereview.chromium.org/112003003/diff/180001/Source/core/rendering/s...
File Source/core/rendering/svg/SVGRenderSupport.cpp (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/rendering/s...
Source/core/rendering/svg/SVGRenderSupport.cpp:370: size_t length =
dashes->numberOfItems();
This may introduce a perf regression due to bounds checking on dashes->at().

Probably not for this patch, but do you think an iterator interface could be
added to NewSVGListPropertyHelper?

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:780: propertyTypes.append(AnimatedNewProperty);
// FIXME
Does this need fixed?

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.cpp:138: RefPtr<SVGLength> length = create();
This function may be hot on performance. It could be useful to add a private
constructor that takes an SVGLength reference (to avoid setting the member
variables in the ctor), but I'm not completely sure.

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLength.h (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:2: * Copyright (C) 2013 Google Inc. All rights
reserved.
Can you check that we're able to change this copyright header? (here, and
elsewhere).

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:88: virtual String valueAsString() const;
Please add OVERRIDE (here, and elsewhere)
virtual String valueAsString() const OVERRIDE;

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:127: // FIXME: add type checking
(here, and elsewhere in this patch)
Can you implement the type checking fixmes?

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:165: SVGLengthTearOff(PassRefPtr<SVGLength>,
SVGElement* contextElement, PropertyIsAnimValType, const QualifiedName&
attributeName = nullQName());
Can you check whether this should be anyQName() or nullQName()?

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthList.cpp (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthList.cpp:74: unsigned size = m_values.size();
Using an iterator here will be faster due to bounds checking.

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGRadi...
File Source/core/svg/SVGRadialGradientElement.cpp (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGRadi...
Source/core/svg/SVGRadialGradientElement.cpp:57: // FIXME: "fr" attribute is not
mentioned anywhere in the spec.
This fixme is incorrect. The fr attribute was recently added to SVG2:
https://svgwg.org/svg2-draft/single-page.html#pservers-RadialGradientElementF...

0% is the correct default value.

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGText...
File Source/core/svg/SVGTextContentElement.cpp (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGText...
Source/core/svg/SVGTextContentElement.cpp:52: SVGTextContentElement*
textContentElement = static_cast<SVGTextContentElement*>(contextElement());
Please use toSVGTextContentElement instead of a static_cast.

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGText...
Source/core/svg/SVGTextContentElement.cpp:257: if (attrName ==
SVGNames::textLengthAttr)
Isn't this handled in parseAttribute(...)?

[kouhei (in TOK), 2014-01-07 09:22:26]

Rebased against master.

Thanks for your comments! I'll continue the work tomorrow.

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:780: propertyTypes.append(AnimatedNewProperty);
// FIXME
On 2014/01/07 06:42:53, pdr wrote:
> Does this need fixed?
This is fixed in http://src.chromium.org/viewvc/blink?view=rev&rev=164326

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLength.h (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:2: * Copyright (C) 2013 Google Inc. All rights
reserved.
On 2014/01/07 06:42:53, pdr wrote:
> Can you check that we're able to change this copyright header? (here, and
> elsewhere).

I'll revert these copyright changes. These were accidentally carried over from
the last WIP patch where it used "NewSVGLength.h"

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:88: virtual String valueAsString() const;
On 2014/01/07 06:42:53, pdr wrote:
> Please add OVERRIDE (here, and elsewhere)
> virtual String valueAsString() const OVERRIDE;

Done.

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:127: // FIXME: add type checking
On 2014/01/07 06:42:53, pdr wrote:
> (here, and elsewhere in this patch)
> Can you implement the type checking fixmes?

Done.

[kouhei (in TOK), 2014-01-08 08:12:06]

I think I addressed all comments now. PTAL.
I'm currently checking ASAN builds and perf regressions.

> For the tests that now fail completely, you may be able to just add them to
> LayoutTests/NeverFixTests. If any of the === tests still have meaningful
> coverage I would recommend commenting out the === parts (maybe with a comment
> linking to your bug) and updating the expectations accordingly.
I changed js-test.js to compare SVGLength by its valueAsString to make tests
pass. I'm not sure if this is a right solution. wdyt?

https://codereview.chromium.org/112003003/diff/180001/Source/core/rendering/s...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/rendering/s...
Source/core/rendering/style/RenderStyle.h:1378: SVGLength* strokeWidth() const {
return svgStyle()->strokeWidth(); }
On 2014/01/07 06:42:53, pdr wrote:
> Could this be introducing UAFs if the existing code gets the strokeWidth then
> changes the renderstyle?

Changed to PassRefPtr<> for safety.
I'm a bit worried if ShapeValue and others also implemented as RefCounted would
have same problem.

https://codereview.chromium.org/112003003/diff/180001/Source/core/rendering/s...
File Source/core/rendering/svg/SVGRenderSupport.cpp (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/rendering/s...
Source/core/rendering/svg/SVGRenderSupport.cpp:370: size_t length =
dashes->numberOfItems();
On 2014/01/07 06:42:53, pdr wrote:
> This may introduce a perf regression due to bounds checking on dashes->at().
> 
> Probably not for this patch, but do you think an iterator interface could be
> added to NewSVGListPropertyHelper?
Done. Added NewSVGListPropertyHelper<>::ConstIterator

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLength.cpp (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.cpp:138: RefPtr<SVGLength> length = create();
On 2014/01/07 06:42:53, pdr wrote:
> This function may be hot on performance. It could be useful to add a private
> constructor that takes an SVGLength reference (to avoid setting the member
> variables in the ctor), but I'm not completely sure.

Done.

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLength.h (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLength.h:165: SVGLengthTearOff(PassRefPtr<SVGLength>,
SVGElement* contextElement, PropertyIsAnimValType, const QualifiedName&
attributeName = nullQName());
On 2014/01/07 06:42:53, pdr wrote:
> Can you check whether this should be anyQName() or nullQName()?
I think nullQName is appropriate here, as we want to handle a non attached
tear-off made by svg.createSVGLength().

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthList.cpp (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthList.cpp:74: unsigned size = m_values.size();
On 2014/01/07 06:42:53, pdr wrote:
> Using an iterator here will be faster due to bounds checking.

Done.

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGRadi...
File Source/core/svg/SVGRadialGradientElement.cpp (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGRadi...
Source/core/svg/SVGRadialGradientElement.cpp:57: // FIXME: "fr" attribute is not
mentioned anywhere in the spec.
On 2014/01/07 06:42:53, pdr wrote:
> This fixme is incorrect. The fr attribute was recently added to SVG2:
>
https://svgwg.org/svg2-draft/single-page.html#pservers-RadialGradientElementF...
> 
> 0% is the correct default value.

Done.

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGText...
File Source/core/svg/SVGTextContentElement.cpp (right):

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGText...
Source/core/svg/SVGTextContentElement.cpp:52: SVGTextContentElement*
textContentElement = static_cast<SVGTextContentElement*>(contextElement());
On 2014/01/07 06:42:53, pdr wrote:
> Please use toSVGTextContentElement instead of a static_cast.

Done.

https://codereview.chromium.org/112003003/diff/180001/Source/core/svg/SVGText...
Source/core/svg/SVGTextContentElement.cpp:257: if (attrName ==
SVGNames::textLengthAttr)
On 2014/01/07 06:42:53, pdr wrote:
> Isn't this handled in parseAttribute(...)?

Done. Removed flag flip in parseAttribute().

[kouhei (in TOK), 2014-01-08 08:50:26]

Perf tests look fine:
https://drive.google.com/file/d/0B5o-0Snm1_rFZ1BoeEo4YjhPS00/edit?usp=sharing

Now running ASAN layout tests.

[pdr., 2014-01-08 23:02:39]

On 2014/01/08 08:50:26, kouhei wrote:
> Perf tests look fine:
> https://drive.google.com/file/d/0B5o-0Snm1_rFZ1BoeEo4YjhPS00/edit?usp=sharing
> 
> Now running ASAN layout tests.

As long as ASAN is happy, I think this looks good. LGTM.

@haraken, @nbarth, could one of you give this patch a once-over as well?

[haraken, 2014-01-09 00:58:51]

I just scanned the CL. LGTM.

https://codereview.chromium.org/112003003/diff/430001/Source/core/rendering/s...
File Source/core/rendering/svg/SVGTextLayoutEngine.cpp (right):

https://codereview.chromium.org/112003003/diff/430001/Source/core/rendering/s...
Source/core/rendering/svg/SVGTextLayoutEngine.cpp:210: if
(textContentElement->textLengthIsSpecifiedByUser())

Don't you need to add:

  else {
    desiredTextLength = 0;
  }

?

https://codereview.chromium.org/112003003/diff/430001/Source/core/svg/SVGAnim...
File Source/core/svg/SVGAnimatedLength.cpp (right):

https://codereview.chromium.org/112003003/diff/430001/Source/core/svg/SVGAnim...
Source/core/svg/SVGAnimatedLength.cpp:2: * Copyright (C) 2013 Google Inc. All
rights reserved.

Nit: 2014

The same comment for all other newly added files.

https://codereview.chromium.org/112003003/diff/430001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthList.h (right):

https://codereview.chromium.org/112003003/diff/430001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthList.h:68: SVGLengthList(SVGLengthMode);

Add explicit.

[kouhei (in TOK), 2014-01-09 02:24:49]

Thanks for review! I tested the patch on local ASAN and not seeing any problems.
I'll check the CQ after seeing results from ASAN trybots.

https://codereview.chromium.org/112003003/diff/430001/Source/core/rendering/s...
File Source/core/rendering/svg/SVGTextLayoutEngine.cpp (right):

https://codereview.chromium.org/112003003/diff/430001/Source/core/rendering/s...
Source/core/rendering/svg/SVGTextLayoutEngine.cpp:210: if
(textContentElement->textLengthIsSpecifiedByUser())
On 2014/01/09 00:58:53, haraken wrote:
> 
> Don't you need to add:
> 
>   else {
>     desiredTextLength = 0;
>   }
> 
> ?

Done.

https://codereview.chromium.org/112003003/diff/430001/Source/core/svg/SVGAnim...
File Source/core/svg/SVGAnimatedLength.cpp (right):

https://codereview.chromium.org/112003003/diff/430001/Source/core/svg/SVGAnim...
Source/core/svg/SVGAnimatedLength.cpp:2: * Copyright (C) 2013 Google Inc. All
rights reserved.
On 2014/01/09 00:58:53, haraken wrote:
> 
> Nit: 2014
> 
> The same comment for all other newly added files.

Done. Happy new year!

https://codereview.chromium.org/112003003/diff/430001/Source/core/svg/SVGLeng...
File Source/core/svg/SVGLengthList.h (right):

https://codereview.chromium.org/112003003/diff/430001/Source/core/svg/SVGLeng...
Source/core/svg/SVGLengthList.h:68: SVGLengthList(SVGLengthMode);
On 2014/01/09 00:58:53, haraken wrote:
> 
> Add explicit.

Done.

[commit-bot: I haz the power, 2014-01-09 08:44:52]

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

[commit-bot: I haz the power, 2014-01-09 11:46:19]

Retried try job too often on mac_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[commit-bot: I haz the power, 2014-01-10 00:34:51]

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

[commit-bot: I haz the power, 2014-01-10 00:36:05]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 857.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
789501710618194e17f9ccf36b40485177538bb1..e649e840ffadebc3bed9fe7d6f18e34666f3f23a
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -857,3 +857,11 @@ crbug.com/331186 [ Debug ]
css3/flexbox/position-absolute-child.html [ Slow ]
 crbug.com/331189
fast/forms/date-multiple-fields/date-multiple-fields-keyboard-events.html [
Failure ]
 crbug.com/331189
fast/forms/datetimelocal-multiple-fields/datetimelocal-multiple-fields-keyboard-events.html
[ Failure ]
 crbug.com/331189
fast/forms/week-multiple-fields/week-multiple-fields-keyboard-events.html [
Failure ]
+
+crbug.com/308818 [ Mac Win ]
http/tests/security/contentSecurityPolicy/img-blocked-no-gc-crash.html [
NeedsRebaseline ]
+crbug.com/308818 [ Mac Win ]
svg/dynamic-updates/SVGFEDropShadowElement-svgdom-shadow-opacity-prop.html [
NeedsRebaseline ]
+crbug.com/308818 [ Mac Win ]
svg/dynamic-updates/SVGFEDropShadowElement-svgdom-shadow-color-prop.html [
NeedsRebaseline ]
+crbug.com/308818 [ Mac Win ]
svg/dynamic-updates/SVGFEDropShadowElement-dom-shadow-color-attr.html [
NeedsRebaseline ]
+crbug.com/308818 [ Mac Win ]
svg/dynamic-updates/SVGFESpecularLightingElement-inherit-lighting-color-css-prop.html
[ NeedsRebaseline ]
+crbug.com/308818 [ Mac Win ]
svg/dynamic-updates/SVGFEDropShadowElement-dom-shadow-opacity-attr.html [
NeedsRebaseline ]
+crbug.com/308818 [ Mac Win ]
svg/dynamic-updates/SVGFESpecularLightingElement-lighting-color-css-prop.html [
NeedsRebaseline ]

[commit-bot: I haz the power, 2014-01-10 03:20:52]

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

[commit-bot: I haz the power, 2014-01-10 06:00:23]

Change committed as 164832