Bug 154081

Created attachment 271313 [details] it begins

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.

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.

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.

Created attachment 271444 [details] patch

Created attachment 271448 [details] patch style fixes

Created attachment 271454 [details] patch

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

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

Created attachment 271493 [details] patch build fix attempt

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

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 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 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.

Created attachment 271518 [details] patch addressed comments.

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 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

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