Bug 51196 – Support createTouchList with Touch list

• Component: DOM
• Status: RESOLVED FIXED

[Chang Shu]

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.

[Chang Shu]

Created attachment 76791 [details]
fix patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

Attachment 76791 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7101078

[WebKit Review Bot]

Attachment 76791 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7135082

[Chang Shu]

Created attachment 77251 [details]
fix patch 2

[Kenneth Rohde Christiansen]

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.

[anton muhin]

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?

[Chang Shu]

(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.

[Chang Shu]

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.

[Chang Shu]

(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!

[Simon Hausmann]

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...

[Chang Shu]

(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?

[Kenneth Rohde Christiansen]

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.

[Chang Shu]

Can you show me more details? My patch includes tests.
> 
> You need to add tests then modifying the generator.

[Kenneth Rohde Christiansen]

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.

[Chang Shu]

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.

[Kenneth Rohde Christiansen]

Comment on attachment 78256 [details]
fix patch 4

LGTM

[Chang Shu]

Comment on attachment 78256 [details]
fix patch 4

thanks for the quick review. :)

[WebKit Commit Bot]

Comment on attachment 78256 [details]
fix patch 4

Clearing flags on attachment: 78256

Committed r75335: <http://trac.webkit.org/changeset/75335>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Suresh Voruganti]

Please cherry pick fix to Qtwebkit 2.2

[Ademar Reis]

(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.

[Suresh Voruganti]

Chang, can you work on backported version?