Bug 51196 - Support createTouchList with Touch list
Summary: Support createTouchList with Touch list
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Chang Shu
URL:
Keywords:
Depends on: 52069
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-16 10:18 PST by Chang Shu
Modified: 2011-02-14 13:26 PST (History)
12 users (show)

See Also:


Attachments
fix patch (7.73 KB, patch)
2010-12-16 11:15 PST, Chang Shu
no flags Details | Formatted Diff | Diff
fix patch 2 (8.85 KB, patch)
2010-12-22 12:42 PST, Chang Shu
kenneth: review-
Details | Formatted Diff | Diff
fix patch 3 (9.46 KB, patch)
2011-01-03 14:22 PST, Chang Shu
no flags Details | Formatted Diff | Diff
fix patch 4 (7.13 KB, patch)
2011-01-07 12:16 PST, Chang Shu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chang Shu 2010-12-16 10:18:10 PST
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.
Comment 1 Chang Shu 2010-12-16 11:15:20 PST
Created attachment 76791 [details]
fix patch
Comment 2 WebKit Review Bot 2010-12-16 11:17:46 PST
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.
Comment 3 WebKit Review Bot 2010-12-16 11:23:14 PST
Attachment 76791 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7101078
Comment 4 WebKit Review Bot 2010-12-16 12:00:06 PST
Attachment 76791 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7135082
Comment 5 Chang Shu 2010-12-22 12:42:02 PST
Created attachment 77251 [details]
fix patch 2
Comment 6 Kenneth Rohde Christiansen 2010-12-24 02:52:59 PST
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 7 anton muhin 2010-12-24 05:17:39 PST
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?
Comment 8 Chang Shu 2011-01-03 08:26:58 PST
(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.
Comment 9 Chang Shu 2011-01-03 14:22:12 PST
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.
Comment 10 Chang Shu 2011-01-03 14:23:24 PST
(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!
Comment 11 Simon Hausmann 2011-01-05 06:30:20 PST
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...
Comment 12 Chang Shu 2011-01-05 06:49:28 PST
(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 13 Kenneth Rohde Christiansen 2011-01-05 12:05:32 PST
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.
Comment 14 Chang Shu 2011-01-06 08:34:00 PST
Can you show me more details? My patch includes tests.
> 
> You need to add tests then modifying the generator.
Comment 15 Kenneth Rohde Christiansen 2011-01-06 11:08:26 PST
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.
Comment 16 Chang Shu 2011-01-07 12:16:09 PST
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 17 Kenneth Rohde Christiansen 2011-01-07 12:21:30 PST
Comment on attachment 78256 [details]
fix patch 4

LGTM
Comment 18 Chang Shu 2011-01-07 12:25:20 PST
Comment on attachment 78256 [details]
fix patch 4

thanks for the quick review. :)
Comment 19 WebKit Commit Bot 2011-01-08 17:08:56 PST
Comment on attachment 78256 [details]
fix patch 4

Clearing flags on attachment: 78256

Committed r75335: <http://trac.webkit.org/changeset/75335>
Comment 20 WebKit Commit Bot 2011-01-08 17:09:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Suresh Voruganti 2011-01-10 06:27:32 PST
Please cherry pick fix to Qtwebkit 2.2
Comment 22 Ademar Reis 2011-01-10 12:41:05 PST
(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.
Comment 23 Suresh Voruganti 2011-01-10 13:37:29 PST
Chang, can you work on backported version?