Add interceptor support for symbols

Add interceptor support for symbols

ObjectTemplate::SetGenericNamedPropertyHandler is a new version of
ObjectTemplate::SetNamedPropertyHandler that can intercept any Name
property access, not just access to String properties.  Eventually we
will deprecate and remove ObjectTemplate::SetNamedPropertyHandler, then
rename ObjectTemplate::SetGenericNamedPropertyHandler back to
ObjectTemplate::SetNamedPropertyHandler.

R=dcarney@chromium.org, rossberg@chromium.org
BUG=v8:3394
LOG=Y

[wingo, 2014-08-13 08:07:59]

A compatible change to add interceptor support for symbols in the API and the
runtime.  After updating blink we can remove the flag and the old definitions.

[wingo, 2014-08-13 08:14:26]

I should mention that this change isn't blocking me on anything; just a random
thing I had hanging around.

[dcarney, 2014-08-13 08:25:40]

lgtm

normally when we do this, we convert all the cctest files to use the new api,
and we introduce one test for the old behaviour (as you've done).

Please do that either in this CL or an immediate follow up.

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

https://codereview.chromium.org/467013003/diff/1/include/v8.h#newcode3730
include/v8.h:3730: void SetGenericNamedPropertyHandler(
should be SetNamedPropertyHandler. easier to get rid of the old version that
way.

https://codereview.chromium.org/467013003/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/467013003/diff/1/src/objects.cc#newcode2914
src/objects.cc:2914: (!name->IsString() &&
!interceptor->can_intercept_symbols())) {
everywhere else you do 

name->IsSymbol() && !interceptor->can_intercept_symbols()

can you please do that here for consistency

https://codereview.chromium.org/467013003/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/467013003/diff/1/src/objects.h#newcode10850
src/objects.h:10850: inline void set_flags(int flags);
can you add a TODO here to remove the flags field when the Generic interceptors
are renamed?

[wingo, 2014-08-13 10:45:40]

Updated patch to make requested changes, with test suite update.  PTAL at the
one breaking API change described in this note.

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

https://codereview.chromium.org/467013003/diff/1/include/v8.h#newcode3730
include/v8.h:3730: void SetGenericNamedPropertyHandler(
On 2014/08/13 08:25:39, dcarney (ooto) wrote:
> should be SetNamedPropertyHandler. easier to get rid of the old version that
> way.

Alack, I just did this, but:

../test/cctest/test-api.cc: In function ‘void
TestGetOwnPropertyNamesWithInterceptor()’:
../test/cctest/test-api.cc:9672:64: error: call of overloaded
‘SetNamedPropertyHandler(NULL, NULL, NULL, NULL, void (&)(const
v8::PropertyCallbackInfo<v8::Array>&))’ is ambiguous

I am thinking that breaking this code is acceptable, though, as Chromium is
unlikely to have code of this form.  WDYT?

https://codereview.chromium.org/467013003/diff/1/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/467013003/diff/1/src/objects.cc#newcode2914
src/objects.cc:2914: (!name->IsString() &&
!interceptor->can_intercept_symbols())) {
On 2014/08/13 08:25:40, dcarney (ooto) wrote:
> everywhere else you do 
> 
> name->IsSymbol() && !interceptor->can_intercept_symbols()
> 
> can you please do that here for consistency
> 

Done.

https://codereview.chromium.org/467013003/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/467013003/diff/1/src/objects.h#newcode10850
src/objects.h:10850: inline void set_flags(int flags);
On 2014/08/13 08:25:40, dcarney (ooto) wrote:
> can you add a TODO here to remove the flags field when the Generic
interceptors
> are renamed? 

Done.

[dcarney, 2014-08-13 11:19:56]

lgtm

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

https://codereview.chromium.org/467013003/diff/1/include/v8.h#newcode3730
include/v8.h:3730: void SetGenericNamedPropertyHandler(
On 2014/08/13 10:45:39, wingo wrote:
> On 2014/08/13 08:25:39, dcarney (ooto) wrote:
> > should be SetNamedPropertyHandler. easier to get rid of the old version that
> > way.
> 
> Alack, I just did this, but:
> 
> ../test/cctest/test-api.cc: In function ‘void
> TestGetOwnPropertyNamesWithInterceptor()’:
> ../test/cctest/test-api.cc:9672:64: error: call of overloaded
> ‘SetNamedPropertyHandler(NULL, NULL, NULL, NULL, void (&)(const
> v8::PropertyCallbackInfo<v8::Array>&))’ is ambiguous
> 
> I am thinking that breaking this code is acceptable, though, as Chromium is
> unlikely to have code of this form.  WDYT?

i didn't even know this was supported.  since it's a primitive form of
interceptor, anything using it should use an interceptor instead .  If chrome
doesn't compile with this, which is super unlikely, we can fix before landing by
casting the first argument.

[rossberg, 2014-08-19 11:36:38]

NOT LGTM

https://codereview.chromium.org/467013003/diff/40001/src/objects.cc
File src/objects.cc (left):

https://codereview.chromium.org/467013003/diff/40001/src/objects.cc#oldcode13612
src/objects.cc:13612: // TODO(rossberg): support symbols in API, and filter here
if needed.
Note that simply relaxing this function to also return symbol names will break
(indirect) use sites. In particular, it is used by JSReceiver::GetKeys, which in
turn is invoked in various places, some of which are not expecting symbols
(e.g., JsonStringifier).

I'm afraid you need to carefully check and adapt all transitive dependencies
(and include respective tests).

https://codereview.chromium.org/467013003/diff/40001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/467013003/diff/40001/src/objects.cc#newcode4258
src/objects.cc:4258: return Maybe<PropertyAttributes>(ABSENT);
Nit: maybe(ABSENT)

[wingo, 2014-08-21 09:55:32]

https://codereview.chromium.org/467013003/diff/40001/src/objects.cc
File src/objects.cc (left):

https://codereview.chromium.org/467013003/diff/40001/src/objects.cc#oldcode13612
src/objects.cc:13612: // TODO(rossberg): support symbols in API, and filter here
if needed.
On 2014/08/19 11:36:37, rossberg wrote:
> Note that simply relaxing this function to also return symbol names will break
> (indirect) use sites. In particular, it is used by JSReceiver::GetKeys, which
in
> turn is invoked in various places, some of which are not expecting symbols
> (e.g., JsonStringifier).
> 
> I'm afraid you need to carefully check and adapt all transitive dependencies
> (and include respective tests).

Good catch.  I've taken a look at other callers and it's only this one that
needs filtering.  OTOH is filtering the right thing?  Would it be better to add
an enumerator callback that returns symbols?

[rossberg, 2014-08-21 11:25:09]

On 2014/08/21 09:55:32, wingo wrote:
> https://codereview.chromium.org/467013003/diff/40001/src/objects.cc
> File src/objects.cc (left):
> 
>
https://codereview.chromium.org/467013003/diff/40001/src/objects.cc#oldcode13612
> src/objects.cc:13612: // TODO(rossberg): support symbols in API, and filter
here
> if needed.
> On 2014/08/19 11:36:37, rossberg wrote:
> > Note that simply relaxing this function to also return symbol names will
break
> > (indirect) use sites. In particular, it is used by JSReceiver::GetKeys,
which
> in
> > turn is invoked in various places, some of which are not expecting symbols
> > (e.g., JsonStringifier).
> > 
> > I'm afraid you need to carefully check and adapt all transitive dependencies
> > (and include respective tests).
> 
> Good catch.  I've taken a look at other callers and it's only this one that
> needs filtering.  OTOH is filtering the right thing?  Would it be better to
add
> an enumerator callback that returns symbols?

Yes, we most likely don't want to filter in this function directly, but
downstream where necessary. For the API, it may be best to have a separate
function to get symbol keys, very much like we have it JS-side.

[wingo, 2014-08-21 11:50:31]

On 2014/08/21 11:25:09, rossberg wrote:
> Would it be better to
> add
> > an enumerator callback that returns symbols?
> 
> Yes, we most likely don't want to filter in this function directly, but
> downstream where necessary. For the API, it may be best to have a separate
> function to get symbol keys, very much like we have it JS-side.

Hm, I think either I don't understand you, or I'm not being precise.  The
current SetNamedPropertyHandler API allows users to specify a callback to
enumerate string-named properties.  I was proposing that maybe we should add an
additional callback to the new SetNamedPropertyHandler call; the new callback
would return symbol-named properties.  In that way we wouldn't have to filter
anywhere; you just call the right functions.  WDYT?

Andy

[rossberg, 2014-08-21 12:07:48]

On 2014/08/21 11:50:31, wingo wrote:
> On 2014/08/21 11:25:09, rossberg wrote:
> > Would it be better to
> > add
> > > an enumerator callback that returns symbols?
> > 
> > Yes, we most likely don't want to filter in this function directly, but
> > downstream where necessary. For the API, it may be best to have a separate
> > function to get symbol keys, very much like we have it JS-side.
> 
> Hm, I think either I don't understand you, or I'm not being precise.  The
> current SetNamedPropertyHandler API allows users to specify a callback to
> enumerate string-named properties.  I was proposing that maybe we should add
an
> additional callback to the new SetNamedPropertyHandler call; the new callback
> would return symbol-named properties.  In that way we wouldn't have to filter
> anywhere; you just call the right functions.  WDYT?

Ah, okay.

Hm, I'm not sure. It seems to me that the API should ultimately be consistent in
the sense that either String and Symbol names are treated together everywhere,
or they are separated everywhere (kind of like "properties" and "elements"
already). Just separating them in some places seems a bit odd from the user
perspective.

[wingo, 2014-08-21 13:08:29]

On 2014/08/21 12:07:48, rossberg wrote:
> Hm, I'm not sure. It seems to me that the API should ultimately be consistent
in
> the sense that either String and Symbol names are treated together everywhere,
> or they are separated everywhere (kind of like "properties" and "elements"
> already). Just separating them in some places seems a bit odd from the user
> perspective.

OK, I will give filtering a go then.

[wingo, 2014-08-26 10:51:01]

I changed the GetKeys implementation to filter out non-symbol keys; PTAL
andreas.  Unfortunately I rebased so the change isn't apparent from the
interdiffs :/

[dcarney, 2014-08-27 09:57:37]

rossberg@ : what would you think of passing a struct containing all the
callbacks to SetNamedPropertyHandler?
It turns out we have a use case for having a GetNamedPropertyHandler method, and
the struct would be ideal for that.

see https://code.google.com/p/chromium/issues/detail?id=404300#c20

[rossberg, 2014-08-27 10:42:31]

On 2014/08/27 09:57:37, dcarney (ooto) wrote:
> rossberg@ : what would you think of passing a struct containing all the
> callbacks to SetNamedPropertyHandler?
> It turns out we have a use case for having a GetNamedPropertyHandler method,
and
> the struct would be ideal for that.
> 
> see https://code.google.com/p/chromium/issues/detail?id=404300#c20

I think grouping them into a struct makes a lot of sense!

[wingo, 2014-08-27 12:13:56]

On 2014/08/27 10:42:31, rossberg wrote:
> On 2014/08/27 09:57:37, dcarney (ooto) wrote:
> > rossberg@ : what would you think of passing a struct containing all the
> > callbacks to SetNamedPropertyHandler?
> > It turns out we have a use case for having a GetNamedPropertyHandler method,
> and
> > the struct would be ideal for that.
> > 
> > see https://code.google.com/p/chromium/issues/detail?id=404300#c20
> 
> I think grouping them into a struct makes a lot of sense!

Agreed wrt a struct making sense, but isn't it reasonable to also have an
old-style API?  (Just trying to limit the scope of this patchset, as it touches
runtime stuff and i have to keep rebasing that.)

[dcarney, 2014-08-27 13:17:29]

> Agreed wrt a struct making sense, but isn't it reasonable to also have an
> old-style API?  (Just trying to limit the scope of this patchset, as it
touches
> runtime stuff and i have to keep rebasing that.)

There are some future interceptor changes that we will want the struct for, and
we don't want to have to change blink twice, so no, and old style api is not a
great idea.  This change shouldn't be a big one to this cl, mostly v8.h and
api.cc changes, and some minor test changes.

[rossberg, 2014-08-27 17:35:09]

lgtm

[wingo, 2014-09-02 19:26:26]

On 2014/08/27 17:35:09, rossberg wrote:
> lgtm

I'm going to be OOO for a week, and figured that it's best to get this in then
make the struct API change when I get back, if it's not done already.  That
should be much less intrusive.  Will land.

[wingo, 2014-09-02 19:53:45]

On 2014/09/02 19:26:26, wingo (holiday until 11 sep) wrote:
> On 2014/08/27 17:35:09, rossberg wrote:
> > lgtm
> 
> I'm going to be OOO for a week, and figured that it's best to get this in then
> make the struct API change when I get back, if it's not done already.  That
> should be much less intrusive.  Will land.

Alack, a simple rebase-and-fixup results in non-passing tests.  Punting for now.

[dcarney, 2014-11-26 14:55:02]

On 2014/09/02 19:53:45, wingo wrote:
> On 2014/09/02 19:26:26, wingo (holiday until 11 sep) wrote:
> > On 2014/08/27 17:35:09, rossberg wrote:
> > > lgtm
> > 
> > I'm going to be OOO for a week, and figured that it's best to get this in
then
> > make the struct API change when I get back, if it's not done already.  That
> > should be much less intrusive.  Will land.
> 
> Alack, a simple rebase-and-fixup results in non-passing tests.  Punting for
now.

fyi - reviving this cl here as I need to make a bunch of interceptor change in
blink:  https://codereview.chromium.org/760883002/