Some WebIDL operations / attributes incorrectly use _current_ realm instead of _relevant_

• Component: DOM
• Status: RESOLVED FIXED

[Alexey Shvayka]

Some WebIDL operations / attributes incorrectly use _current_ realm instead of _relevant_

[Alexey Shvayka]

Created attachment 439585 [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

[Radar WebKit Bug Importer]

<rdar://problem/83675608>

[Sam Weinig]

Comment on attachment 439585 [details]
Patch

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

> Source/WebCore/ChangeLog:22
> +        Most of the remaining [CallWith=GlobalObject] instances are correctly used for
> +        converting JS arguments to WebIDL values; the rest, along with _current_ Document
> +        and ScriptExecutionContext, either match the spec or replacing them with _relevant_
> +        global object is not directly observable (see condition #3).

It would be great to have some guidance to others in the future that need to figure out which the correct document/globalObject,scriptExecutionContext is the right one to use (though I don't have a great idea where to put this guidance).

[Sam Weinig]

Perhaps also remaining the non-prefixed ones to use Current/Entry/etc prefixes would help, since people would feel they need to understand the concept before picking.

[Alexey Shvayka]

(In reply to Sam Weinig from comment #5)

Thanks for review!

> Perhaps also remaining the non-prefixed ones to use Current/Entry/etc
> prefixes would help, since people would feel they need to understand the
> concept before picking.

Yeah, that is the plan. Having _current_ by default, for non-prefixed values, is prone to introducing incompatibilities.

For now I will add a spec link to both IDLAttributes.json and CodeGeneratorJS.pm; WHATWG editors did a great job explaining the concepts.

[Sam Weinig]

(In reply to Alexey Shvayka from comment #6)
> (In reply to Sam Weinig from comment #5)
> 
> Thanks for review!
> 
> > Perhaps also remaining the non-prefixed ones to use Current/Entry/etc
> > prefixes would help, since people would feel they need to understand the
> > concept before picking.
> 
> Yeah, that is the plan. Having _current_ by default, for non-prefixed
> values, is prone to introducing incompatibilities.
> 
> For now I will add a spec link to both IDLAttributes.json and
> CodeGeneratorJS.pm; WHATWG editors did a great job explaining the concepts.

Sounds great. (The more we can use spec names in our code, the better in my book. I would love to see more done in this area, especially in the area of JSC functions that WebCore bindings have to interact with).

[Alexey Shvayka]

Created attachment 446847 [details]
Patch for landing

[Alexey Shvayka]

(In reply to Alexey Shvayka from comment #8)
> Created attachment 446847 [details]
> Patch for landing

Also added bindings tests for newly added Relevant* values.

[EWS]

Committed r286895 (245123@main): <https://commits.webkit.org/245123@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446847 [details].

[Alexey Shvayka]

(In reply to Sam Weinig from comment #5)
> Perhaps also remaining the non-prefixed ones to use Current/Entry/etc
> prefixes would help, since people would feel they need to understand the
> concept before picking.

Hey Sam, that change is now up for available for review: https://bugs.webkit.org/show_bug.cgi?id=236137. Thanks!