Bug 154081 – Implement Proxy [[Get]]

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 271313 [details]
it begins

[Filip Pizlo]

Comment on attachment 271313 [details]
it begins

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

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:177
> +    PropertySlot slot(thisObject, PropertySlot::TrapType::Get); // FIXME: what to do here???

VMInquiry would go to GetPropertySlot, right?  That seems least wrong.  A "Get" could be serviced by calling the get method, which would not tell you if there was a getter - it would just return its value.

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:204
> +    PropertySlot slot(thisObject, PropertySlot::TrapType::GetOwnProperty); // FIXME: what to do here??? Is this in the spec?

Ditto.

[Saam Barati]

Created attachment 271397 [details]
patch

almost done.
We're failing various things still inside es6.yaml which I think are unrelated to Proxy but just bugs we have in our code.
I'll start going through those individually.

[WebKit Commit Bot]

Attachment 271397 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/ProxyConstructor.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/ProxyObject.cpp:29:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/ProxyObject.cpp:32:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/ProxyObject.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/JavaScriptCore/runtime/ProxyConstructor.h:38:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/ProxyConstructor.h:38:  The parameter name "structure" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/ProxyConstructor.h:47:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 8 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 271444 [details]
patch

[Saam Barati]

Created attachment 271448 [details]
patch

style fixes

[Saam Barati]

Created attachment 271454 [details]
patch

[Build Bot]

Comment on attachment 271454 [details]
patch

Attachment 271454 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/841450

New failing tests:
storage/indexeddb/odd-strings-private.html

[Build Bot]

Created attachment 271473 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Saam Barati]

Created attachment 271493 [details]
patch

build fix attempt

[Build Bot]

Comment on attachment 271493 [details]
patch

Attachment 271493 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/842179

New failing tests:
js/regress/Float64Array-to-Int16Array-set.html

[Build Bot]

Created attachment 271507 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Michael Saboff]

Comment on attachment 271493 [details]
patch

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

Looks pretty good. I just have a few questions.

> Source/JavaScriptCore/ChangeLog:13
> +        to mean more than operation in the spec, having TrapType gives

This sentence doesn't make sense to me, especially the phrase "mean more than operation int he spec".

> Source/JavaScriptCore/ChangeLog:15
> +        TrapType is an enum with the following values:

I don't understand why this is called TrapType.  When I think of "Trap", I think of an exception and its handler, e.g. divide by zero trap.  Would a name like UseType make more sense?

> Source/JavaScriptCore/ChangeLog:30
> +        TrapType::Has and TrapType::GetOwnProperty, which will both be defined

Do you mean "HasProperty"?

> Source/JavaScriptCore/runtime/PropertySlot.h:82
> +    explicit PropertySlot(const JSValue thisValue, TrapType trapType)

Can we default trapType to TrapType::Get?  It seems to be the most common value.

> Source/JavaScriptCore/runtime/ProxyObject.cpp:74
> +    // FIXME: make it so that custom getters take both the |this| value and the slotBase (property holder).

Bug number?

> Source/JavaScriptCore/runtime/ProxyObject.cpp:143
> +    // FIXME: Implement other proxy traps.

Bug number?

[Saam Barati]

Comment on attachment 271493 [details]
patch

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

>> Source/JavaScriptCore/ChangeLog:13
>> +        to mean more than operation in the spec, having TrapType gives
> 
> This sentence doesn't make sense to me, especially the phrase "mean more than operation int he spec".

Maybe this is better written as:

We use getOwnPropertySlot to implement more than one
Internal Method in the spec. Because of this, we need TrapType
to give us context about which Internal Method we're executing.
Specifically, Proxy will call into different handlers based on this information.

>> Source/JavaScriptCore/ChangeLog:15
>> +        TrapType is an enum with the following values:
> 
> I don't understand why this is called TrapType.  When I think of "Trap", I think of an exception and its handler, e.g. divide by zero trap.  Would a name like UseType make more sense?

Michael and I spoke over IRC.
The spec calls these "internal methods".
I'm going to go with InternalMethodType

>> Source/JavaScriptCore/ChangeLog:30
>> +        TrapType::Has and TrapType::GetOwnProperty, which will both be defined
> 
> Do you mean "HasProperty"?

yeah. Will fix.

>> Source/JavaScriptCore/runtime/ProxyObject.cpp:74
>> +    // FIXME: make it so that custom getters take both the |this| value and the slotBase (property holder).
> 
> Bug number?

will create a bug and link.

>> Source/JavaScriptCore/runtime/ProxyObject.cpp:143
>> +    // FIXME: Implement other proxy traps.
> 
> Bug number?

will link bugs.

[Saam Barati]

Created attachment 271518 [details]
patch

addressed comments.

[WebKit Commit Bot]

Attachment 271518 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Comment on attachment 271518 [details]
patch

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

LGTM

> Source/JavaScriptCore/ChangeLog:35
> +        user oversable effects.

Spelling: observable

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/196722