Bug 47676 - Missing support for document.createTouch and document.createTouchList
Summary: Missing support for document.createTouch and document.createTouchList
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ben Murdoch
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-14 10:12 PDT by Ben Murdoch
Modified: 2010-10-19 05:53 PDT (History)
9 users (show)

See Also:


Attachments
Patch and layout tests. (19.52 KB, patch)
2010-10-15 09:40 PDT, Ben Murdoch
no flags Details | Formatted Diff | Diff
Patch and layout tests v2 (20.58 KB, patch)
2010-10-18 07:41 PDT, Ben Murdoch
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Murdoch 2010-10-14 10:12:05 PDT
The Document object is missing support for the createTouch and createTouchList APIs, as defined here: http://developer.apple.com/library/safari/#documentation/UserExperience/Reference/DocumentAdditionsReference/DocumentAdditions/DocumentAdditions.html

Many sites (including New York Times) use these APIs (especially createTouch) to determine if the browser has support for touch events.

Patch and layout test to follow.
Comment 1 Ben Murdoch 2010-10-15 09:40:07 PDT
Created attachment 70877 [details]
Patch and layout tests.
Comment 2 Steve Block 2010-10-15 10:57:16 PDT
Comment on attachment 70877 [details]
Patch and layout tests.

View in context: https://bugs.webkit.org/attachment.cgi?id=70877&action=review

> WebCore/dom/Document.cpp:4775
> +    Frame* frame = window ? window->frame() : this->frame();

Why 'this'?

> WebCore/dom/Document.idl:326
> +                                       in Node target,

Shouldn't this be of type EventTarget?

> WebCore/dom/Document.idl:332
> +            raises (DOMException);

Should perhaps be on the line above?

> WebCore/dom/Document.idl:334
> +            raises (DOMException);

Can be on one line?

> LayoutTests/fast/events/touch/script-tests/document-create-touch.js:12
> +var touch = document.createTouch(window, target, 1, 100, 101, 102, 103);

When should the method throw an exception? Can you add some test cases for this? We should also be sure to test passing the wrong argument types.
Comment 3 Ben Murdoch 2010-10-17 10:07:03 PDT
> > WebCore/dom/Document.cpp:4775
> > +    Frame* frame = window ? window->frame() : this->frame();
> 
> Why 'this'?
> 

The compiler didn't like using a local that had the same name as a member function, and 'this' resolved the ambiguity. I did think about changing the name of the local first, but frame seemed most suitable. However I can use a different name if you'd prefer that over using 'this'.

> > WebCore/dom/Document.idl:326
> > +                                       in Node target,
> 
> Shouldn't this be of type EventTarget?
> 

Yes, you're right. I'll correct that and send a new patch.

> > WebCore/dom/Document.idl:332
> > +            raises (DOMException);
> 
> Should perhaps be on the line above?
> 
> > WebCore/dom/Document.idl:334
> > +            raises (DOMException);
> 
> Can be on one line?

Judging by the style of the rest of the file, raises(...) seems to always go on the next line indented by 4 spaces.

> 
> > LayoutTests/fast/events/touch/script-tests/document-create-touch.js:12
> > +var touch = document.createTouch(window, target, 1, 100, 101, 102, 103);
> 
> When should the method throw an exception? Can you add some test cases for this? We should also be sure to test passing the wrong argument types.

The documentation doesn't mention when the methods would throw. I tried a few things on the iPod Touch, but couldn't get it to throw. I can add an extra test to pass incorrect argument types, but I think that type checking is taken care of by the generated portion of the bindings so I would've thought it was already quite well tested?

Thanks, Ben
Comment 4 Ben Murdoch 2010-10-18 07:41:10 PDT
Created attachment 71035 [details]
Patch and layout tests v2
Comment 5 Steve Block 2010-10-18 07:53:52 PDT
Comment on attachment 71035 [details]
Patch and layout tests v2

Looks good to me. Simon, do you have any comments before this is submitted?
Comment 6 Steve Block 2010-10-19 04:17:13 PDT
Comment on attachment 71035 [details]
Patch and layout tests v2

I think we can submit now, given that we have https://bugs.webkit.org/show_bug.cgi?id=47819 to track any changes required to get the required exception behaviour.
Comment 7 WebKit Commit Bot 2010-10-19 04:50:14 PDT
Comment on attachment 71035 [details]
Patch and layout tests v2

Clearing flags on attachment: 71035

Committed r70047: <http://trac.webkit.org/changeset/70047>
Comment 8 WebKit Commit Bot 2010-10-19 04:50:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 2010-10-19 05:26:08 PDT
http://trac.webkit.org/changeset/70047 might have broken Qt Linux Release
The following tests are not passing:
fast/dom/Window/window-properties.html
Comment 10 Ben Murdoch 2010-10-19 05:29:19 PDT
(In reply to comment #9)
> http://trac.webkit.org/changeset/70047 might have broken Qt Linux Release
> The following tests are not passing:
> fast/dom/Window/window-properties.html

Oops, forgot the update to the expected output. Patch on the way.
Comment 11 Ben Murdoch 2010-10-19 05:53:26 PDT
http://trac.webkit.org/changeset/70052 should take care of it.