Bug 154081 - Implement Proxy [[Get]]
Summary: Implement Proxy [[Get]]
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:
Depends on:
Blocks: 35731 154313 154314
  Show dependency treegraph
 
Reported: 2016-02-10 12:33 PST by Saam Barati
Modified: 2016-02-17 14:12 PST (History)
13 users (show)

See Also:


Attachments
it begins (56.56 KB, patch)
2016-02-14 15:15 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (84.17 KB, patch)
2016-02-15 17:34 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (85.01 KB, patch)
2016-02-16 11:13 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (84.89 KB, patch)
2016-02-16 11:17 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (84.96 KB, patch)
2016-02-16 11:23 PST, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-yosemite (1.05 MB, application/zip)
2016-02-16 12:52 PST, Build Bot
no flags Details
patch (84.68 KB, patch)
2016-02-16 15:21 PST, Saam Barati
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-yosemite (888.88 KB, application/zip)
2016-02-16 16:26 PST, Build Bot
no flags Details
patch (88.59 KB, patch)
2016-02-16 18:45 PST, Saam Barati
msaboff: 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-10 12:33:11 PST
...
Comment 1 Saam Barati 2016-02-14 15:15:19 PST
Created attachment 271313 [details]
it begins
Comment 2 Filip Pizlo 2016-02-14 15:17:03 PST
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.
Comment 3 Saam Barati 2016-02-15 17:34:39 PST
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.
Comment 4 WebKit Commit Bot 2016-02-15 17:36:42 PST
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.
Comment 5 Saam Barati 2016-02-16 11:13:23 PST
Created attachment 271444 [details]
patch
Comment 6 Saam Barati 2016-02-16 11:17:24 PST
Created attachment 271448 [details]
patch

style fixes
Comment 7 Saam Barati 2016-02-16 11:23:10 PST
Created attachment 271454 [details]
patch
Comment 8 Build Bot 2016-02-16 12:52:17 PST
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
Comment 9 Build Bot 2016-02-16 12:52:20 PST
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
Comment 10 Saam Barati 2016-02-16 15:21:07 PST
Created attachment 271493 [details]
patch

build fix attempt
Comment 11 Build Bot 2016-02-16 16:25:59 PST
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
Comment 12 Build Bot 2016-02-16 16:26:03 PST
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
Comment 13 Michael Saboff 2016-02-16 17:09:21 PST
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?
Comment 14 Saam Barati 2016-02-16 18:24:24 PST
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.
Comment 15 Saam Barati 2016-02-16 18:45:37 PST
Created attachment 271518 [details]
patch

addressed comments.
Comment 16 WebKit Commit Bot 2016-02-16 18:47:18 PST
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.
Comment 17 Michael Saboff 2016-02-17 13:33:18 PST
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
Comment 18 Saam Barati 2016-02-17 14:12:02 PST
landed in:
http://trac.webkit.org/changeset/196722