Bug 154413 – Binding generator should support key value iterable

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[youenn fablet]

WebIDL iterable keyword should be supported by binding generator.
It may be good to start with Key Value iterables.

[youenn fablet]

Created attachment 271694 [details]
WIP

[youenn fablet]

Created attachment 271753 [details]
Patch

[youenn fablet]

Created attachment 271857 [details]
Throwing in case of bad forEach argument

[Darin Adler]

Comment on attachment 271857 [details]
Throwing in case of bad forEach argument

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

> Source/WebCore/Modules/fetch/FetchHeaders.h:68
>          bool next(String& nextKey, String& nextValue);

The interface we have chosen for next is really strange. I think we could come up with something much cleaner. Does this interface come from some standard or is it entirely an internal WebKit thing? Here’s what I find strange:

1) The boolean that returns true only if there is no “next”. That seems really peculiar.
2) The use of two out arguments for the key/value pair.

I think that instead we should just use Optional<std::tuple<A, B>> as the return value and not have out arguments at all.

> Source/WebCore/bindings/js/JSKeyValueIterator.h:137
> +        JSC::MarkedArgumentBuffer arguments;
> +        arguments.append(toJS(&state, wrapper->globalObject(), nextValue));
> +        arguments.append(toJS(&state, wrapper->globalObject(), nextKey));
> +        arguments.append(wrapper);
> +        JSC::call(&state, state.argument(0), callType, callData, wrapper, arguments);

Loop needs an exit if there’s an exception, right? Can we add a test that covers that case, please?

[youenn fablet]

Created attachment 271908 [details]
Patch for landing - Fixing callback throwing exception

[youenn fablet]

Created attachment 271909 [details]
Patch for landing - Fixing callback throwing exception

[youenn fablet]

Thanks for the review.

(In reply to comment #4)
> Comment on attachment 271857 [details]
> Throwing in case of bad forEach argument
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271857&action=review
> 
> > Source/WebCore/Modules/fetch/FetchHeaders.h:68
> >          bool next(String& nextKey, String& nextValue);
> 
> The interface we have chosen for next is really strange. I think we could
> come up with something much cleaner. Does this interface come from some
> standard or is it entirely an internal WebKit thing? Here’s what I find
> strange:
> 
> 1) The boolean that returns true only if there is no “next”. That seems
> really peculiar.

This mimics "done" JS iterator
JSMapIterator has a next() method returning true.

> 2) The use of two out arguments for the key/value pair.
> 
> I think that instead we should just use Optional<std::tuple<A, B>> as the
> return value and not have out arguments at all.

JSMapIterator uses WTF::KeyValuePair as an out parameter.
I will tackle this issue as a follow-up bug.

> > Source/WebCore/bindings/js/JSKeyValueIterator.h:137
> > +        JSC::MarkedArgumentBuffer arguments;
> > +        arguments.append(toJS(&state, wrapper->globalObject(), nextValue));
> > +        arguments.append(toJS(&state, wrapper->globalObject(), nextKey));
> > +        arguments.append(wrapper);
> > +        JSC::call(&state, state.argument(0), callType, callData, wrapper, arguments);
> 
> Loop needs an exit if there’s an exception, right? Can we add a test that
> covers that case, please?

Right.
This is fixed in the current patch.

[WebKit Commit Bot]

Comment on attachment 271909 [details]
Patch for landing - Fixing callback throwing exception

Clearing flags on attachment: 271909

Committed r196900: <http://trac.webkit.org/changeset/196900>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Myles C. Maxfield]

Comment on attachment 271909 [details]
Patch for landing - Fixing callback throwing exception

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1062
> +            push(@headerContent, "    using IteratorKey = $keyType;\n");

This isn't going to work for the CSS Font Loading spec. FontFaceSet's iterator needs to be able to return new objects, but the $keyType is a raw pointer. It needs to be able to retain the object returned.

[Myles C. Maxfield]

(In reply to comment #10)
> Comment on attachment 271909 [details]
> Patch for landing - Fixing callback throwing exception
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271909&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1062
> > +            push(@headerContent, "    using IteratorKey = $keyType;\n");
> 
> This isn't going to work for the CSS Font Loading spec. FontFaceSet's
> iterator needs to be able to return new objects, but the $keyType is a raw
> pointer. It needs to be able to retain the object returned.

It looks like there is a special exception for DOMStringList in GetNativeType(). I guess I'll add another exception for FontFace.

[Myles C. Maxfield]

Thank you very much for implementing this!!! 👍👍👍👍👍

[youenn fablet]

(In reply to comment #12)
> Thank you very much for implementing this!!! 👍👍👍👍👍

You 're welcome ;)

(In reply to comment #11)
> (In reply to comment #10)
> > Comment on attachment 271909 [details]
> > Patch for landing - Fixing callback throwing exception
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=271909&action=review
> > 
> > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1062
> > > +            push(@headerContent, "    using IteratorKey = $keyType;\n");
> > 
> > This isn't going to work for the CSS Font Loading spec. FontFaceSet's
> > iterator needs to be able to return new objects, but the $keyType is a raw
> > pointer. It needs to be able to retain the object returned.
> 
> It looks like there is a special exception for DOMStringList in
> GetNativeType(). I guess I'll add another exception for FontFace.

I don't know. Maybe it is better to add a new GetXXType routine that takes care of new objets. cdumez, wdyt ?

[youenn fablet]

Patch from bug 154531 removes the need to tweak the binding generator.
See bug 154577 as a follow-up.