[SVG] SVGAnimatedRect migration to new SVG property impl.

[SVG] SVGAnimatedRect migration to new SVG property impl.

This CL replaces SVGAnimatedRect class with NewSVGProperty implementation.

This CL changes the behavior of SVGViewSpec.viewBox readonly animated property.
In the previous implementation, changes to readonly property was silently ignored.
This CL uses new implementation of animated property which follows the spec.
It throws exception when they are tried to be modified.

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

BUG=308818
R=haraken@chromium.org, pdr@chromium.org

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

[kouhei (in TOK), 2014-01-16 02:13:58]

pdr, haraken: Would you take a look?

[pdr., 2014-01-16 03:25:46]

LGTM

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

https://codereview.chromium.org/132233010/diff/1/Source/core/svg/SVGElement.h...
Source/core/svg/SVGElement.h:183: friend class SVGFitToViewBox;
Any reason this is listed here instead of at the top?

https://codereview.chromium.org/132233010/diff/1/Source/core/svg/SVGFitToView...
File Source/core/svg/SVGFitToViewBox.h (right):

https://codereview.chromium.org/132233010/diff/1/Source/core/svg/SVGFitToView...
Source/core/svg/SVGFitToViewBox.h:55: return true;
Should these return false instead of true?

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

https://codereview.chromium.org/132233010/diff/1/Source/core/svg/SVGRect.cpp#...
Source/core/svg/SVGRect.cpp:158: // FIXME: Distance calculation is not possible
for SVGRect right now. We need the distance of for every single value.
// FIXME: Distance calculation is not possible for SVGRect right now. We need
the distance for every single value.

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

https://codereview.chromium.org/132233010/diff/1/Source/core/svg/properties/N...
Source/core/svg/properties/NewSVGAnimatedProperty.h:212: // Use currentValue()
from C++ code.
Should this comment be updated?

[kouhei (in TOK), 2014-01-16 03:42:26]

Thanks for the review!

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

https://codereview.chromium.org/132233010/diff/1/Source/core/svg/SVGElement.h...
Source/core/svg/SVGElement.h:183: friend class SVGFitToViewBox;
On 2014/01/16 03:25:47, pdr wrote:
> Any reason this is listed here instead of at the top?

I wanted to be explicit that SVGFitToViewBox is made a friend class just to
allow use of "reportAttributeParsingError". I added a comment here.

In future, reportAttributeParsingError should be removed and parsing errors
should be reported via ExceptionState.

https://codereview.chromium.org/132233010/diff/1/Source/core/svg/SVGFitToView...
File Source/core/svg/SVGFitToViewBox.h (right):

https://codereview.chromium.org/132233010/diff/1/Source/core/svg/SVGFitToView...
Source/core/svg/SVGFitToViewBox.h:55: return true;
On 2014/01/16 03:25:47, pdr wrote:
> Should these return false instead of true?

This is tricky, but these must return true. This function returns true when it
parsed an attribute with a known name, even if the parse failed.

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

https://codereview.chromium.org/132233010/diff/1/Source/core/svg/SVGRect.cpp#...
Source/core/svg/SVGRect.cpp:158: // FIXME: Distance calculation is not possible
for SVGRect right now. We need the distance of for every single value.
On 2014/01/16 03:25:47, pdr wrote:
> // FIXME: Distance calculation is not possible for SVGRect right now. We need
> the distance for every single value.

Done.

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

https://codereview.chromium.org/132233010/diff/1/Source/core/svg/properties/N...
Source/core/svg/properties/NewSVGAnimatedProperty.h:212: // Use currentValue()
from C++ code.
On 2014/01/16 03:25:47, pdr wrote:
> Should this comment be updated?

I think they are fine. currentValue() definition is in the parent class
NewSVGAnimatedPropertyCommon<Property>.

[commit-bot: I haz the power, 2014-01-16 03:42:45]

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

[commit-bot: I haz the power, 2014-01-16 04:04:25]

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

[kouhei (in TOK), 2014-01-16 04:05:15]

haraken: Would you rubberstamp? Thanks.

[haraken, 2014-01-16 04:06:23]

On 2014/01/16 04:05:15, kouhei wrote:
> haraken: Would you rubberstamp? Thanks.

rubberstamp LGTM.

I'm a bit busy now, so let me take a detailed look in hours.

[commit-bot: I haz the power, 2014-01-16 04:08:40]

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

[haraken, 2014-01-16 04:56:42]

LGTM.

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGFitT...
File Source/core/svg/SVGFitToViewBox.h (right):

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGFitT...
Source/core/svg/SVGFitToViewBox.h:52: if
(target->viewBox()->baseValue()->width() < 0.0f) { // check that width is
positive

Nit: Remove the comment.

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGFitT...
Source/core/svg/SVGFitToViewBox.h:57: if
(target->viewBox()->baseValue()->height() < 0.0f) { // check that height is
positive

Nit: Remove the comment.

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGView...
File Source/core/svg/SVGViewSpec.cpp (right):

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGView...
Source/core/svg/SVGViewSpec.cpp:200: if (!(parseNumber(ptr, end, x) &&
parseNumber(ptr, end, y) && parseNumber(ptr, end, width) && parseNumber(ptr,
end, height, false)))

Nit: Why do we need 'false' when parsing 'height'?

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGView...
File Source/core/svg/SVGViewSpec.h (right):

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGView...
Source/core/svg/SVGViewSpec.h:73: SVGAnimatedRect* viewBox() { return
m_contextElement ? m_viewBox.get() : 0; }

Isn't it a bit dangerous to allow m_viewBox to hold a non-NULL value even though
m_contextElement is cleared? I guess we should clear m_viewBox when
m_contextElement is cleared.

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGZoom...
File Source/core/svg/SVGZoomEvent.cpp (right):

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGZoom...
Source/core/svg/SVGZoomEvent.cpp:23: 

Nit: This empty line is not needed.

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGZoom...
Source/core/svg/SVGZoomEvent.cpp:41: rectTearOff->setIsReadOnlyProperty();

Just help me understand: Why was previous code working without setting
read-only?

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/propert...
File Source/core/svg/properties/NewSVGAnimatedProperty.h (right):

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/propert...
Source/core/svg/properties/NewSVGAnimatedProperty.h:218:
m_baseValTearOff->setIsReadOnlyProperty();

Ditto.

[kouhei (in TOK), 2014-01-16 05:12:36]

Thanks for your comments. I'll queue this after I try the m_viewBox lifetime
issue.

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGFitT...
File Source/core/svg/SVGFitToViewBox.h (right):

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGFitT...
Source/core/svg/SVGFitToViewBox.h:52: if
(target->viewBox()->baseValue()->width() < 0.0f) { // check that width is
positive
On 2014/01/16 04:56:43, haraken wrote:
> 
> Nit: Remove the comment.

Done.

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGFitT...
Source/core/svg/SVGFitToViewBox.h:57: if
(target->viewBox()->baseValue()->height() < 0.0f) { // check that height is
positive
On 2014/01/16 04:56:43, haraken wrote:
> 
> Nit: Remove the comment.

Done.

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGView...
File Source/core/svg/SVGViewSpec.cpp (right):

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGView...
Source/core/svg/SVGViewSpec.cpp:200: if (!(parseNumber(ptr, end, x) &&
parseNumber(ptr, end, y) && parseNumber(ptr, end, width) && parseNumber(ptr,
end, height, false)))
On 2014/01/16 04:56:43, haraken wrote:
> 
> Nit: Why do we need 'false' when parsing 'height'?

This skips removing spaces at the end.

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGView...
File Source/core/svg/SVGViewSpec.h (right):

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGView...
Source/core/svg/SVGViewSpec.h:73: SVGAnimatedRect* viewBox() { return
m_contextElement ? m_viewBox.get() : 0; }
On 2014/01/16 04:56:43, haraken wrote:
> 
> Isn't it a bit dangerous to allow m_viewBox to hold a non-NULL value even
though
> m_contextElement is cleared? I guess we should clear m_viewBox when
> m_contextElement is cleared.
Ack. I'll try that.

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGZoom...
File Source/core/svg/SVGZoomEvent.cpp (right):

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGZoom...
Source/core/svg/SVGZoomEvent.cpp:41: rectTearOff->setIsReadOnlyProperty();
On 2014/01/16 04:56:43, haraken wrote:
> 
> Just help me understand: Why was previous code working without setting
> read-only?

Because it was violating the spec. ^p^

[kouhei (in TOK), 2014-01-16 06:07:58]

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGView...
File Source/core/svg/SVGViewSpec.h (right):

https://codereview.chromium.org/132233010/diff/160001/Source/core/svg/SVGView...
Source/core/svg/SVGViewSpec.h:73: SVGAnimatedRect* viewBox() { return
m_contextElement ? m_viewBox.get() : 0; }
On 2014/01/16 05:12:37, kouhei wrote:
> On 2014/01/16 04:56:43, haraken wrote:
> > 
> > Isn't it a bit dangerous to allow m_viewBox to hold a non-NULL value even
> though
> > m_contextElement is cleared? I guess we should clear m_viewBox when
> > m_contextElement is cleared.
> Ack. I'll try that.

Chatted offline. WeakPtr do not provide "on-clear" callback, so it is a bit
difficult to implement the idea. m_viewBox is owned by SVGViewSpec, so it is
ensured to be alive. The tearoff is also safe as its contextElement is kept
alive from the wrapper.

"return null" was removed as there was no such spec. Modified the LayoutTest
case which tests this.

[commit-bot: I haz the power, 2014-01-16 06:45:04]

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

[commit-bot: I haz the power, 2014-01-16 07:32:05]

Retried try job too often on linux_blink_rel for step(s)
blink_platform_unittests, webkit_lint, webkit_python_tests, webkit_tests,
webkit_unit_tests, wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[commit-bot: I haz the power, 2014-01-16 08:35:43]

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

[commit-bot: I haz the power, 2014-01-16 09:13:53]

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

[commit-bot: I haz the power, 2014-01-17 07:42:30]

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

[commit-bot: I haz the power, 2014-01-17 10:46:15]

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-20 01:15:53]

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

[commit-bot: I haz the power, 2014-01-20 01:50:38]

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

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

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

[commit-bot: I haz the power, 2014-01-20 02:42:34]

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

[commit-bot: I haz the power, 2014-01-20 02:55:23]

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

[commit-bot: I haz the power, 2014-01-20 03:21:28]

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

[kouhei (in TOK), 2014-01-20 05:50:48]

Committed patchset #7 manually as r165382 (presubmit successful).