Get rid of special casing for SVGNumber in the bindings generator

Get rid of special casing for SVGNumber in the bindings generator

Get rid of special casing for SVGNumber in the bindings generator by
adding a new SVGNumber.h header containing a typedef from SVGNumber
to float. The 'value' attribute getter / setter for SVGNumber now
have a custom implementation since SVGNumber is not a class.

[haraken, 2013-07-15 10:59:15]

https://codereview.chromium.org/19154006/diff/3001/Source/bindings/v8/custom/...
File Source/bindings/v8/custom/V8SVGNumberCustom.cpp (right):

https://codereview.chromium.org/19154006/diff/3001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8SVGNumberCustom.cpp:37: void
V8SVGNumber::valueAttrGetterCustom(v8::Local<v8::String> name, const
v8::PropertyCallbackInfo<v8::Value>& info)

I guess this getter doesn't need to be custom. You can use
[ImplementedAs=propertyReference], or you can add a helper method
SVGNumber::value() that redirects to SVGNumber::propertyReference().

https://codereview.chromium.org/19154006/diff/3001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8SVGNumberCustom.cpp:54: wrapper->commitChange();

Would it be possible to get rid of the custom setter? (I'm not sure.) You might
be able to implement some helper method in SVGNumber and move the isReadOnly()
check, wrapper->propertyReference() and wrapper->commitChange() into the helper
method. Then we can use normal generated code.

[do-not-use, 2013-07-15 11:11:13]

https://codereview.chromium.org/19154006/diff/3001/Source/bindings/v8/custom/...
File Source/bindings/v8/custom/V8SVGNumberCustom.cpp (right):

https://codereview.chromium.org/19154006/diff/3001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8SVGNumberCustom.cpp:37: void
V8SVGNumber::valueAttrGetterCustom(v8::Local<v8::String> name, const
v8::PropertyCallbackInfo<v8::Value>& info)
On 2013/07/15 10:59:15, haraken wrote:
> 
> I guess this getter doesn't need to be custom. You can use
> [ImplementedAs=propertyReference], or you can add a helper method
> SVGNumber::value() that redirects to SVGNumber::propertyReference().

This does not work. SVGNumber is a typedef to float.
I tried the ImplementedAs approach and it resulted in the following code: 
    SVGPropertyTearOff<SVGNumber>* wrapper =
V8SVGNumber::toNative(info.Holder());
    SVGNumber& impInstance = wrapper->propertyReference();
    SVGNumber* imp = &impInstance;
    v8SetReturnValue(info, imp->propertyReference());

// imp->propertyReference() is wrong is imp is a float*.

I will see if I find a way to get rid of custom code though...

[do-not-use, 2013-07-15 11:15:42]

On 2013/07/15 11:11:13, Christophe Dumez wrote:
>
https://codereview.chromium.org/19154006/diff/3001/Source/bindings/v8/custom/...
> File Source/bindings/v8/custom/V8SVGNumberCustom.cpp (right):
> 
>
https://codereview.chromium.org/19154006/diff/3001/Source/bindings/v8/custom/...
> Source/bindings/v8/custom/V8SVGNumberCustom.cpp:37: void
> V8SVGNumber::valueAttrGetterCustom(v8::Local<v8::String> name, const
> v8::PropertyCallbackInfo<v8::Value>& info)
> On 2013/07/15 10:59:15, haraken wrote:
> > 
> > I guess this getter doesn't need to be custom. You can use
> > [ImplementedAs=propertyReference], or you can add a helper method
> > SVGNumber::value() that redirects to SVGNumber::propertyReference().
> 
> This does not work. SVGNumber is a typedef to float.
> I tried the ImplementedAs approach and it resulted in the following code: 
>     SVGPropertyTearOff<SVGNumber>* wrapper =
> V8SVGNumber::toNative(info.Holder());
>     SVGNumber& impInstance = wrapper->propertyReference();
>     SVGNumber* imp = &impInstance;
>     v8SetReturnValue(info, imp->propertyReference());
> 
> // imp->propertyReference() is wrong is imp is a float*.
> 
> I will see if I find a way to get rid of custom code though...

Note that I also tried to create a proper SVGNumber class and use
Vector<SVGNumber> for SVGNumberList. This works without custom code but would
become less efficient because WebCore expects Vector<float> and we need some
extra conversion from Vector<SVGNumber> to Vector<float>.

[do-not-use, 2013-07-15 12:07:25]

I am not quite sure how we can do better:

1. I don't think it is worth introducing a new extended attribute to generalize
this as this is specific to SVGNumber. This is basically an optimization
allowing the implementation to use float instead of a SVGNumber class. Custom
code for such specific case seems adequate to me.
2. Introducing a new SVGNumber class in the implementation and using it instead
of float would get rid of the custom code but revert the optimization. Namely,
we would need to convert from SVGNumberList (i.e. Vector<SVGNumber>) to
Vector<float> before interacting with WebCore (i.e. by iterating over the vector
and calling SVGNumber::value() for each item).

Please let me know if I'm missing something or if you have a better proposal.
Your comments above seem to hint that a SVGNumber class would exist but it does
not in practice.

[haraken, 2013-07-15 13:39:33]

On 2013/07/15 12:07:25, Christophe Dumez wrote:
> I am not quite sure how we can do better:
> 
> 1. I don't think it is worth introducing a new extended attribute to
generalize
> this as this is specific to SVGNumber. This is basically an optimization
> allowing the implementation to use float instead of a SVGNumber class. Custom
> code for such specific case seems adequate to me.
> 2. Introducing a new SVGNumber class in the implementation and using it
instead
> of float would get rid of the custom code but revert the optimization. Namely,
> we would need to convert from SVGNumberList (i.e. Vector<SVGNumber>) to
> Vector<float> before interacting with WebCore (i.e. by iterating over the
vector
> and calling SVGNumber::value() for each item).
> 
> Please let me know if I'm missing something or if you have a better proposal.
> Your comments above seem to hint that a SVGNumber class would exist but it
does
> not in practice.

Thanks for the detailed clarification. My feeling is that:

- Introducing a new SVGNumber class would be a reasonable way to implement the
SVGNumber in the spec.

- If the performance of the SVGNumber indirection is not acceptable, we need to
introduce some optimization. Using custom code sounds reasonable for this
purpose.

So, I think the key would be whether the performance is important or not. SVG
experts might have an idea.

[Stephen Chennney, 2013-07-15 16:52:40]

The performance hit of converting at runtime from SVGNumber to float would be
bad, particularly as these lists are used in animation code (it's not such a bug
deal for filter matrix code, which seems to be the other big user).

I'm happy with this as it is, but I'll leave the final call to haraken.

[pdr., 2013-07-15 17:33:11]

On 2013/07/15 16:52:40, Stephen Chenney wrote:
> The performance hit of converting at runtime from SVGNumber to float would be
> bad, particularly as these lists are used in animation code (it's not such a
bug
> deal for filter matrix code, which seems to be the other big user).
> 
> I'm happy with this as it is, but I'll leave the final call to haraken.

Am I correct that the performance concerns about SVGNumberList are limited to
SVGAnimatedNumberList? If so, I think the performance concerns are negligible
(and I hope the animated tearoffs go away with Web Animations anyway).

[krit, 2013-07-15 17:44:17]

On 2013/07/15 17:33:11, pdr wrote:
> On 2013/07/15 16:52:40, Stephen Chenney wrote:
> > The performance hit of converting at runtime from SVGNumber to float would
be
> > bad, particularly as these lists are used in animation code (it's not such a
> bug
> > deal for filter matrix code, which seems to be the other big user).
> > 
> > I'm happy with this as it is, but I'll leave the final call to haraken.
> 
> Am I correct that the performance concerns about SVGNumberList are limited to
> SVGAnimatedNumberList? If so, I think the performance concerns are negligible
> (and I hope the animated tearoffs go away with Web Animations anyway).

I wonder how Web animation will do that :) Especially because Web Animation does
not replace SVGAnimate* at all but builds on top of it.

[pdr., 2013-07-15 17:46:28]

On 2013/07/15 17:44:17, krit wrote:
> On 2013/07/15 17:33:11, pdr wrote:
> > On 2013/07/15 16:52:40, Stephen Chenney wrote:
> > > The performance hit of converting at runtime from SVGNumber to float would
> be
> > > bad, particularly as these lists are used in animation code (it's not such
a
> > bug
> > > deal for filter matrix code, which seems to be the other big user).
> > > 
> > > I'm happy with this as it is, but I'll leave the final call to haraken.
> > 
> > Am I correct that the performance concerns about SVGNumberList are limited
to
> > SVGAnimatedNumberList? If so, I think the performance concerns are
negligible
> > (and I hope the animated tearoffs go away with Web Animations anyway).
> 
> I wonder how Web animation will do that :) Especially because Web Animation
does
> not replace SVGAnimate* at all but builds on top of it.

SVGNumberList is only accessible through animVal and baseVal which I hope we can
deprecate. I've committed https://codereview.chromium.org/18332026 to help my
case and will be following up with the WG as soon as I have good numbers on SMIL
usage.

[do-not-use, 2013-07-15 18:43:47]

On 2013/07/15 17:33:11, pdr wrote:
> On 2013/07/15 16:52:40, Stephen Chenney wrote:
> > The performance hit of converting at runtime from SVGNumber to float would
be
> > bad, particularly as these lists are used in animation code (it's not such a
> bug
> > deal for filter matrix code, which seems to be the other big user).
> > 
> > I'm happy with this as it is, but I'll leave the final call to haraken.
> 
> Am I correct that the performance concerns about SVGNumberList are limited to
> SVGAnimatedNumberList? If so, I think the performance concerns are negligible
> (and I hope the animated tearoffs go away with Web Animations anyway).

I uploaded the alternative proposal here:
https://codereview.chromium.org/19263002/

This way, we should more easily be able to evaluate the costs.

[haraken, 2013-07-16 00:08:48]

In terms of implementation, both CLs look good. (I don't have enough knowledge
about SVG performance to judge which CL we should take.)

[pdr., 2013-07-17 02:24:05]

On 2013/07/16 00:08:48, haraken wrote:
> In terms of implementation, both CLs look good. (I don't have enough knowledge
> about SVG performance to judge which CL we should take.)

I've looked at both and I prefer this CL (typedef to float) over the SVGNumber
class. I like that the typedef is both clean and performant.

If there are no further concerns from haraken, LGTM from me for this CL :)

[haraken, 2013-07-17 02:32:45]

On 2013/07/17 02:24:05, pdr wrote:
> I've looked at both and I prefer this CL (typedef to float) over the SVGNumber
> class. I like that the typedef is both clean and performant.
> 
> If there are no further concerns from haraken, LGTM from me for this CL :)

I agree that this CL is more performant, but this CL is less clean in that it
uses custom bindings. Custom bindings are always buggy. I think we should take
the other CL if there is no performance concern.

Your comment #10 is saying that SVGNumber is used only in limited attributes
that are going to be deprecated. If that's the case and thus the performance
wouldn't matter, I might want to take the other CL. WDYT?

[pdr., 2013-07-17 02:53:37]

On 2013/07/17 02:32:45, haraken wrote:
> On 2013/07/17 02:24:05, pdr wrote:
> > I've looked at both and I prefer this CL (typedef to float) over the
SVGNumber
> > class. I like that the typedef is both clean and performant.
> > 
> > If there are no further concerns from haraken, LGTM from me for this CL :)
> 
> I agree that this CL is more performant, but this CL is less clean in that it
> uses custom bindings. Custom bindings are always buggy. I think we should take
> the other CL if there is no performance concern.
> 
> Your comment #10 is saying that SVGNumber is used only in limited attributes
> that are going to be deprecated. If that's the case and thus the performance
> wouldn't matter, I might want to take the other CL. WDYT?

This really isn't performance sensitive code, at least today. There are a couple
of non-SMIL uses in filters but my understanding is that the usage of these
properties is quite low and we can revisit this if that changes. If you think
the custom bindings code will be an undue maintenance burden, lets go ahead with
the second patch.

[haraken, 2013-07-17 03:58:43]

> This really isn't performance sensitive code, at least today. There are a
couple
> of non-SMIL uses in filters but my understanding is that the usage of these
> properties is quite low and we can revisit this if that changes. If you think
> the custom bindings code will be an undue maintenance burden, lets go ahead
with
> the second patch.

Thanks for the detailed clarification! Then I might want to take the second
patch.