Bug 147925 - [ES6] Implement Reflect.get
Summary: [ES6] Implement Reflect.get
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-11 23:32 PDT by Yusuke Suzuki
Modified: 2015-08-17 11:56 PDT (History)
6 users (show)

See Also:


Attachments
Patch (7.96 KB, patch)
2015-08-11 23:33 PDT, Yusuke Suzuki
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2015-08-11 23:32:05 PDT
[ES6] Implement Reflect.get
Comment 1 Yusuke Suzuki 2015-08-11 23:33:22 PDT
Created attachment 258809 [details]
Patch
Comment 2 Yusuke Suzuki 2015-08-11 23:34:28 PDT
Comment on attachment 258809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258809&action=review

> Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp:-154
> -}

This is only the place that does not use the PropertySlot::thisValue() as the |this| value.
But this code is not used anymore, so just drop it.
Comment 3 Geoffrey Garen 2015-08-17 11:16:42 PDT
Comment on attachment 258809 [details]
Patch

Should we be implementing these in JavaScript? Seems like it might be faster that way.
Comment 4 Yusuke Suzuki 2015-08-17 11:22:05 PDT
(In reply to comment #3)
> Comment on attachment 258809 [details]
> Patch
> 
> Should we be implementing these in JavaScript? Seems like it might be faster
> that way.

Reflect.get can take the receiver as its optional argument.
`Reflect.get ( target, propertyKey [ , receiver ])`

When the receiver is specified, we need to call the getter with the receiver as the |this|.
So, how about the mixed implementation?

in JS:

if (receiver === target)
    return target[propertyName];
return @getSlowCase(target, propertyKey, receiver);

in C++
the above code.

http://www.ecma-international.org/ecma-262/6.0/#sec-reflect.get
Comment 5 Geoffrey Garen 2015-08-17 11:28:34 PDT
> When the receiver is specified, we need to call the getter with the receiver
> as the |this|.
> So, how about the mixed implementation?
> 
> in JS:
> 
> if (receiver === target)
>     return target[propertyName];
> return @getSlowCase(target, propertyKey, receiver);

Interesting.

Ryosuke is working on a similar problem, where the same behavior is required by super.getter.

I can see that it would be awkward to override the receiver in JS. Perhaps we should just go with your current patch for now.
Comment 6 Yusuke Suzuki 2015-08-17 11:46:34 PDT
(In reply to comment #5)
> > When the receiver is specified, we need to call the getter with the receiver
> > as the |this|.
> > So, how about the mixed implementation?
> > 
> > in JS:
> > 
> > if (receiver === target)
> >     return target[propertyName];
> > return @getSlowCase(target, propertyKey, receiver);
> 
> Interesting.
> 
> Ryosuke is working on a similar problem, where the same behavior is required
> by super.getter.
> 
> I can see that it would be awkward to override the receiver in JS. Perhaps
> we should just go with your current patch for now.

Oh, that's very nice!
Maybe, super.setter = value; also has the same problem. Reflect.set has the receiver argument optionally.

Anyway, if Ryosuke is working on this "taking receiver optionally" by adding the byte code, when landing this change, we can easily use it for Reflect.get by adding the byte code intrinsics (maybe @get()?). This makes the Reflect.get written in JS (in the future) and seems cleanest way.

So agree with you. In the meantime, I'll land this current C++ implementation patch.
When the new byte code is introduced, we'll replace Reflect.get with the JS implementation :)
Comment 7 Yusuke Suzuki 2015-08-17 11:56:56 PDT
Committed r188532: <http://trac.webkit.org/changeset/188532>