Bug 154413

Summary: Binding generator should support key value iterable
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore JavaScriptAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, mmaxfield
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
none
Patch
none
Throwing in case of bad forEach argument
none
Patch for landing - Fixing callback throwing exception
none
Patch for landing - Fixing callback throwing exception none

Description youenn fablet 2016-02-18 13:56:31 PST
WebIDL iterable keyword should be supported by binding generator.
It may be good to start with Key Value iterables.
Comment 1 youenn fablet 2016-02-18 14:19:31 PST
Created attachment 271694 [details]
WIP
Comment 2 youenn fablet 2016-02-19 07:30:00 PST
Created attachment 271753 [details]
Patch
Comment 3 youenn fablet 2016-02-20 03:40:42 PST
Created attachment 271857 [details]
Throwing in case of bad forEach argument
Comment 4 Darin Adler 2016-02-21 17:41:38 PST
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?
Comment 5 youenn fablet 2016-02-21 23:29:14 PST
Created attachment 271908 [details]
Patch for landing - Fixing callback throwing exception
Comment 6 youenn fablet 2016-02-21 23:30:42 PST
Created attachment 271909 [details]
Patch for landing - Fixing callback throwing exception
Comment 7 youenn fablet 2016-02-21 23:34:25 PST
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.
Comment 8 WebKit Commit Bot 2016-02-22 00:31:12 PST
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>
Comment 9 WebKit Commit Bot 2016-02-22 00:31:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Myles C. Maxfield 2016-02-22 12:08:55 PST
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.
Comment 11 Myles C. Maxfield 2016-02-22 12:10:55 PST
(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.
Comment 12 Myles C. Maxfield 2016-02-22 12:29:38 PST
Thank you very much for implementing this!!! 👍👍👍👍👍
Comment 13 youenn fablet 2016-02-22 12:43:33 PST
(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 ?
Comment 14 youenn fablet 2016-02-23 01:55:58 PST
Patch from bug 154531 removes the need to tweak the binding generator.
See bug 154577 as a follow-up.