Creating a TouchList with non Touch items should throw an exception.
Created attachment 280330 [details] Patch
Created attachment 280331 [details] Patch
This patch would also permit to fix an issue related to a discussion in bug 157985. It will make it possible to pass a Ref&& instead of RefPtr&& in TouchList::append().
Comment on attachment 280331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280331&action=review > Source/WebCore/bindings/js/JSDocumentCustom.cpp:136 > + for (size_t i = 0; i < state.argumentCount(); i++) { ++i > Source/WebCore/bindings/js/JSDocumentCustom.cpp:143 > return toJS(&state, globalObject(), touchList); return toJSNewlyCreated(&state, globalObject(), WTFMove(touchList)); > LayoutTests/fast/events/touch/script-tests/document-create-touch-list-crash.js:19 > Can you add a test to make sure we do not throw if createTouchList() is called without parameter and no make sure that the list length is 0?
Comment on attachment 280331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280331&action=review > LayoutTests/fast/events/touch/script-tests/document-create-touch-list-crash.js:8 > +shouldThrow('document.createTouchList(undefined).item(0)'); I don't believe document.createTouchList(undefined) should throw. A variadic parameter is considered optional in Web IDL. So passing undefined should be equivalent to not passing any parameter in this case (since there is no default value for the parameter).
Comment on attachment 280331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280331&action=review >> LayoutTests/fast/events/touch/script-tests/document-create-touch-list-crash.js:8 >> +shouldThrow('document.createTouchList(undefined).item(0)'); > > I don't believe document.createTouchList(undefined) should throw. A variadic parameter is considered optional in Web IDL. So passing undefined should be equivalent to not passing any parameter in this case (since there is no default value for the parameter). See: https://heycam.github.io/webidl/#es-overloads (step 10.4) Also from the IDL spec: An argument is considered to be an optional argument if it is declared with the optional keyword. The final argument of a variadic operation is also considered to be an optional argument. Declaring an argument to be optional indicates that the argument value can be omitted when the operation is invoked. The final argument in an operation must not explicitly be declared to be optional if the operation is variadic.
Comment on attachment 280331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280331&action=review > Source/WebCore/bindings/js/JSDocumentCustom.cpp:137 > + auto* item = JSTouch::toWrapped(state.argument(i)); Should be state.uncheckedArgument(i)
Comment on attachment 280331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280331&action=review >> Source/WebCore/bindings/js/JSDocumentCustom.cpp:137 >> + auto* item = JSTouch::toWrapped(state.argument(i)); > > Should be state.uncheckedArgument(i) Maybe something like this: JSValue argument = state.uncheckedArgument(I); If (argument.isUndefined()) continue; auto* touch = JSTouch::toWrapped(argument); ...
Created attachment 280602 [details] Patch
Thanks for the review. Ok I modified tests in uploaded patch. It is checked that no exception is thrown when createTouchList() is called without parameter. The list length is also checked. Concerning support of "undefined" arguments, it seems that they should be converted to the type of the variadic as would be done for non-optional arguments (c.f. http://heycam.github.io/webidl/#es-overloads. step 16. last sentence of the note). Conversion of Touch objects from undefined should thus throw a TypeError exception as per section 4.2.19 (http://heycam.github.io/webidl/#es-object). I also tried to remove the custom binding since probably not necessary here but a compilation error is observed in GObject binding code.
Comment on attachment 280602 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280602&action=review > Source/WebCore/bindings/js/JSDocumentCustom.cpp:137 > + if (!state.uncheckedArgument(i).inherits(JSTouch::info())) Why not null check the value returned by JSTouch::toWrapped() instead? JSTouch::toWrapped() already does a inherits(JSTouch::info()) check internally. > Source/WebCore/bindings/js/JSDocumentCustom.cpp:138 > + return JSValue::decode(throwArgumentTypeError(state, i, "touches", "Document", "createTouchList", "TouchList")); The last parameter should probably be "Touch". > Source/WebCore/bindings/js/JSDocumentCustom.cpp:140 > + auto* item = JSTouch::toWrapped(state.argument(i)); uncheckedArgument(i) > Source/WebCore/bindings/js/JSDocumentCustom.cpp:142 > + touchList->append(item); In a follow-up patch, we should update append() to take a reference instead of a raw pointer. > LayoutTests/fast/events/touch/document-create-touch-list-crash-expected.txt:14 > +PASS document.createTouchList(t, document, t2); threw exception TypeError: Argument 2 ('touches') to Document.createTouchList must be an instance of TouchList. The message should probably says "... Must be an instance of Touch.", not TouchList.
Created attachment 280683 [details] Patch
Comment on attachment 280683 [details] Patch r=me Might be worth looking at updating the binding generated code for variadics and applying it for touch lists and more.
Thanks for reviews. I made the corrections suggested by Chris in the uploaded patch. I file also a new bug (bug 158469) to pass a Ref instead of RefPtr in TouchList::append() method.
Comment on attachment 280683 [details] Patch Clearing flags on attachment: 280683 Committed r201747: <http://trac.webkit.org/changeset/201747>
All reviewed patches have been landed. Closing bug.
Comment on attachment 280683 [details] Patch Darn, Youenn stole my review :) Anyway, belated lgtm.
Comment on attachment 280683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280683&action=review > Source/WebCore/bindings/js/JSDocumentCustom.cpp:143 > auto touchList = TouchList::create(); > > - for (size_t i = 0; i < state.argumentCount(); i++) > - touchList->append(JSTouch::toWrapped(state.argument(i))); > + for (size_t i = 0; i < state.argumentCount(); ++i) { > + auto* item = JSTouch::toWrapped(state.uncheckedArgument(i)); > + if (!item) > + return JSValue::decode(throwArgumentTypeError(state, i, "touches", "Document", "createTouchList", "Touch")); > > - return toJS(&state, globalObject(), touchList); > + touchList->append(item); > + } > + return toJSNewlyCreated(&state, globalObject(), WTFMove(touchList)); Paragraphing here is a little peculiar. If there’s a space before the loop, then I suggest a space after the loop too. > Source/WebCore/dom/Document.idl:304 > - [NewObject, Custom, RaisesException] TouchList createTouchList(); > + [NewObject, Custom] TouchList createTouchList(Touch... touches); What was the rationale for removing RaisesException?
Comment on attachment 280683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280683&action=review >> Source/WebCore/dom/Document.idl:304 >> + [NewObject, Custom] TouchList createTouchList(Touch... touches); > > What was the rationale for removing RaisesException? Oh, I understand now. The only exception that can occur is in argument handling, not in the function itself.
Comment on attachment 280683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=280683&action=review >>> Source/WebCore/dom/Document.idl:304 >>> + [NewObject, Custom] TouchList createTouchList(Touch... touches); >> >> What was the rationale for removing RaisesException? > > Oh, I understand now. The only exception that can occur is in argument handling, not in the function itself. Well, it is marked as [Custom] so [NewObject] and [RaisesException] have no impact anyway.
For a regular binding only NewObject attribute is needed for createTouchList. Thus to remove the custom binding we need just to delete "Custom" word.