[webkit-reviews] review granted: [Bug 218849] Align legacy platform objects with other implementations : [Attachment 414808] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 218849: Align legacy platform objects with other implementations
https://bugs.webkit.org/show_bug.cgi?id=218849

Attachment 414808: Patch

https://bugs.webkit.org/attachment.cgi?id=414808&action=review




--- Comment #16 from Darin Adler <darin at apple.com> ---
Comment on attachment 414808
  --> https://bugs.webkit.org/attachment.cgi?id=414808
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/DefineOwnPr
operty-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.tx
t: Removed.
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/DefineOwnPr
operty-002-expected.txt: Added.
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/DefineOwnPr
operty-002.html: Added.
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/DefineOwnPr
operty-expected.txt:
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/DefineOwnPr
operty-gen.tentative-expected.txt: Added.
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/DefineOwnPr
operty-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/GetOwnPrope
rty-gen-expected.txt: Added.
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/GetOwnPrope
rty-gen.html: Added.
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/HTMLDocumen
t-unforgeable-expected.txt: Added.
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/HTMLDocumen
t-unforgeable.html: Added.
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/PreventExte
nsions-expected.txt: Added.
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/PreventExte
nsions.html: Added.
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Set-expecte
d.txt:
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Set-gen-rec
eiver-altered-expected.txt: Added.
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Set-gen-rec
eiver-altered.html: Added.
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Set-gen.ten
tative-expected.txt: Added.
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/Set-gen.ten
tative.html: Added.
> +	   *
web-platform-tests/WebIDL/ecmascript-binding/legacy-platform-object/lpo-interfa
ces.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-ex
pected.txt:
> +	   *
web-platform-tests/dom/collections/HTMLCollection-supported-property-names-expe
cted.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/Te
xtTrackCueList/getter-expected.txt:
> +	   *
web-platform-tests/html/semantics/embedded-content/media-elements/interfaces/Te
xtTrackList/getter-expected.txt:
> +	   *
web-platform-tests/html/semantics/forms/the-form-element/form-indexed-element-e
xpected.txt:
> +	   *
web-platform-tests/html/semantics/forms/the-form-element/form-nameditem-expecte
d.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!