Bug 163381 – [WebIDL] Add support to bindings generator for half-open dictionaries

• Status: ASSIGNED
• Component: Bindings

[Rawinder Singh]

This bug has superseded Bug #158509 ([WebIDL] Add support for sequence<Dictionary>).

Add support to bindings generator for half-open dictionaries, e.g.:
(see Bug #158509, comment #14 for further discussion of why this feature is being added)

[
    OpenPropertyType=DOMString
]
dictionary OpenPropertyTypeDictionary1 {
   ...
};

This will be used for the Web Animations API implementation (see Bug #158508). Further test cases for this feature will be added as part of the implementation of Bug #158508.

[Rawinder Singh]

Created attachment 291457 [details]
Patch

[Rawinder Singh]

*** Bug 158509 has been marked as a duplicate of this bug. ***

[Anne van Kesteren]

Note the discussion in https://github.com/heycam/webidl/pull/180. You might want to align your syntax or give input with respect to the requirements.

[Rawinder Singh]

Created attachment 291458 [details]
Patch

[Rawinder Singh]

(In reply to comment #3)
> Note the discussion in https://github.com/heycam/webidl/pull/180. You might
> want to align your syntax or give input with respect to the requirements.

Okay, thanks for the link - I'll have a look and possibly update the patch.

In the meanwhile, could Sam or Chris have a look at the general implementation and let me know if you have any comments (as this overlaps with changes you have both been making in the generators and converters recently).

[Chris Dumez]

Comment on attachment 291458 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        WebKit IDL extended dictionary attribute "OpenPropertyType".

Can you point me to the specification for this? I cannot find OpenPropertyType at https://heycam.github.io/webidl/

[Chris Dumez]

Comment on attachment 291458 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMConvert.h:345
> +    template<typename ReturnType, typename T> bool addResultIfValid(ReturnType &result, WTF::Optional<T> &value)

& is on the wrong side.

> Source/WebCore/bindings/js/JSDOMConvert.h:638
> +template<typename IDLType> HashMap<String, typename IDLType::ImplementationType> convertDictionaryProperties(JSC::ExecState& state, JSC::JSObject& object)

I think there should be "Open" somewhere in the name. Maybe convertOpenDictionary? or convertOpenDictionaryType?

> Source/WebCore/bindings/js/JSDOMConvert.h:648
> +        result.add(propertyName.string(), value);

WTFMove(value) ?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1154
> +    # 6. Process WebKit extended IDL attribute (OpenPropertyType) for half-open dictionary:

The steps numbering currently match the specification. However, I cannot find this new step in the spec?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1155
> +    my $idlOpenPropertyType = $dictionary->extendedAttributes->{"OpenPropertyType"};

quotes are not needed.

[Chris Dumez]

Comment on attachment 291458 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        WebKit IDL extended dictionary attribute "OpenPropertyType".
> 
> Can you point me to the specification for this? I cannot find OpenPropertyType at https://heycam.github.io/webidl/

Ok, I found https://github.com/heycam/webidl/pull/180 in the comments. Should we wait until people agree upon a given syntax before we land this patch? It seems the latest proposals look pretty different from what's implemented here.

[Rawinder Singh]

(In reply to comment #8)
> Comment on attachment 291458 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291458&action=review
> 
> >> Source/WebCore/ChangeLog:9
> >> +        WebKit IDL extended dictionary attribute "OpenPropertyType".
> > 
> > Can you point me to the specification for this? I cannot find OpenPropertyType at https://heycam.github.io/webidl/
> 
> Ok, I found https://github.com/heycam/webidl/pull/180 in the comments.
> Should we wait until people agree upon a given syntax before we land this
> patch? It seems the latest proposals look pretty different from what's
> implemented here.

"OpenPropertyType" is equivalent to the syntax that Sam described in Bug #158509, comment #14 (in his suggested syntax it was called "HalfOpenType") - it is the specialised WebKit IDL extended attribute for dictionaries that was discussed.

According to Bug #158509, comment #5, the Web IDL syntax for open dictionaries is still being discussed and will not become stable for a while. I am worried that this will stall the Web Animations API implementation - I would like to put this in temporarily (maybe with a FIXME) and update the implementation when the Web IDL syntax does finally become stable - what do you think?

[Rawinder Singh]

Created attachment 292129 [details]
Patch

[Rawinder Singh]

Created attachment 292131 [details]
Patch

[Rawinder Singh]

Created attachment 292142 [details]
Patch

[Brady Eidson]

Comment on attachment 292142 [details]
Patch

Patches that have been up for review since 2016 are almost certainly too stale to be relevant to trunk in their current form.

If this patch is still important please rebase it and post it for review again.