Bug 222918 – Align internal methods of WindowProperties object with the spec

• Component: DOM
• Status: RESOLVED FIXED

[Alexey Shvayka]

Align Window named properties object with the spec

[Alexey Shvayka]

Created attachment 422588 [details]
Patch

[EWS Watchlist]

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

[Alexey Shvayka]

Comment on attachment 422588 [details]
Patch

Removing r? since wpt/WebIDL/ecmascript-binding/class-string-named-properties-object.window.html depends on symbol properties being treated as ordinary by WindowProperties, which does contradict the current spec, but 1) feels like an expected behavior, 2) aligns with legacy platform objects, and 3) is implemented by Blink.

Let's align with Blink and change the spec.

[Radar WebKit Bug Importer]

<rdar://problem/75436364>

[Alexey Shvayka]

Created attachment 427313 [details]
Patch

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #3)
> Let's align with Blink and change the spec.

Gecko team implements WindowProperties without underlying structure and rightfully doesn't want to add one just for symbols. Also, even with proposed change (https://github.com/heycam/webidl/pull/963), [[DefineOwnProperty]] would remain weird with supported properties and their current descriptors.

There is a consensus to align implementations with current spec, which is what new patch does.

[Sam Weinig]

Comment on attachment 427313 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMWindowProperties.cpp:138
> +    return typeError(lexicalGlobalObject, scope, shouldThrow, "Unable to define a property on a WindowProperties object."_s);

This error message reads a little weird to me. Maybe, "Defining a property on a WindowProperties is not allowed." or something like that to indicate it is not just failing because something went wrong, but we are actively preventing this on purpose?

> LayoutTests/imported/w3c/ChangeLog:7
> +

Are test changes here from upstream or do you plan to upstream them?

[Alexey Shvayka]

(In reply to Sam Weinig from comment #7)

Thank you for review!

> > Source/WebCore/bindings/js/JSDOMWindowProperties.cpp:138
> > +    return typeError(lexicalGlobalObject, scope, shouldThrow, "Unable to define a property on a WindowProperties object."_s);
> 
> This error message reads a little weird to me. Maybe, "Defining a property
> on a WindowProperties is not allowed." or something like that to indicate it
> is not just failing because something went wrong, but we are actively
> preventing this on purpose?

Nicely noted, will change to "is not allowed".

> > LayoutTests/imported/w3c/ChangeLog:7
> > +
> 
> Are test changes here from upstream or do you plan to upstream them?

Since test changes are rather substantial, I would rather have them reviewed by WPT people as well, before upstreaming (https://github.com/web-platform-tests/wpt/pull/27970).

[Sam Weinig]

(In reply to Alexey Shvayka from comment #8)
> (In reply to Sam Weinig from comment #7)
> 
> Thank you for review!
> 
> > > Source/WebCore/bindings/js/JSDOMWindowProperties.cpp:138
> > > +    return typeError(lexicalGlobalObject, scope, shouldThrow, "Unable to define a property on a WindowProperties object."_s);
> > 
> > This error message reads a little weird to me. Maybe, "Defining a property
> > on a WindowProperties is not allowed." or something like that to indicate it
> > is not just failing because something went wrong, but we are actively
> > preventing this on purpose?
> 
> Nicely noted, will change to "is not allowed".
> 
> > > LayoutTests/imported/w3c/ChangeLog:7
> > > +
> > 
> > Are test changes here from upstream or do you plan to upstream them?
> 
> Since test changes are rather substantial, I would rather have them reviewed
> by WPT people as well, before upstreaming
> (https://github.com/web-platform-tests/wpt/pull/27970).

Sounds good.

[Alexey Shvayka]

Created attachment 429205 [details]
Patch for landing

Reword error message and sync now reviewed WPT from upstream.

[Alexey Shvayka]

Committed r277829 (237975@main): <https://commits.webkit.org/237975@main>