Align legacy platform objects with other implementations
Created attachment 413936 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment on attachment 413936 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413936&action=review Looks really good. Hard to fully predict the compatibility impact, but I *hope* it’s a plus for interoperability without harming old content that could accidentally on details like these (in iOS apps especially). I think you didn’t do "run-bindings-tests --reset-results", so please do that in the next version of the patch so we can look over the changes to the bindings tests. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1167 > - push(@$outputArray, $indent . "RETURN_IF_EXCEPTION(throwScope, true);\n"); > + push(@$outputArray, $indent . "RETURN_IF_EXCEPTION(throwScope, false);\n"); If this an important change? I’m OK with it, but I think that using true rather than false is not observable. Is that right? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1184 > - push(@$outputArray, $indent . "RETURN_IF_EXCEPTION(throwScope, true);\n"); > + push(@$outputArray, $indent . "RETURN_IF_EXCEPTION(throwScope, false);\n"); Same question. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1241 > + push(@$outputArray, " return typeError(lexicalGlobalObject, throwScope, putPropertySlot.isStrictMode(), \"Failed to set an indexed property on '${interfaceName}': Indexed property setter is not supported.\"_s);\n"); A side note: Generally we have an overall goal to keep the generated code as simple as possible. I can imagine tightening this slightly by just passing interfaceName to a helper function that builds the string dynamically. Since it’s probably not super-performance critical it would make things slightly simpler and also might make the binary smaller since we would not compile so many different long-ish string constants. I don’t see an obvious way to cut down the number of arguments, though. I think it’s important to think about this any time we tweak the generated code. Long term it would probably be good to have more C++ code and less perl code, even if the C++ code ends up using more template meta-programming and such. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1263 > + push(@$outputArray, " if (thisObject->wrapped().isSupportedPropertyName(propertyNameToAtomString(propertyName)))\n"); I see this idiom multiple times. Could we tighten things *slightly* if we write a helper for this instead of writing it out each time? > Source/WebCore/css/CSSStyleDeclaration.cpp:312 > + auto* settings = parentElement() ? &parentElement()->document().settings() : nullptr; Now that we are doing this a third time, maybe we should write a settings function for use in this class’s implement. It can be inline and either a member function or a non-member. > Source/WebCore/css/CSSStyleDeclaration.cpp:314 > + auto propertyInfo = propertyInfoFromJavaScriptCSSPropertyName(propertyName, settings); > + return !!propertyInfo.propertyID; Not sure the local variable helps here. Can we merge these into one line? > Source/WebCore/css/CSSStyleDeclaration.h:78 > + bool deleteNamedProperty(const AtomString&) const { return true; } I suggest "static" instead of "const" on this member function. > Source/WebCore/css/StyleSheetList.cpp:112 > + if (!m_document) > + return false; > + > + Element* element = m_document->getElementById(name); > + return is<HTMLStyleElement>(element); Reads really cleanly as a one-liner: return m_document && is<HTMLStyleElement>(m_document->getElementById(name)); > Source/WebCore/dom/NamedNodeMap.cpp:70 > + return !!m_element.getAttributeNode(name); I’m not the biggest fan of !!, but I don’t have a better idiom to suggest. > Source/WebCore/html/HTMLFrameSetElement.cpp:237 > + auto frameElement = makeRefPtr(children()->namedItem(name)); > + return is<HTMLFrameElement>(frameElement); Seems like this should be a one-liner. Also makes it clear we don’t need a RefPtr: return is<HTMLFrameElement>(children()->namedItem(name)); > Source/WebCore/page/UserMessageHandlersNamespace.h:54 > + bool isSupportedPropertyName(const AtomString& propertyName) const; Given the phrase "property name" is in the function name, I suggest we not name the argument. > Source/WebCore/plugins/DOMMimeTypeArray.cpp:72 > + for (auto& type : m_types) { > + if (type->type() == propertyName) > + return true; > + } > + return false; Repetitiveness of writing this three times is annoying. I hope to find a way to cut down / refactor this some day. Like exposing a range based iterator would allow them all to share code, or something. > Source/WebCore/plugins/DOMMimeTypeArray.h:43 > + bool isSupportedPropertyName(const AtomString& propertyName) const; Ditto. > Source/WebCore/plugins/DOMPlugin.h:49 > + bool isSupportedPropertyName(const AtomString& propertyName) const; Ditto. > Source/WebCore/plugins/DOMPluginArray.h:44 > + bool isSupportedPropertyName(const AtomString& propertyName) const; Ditto.
Thanks for working on this! Mind running `run-bindings-tests --reset-results` and uploading a version with the uploaded bindings tests results. That can help in reviewing as we can see what the new bindings look like.
<rdar://problem/71587194>
Created attachment 414808 [details] Patch
(In reply to Darin Adler from comment #3) > Looks really good. Hard to fully predict the compatibility impact, but I > *hope* it’s a plus for interoperability without harming old content that > could accidentally on details like these (in iOS apps especially). Your feedback is highly appreciated! I've took some time to research web-compatibility impact of this patch by crafting thorough generated tests. It appears that every single test case is passing on at least Gecko or Blink, except for this one: Object.defineProperty(sessionStorage, %key%, { value: 1 }); sessionStorage.getItem(%key%); // patch: null, trunk and other runtimes: "1" where %key% is a property of Storage.prototype or Object.prototype. sessionStorage.%key% semantics hasn't changed: it returns the prototype method as before. I believe it is safe: WebStorage is notoriously unreliable (private tabs), only Object.defineProperty() is affected, and only with small set of names. We need this change to align property visibility of [[DefineOwnProperty]] with [[Set]], so OrdinarySet (https://tc39.es/ecma262/#sec-ordinaryset) could be used in the WebIDL spec. > I think you didn’t do "run-bindings-tests --reset-results", so please do > that in the next version of the patch so we can look over the changes to the > bindings tests. Done. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1167 > > - push(@$outputArray, $indent . "RETURN_IF_EXCEPTION(throwScope, true);\n"); > > + push(@$outputArray, $indent . "RETURN_IF_EXCEPTION(throwScope, false);\n"); > > If this an important change? I’m OK with it, but I think that using true > rather than false is not observable. Is that right? Right, it's not observable nor important (noted in the ChangeLog). However, I think it's worth changing per Saam's post-review feedback on https://bugs.webkit.org/show_bug.cgi?id=172687#c11 and to enable us enforcing this contract the same we do for [[Get]]: bool hasProperty = target->getPropertySlot(globalObject, propertyName, slot); EXCEPTION_ASSERT(!scope.exception() || !hasProperty); > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1241 > > + push(@$outputArray, " return typeError(lexicalGlobalObject, throwScope, putPropertySlot.isStrictMode(), \"Failed to set an indexed property on '${interfaceName}': Indexed property setter is not supported.\"_s);\n"); > > A side note: Generally we have an overall goal to keep the generated code as > simple as possible. I can imagine tightening this slightly by just passing > interfaceName to a helper function that builds the string dynamically. Since > it’s probably not super-performance critical it would make things slightly > simpler and also might make the binary smaller since we would not compile so > many different long-ish string constants. I don’t see an obvious way to cut > down the number of arguments, though. I think it’s important to think about > this any time we tweak the generated code. Long term it would probably be > good to have more C++ code and less perl code, even if the C++ code ends up > using more template meta-programming and such. Great suggestion, thank you! I've extracted a few make*ErrorMessage() helpers. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1263 > > + push(@$outputArray, " if (thisObject->wrapped().isSupportedPropertyName(propertyNameToAtomString(propertyName)))\n"); > > I see this idiom multiple times. Could we tighten things *slightly* if we > write a helper for this instead of writing it out each time? In a follow-up, Storage and DOMStringMap methods can be changed to consistently accept AtomString&, allowing us to save propertyNameToAtomString() result to a variable & reuse. > > Source/WebCore/css/CSSStyleDeclaration.cpp:312 > > + auto* settings = parentElement() ? &parentElement()->document().settings() : nullptr; > > Now that we are doing this a third time, maybe we should write a settings > function for use in this class’s implement. It can be inline and either a > member function or a non-member. Let's get back to this once https://bugs.webkit.org/show_bug.cgi?id=217623 is fixed. I have an idea how to re-land it, removing CSSStyleDeclaration::namedItem() and [CallNamedSetterOnlyForSupportedProperties]. > > Source/WebCore/css/CSSStyleDeclaration.cpp:314 > > + auto propertyInfo = propertyInfoFromJavaScriptCSSPropertyName(propertyName, settings); > > + return !!propertyInfo.propertyID; > > Not sure the local variable helps here. Can we merge these into one line? Merged. > > Source/WebCore/css/CSSStyleDeclaration.h:78 > > + bool deleteNamedProperty(const AtomString&) const { return true; } > > I suggest "static" instead of "const" on this member function. Could you please provide a code sample? Direct replacement doesn't compile. > > Source/WebCore/css/StyleSheetList.cpp:112 > > + if (!m_document) > > + return false; > > + > > + Element* element = m_document->getElementById(name); > > + return is<HTMLStyleElement>(element); > > Reads really cleanly as a one-liner: Replaced. > > Source/WebCore/dom/NamedNodeMap.cpp:70 > > + return !!m_element.getAttributeNode(name); > > I’m not the biggest fan of !!, but I don’t have a better idiom to suggest. Replaced with Element::hasAttribute(), saving Attr node creation. > > Source/WebCore/html/HTMLFrameSetElement.cpp:237 > > + auto frameElement = makeRefPtr(children()->namedItem(name)); > > + return is<HTMLFrameElement>(frameElement); > > Seems like this should be a one-liner. Also makes it clear we don’t need a RefPtr: Replaced. > > Source/WebCore/page/UserMessageHandlersNamespace.h:54 > > + bool isSupportedPropertyName(const AtomString& propertyName) const; > > Given the phrase "property name" is in the function name, I suggest we not name the argument. Removed parameter name here and in few other places. > > Source/WebCore/plugins/DOMMimeTypeArray.cpp:72 > > + for (auto& type : m_types) { > > + if (type->type() == propertyName) > > + return true; > > + } > > + return false; > > Repetitiveness of writing this three times is annoying. I hope to find a way > to cut down / refactor this some day. Like exposing a range based iterator > would allow them all to share code, or something. Reduced this to a one-liner using Vector::findMatching(). In a follow-up, we can introduce Vector::containsMatching() to make these bits even cleaner (and other call sites).
(In reply to Saam Barati from comment https://bugs.webkit.org/show_bug.cgi?id=172687#c10) > Comment on attachment 311961 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=311961&action=review > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:483 > > + push(@$outputArray, " JSValue proto = thisObject->getPrototypeDirect();\n"); > > + push(@$outputArray, " if (proto.isObject() && jsCast<JSObject*>(proto)->hasProperty(state, propertyName))\n"); > > + push(@$outputArray, " return false;\n\n"); > > Is it safe to use getPrototypeDirect here? Do we not expect that this is > generated for something that has a custom getPrototype hook? Would that even > matter? No legacy platform object currently override getPrototype(), but the spec was changed to perform [[GetPrototypeOf]] instead. I've added a FIXME. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:763 > > + push(@$outputArray, " PropertySlot slot { thisObject, PropertySlot::InternalMethodType::VMInquiry };\n"); > > VMInquiry looks wrong here. I think you want something else, but it depends > on which operation you're trying to perform. The intention is to avoid ProxyObject traps. The spec uses ordinary [[GetOwnProperty]] (step 5.1 of https://heycam.github.io/webidl/#dfn-named-property-visibility), other runtimes agree. We could use getDirectOffset() + loop instead. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:887 > > + # FIXME: Is Base::getOwnPropertySlot the right function to call? Is there a function that will > > + # only look at the actual properties, and not call into our implementation of the > > + # [[GetOwnProperty]] hook? > > This seems correct to me, but it probably depends on what Base is. JSObject::getOwnPropertySlot() is now used instead. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:904 > > + # 2.2.1. If the result of calling IsDataDescriptor(Desc) is false, then return false. > > + push(@$outputArray, $additionalIndent . " if (!propertyDescriptor.isDataDescriptor())\n"); > > + push(@$outputArray, $additionalIndent . " return false;\n"); > > + > > + # 2.2.2. Invoke the named property setter with P and Desc.[[Value]]. > > + GenerateInvokeNamedPropertySetter($outputArray, $additionalIndent . " ", $interface, $namedSetterOperation, "propertyDescriptor.value()"); > > These specced semantics are bizarre. I'm sure there is some reason they are > the way they are, but it's counter-intuitive that defineOwnProperty can mean > "call this setter instead". Agreed, yet both Gecko and Blink implement this. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:634 > > + push(@$outputArray, $indent . "RETURN_IF_EXCEPTION(throwScope, true);\n"); > > nit: Should not matter in practice, but I'd return false in case of an > exception. Fixed.
Comment on attachment 413936 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=413936&action=review >> Source/WebCore/css/CSSStyleDeclaration.h:78 >> + bool deleteNamedProperty(const AtomString&) const { return true; } > > I suggest "static" instead of "const" on this member function. static bool deleteNamedProperty(const AtomString&) { return true; }
(In reply to Alexey Shvayka from comment #8) > (In reply to Saam Barati from comment > https://bugs.webkit.org/show_bug.cgi?id=172687#c10) > > Comment on attachment 311961 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=311961&action=review > > > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:483 > > > + push(@$outputArray, " JSValue proto = thisObject->getPrototypeDirect();\n"); > > > + push(@$outputArray, " if (proto.isObject() && jsCast<JSObject*>(proto)->hasProperty(state, propertyName))\n"); > > > + push(@$outputArray, " return false;\n\n"); > > > > Is it safe to use getPrototypeDirect here? Do we not expect that this is > > generated for something that has a custom getPrototype hook? Would that even > > matter? > > No legacy platform object currently override getPrototype(), but the spec > was changed to perform [[GetPrototypeOf]] instead. I've added a FIXME. > > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:763 > > > + push(@$outputArray, " PropertySlot slot { thisObject, PropertySlot::InternalMethodType::VMInquiry };\n"); > > > > VMInquiry looks wrong here. I think you want something else, but it depends > > on which operation you're trying to perform. > > The intention is to avoid ProxyObject traps. > The spec uses ordinary [[GetOwnProperty]] (step 5.1 of > https://heycam.github.io/webidl/#dfn-named-property-visibility), other > runtimes agree. > We could use getDirectOffset() + loop instead. Can't GetOwnProperty invoke a proxy trap? > > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:887 > > > + # FIXME: Is Base::getOwnPropertySlot the right function to call? Is there a function that will > > > + # only look at the actual properties, and not call into our implementation of the > > > + # [[GetOwnProperty]] hook? > > > > This seems correct to me, but it probably depends on what Base is. > > JSObject::getOwnPropertySlot() is now used instead. > > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:904 > > > + # 2.2.1. If the result of calling IsDataDescriptor(Desc) is false, then return false. > > > + push(@$outputArray, $additionalIndent . " if (!propertyDescriptor.isDataDescriptor())\n"); > > > + push(@$outputArray, $additionalIndent . " return false;\n"); > > > + > > > + # 2.2.2. Invoke the named property setter with P and Desc.[[Value]]. > > > + GenerateInvokeNamedPropertySetter($outputArray, $additionalIndent . " ", $interface, $namedSetterOperation, "propertyDescriptor.value()"); > > > > These specced semantics are bizarre. I'm sure there is some reason they are > > the way they are, but it's counter-intuitive that defineOwnProperty can mean > > "call this setter instead". > > Agreed, yet both Gecko and Blink implement this. > > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:634 > > > + push(@$outputArray, $indent . "RETURN_IF_EXCEPTION(throwScope, true);\n"); > > > > nit: Should not matter in practice, but I'd return false in case of an > > exception. > > Fixed.
(In reply to Saam Barati from comment #10) > > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:763 > > > > + push(@$outputArray, " PropertySlot slot { thisObject, PropertySlot::InternalMethodType::VMInquiry };\n"); > > > > > > VMInquiry looks wrong here. I think you want something else, but it depends > > > on which operation you're trying to perform. > > > > The intention is to avoid ProxyObject traps. > > The spec uses ordinary [[GetOwnProperty]] (step 5.1 of > > https://heycam.github.io/webidl/#dfn-named-property-visibility), other > > runtimes agree. > > We could use getDirectOffset() + loop instead. > > Can't GetOwnProperty invoke a proxy trap? Apologies, my wording was poor. The spec text exactly is "prototype has an own property named P", matching the prose in OrdinaryGetOwnProperty (step 2 of https://tc39.es/ecma262/#sec-ordinarygetownproperty), which performs structure lookup only, ignoring all "virtual" properties.
(In reply to Alexey Shvayka from comment #11) > (In reply to Saam Barati from comment #10) > > > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:763 > > > > > + push(@$outputArray, " PropertySlot slot { thisObject, PropertySlot::InternalMethodType::VMInquiry };\n"); > > > > > > > > VMInquiry looks wrong here. I think you want something else, but it depends > > > > on which operation you're trying to perform. > > > > > > The intention is to avoid ProxyObject traps. > > > The spec uses ordinary [[GetOwnProperty]] (step 5.1 of > > > https://heycam.github.io/webidl/#dfn-named-property-visibility), other > > > runtimes agree. > > > We could use getDirectOffset() + loop instead. > > > > Can't GetOwnProperty invoke a proxy trap? > > Apologies, my wording was poor. > The spec text exactly is "prototype has an own property named P", matching > the prose in OrdinaryGetOwnProperty (step 2 of > https://tc39.es/ecma262/#sec-ordinarygetownproperty), which performs > structure lookup only, ignoring all "virtual" properties. Does that actually map to how Structures always work? Why can't an implementation of getOwnPropertySlot also present one of those properties?
(In reply to Saam Barati from comment #12) > (In reply to Alexey Shvayka from comment #11) > > (In reply to Saam Barati from comment #10) > > > > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:763 > > > > > > + push(@$outputArray, " PropertySlot slot { thisObject, PropertySlot::InternalMethodType::VMInquiry };\n"); > > > > > > > > > > VMInquiry looks wrong here. I think you want something else, but it depends > > > > > on which operation you're trying to perform. > > > > > > > > The intention is to avoid ProxyObject traps. > > > > The spec uses ordinary [[GetOwnProperty]] (step 5.1 of > > > > https://heycam.github.io/webidl/#dfn-named-property-visibility), other > > > > runtimes agree. > > > > We could use getDirectOffset() + loop instead. > > > > > > Can't GetOwnProperty invoke a proxy trap? > > > > Apologies, my wording was poor. > > The spec text exactly is "prototype has an own property named P", matching > > the prose in OrdinaryGetOwnProperty (step 2 of > > https://tc39.es/ecma262/#sec-ordinarygetownproperty), which performs > > structure lookup only, ignoring all "virtual" properties. > > Does that actually map to how Structures always work? Why can't an > implementation of getOwnPropertySlot also present one of those properties? This wording is specifically used to avoid calling into an implementation of getOwnPropertySlot, which may present the property, so the stack would not overflow.
(In reply to Alexey Shvayka from comment #13) > (In reply to Saam Barati from comment #12) > > (In reply to Alexey Shvayka from comment #11) > > > (In reply to Saam Barati from comment #10) > > > > > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:763 > > > > > > > + push(@$outputArray, " PropertySlot slot { thisObject, PropertySlot::InternalMethodType::VMInquiry };\n"); > > > > > > > > > > > > VMInquiry looks wrong here. I think you want something else, but it depends > > > > > > on which operation you're trying to perform. > > > > > > > > > > The intention is to avoid ProxyObject traps. > > > > > The spec uses ordinary [[GetOwnProperty]] (step 5.1 of > > > > > https://heycam.github.io/webidl/#dfn-named-property-visibility), other > > > > > runtimes agree. > > > > > We could use getDirectOffset() + loop instead. > > > > > > > > Can't GetOwnProperty invoke a proxy trap? > > > > > > Apologies, my wording was poor. > > > The spec text exactly is "prototype has an own property named P", matching > > > the prose in OrdinaryGetOwnProperty (step 2 of > > > https://tc39.es/ecma262/#sec-ordinarygetownproperty), which performs > > > structure lookup only, ignoring all "virtual" properties. > > > > Does that actually map to how Structures always work? Why can't an > > implementation of getOwnPropertySlot also present one of those properties? > > This wording is specifically used to avoid calling into an implementation of > getOwnPropertySlot, which may present the property, so the stack would not > overflow. I guess I'm confused since Structure vs getOwnPropertySlot presenting a property is an implementation detail. How does the spec say anything about that?
(In reply to Saam Barati from comment #14) > (In reply to Alexey Shvayka from comment #13) > > (In reply to Saam Barati from comment #12) > > > (In reply to Alexey Shvayka from comment #11) > > > > (In reply to Saam Barati from comment #10) > > > > > > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:763 > > > > > > > > + push(@$outputArray, " PropertySlot slot { thisObject, PropertySlot::InternalMethodType::VMInquiry };\n"); > > > > > > > > > > > > > > VMInquiry looks wrong here. I think you want something else, but it depends > > > > > > > on which operation you're trying to perform. > > > > > > > > > > > > The intention is to avoid ProxyObject traps. > > > > > > The spec uses ordinary [[GetOwnProperty]] (step 5.1 of > > > > > > https://heycam.github.io/webidl/#dfn-named-property-visibility), other > > > > > > runtimes agree. > > > > > > We could use getDirectOffset() + loop instead. > > > > > > > > > > Can't GetOwnProperty invoke a proxy trap? > > > > > > > > Apologies, my wording was poor. > > > > The spec text exactly is "prototype has an own property named P", matching > > > > the prose in OrdinaryGetOwnProperty (step 2 of > > > > https://tc39.es/ecma262/#sec-ordinarygetownproperty), which performs > > > > structure lookup only, ignoring all "virtual" properties. > > > > > > Does that actually map to how Structures always work? Why can't an > > > implementation of getOwnPropertySlot also present one of those properties? > > > > This wording is specifically used to avoid calling into an implementation of > > getOwnPropertySlot, which may present the property, so the stack would not > > overflow. > > I guess I'm confused since Structure vs getOwnPropertySlot presenting a > property is an implementation detail. How does the spec say anything about > that? The spec has this definition of "own property": https://tc39.es/ecma262/#sec-own-property, which I would roughly translate to structure property or JSObject::getOwnPropertySlot(). It is used by OrdinaryDelete (step 4.a of https://tc39.es/ecma262/#sec-ordinarydelete) and other "ordinary" methods. Usually, other specs use [[GetOwnProperty]], with WebIDL being an exception here.
Comment on attachment 414808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414808&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1216 > + push(@$outputArray, " VM& vm = JSC::getVM(lexicalGlobalObject);\n"); Seems strange that we can write VM& here, not JSC::VM&, but we need the JSC prefix on JSC::getVM. Do you know why? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1511 > + push(@$outputArray, " slot.disableCaching();\n"); Seems like this needs a "why" comment, either in the generator or even possible in the generated code. Not obvious to me why it’s needed. Stands out from the rest of the code that seems largely self-explanatory. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1567 > + push(@$outputArray, "bool ${className}::preventExtensions(JSC::JSObject*, JSC::JSGlobalObject*) { return false; }\n\n"); Add "static"? > LayoutTests/ChangeLog:9 > + * fast/dom/htmlcollection-getownproperty-expected.txt: > + * fast/dom/htmlcollection-getownproperty.html: Would be nice to have a comment saying what the change is. > LayoutTests/ChangeLog:12 > + * js/dom/legacy-platform-object-defineOwnProperty-expected.txt: > + * js/dom/legacy-platform-object-defineOwnProperty.html: > + Exported as web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/DefineOwnProperty-002.html I think the terminology here is peculiar. It’s good that we turned this into a WPT test. And fine to delete our separate test. But I don’t understand that from the term "Exported". > LayoutTests/ChangeLog:16 > + * js/dom/named-property-deleter-expected.txt: > + * js/dom/named-property-deleter.html: > + Exported as web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Delete-001.html Ditto. > LayoutTests/ChangeLog:19 > + * js/dom/reflect-set-onto-dom-expected.txt: > + * js/dom/script-tests/reflect-set-onto-dom.js: Would be nice to have a comment saying what the change is. > LayoutTests/imported/w3c/ChangeLog:29 > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object-expected.txt: Removed. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/DefineOwnProperty-002-expected.txt: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/DefineOwnProperty-002.html: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/DefineOwnProperty-expected.txt: > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/DefineOwnProperty-gen.tentative-expected.txt: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/DefineOwnProperty-gen.tentative.html: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Delete-001-expected.txt: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Delete-001.html: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Delete-gen-expected.txt: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Delete-gen.html: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/GetOwnProperty-gen-expected.txt: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/GetOwnProperty-gen.html: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/HTMLDocument-unforgeable-expected.txt: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/HTMLDocument-unforgeable.html: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/PreventExtensions-expected.txt: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/PreventExtensions.html: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Set-expected.txt: > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Set-gen-receiver-altered-expected.txt: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Set-gen-receiver-altered.html: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Set-gen.tentative-expected.txt: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Set-gen.tentative.html: Added. > + * web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/lpo-interfaces.js: Added. I don’t know how to review patches that add new Web Platform Tests. Can we add these separately with expected failures first? How will these tests get upstreamed? > LayoutTests/imported/w3c/ChangeLog:39 > + * web-platform-tests/dom/collections/HTMLCollection-delete-expected.txt: > + * web-platform-tests/dom/collections/HTMLCollection-own-props-expected.txt: > + * web-platform-tests/dom/collections/HTMLCollection-supported-property-indices-expected.txt: > + * web-platform-tests/dom/collections/HTMLCollection-supported-property-names-expected.txt: > + * web-platform-tests/dom/nodes/Document-getElementsByTagName-expected.txt: > + * web-platform-tests/dom/nodes/Element-getElementsByTagName-expected.txt: > + * web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrackCueList/getter-expected.txt: > + * web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/TextTrackList/getter-expected.txt: > + * web-platform-tests/html/semantics/forms/the-form-element/form-indexed-element-expected.txt: > + * web-platform-tests/html/semantics/forms/the-form-element/form-nameditem-expected.txt: I think it would be nice to paragraph this separately from the added tests and add a comment saying that all of these are additional PASS expectations. Hooray!
*** Bug 135566 has been marked as a duplicate of this bug. ***