The current API and implementation of createTouchList can only create a TouchList object with no list of Touch objects attached. Hence, it has to be enhanced for testing purpose.
Created attachment 76791 [details] fix patch
Attachment 76791 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/events/touch/document-create-touch-list-expected.txt', u'LayoutTests/fast/events/touch/script-tests/document-create-touch-list.js', u'WebCore/ChangeLog', u'WebCore/bindings/js/JSDocumentCustom.cpp', u'WebCore/bindings/scripts/CodeGeneratorJS.pm', u'WebCore/dom/Document.idl', u'WebCore/dom/TouchEvent.cpp']" exit_code: 1 WebCore/bindings/js/JSDocumentCustom.cpp:137: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 76791 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7101078
Attachment 76791 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7135082
Created attachment 77251 [details] fix patch 2
Comment on attachment 77251 [details] fix patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=77251&action=review It this a useful feature? Does other platforms support it? Why is it needed. R- due to not detailing this. > WebCore/ChangeLog:9 > + 1. Added support of ArgList as an argument for a Custom function. > + 2. Implemented JS/V8 bindings for createTouchList. You should detail why this is needed at all.
Comment on attachment 77251 [details] fix patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=77251&action=review > WebCore/bindings/v8/custom/V8DocumentCustom.cpp:175 > + touchList->append(V8Touch::toNative(args[i]->ToObject())); chances are ToObject can throw, is it fine? May we cover this case in LayoutTests?
(In reply to comment #6) > (From update of attachment 77251 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77251&action=review > > It this a useful feature? Does other platforms support it? Why is it needed. R- due to not detailing this. > > > WebCore/ChangeLog:9 > > + 1. Added support of ArgList as an argument for a Custom function. > > + 2. Implemented JS/V8 bindings for createTouchList. > > You should detail why this is needed at all. Thanks, Kenneth. This feature is useful for testing purpose. I have seen it implemented in iOS. The bug was originally reported from this link: https://projects.maemo.org/bugzilla/show_bug.cgi?id=193143.
Created attachment 77847 [details] fix patch 3 Based on Kenneth and Anton's comments: 1. added purpose for this new feature. 2. added code to handle invalid arguments for V8.
(In reply to comment #7) > (From update of attachment 77251 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77251&action=review > > > WebCore/bindings/v8/custom/V8DocumentCustom.cpp:175 > > + touchList->append(V8Touch::toNative(args[i]->ToObject())); > > chances are ToObject can throw, is it fine? May we cover this case in LayoutTests? good catch! I have added code and test case for this. thanks!
Hmm, I see that would make us incompatible with Apple's implementation. See http://developer.apple.com/library/safari/#documentation/UserExperience/Reference/DocumentAdditionsReference/DocumentAdditions/DocumentAdditions.html - createTouchList() doesn't take an argument...
(In reply to comment #11) > Hmm, I see that would make us incompatible with Apple's implementation. See http://developer.apple.com/library/safari/#documentation/UserExperience/Reference/DocumentAdditionsReference/DocumentAdditions/DocumentAdditions.html - createTouchList() doesn't take an argument... From the iOS source code and test on iphone, createTouchList() with arguments is supported. Maybe the documentation is out-of-date?
Comment on attachment 77847 [details] fix patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=77847&action=review > WebCore/bindings/scripts/CodeGeneratorJS.pm:863 > - push(@headerContent, " JSC::JSValue " . $functionImplementationName . "(JSC::ExecState*);\n"); > + my $parameter = @{$function->parameters}[0]; > + if ($parameter && $codeGenerator->StripModule($parameter->type) =~ /argList/) { > + push(@headerContent, " JSC::JSValue " . $functionImplementationName . "(JSC::ExecState*, const JSC::ArgList&);\n"); > + } else { > + push(@headerContent, " JSC::JSValue " . $functionImplementationName . "(JSC::ExecState*);\n"); > + } You need to add tests then modifying the generator.
Can you show me more details? My patch includes tests. > > You need to add tests then modifying the generator.
It includes tests for the feature, not for the generator. You modified the generator and so you need to expand the test of the generator to cover your changes. Look in bindings/scripts/test I have never done any of these tests myself, but I have seen it requested whenever the generator was changed.
Created attachment 78256 [details] fix patch 4 It turns out that the changes in CodeGeneratorJS.pm are not needed. Thanks, Sam, for pointing out this.
Comment on attachment 78256 [details] fix patch 4 LGTM
Comment on attachment 78256 [details] fix patch 4 thanks for the quick review. :)
Comment on attachment 78256 [details] fix patch 4 Clearing flags on attachment: 78256 Committed r75335: <http://trac.webkit.org/changeset/75335>
All reviewed patches have been landed. Closing bug.
Please cherry pick fix to Qtwebkit 2.2
(In reply to comment #21) > Please cherry pick fix to Qtwebkit 2.2 We'll need a backport for this: besides the fact that the files have been moved on trunk, the code also appears to be quite different.
Chang, can you work on backported version?