[SVG] SVGAnimatedBoolean migration to new SVG property impl.

[SVG] SVGAnimatedBoolean migration to new SVG property impl.

This CL replaces SVGAnimatedBoolean class 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=165053

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

haraken: Would you take a look?

[haraken, 2014-01-09 11:09:22]

I want to have pdr@ to take a look at changes to NewSVGAnimatedProperty.

https://codereview.chromium.org/131253002/diff/1/Source/core/svg/SVGAnimatedT...
File Source/core/svg/SVGAnimatedType.cpp (right):

https://codereview.chromium.org/131253002/diff/1/Source/core/svg/SVGAnimatedT...
Source/core/svg/SVGAnimatedType.cpp:79: case AnimatedBoolean:

Add a comment:

// Below properties have migrated to new property implementation.

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

https://codereview.chromium.org/131253002/diff/1/Source/core/svg/SVGBoolean.c...
Source/core/svg/SVGBoolean.cpp:44: RefPtr<SVGBoolean> ret = create();

ret => svgBoolean

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

https://codereview.chromium.org/131253002/diff/1/Source/core/svg/SVGBoolean.h...
Source/core/svg/SVGBoolean.h:40: // SVGBoolean do not have a tear-off type.

do not => does not

https://codereview.chromium.org/131253002/diff/1/Source/core/svg/SVGBoolean.h...
Source/core/svg/SVGBoolean.h:78: inline PassRefPtr<SVGBoolean>
toSVGBoolean(PassRefPtr<NewSVGPropertyBase> passBase)

It looks a bit strange that toXXX takes a PassRefPtr and returns a PassRefPtr. I
think normally toXXX takes a raw pointer and returns a raw pointer.

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

https://codereview.chromium.org/131253002/diff/1/Source/core/svg/properties/N...
Source/core/svg/properties/NewSVGAnimatedProperty.h:178: // Implementation of
SVGAnimatedProperty which have tear-off value types.

have => has

https://codereview.chromium.org/131253002/diff/1/Source/core/svg/properties/N...
Source/core/svg/properties/NewSVGAnimatedProperty.h:250: // Implementation of
SVGAnimatedProperty which use primitive types.

use => uses

[kouhei (in TOK), 2014-01-10 08:16:10]

pdr: Would you take a look at NewSVGAnimatedProperty.h?

The change is for supporting what previously was called
"SVGAnimatedStaticProperties," which returns non-TearOff primitive types for
"animVal".

https://codereview.chromium.org/131253002/diff/1/Source/core/svg/SVGAnimatedT...
File Source/core/svg/SVGAnimatedType.cpp (right):

https://codereview.chromium.org/131253002/diff/1/Source/core/svg/SVGAnimatedT...
Source/core/svg/SVGAnimatedType.cpp:79: case AnimatedBoolean:
On 2014/01/09 11:09:23, haraken wrote:
> 
> Add a comment:
> 
> // Below properties have migrated to new property implementation.

Done.

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

https://codereview.chromium.org/131253002/diff/1/Source/core/svg/SVGBoolean.c...
Source/core/svg/SVGBoolean.cpp:44: RefPtr<SVGBoolean> ret = create();
On 2014/01/09 11:09:23, haraken wrote:
> 
> ret => svgBoolean

Done.

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

https://codereview.chromium.org/131253002/diff/1/Source/core/svg/SVGBoolean.h...
Source/core/svg/SVGBoolean.h:40: // SVGBoolean do not have a tear-off type.
On 2014/01/09 11:09:23, haraken wrote:
> 
> do not => does not

Done.

https://codereview.chromium.org/131253002/diff/1/Source/core/svg/SVGBoolean.h...
Source/core/svg/SVGBoolean.h:78: inline PassRefPtr<SVGBoolean>
toSVGBoolean(PassRefPtr<NewSVGPropertyBase> passBase)
On 2014/01/09 11:09:23, haraken wrote:
> 
> It looks a bit strange that toXXX takes a PassRefPtr and returns a PassRefPtr.
I
> think normally toXXX takes a raw pointer and returns a raw pointer.

Discussed offline. Keeping this.

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

https://codereview.chromium.org/131253002/diff/1/Source/core/svg/properties/N...
Source/core/svg/properties/NewSVGAnimatedProperty.h:178: // Implementation of
SVGAnimatedProperty which have tear-off value types.
On 2014/01/09 11:09:23, haraken wrote:
> 
> have => has

Done.

https://codereview.chromium.org/131253002/diff/1/Source/core/svg/properties/N...
Source/core/svg/properties/NewSVGAnimatedProperty.h:250: // Implementation of
SVGAnimatedProperty which use primitive types.
On 2014/01/09 11:09:23, haraken wrote:
> 
> use => uses

Done.

[pdr., 2014-01-10 18:57:32]

LGTM, another great change.


Just two optional comments.

https://codereview.chromium.org/131253002/diff/80001/Source/core/svg/SVGFECon...
File Source/core/svg/SVGFEConvolveMatrixElement.h (right):

https://codereview.chromium.org/131253002/diff/80001/Source/core/svg/SVGFECon...
Source/core/svg/SVGFEConvolveMatrixElement.h:73: SVGAnimatedBoolean*
preserveAlpha() const { return m_preserveAlpha.get(); }
Can we refptr-ize this return?

https://codereview.chromium.org/131253002/diff/80001/Source/core/svg/properti...
File Source/core/svg/properties/NewSVGAnimatedProperty.h (right):

https://codereview.chromium.org/131253002/diff/80001/Source/core/svg/properti...
Source/core/svg/properties/NewSVGAnimatedProperty.h:85: bool isReadOnly() const
Just FYI: I think these isReadOnly/setReadOnly methods are unused.

You're just translating this from the old tear off to the new one, so no need to
fix this now.

[haraken, 2014-01-14 03:32:46]

https://codereview.chromium.org/131253002/diff/80001/Source/core/svg/SVGFECon...
File Source/core/svg/SVGFEConvolveMatrixElement.h (right):

https://codereview.chromium.org/131253002/diff/80001/Source/core/svg/SVGFECon...
Source/core/svg/SVGFEConvolveMatrixElement.h:73: SVGAnimatedBoolean*
preserveAlpha() const { return m_preserveAlpha.get(); }
On 2014/01/10 18:57:32, pdr wrote:
> Can we refptr-ize this return?

I don't have a strong opinion here, but as far as I see other code in Blink, we
return raw pointers from getters when it is clear that raw pointers are OK.

[kouhei (in TOK), 2014-01-14 04:12:50]

Thanks for review!

https://codereview.chromium.org/131253002/diff/80001/Source/core/svg/properti...
File Source/core/svg/properties/NewSVGAnimatedProperty.h (right):

https://codereview.chromium.org/131253002/diff/80001/Source/core/svg/properti...
Source/core/svg/properties/NewSVGAnimatedProperty.h:85: bool isReadOnly() const
On 2014/01/10 18:57:32, pdr wrote:
> Just FYI: I think these isReadOnly/setReadOnly methods are unused.
> 
> You're just translating this from the old tear off to the new one, so no need
to
> fix this now.

Yes. These were unused, but I found some cases where they were not specified
while the spec says they should be specified. I'll use them in future CL.

[commit-bot: I haz the power, 2014-01-14 04:29:22]

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

[pdr., 2014-01-14 04:35:06]

On 2014/01/14 03:32:46, haraken wrote:
>
https://codereview.chromium.org/131253002/diff/80001/Source/core/svg/SVGFECon...
> File Source/core/svg/SVGFEConvolveMatrixElement.h (right):
> 
>
https://codereview.chromium.org/131253002/diff/80001/Source/core/svg/SVGFECon...
> Source/core/svg/SVGFEConvolveMatrixElement.h:73: SVGAnimatedBoolean*
> preserveAlpha() const { return m_preserveAlpha.get(); }
> On 2014/01/10 18:57:32, pdr wrote:
> > Can we refptr-ize this return?
> 
> I don't have a strong opinion here, but as far as I see other code in Blink,
we
> return raw pointers from getters when it is clear that raw pointers are OK.

This sounds reasonable to me too. LGTM :)

[commit-bot: I haz the power, 2014-01-14 07:10:25]

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

[commit-bot: I haz the power, 2014-01-14 07:23:48]

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

[commit-bot: I haz the power, 2014-01-14 09:12:47]

Failed to apply patch for Source/core/svg/SVGTextContentElement.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/svg/SVGTextContentElement.cpp
  Hunk #1 FAILED at 47.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/svg/SVGTextContentElement.cpp.rej

Patch:       Source/core/svg/SVGTextContentElement.cpp
Index: Source/core/svg/SVGTextContentElement.cpp
diff --git a/Source/core/svg/SVGTextContentElement.cpp
b/Source/core/svg/SVGTextContentElement.cpp
index
0da391b7078dd1aa7648f1e8968014aafee00983..9bba473e1654971a8e142a391a5c6d95dceec226
100644
--- a/Source/core/svg/SVGTextContentElement.cpp
+++ b/Source/core/svg/SVGTextContentElement.cpp
@@ -47,7 +47,7 @@ public:
         return adoptRef(new SVGAnimatedTextLength(contextElement));
     }
 
-    virtual SVGLengthTearOff* baseVal()
+    virtual SVGLengthTearOff* baseVal() OVERRIDE
     {
         SVGTextContentElement* textContentElement =
toSVGTextContentElement(contextElement());
         if (!textContentElement->textLengthIsSpecifiedByUser())

[commit-bot: I haz the power, 2014-01-14 09:22:17]

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

[commit-bot: I haz the power, 2014-01-14 10:22:33]

Change committed as 165053