Bug 47676 – Missing support for document.createTouch and document.createTouchList

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ben Murdoch]

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.

[Ben Murdoch]

Created attachment 70877 [details]
Patch and layout tests.

[Steve Block]

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.

[Ben Murdoch]

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

[Ben Murdoch]

Created attachment 71035 [details]
Patch and layout tests v2

[Steve Block]

Comment on attachment 71035 [details]
Patch and layout tests v2

Looks good to me. Simon, do you have any comments before this is submitted?

[Steve Block]

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.

[WebKit Commit Bot]

Comment on attachment 71035 [details]
Patch and layout tests v2

Clearing flags on attachment: 71035

Committed r70047: <http://trac.webkit.org/changeset/70047>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/70047 might have broken Qt Linux Release
The following tests are not passing:
fast/dom/Window/window-properties.html

[Ben Murdoch]

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

[Ben Murdoch]

http://trac.webkit.org/changeset/70052 should take care of it.