IDL: Support [DeprecateAs] and [MeasureAs] on constants

IDL: Support [DeprecateAs] and [MeasureAs] on constants

For constants with [DeprecateAs] or [MeasureAs], create the property
using v8::Template::SetNativeDataProperty() instead, using a simple
accessor callback function that handles the deprecation/measuring and
returns the constant value.

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

[Jens Widell, 2014-09-18 16:34:09]

jl@opera.com changed reviewers:
+ haraken@chromium.org

[Jens Widell, 2014-09-18 16:34:11]

haraken@: do you think we should support this, given the complexity added, and
if so, help me, my code doesn't compile. :-)

https://codereview.chromium.org/577303002/diff/1/Source/bindings/core/v8/V8DO...
File Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/577303002/diff/1/Source/bindings/core/v8/V8DO...
Source/bindings/core/v8/V8DOMConfiguration.cpp:107:
functionDescriptor->SetAccessor(constantName, getter, 0,
v8::Handle<v8::Value>(), v8::DEFAULT,
static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete));
This line doesn't compile: there is no v8::FunctionTemplate::SetAccessor().

[haraken, 2014-09-18 16:57:10]

I don't think we should support this.

If we use SetAccessor to install a constant, a JS accessor for the constant is
exposed to user's JS. This is violating the IDL spec.

Actually, I don't think there is an easy way to support [MeasureAs] for
constants..

[Jens Widell, 2014-09-18 18:07:46]

On 2014/09/18 16:57:10, haraken(OOO til Sep 21) wrote:
> If we use SetAccessor to install a constant, a JS accessor for the constant is
> exposed to user's JS. This is violating the IDL spec.

Don't you mean SetAccessorProperty, rather than SetAccessor? (The naming in the
V8 API isn't brilliantly clear here, and the documentation in v8.h not so
enlightening either.)

In my testing, with

 
console.log(JSON.stringify(Object.getOwnPropertyDescriptor(MediaError.prototype,
"MEDIA_ERR_NETWORK")));
 
console.log(JSON.stringify(Object.getOwnPropertyDescriptor(MediaError.prototype,
"MEDIA_ERR_ENCRYPTED")));

I get

  [...:INFO:CONSOLE(2)]
"{"value":2,"writable":false,"enumerable":true,"configurable":false}", source:
http://localhost/~jl/constants.html (2)
  [...:INFO:CONSOLE(0)] "'MediaError.MEDIA_ERR_ENCRYPTED' is deprecated. This
error code is never used.", source:  (0)
  [...:INFO:CONSOLE(3)]
"{"value":5,"writable":false,"enumerable":true,"configurable":false}", source:
http://localhost/~jl/constants.html (3)

which I interpret as this being relatively invisible on the user JS side.

[haraken, 2014-09-18 23:17:47]

On 2014/09/18 18:07:46, Jens Widell wrote:
> On 2014/09/18 16:57:10, haraken(OOO til Sep 21) wrote:
> > If we use SetAccessor to install a constant, a JS accessor for the constant
is
> > exposed to user's JS. This is violating the IDL spec.
> 
> Don't you mean SetAccessorProperty, rather than SetAccessor? (The naming in
the
> V8 API isn't brilliantly clear here, and the documentation in v8.h not so
> enlightening either.)
> 
> In my testing, with
> 
>  
>
console.log(JSON.stringify(Object.getOwnPropertyDescriptor(MediaError.prototype,
> "MEDIA_ERR_NETWORK")));
>  
>
console.log(JSON.stringify(Object.getOwnPropertyDescriptor(MediaError.prototype,
> "MEDIA_ERR_ENCRYPTED")));
> 
> I get
> 
>   [...:INFO:CONSOLE(2)]
> "{"value":2,"writable":false,"enumerable":true,"configurable":false}", source:
> http://localhost/~jl/constants.html (2)
>   [...:INFO:CONSOLE(0)] "'MediaError.MEDIA_ERR_ENCRYPTED' is deprecated. This
> error code is never used.", source:  (0)
>   [...:INFO:CONSOLE(3)]
> "{"value":5,"writable":false,"enumerable":true,"configurable":false}", source:
> http://localhost/~jl/constants.html (3)
> 
> which I interpret as this being relatively invisible on the user JS side.

Thanks, you are right.

Hmm, I understand SetAccessor will "work", but it isn't that nice to use
SetAccessor since the API is going to be deprecated. In terms of semantics,
constants should be installed by Set...

(However, I don't have any alternative idea.)

[Jens Widell, 2014-09-19 06:27:23]

On 2014/09/18 23:17:47, haraken(OOO til Sep 21) wrote:
> Hmm, I understand SetAccessor will "work", but it isn't that nice to use
> SetAccessor since the API is going to be deprecated. In terms of semantics,
> constants should be installed by Set...

In favor of SetAccessorProperty()? So any property with a getter/setter (even
implemented in C++) will be exposed to scripts as an accessor property with
getter/setter function objects?

Agree that constants ought to be installed by Set normally. Counted/deprecated
constants ought to be a special case though, and not common at all.

> (However, I don't have any alternative idea.)

I guess our options would be:

1) Extend V8 to support ObjectTemplate::SetAccessor() functionality on
FunctionTemplate. I have no idea what this would involve, or if it's even
feasible. And if ObjectTemplate::SetAccessor() is scheduled for deprecation, it
obviously doesn't seem a good direction to go in.

2) Don't support [MeasureAs] or [DeprecateAs] on constants. In that case, I
propose we make the code generator reject IDL files that try to use them on
constants, just to make it clear that this is not supported.

[haraken, 2014-09-19 06:40:31]

On 2014/09/19 06:27:23, Jens Widell wrote:
> On 2014/09/18 23:17:47, haraken(OOO til Sep 21) wrote:
> > Hmm, I understand SetAccessor will "work", but it isn't that nice to use
> > SetAccessor since the API is going to be deprecated. In terms of semantics,
> > constants should be installed by Set...
> 
> In favor of SetAccessorProperty()? So any property with a getter/setter (even
> implemented in C++) will be exposed to scripts as an accessor property with
> getter/setter function objects?
> 

Yes, that's our plan :)


> Agree that constants ought to be installed by Set normally. Counted/deprecated
> constants ought to be a special case though, and not common at all.
> 
> > (However, I don't have any alternative idea.)
> 
> I guess our options would be:
> 
> 1) Extend V8 to support ObjectTemplate::SetAccessor() functionality on
> FunctionTemplate. I have no idea what this would involve, or if it's even
> feasible. And if ObjectTemplate::SetAccessor() is scheduled for deprecation,
it
> obviously doesn't seem a good direction to go in.
> 
> 2) Don't support [MeasureAs] or [DeprecateAs] on constants. In that case, I
> propose we make the code generator reject IDL files that try to use them on
> constants, just to make it clear that this is not supported.

Do we have any real use case where we want to support [MeasureAs] for constants?
If no, I think we can just make it an error at the moment and get back to this
issue later.

[Erik Dahlström (inactive), 2014-09-19 07:29:49]

On 2014/09/19 06:40:31, haraken(OOO til Sep 21) wrote:
...
> Do we have any real use case where we want to support [MeasureAs] for
constants?
> If no, I think we can just make it an error at the moment and get back to this
> issue later.

In https://codereview.chromium.org/474013004/ I'd like to measure usage of
constants in an interface. For my use-case, I'd settle for a counter for use of
any of the constants, it doesn't need to be down to each constant. I'm hoping to
be able to remove a bunch of constants from the SVG IDL files, and in some cases
the interfaces themselves may need to be kept for web compat. It's easy enough
to remove constants together with the whole interface, but when you want to
remove just the constants...

In any case, if it's hard to add MeasureAs for constants then I'll probably
remove these lines from my CL.

[haraken, 2014-09-19 07:38:41]

On 2014/09/19 07:29:49, Erik Dahlström wrote:
> On 2014/09/19 06:40:31, haraken(OOO til Sep 21) wrote:
> ...
> > Do we have any real use case where we want to support [MeasureAs] for
> constants?
> > If no, I think we can just make it an error at the moment and get back to
this
> > issue later.
> 
> In https://codereview.chromium.org/474013004/ I'd like to measure usage of
> constants in an interface. For my use-case, I'd settle for a counter for use
of
> any of the constants, it doesn't need to be down to each constant. I'm hoping
to
> be able to remove a bunch of constants from the SVG IDL files, and in some
cases
> the interfaces themselves may need to be kept for web compat. It's easy enough
> to remove constants together with the whole interface, but when you want to
> remove just the constants...
> 
> In any case, if it's hard to add MeasureAs for constants then I'll probably
> remove these lines from my CL.

Thanks for the detailed context. Understood.

Hmm, at the very least I can say it's not easy to support [MeasureAs] for
constants. So if you can live without it, it seems better to go without it at
the moment :-/

[philipj_slow, 2014-09-19 07:58:35]

Some more constants that needs removing:

MediaError.MEDIA_ERR_ENCRYPTED (already DeprecateAs, but obviously that doesn't
work)
Element.ALLOW_KEYBOARD_INPUT
Event.MOUSEDOWN through Event.CHANGE (16 constants)

[Jens Widell, 2014-09-19 09:18:21]

On 2014/09/19 06:27:23, Jens Widell wrote:
> I guess our options would be:
> 
> 1) Extend V8 to support ObjectTemplate::SetAccessor() functionality on
> FunctionTemplate. I have no idea what this would involve, or if it's even
> feasible. And if ObjectTemplate::SetAccessor() is scheduled for deprecation,
it
> obviously doesn't seem a good direction to go in.

So, for completeness, I had a quick go at finding out what supporting
FunctionTemplate::SetAccessor() would involve. This appears to be all that's
required: https://codereview.chromium.org/582383002

[haraken, 2014-09-19 09:27:14]

On 2014/09/19 09:18:21, Jens Widell wrote:
> On 2014/09/19 06:27:23, Jens Widell wrote:
> > I guess our options would be:
> > 
> > 1) Extend V8 to support ObjectTemplate::SetAccessor() functionality on
> > FunctionTemplate. I have no idea what this would involve, or if it's even
> > feasible. And if ObjectTemplate::SetAccessor() is scheduled for deprecation,
> it
> > obviously doesn't seem a good direction to go in.
> 
> So, for completeness, I had a quick go at finding out what supporting
> FunctionTemplate::SetAccessor() would involve. This appears to be all that's
> required: https://codereview.chromium.org/582383002

Dan and Jochen would know better.

[haraken, 2014-09-19 09:29:01]

haraken@chromium.org changed reviewers:
+ dcarney@chromium.org, jochen@chromium.org

[haraken, 2014-09-19 09:29:02]

+Dan, Jochen

[dcarney, 2014-09-22 07:40:00]

> So, for completeness, I had a quick go at finding out what supporting
> FunctionTemplate::SetAccessor() would involve. This appears to be all that's
> required: https://codereview.chromium.org/582383002


SetAccessor is available on Template already, it's just called
SetNativeDataProperty.  SetAccessor is deprecated on ObjectTemplate only in the
sense that it's a bad name.

[Jens Widell, 2014-09-23 06:33:38]

On 2014/09/22 07:40:00, dcarney wrote:
> > So, for completeness, I had a quick go at finding out what supporting
> > FunctionTemplate::SetAccessor() would involve. This appears to be all that's
> > required: https://codereview.chromium.org/582383002
> 
> SetAccessor is available on Template already, it's just called
> SetNativeDataProperty.  SetAccessor is deprecated on ObjectTemplate only in
the
> sense that it's a bad name.

Thanks for the information, Dan!

Patch updated to use SetNativeDataProperty().

@haraken: PTAL?

[haraken, 2014-09-23 06:37:01]

On 2014/09/23 06:33:38, Jens Widell wrote:
> On 2014/09/22 07:40:00, dcarney wrote:
> > > So, for completeness, I had a quick go at finding out what supporting
> > > FunctionTemplate::SetAccessor() would involve. This appears to be all
that's
> > > required: https://codereview.chromium.org/582383002
> > 
> > SetAccessor is available on Template already, it's just called
> > SetNativeDataProperty.  SetAccessor is deprecated on ObjectTemplate only in
> the
> > sense that it's a bad name.
> 
> Thanks for the information, Dan!
> 
> Patch updated to use SetNativeDataProperty().
> 
> @haraken: PTAL?

Can we use SetNativeDataProperty for all constants (not only for constants that
have [MeasureAs] or [DeprecateAs])? Is it not allowed performance-wise? It seems
a bit consistent to use Set for some constants and use SetNativeDataProperty for
other constants.

[Jens Widell, 2014-09-23 06:51:22]

On 2014/09/23 06:37:01, haraken wrote:
> Can we use SetNativeDataProperty for all constants (not only for constants
that
> have [MeasureAs] or [DeprecateAs])? Is it not allowed performance-wise? It
seems
> a bit consistent to use Set for some constants and use SetNativeDataProperty
for
> other constants.

Constants handled via SetNativeDataProperty() seem to be about five times slower
than constants set with Set(). I don't think it's good idea to introduce that
performance regression only to achieve consistency. It would also increase
binary size a bit, I'd assume; there are almost 500 constants in our IDL files.

[haraken, 2014-09-23 07:52:28]

LGTM

https://codereview.chromium.org/577303002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/577303002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8DOMConfiguration.cpp:104: void
V8DOMConfiguration::installConstant(v8::Handle<v8::FunctionTemplate>
functionDescriptor, v8::Handle<v8::ObjectTemplate> prototype, const char* name,
v8::AccessorGetterCallback getter, v8::Isolate* isolate)

Would you add a comment about why installConstant uses SetNativeDataProperty
whereas instalConstants uses Set?

https://codereview.chromium.org/577303002/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/577303002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:266: 'constant_configuration_constants':
constant_configuration_constants,

constant_configuration_constants => default_constants ?

[Jens Widell, 2014-09-23 08:02:51]

https://codereview.chromium.org/577303002/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/577303002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:266: 'constant_configuration_constants':
constant_configuration_constants,
On 2014/09/23 07:52:28, haraken wrote:
> 
> constant_configuration_constants => default_constants ?

Picked the name to match 'method_configuration_methods' below. I can rename both
if you prefer "default_" (which is not without merit.)

[haraken, 2014-09-23 08:31:43]

https://codereview.chromium.org/577303002/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/577303002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:266: 'constant_configuration_constants':
constant_configuration_constants,
On 2014/09/23 08:02:51, Jens Widell wrote:
> On 2014/09/23 07:52:28, haraken wrote:
> > 
> > constant_configuration_constants => default_constants ?
> 
> Picked the name to match 'method_configuration_methods' below. I can rename
both
> if you prefer "default_" (which is not without merit.)

Either is fine :)

[Jens Widell, 2014-09-23 09:01:31]

On 2014/09/23 08:31:43, haraken wrote:
>
https://codereview.chromium.org/577303002/diff/40001/Source/bindings/scripts/...
> File Source/bindings/scripts/v8_interface.py (right):
> 
>
https://codereview.chromium.org/577303002/diff/40001/Source/bindings/scripts/...
> Source/bindings/scripts/v8_interface.py:266:
'constant_configuration_constants':
> constant_configuration_constants,
> On 2014/09/23 08:02:51, Jens Widell wrote:
> > On 2014/09/23 07:52:28, haraken wrote:
> > > 
> > > constant_configuration_constants => default_constants ?
> > 
> > Picked the name to match 'method_configuration_methods' below. I can rename
> both
> > if you prefer "default_" (which is not without merit.)
> 
> Either is fine :)

Since both of these are used as a truth value (in the templates) when deciding
whether to have a {Method,Constant}Configuration array at all, I think the
current names are slightly better. I.e., we have

{% if method_configuration_methods %}
  do MethodConfiguration stuff
{% endif %}

instead of

{% if default_methods %}
  do MethodConfiguration stuff
{% endif %}

[Jens Widell, 2014-09-23 09:07:33]

https://codereview.chromium.org/577303002/diff/40001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/577303002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8DOMConfiguration.cpp:104: void
V8DOMConfiguration::installConstant(v8::Handle<v8::FunctionTemplate>
functionDescriptor, v8::Handle<v8::ObjectTemplate> prototype, const char* name,
v8::AccessorGetterCallback getter, v8::Isolate* isolate)
On 2014/09/23 07:52:28, haraken wrote:
> 
> Would you add a comment about why installConstant uses SetNativeDataProperty
> whereas instalConstants uses Set?

Comment added.

[haraken, 2014-09-23 09:09:34]

On 2014/09/23 09:01:31, Jens Widell wrote:
> On 2014/09/23 08:31:43, haraken wrote:
> >
>
https://codereview.chromium.org/577303002/diff/40001/Source/bindings/scripts/...
> > File Source/bindings/scripts/v8_interface.py (right):
> > 
> >
>
https://codereview.chromium.org/577303002/diff/40001/Source/bindings/scripts/...
> > Source/bindings/scripts/v8_interface.py:266:
> 'constant_configuration_constants':
> > constant_configuration_constants,
> > On 2014/09/23 08:02:51, Jens Widell wrote:
> > > On 2014/09/23 07:52:28, haraken wrote:
> > > > 
> > > > constant_configuration_constants => default_constants ?
> > > 
> > > Picked the name to match 'method_configuration_methods' below. I can
rename
> > both
> > > if you prefer "default_" (which is not without merit.)
> > 
> > Either is fine :)
> 
> Since both of these are used as a truth value (in the templates) when deciding
> whether to have a {Method,Constant}Configuration array at all, I think the
> current names are slightly better. I.e., we have
> 
> {% if method_configuration_methods %}
>   do MethodConfiguration stuff
> {% endif %}
> 
> instead of
> 
> {% if default_methods %}
>   do MethodConfiguration stuff
> {% endif %}

Makes sense.

[Jens Widell, 2014-09-23 12:08:28]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-09-23 12:08:59]

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

[commit-bot: I haz the power, 2014-09-23 12:50:33]

Committed patchset #5 (id:80001) as 182492

[Nils Barth (inactive), 2014-09-23 14:02:12]

Thanks Jens!
Docs updated:
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes