bindings: Makes SVGAnimatedProperty ScriptWrappable.

bindings: Makes SVGAnimatedProperty ScriptWrappable.

Makes SVGAnimatedProperty and its subclasses inherit from ScriptWrappable.
This change increases the size of SVGAnimatedProperty and its subclasses
by 8 bytes (on 64-bit env).

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

[Yuki, 2014-11-13 06:19:01]

yukishiino@chromium.org changed reviewers:
+ haraken@chromium.org, kouhei@chromium.org

[Yuki, 2014-11-13 06:19:02]

Could you guys review this CL?

[haraken, 2014-11-13 07:46:36]

haraken@chromium.org changed reviewers:
+ pdr@chromium.org, schenney@chromium.org

[haraken, 2014-11-13 07:46:38]

shiino-san: Would you evaluate memory impact of this CL again (using the SVG
benchmarks you used before)?

I'd like to ask pdr, schenney and kouhei for making a final decision.

https://codereview.chromium.org/720113003/diff/1/Source/bindings/core/v8/Scri...
File Source/bindings/core/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/720113003/diff/1/Source/bindings/core/v8/Scri...
Source/bindings/core/v8/ScriptWrappable.h:268: #define
DEFINE_WRAPPERTYPEINFO_NOT_REACHED() \

Since this macro is used only by the SVGAnimatedProperty hierarchy, I'd move the
macro to SVGAnimatedPropertyBase.h. (We don't want to use this hacky macro in
other places.)

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedL...
File Source/core/svg/SVGAnimatedLengthList.h (right):

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedL...
Source/core/svg/SVGAnimatedLengthList.h:40: class SVGAnimatedLengthList : public
SVGAnimatedProperty<SVGLengthList> {

Add final.

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedL...
Source/core/svg/SVGAnimatedLengthList.h:43: static
PassRefPtr<SVGAnimatedLengthList> create(SVGElement* contextElement, const
QualifiedName& attributeName, PassRefPtr<SVGLengthList> initialValue)

PassRefPtr => PassRefPtrWillBeRawPtr?

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedL...
Source/core/svg/SVGAnimatedLengthList.h:45: return adoptRef(new
SVGAnimatedLengthList(contextElement, attributeName, initialValue));

adoptRef => adoptRefWillBeNoop ?

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedL...
Source/core/svg/SVGAnimatedLengthList.h:49: SVGAnimatedLengthList(SVGElement*
contextElement, const QualifiedName& attributeName, PassRefPtr<SVGLengthList>
initialValue)

PassRefPtr => PassRefPtrWillBeRawPtr?

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedN...
File Source/core/svg/SVGAnimatedNumberList.h (right):

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedN...
Source/core/svg/SVGAnimatedNumberList.h:43: static
PassRefPtr<SVGAnimatedNumberList> create(SVGElement* contextElement, const
QualifiedName& attributeName, PassRefPtr<SVGNumberList> initialValue)

PassRefPtrWillBeRawPtr

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedN...
Source/core/svg/SVGAnimatedNumberList.h:45: return adoptRef(new
SVGAnimatedNumberList(contextElement, attributeName, initialValue));

adoptRefWillBeNoop

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedN...
Source/core/svg/SVGAnimatedNumberList.h:49: SVGAnimatedNumberList(SVGElement*
contextElement, const QualifiedName& attributeName, PassRefPtr<SVGNumberList>
initialValue)

PassRefPtrWillBeRawPtr

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedT...
File Source/core/svg/SVGAnimatedTransformList.h (right):

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedT...
Source/core/svg/SVGAnimatedTransformList.h:43: static
PassRefPtr<SVGAnimatedTransformList> create(SVGElement* contextElement, const
QualifiedName& attributeName, PassRefPtr<SVGTransformList> initialValue)

Ditto.

[Yuki, 2014-11-13 08:16:39]

Will do an evaluation of memory usage with this change.

https://codereview.chromium.org/720113003/diff/1/Source/bindings/core/v8/Scri...
File Source/bindings/core/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/720113003/diff/1/Source/bindings/core/v8/Scri...
Source/bindings/core/v8/ScriptWrappable.h:268: #define
DEFINE_WRAPPERTYPEINFO_NOT_REACHED() \
On 2014/11/13 07:46:37, haraken wrote:
> 
> Since this macro is used only by the SVGAnimatedProperty hierarchy, I'd move
the
> macro to SVGAnimatedPropertyBase.h. (We don't want to use this hacky macro in
> other places.)

The definition of DEFINE_WRAPPERTYPEINFO_NOT_REACHED must be always consistent
with DEFINE_WRAPPERTYPEINFO, so I'd like to put them very close to each other. 
Otherwise, it's quite hard to maintain them.

In general, consumers must not need to know the internals of what providers
provide.

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedL...
File Source/core/svg/SVGAnimatedLengthList.h (right):

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedL...
Source/core/svg/SVGAnimatedLengthList.h:40: class SVGAnimatedLengthList : public
SVGAnimatedProperty<SVGLengthList> {
On 2014/11/13 07:46:37, haraken wrote:
> 
> Add final.

Done.

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedL...
Source/core/svg/SVGAnimatedLengthList.h:43: static
PassRefPtr<SVGAnimatedLengthList> create(SVGElement* contextElement, const
QualifiedName& attributeName, PassRefPtr<SVGLengthList> initialValue)
On 2014/11/13 07:46:37, haraken wrote:
> 
> PassRefPtr => PassRefPtrWillBeRawPtr?

SVGAnimatedLength is using PassRefPtr.
Is it really okay to rewrite to PassRefPtrWillBeRawPtr?

[kouhei (in TOK), 2014-11-13 08:20:30]

lgtm as long as no regression on svg perf.

[Yuki, 2014-11-13 08:30:00]

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedL...
File Source/core/svg/SVGAnimatedLengthList.h (right):

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedL...
Source/core/svg/SVGAnimatedLengthList.h:43: static
PassRefPtr<SVGAnimatedLengthList> create(SVGElement* contextElement, const
QualifiedName& attributeName, PassRefPtr<SVGLengthList> initialValue)
On 2014/11/13 08:16:38, Yuki wrote:
> On 2014/11/13 07:46:37, haraken wrote:
> > 
> > PassRefPtr => PassRefPtrWillBeRawPtr?
> 
> SVGAnimatedLength is using PassRefPtr.
> Is it really okay to rewrite to PassRefPtrWillBeRawPtr?

Sorry, I synced up to the head, and I found things changed.
Follow your comments.

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedL...
Source/core/svg/SVGAnimatedLengthList.h:45: return adoptRef(new
SVGAnimatedLengthList(contextElement, attributeName, initialValue));
On 2014/11/13 07:46:37, haraken wrote:
> 
> adoptRef => adoptRefWillBeNoop ?

Done.

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedL...
Source/core/svg/SVGAnimatedLengthList.h:49: SVGAnimatedLengthList(SVGElement*
contextElement, const QualifiedName& attributeName, PassRefPtr<SVGLengthList>
initialValue)
On 2014/11/13 07:46:37, haraken wrote:
> 
> PassRefPtr => PassRefPtrWillBeRawPtr?

Done.

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedN...
File Source/core/svg/SVGAnimatedNumberList.h (right):

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedN...
Source/core/svg/SVGAnimatedNumberList.h:43: static
PassRefPtr<SVGAnimatedNumberList> create(SVGElement* contextElement, const
QualifiedName& attributeName, PassRefPtr<SVGNumberList> initialValue)
On 2014/11/13 07:46:38, haraken wrote:
> 
> PassRefPtrWillBeRawPtr

Done.

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedN...
Source/core/svg/SVGAnimatedNumberList.h:45: return adoptRef(new
SVGAnimatedNumberList(contextElement, attributeName, initialValue));
On 2014/11/13 07:46:38, haraken wrote:
> 
> adoptRefWillBeNoop

Done.

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedN...
Source/core/svg/SVGAnimatedNumberList.h:49: SVGAnimatedNumberList(SVGElement*
contextElement, const QualifiedName& attributeName, PassRefPtr<SVGNumberList>
initialValue)
On 2014/11/13 07:46:38, haraken wrote:
> 
> PassRefPtrWillBeRawPtr

Done.

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedT...
File Source/core/svg/SVGAnimatedTransformList.h (right):

https://codereview.chromium.org/720113003/diff/1/Source/core/svg/SVGAnimatedT...
Source/core/svg/SVGAnimatedTransformList.h:43: static
PassRefPtr<SVGAnimatedTransformList> create(SVGElement* contextElement, const
QualifiedName& attributeName, PassRefPtr<SVGTransformList> initialValue)
On 2014/11/13 07:46:38, haraken wrote:
> 
> Ditto.

Done.

[Yuki, 2014-11-13 09:39:14]

In terms of speed performance, I see no regression running
"tools/perf/run_benchmark blink_perf.svg".

In terms of memory usage, chrome://memory tells that it actually increases by 8
bytes per SVGAnimatedProperty instance.

For example, SierpinskiCarpet.svg (the largest SVG in our tests in terms of the
number of SVGAnimatedProperty instances) creates 1.2 mega instances, so the
memory usage increases by 9.6 (= 1.2 * 8) Mbytes.  As per chrome://memory, the
private memory increased by 2.4% (371Mbytes => 380Mbytes).

For the smallest example, FoodLeifLodahl.svg, which creates only 99 instances, I
cannot see the increase of memory usage at chrome://memory because it's too
small (18Mbytes => 18Mbytes).

[Yuki, 2014-11-13 10:03:03]

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

[pdr., 2014-11-14 01:45:04]

This is my first time reading about how ScriptWrappable works and reading the
docs around it so please forgive some ignorance. Why do we need ScriptWrappable
for the properties themselves when the tear offs (e.g., SVGRectTearOff) already
derive from ScriptWrappable?

[pdr., 2014-11-14 01:50:31]

On 2014/11/14 at 01:45:04, pdr wrote:
> This is my first time reading about how ScriptWrappable works and reading the
docs around it so please forgive some ignorance. Why do we need ScriptWrappable
for the properties themselves when the tear offs (e.g., SVGRectTearOff) already
derive from ScriptWrappable?

(I clearly don't know enough about this area to delay this change so please
proceed with landing this based on kouhei's review.)

[Yuki, 2014-11-14 04:17:09]

On 2014/11/14 01:50:31, pdr wrote:
> On 2014/11/14 at 01:45:04, pdr wrote:
> > This is my first time reading about how ScriptWrappable works and reading
the
> docs around it so please forgive some ignorance. Why do we need
ScriptWrappable
> for the properties themselves when the tear offs (e.g., SVGRectTearOff)
already
> derive from ScriptWrappable?
> 
> (I clearly don't know enough about this area to delay this change so please
> proceed with landing this based on kouhei's review.)

For your question, SVGRect.idl (implemented as SVGRectTearOff) and
SVGAnimatedRect.idl are simply different interfaces, and they're independent
(though, SVGAnimatedRect has SVGRects as properties).  We need wrappers for both
of them, and we'd like both of them to inherit from ScriptWrappable.

[Yuki, 2014-11-14 04:17:18]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2014-11-14 04:17:56]

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

[commit-bot: I haz the power, 2014-11-14 04:22:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-14 04:22:10]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[haraken, 2014-11-14 04:23:29]

LGTM

[haraken, 2014-11-14 04:23:35]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-11-14 04:24:21]

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

[commit-bot: I haz the power, 2014-11-14 04:27:11]

Committed patchset #4 (id:60001) as 185346