WebIDL iterable keyword should be supported by binding generator. It may be good to start with Key Value iterables.
Created attachment 271694 [details] WIP
Created attachment 271753 [details] Patch
Created attachment 271857 [details] Throwing in case of bad forEach argument
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?
Created attachment 271908 [details] Patch for landing - Fixing callback throwing exception
Created attachment 271909 [details] Patch for landing - Fixing callback throwing exception
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 on attachment 271909 [details] Patch for landing - Fixing callback throwing exception Clearing flags on attachment: 271909 Committed r196900: <http://trac.webkit.org/changeset/196900>
All reviewed patches have been landed. Closing bug.
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.
(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.
Thank you very much for implementing this!!! 👍👍👍👍👍
(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 ?
Patch from bug 154531 removes the need to tweak the binding generator. See bug 154577 as a follow-up.