Push SetAccessor to Template

Push SetAccessor to Template

R=mstarzinger@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=16515

[Michael Starzinger, 2013-08-20 17:05:40]

First comment about the API names. Looping in Andreas and Sven about the naming.

https://codereview.chromium.org/23182003/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/23182003/diff/1/include/v8.h#newcode3008
include/v8.h:3008: void SetNativeAccessor(Handle<String> name,
As discussed offline, the following naming scheme makes the most sense to me and
Andreas. WDYT?

- SetDataProperty(), for normal data properties taking HValue
- SetNativeProperty(), for data properties using Accessor[G/S]etterCallback
- SetAccessorProperty(), for JavaScript accessors

To be more precise, this would yield the following signatures:

void SetDataProperty(Handle<String> name,
                     Handle<Data> value,
                     PropertyAttribute attributes = None);

void SetNativeProperty(Handle<String> name,
                       AccessorGetterCallback getter,
                       AccessorSetterCallback setter = 0,
                       Handle<Value> data = Handle<Value>(),
                       AccessControl settings = DEFAULT,
                       PropertyAttribute attribute = None,
                       Handle<AccessorSignature> signature =
                           Handle<AccessorSignature>());

void SetAccessorProperty(Handle<String> name,
                         Handle<FunctionTemplate> getter,
                         Handle<FunctionTemplate> setter =
                             Handle<FunctionTemplate>(),
                         AccessControl settings = DEFAULT,
                         PropertyAttribute attribute = None);

https://codereview.chromium.org/23182003/diff/1/src/apinatives.js
File src/apinatives.js (right):

https://codereview.chromium.org/23182003/diff/1/src/apinatives.js#newcode96
src/apinatives.js:96: cache[serialNumber] = %ToFastProperties(fun);
It seems dangerous to re-set the cached value down here, even under the
assumption that the return value will always be "fun" again. Let's just leave
the return value of %ToFastProperties() unused here.

[Sven Panne, 2013-08-21 10:01:53]

On 2013/08/20 17:05:40, Michael Starzinger wrote:
> [...]
> - SetDataProperty(), for normal data properties taking HValue
> - SetNativeProperty(), for data properties using Accessor[G/S]etterCallback
> - SetAccessorProperty(), for JavaScript accessors

I would prefer having "Data" somehow in the 2nd version, e.g.
"SetNativeDataProperty" or perhaps just overloading things via
"SetDataProperty". From the signature is blindingly obvious that it is using
some kind of callback mechanism, but it is highly unclear that this results in a
*data* property.

> To be more precise, this would yield the following signatures:
> 
> void SetDataProperty(Handle<String> name,
>                      Handle<Data> value,
>                      PropertyAttribute attributes = None);
> 
> void SetNativeProperty(Handle<String> name,
>                        AccessorGetterCallback getter,
>                        AccessorSetterCallback setter = 0,
>                        Handle<Value> data = Handle<Value>(),
>                        AccessControl settings = DEFAULT,
>                        PropertyAttribute attribute = None,
>                        Handle<AccessorSignature> signature =
>                            Handle<AccessorSignature>());
> 
> void SetAccessorProperty(Handle<String> name,
>                          Handle<FunctionTemplate> getter,
>                          Handle<FunctionTemplate> setter =
>                              Handle<FunctionTemplate>(),
>                          AccessControl settings = DEFAULT,
>                          PropertyAttribute attribute = None);

Looking at this, we could refactor this into a single function "DefineProperty",
getting a name and a property descriptor, mirroring what JavaScript does (with
the additional variation of a getter/setter pair exposed as a data property).
Partly joking, partly dreaming of a good API... :-]

[rossberg, 2013-08-21 10:07:07]

On 2013/08/21 10:01:53, Sven Panne wrote:
> On 2013/08/20 17:05:40, Michael Starzinger wrote:
> > [...]
> > - SetDataProperty(), for normal data properties taking HValue
> > - SetNativeProperty(), for data properties using Accessor[G/S]etterCallback
> > - SetAccessorProperty(), for JavaScript accessors
> 
> I would prefer having "Data" somehow in the 2nd version, e.g.
> "SetNativeDataProperty" or perhaps just overloading things via
> "SetDataProperty". From the signature is blindingly obvious that it is using
> some kind of callback mechanism, but it is highly unclear that this results in
a
> *data* property.

I strongly dislike the overloading -- it may be blindingly clear from reading
the _signature_, but not from reading random _code_ that uses it. I can
certainly live with SetNativeDataProperty, too, though it sounds a bit verbose
(and their won't ever be native accessor properties).

[dcarney, 2013-08-22 15:51:24]

okay, this is ready for a look

I've used the excessive SetNativeDataProperty name, but there's a chance we can
remove this entirely in the future...

[Sven Panne, 2013-08-26 07:20:24]

Next round of comments. BTW: I'm not sure if "make grokdump" is needed to update
tools/v8heapconst.py.

https://codereview.chromium.org/23182003/diff/12001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/23182003/diff/12001/src/objects.cc#newcode3135
src/objects.cc:3135: static bool InsertUnique(Name* key,
Splitting InsertUnique into 2 methods

   Contains(key,valid_descriptors,array)
   Insert(key,entry,valid_descriptors,array)

makes it a bit clearer what's going on.

https://codereview.chromium.org/23182003/diff/12001/src/objects.cc#newcode3158
src/objects.cc:3158: array->set(valid_descriptors, entry);
Why is there always room for the new entry?

[dcarney, 2013-08-26 07:32:22]

https://codereview.chromium.org/23182003/diff/12001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/23182003/diff/12001/src/objects.cc#newcode3135
src/objects.cc:3135: static bool InsertUnique(Name* key,
On 2013/08/26 07:20:24, Sven Panne wrote:
> Splitting InsertUnique into 2 methods
> 
>    Contains(key,valid_descriptors,array)
>    Insert(key,entry,valid_descriptors,array)
> 
> makes it a bit clearer what's going on.

okay

https://codereview.chromium.org/23182003/diff/12001/src/objects.cc#newcode3158
src/objects.cc:3158: array->set(valid_descriptors, entry);
On 2013/08/26 07:20:24, Sven Panne wrote:
> Why is there always room for the new entry?

I'll add an assert, like appendcallbackdescriptors has

[Michael Starzinger, 2013-09-03 14:00:56]

LGTM.

https://codereview.chromium.org/23182003/diff/27001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/23182003/diff/27001/src/factory.cc#newcode1536
src/factory.cc:1536: Handle<AccessorInfo> accessor(value, isolate());
nit: This can be simplified to just ...

Handle<AccessorInfo> accessor(AccessorInfo::cast(array->get(i)));

https://codereview.chromium.org/23182003/diff/27001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/23182003/diff/27001/src/objects.h#newcode9529
src/objects.h:9529: static int AppendUnique(Handle<Object> descriptors,
nit: Can we have a short one-liner comment describing this method?

[dcarney, 2013-09-04 07:45:51]

Committed patchset #5 manually as r16515 (presubmit successful).