Bug 154314 - Implement Proxy.[[GetOwnProperty]]
Summary: Implement Proxy.[[GetOwnProperty]]
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
: 154353 (view as bug list)
Depends on: 154081
Blocks: 35731
  Show dependency treegraph
 
Reported: 2016-02-16 15:22 PST by Saam Barati
Modified: 2020-04-08 16:34 PDT (History)
11 users (show)

See Also:


Attachments
it begins (93.01 KB, patch)
2016-02-17 13:02 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (29.54 KB, patch)
2016-02-17 19:06 PST, Saam Barati
fpizlo: review+
Details | Formatted Diff | Diff
follow up fix (4.05 KB, patch)
2016-02-18 13:48 PST, Saam Barati
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-02-16 15:22:51 PST
...
Comment 1 Saam Barati 2016-02-17 13:02:55 PST
Created attachment 271582 [details]
it begins

this patch still has the contents of the [[Get]] patch until it lands
Comment 2 Saam Barati 2016-02-17 14:33:19 PST
*** Bug 154353 has been marked as a duplicate of this bug. ***
Comment 3 Saam Barati 2016-02-17 19:06:01 PST
Created attachment 271616 [details]
patch
Comment 4 WebKit Commit Bot 2016-02-17 19:07:43 PST
Attachment 271616 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSObject.h:720:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:720:  The parameter name "callData" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:720:  The parameter name "callType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:720:  The parameter name "ident" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSObject.h:1477:  The parameter name "descriptor" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Saam Barati 2016-02-17 19:08:52 PST
Comment on attachment 271616 [details]
patch

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

> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:167
> +    if (targetObject->hasOwnProperty(exec, exec->propertyNames().length)) {
> +        if (exec->hadException())
> +            return JSValue::encode(jsUndefined());
> +
>          // a. Let L be the length property of Target minus the length of A.
>          // b. Set the length own property of F to either 0 or L, whichever is larger.
> -        unsigned targetLength = (unsigned)target.get(exec, exec->propertyNames().length).asNumber();
> -        if (targetLength > numBoundArgs)
> -            length = targetLength - numBoundArgs;
> +        JSValue lengthValue = target.get(exec, exec->propertyNames().length);
> +        if (lengthValue.isNumber()) {
> +            unsigned targetLength = (unsigned)lengthValue.asNumber();
> +            if (targetLength > numBoundArgs)
> +                length = targetLength - numBoundArgs;
> +        }

I added this here because it's wrong w.r.t to the spec.
There is a kangax test that tests this in combination with Proxy.[[GetOwnProperty]].
To make us pass that test, I need to also implement Proxy.[[Call]]. I can wait
or open another bug for this if anybody objects about having it in this patch.
Comment 6 Saam Barati 2016-02-18 12:29:51 PST
landed in:
http://trac.webkit.org/changeset/196772
Comment 7 Saam Barati 2016-02-18 13:48:18 PST
Created attachment 271689 [details]
follow up fix
Comment 8 Saam Barati 2016-02-18 13:56:30 PST
follow up landed in:
http://trac.webkit.org/changeset/196775
Comment 9 Darin Adler 2016-02-23 13:30:24 PST
Comment on attachment 271616 [details]
patch

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

> Source/JavaScriptCore/runtime/JSObject.cpp:2942
> +    if (method.isUndefined() || method.isNull())
> +        return jsUndefined();
> +
> +    if (!method.isCell()) {
> +        throwVMTypeError(exec, errorMessage);
> +        return jsUndefined();
> +    }

I would have written:

    if (!method.isCell()) {
        if (method.isUndefinedOrNull())
            return jsUndefined();
        return throwVMTypeError(exec, errorMessage);
    }

Having one less branch seems slightly more elegant to me.
Comment 10 Saam Barati 2016-02-23 13:44:01 PST
Comment on attachment 271616 [details]
patch

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

>> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:167
>> +    if (targetObject->hasOwnProperty(exec, exec->propertyNames().length)) {
>> +        if (exec->hadException())
>> +            return JSValue::encode(jsUndefined());
>> +
>>          // a. Let L be the length property of Target minus the length of A.
>>          // b. Set the length own property of F to either 0 or L, whichever is larger.
>> -        unsigned targetLength = (unsigned)target.get(exec, exec->propertyNames().length).asNumber();
>> -        if (targetLength > numBoundArgs)
>> -            length = targetLength - numBoundArgs;
>> +        JSValue lengthValue = target.get(exec, exec->propertyNames().length);
>> +        if (lengthValue.isNumber()) {
>> +            unsigned targetLength = (unsigned)lengthValue.asNumber();
>> +            if (targetLength > numBoundArgs)
>> +                length = targetLength - numBoundArgs;
>> +        }
> 
> I added this here because it's wrong w.r.t to the spec.
> There is a kangax test that tests this in combination with Proxy.[[GetOwnProperty]].
> To make us pass that test, I need to also implement Proxy.[[Call]]. I can wait
> or open another bug for this if anybody objects about having it in this patch.

I added this here because it's wrong w.r.t to the spec.
There is a kangax test that tests this in combination with Proxy.[[GetOwnProperty]].
To make us pass that test, I need to also implement Proxy.[[Call]]. I can wait
or open another bug for this if anybody objects about having it in this patch.

>> Source/JavaScriptCore/runtime/JSObject.cpp:2942
>> +    }
> 
> I would have written:
> 
>     if (!method.isCell()) {
>         if (method.isUndefinedOrNull())
>             return jsUndefined();
>         return throwVMTypeError(exec, errorMessage);
>     }
> 
> Having one less branch seems slightly more elegant to me.

I agree.
I'll upload a fix.
Comment 11 Saam Barati 2016-02-23 13:44:44 PST
(In reply to comment #10)
> Comment on attachment 271616 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271616&action=review
> 
> >> Source/JavaScriptCore/runtime/FunctionPrototype.cpp:167
> >> +    if (targetObject->hasOwnProperty(exec, exec->propertyNames().length)) {
> >> +        if (exec->hadException())
> >> +            return JSValue::encode(jsUndefined());
> >> +
> >>          // a. Let L be the length property of Target minus the length of A.
> >>          // b. Set the length own property of F to either 0 or L, whichever is larger.
> >> -        unsigned targetLength = (unsigned)target.get(exec, exec->propertyNames().length).asNumber();
> >> -        if (targetLength > numBoundArgs)
> >> -            length = targetLength - numBoundArgs;
> >> +        JSValue lengthValue = target.get(exec, exec->propertyNames().length);
> >> +        if (lengthValue.isNumber()) {
> >> +            unsigned targetLength = (unsigned)lengthValue.asNumber();
> >> +            if (targetLength > numBoundArgs)
> >> +                length = targetLength - numBoundArgs;
> >> +        }
> > 
> > I added this here because it's wrong w.r.t to the spec.
> > There is a kangax test that tests this in combination with Proxy.[[GetOwnProperty]].
> > To make us pass that test, I need to also implement Proxy.[[Call]]. I can wait
> > or open another bug for this if anybody objects about having it in this patch.
> 
> I added this here because it's wrong w.r.t to the spec.
> There is a kangax test that tests this in combination with
> Proxy.[[GetOwnProperty]].
> To make us pass that test, I need to also implement Proxy.[[Call]]. I can
> wait
> or open another bug for this if anybody objects about having it in this
> patch.
I definitely didn't mean to post this comment again. I think bugzilla cached my comment from before.
Comment 12 Saam Barati 2016-02-23 13:45:10 PST
(In reply to comment #10)
> Comment on attachment 271616 [details]
> patch
> >> Source/JavaScriptCore/runtime/JSObject.cpp:2942
> >> +    }
> > 
> > I would have written:
> > 
> >     if (!method.isCell()) {
> >         if (method.isUndefinedOrNull())
> >             return jsUndefined();
> >         return throwVMTypeError(exec, errorMessage);
> >     }
> > 
> > Having one less branch seems slightly more elegant to me.
> 
> I agree.
> I'll upload a fix.

the bug for the fix:
https://bugs.webkit.org/show_bug.cgi?id=154603