Summary: | Make WebKit expose extra touch information | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | miletus | ||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | anicolao, ap, commit-queue, eric, fishd, miletus, mjs, rjkroege | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
miletus
2011-04-20 15:57:25 PDT
Created attachment 90431 [details]
Patch
update cc list Comment on attachment 90431 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90431&action=review > Source/WebCore/dom/Touch.idl:39 > + readonly attribute float radius; what specification covers these additions? if none, then shouldn't we vendor prefix these to avoid possible collisions in the future with a formal specification that may define these fields differently? i confess that i don't know much about the history of this file. is there already a spec covering the existing fields? If there is no specification, then adding this code to WebKit even under an #if and even with a vendor prefix shouldn't be taken lightly. Also, this patch has style errors that the bot didn't find. Created attachment 90568 [details]
Patch
(In reply to comment #3) > (From update of attachment 90431 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=90431&action=review > > > Source/WebCore/dom/Touch.idl:39 > > + readonly attribute float radius; > > what specification covers these additions? if none, then shouldn't > we vendor prefix these to avoid possible collisions in the future > with a formal specification that may define these fields differently? > > i confess that i don't know much about the history of this file. > is there already a spec covering the existing fields? W3C has a Draft of the "Touch Events Specification" at http://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html It is largely compatible with the touch event interface used in the Safari DOM model (and the API exposed by WebKit). Also the draft is going to add some extra properties for touch point, including radiusX, radiusY (the X and Y axis of the touch ellipse) and roatationAngle, which is covered by this patch. Even Safari/WebKit is not going to follow the draft spec (which is true currently since the Safari/WebKit DOM model is already there while the spec is still being drafted, and also note that Mozilla exposes different set of API for the touch events), it is still desirable to have the features to expose these touch information. (In reply to comment #4) > If there is no specification, then adding this code to WebKit even under an #if and even with a vendor prefix shouldn't be taken lightly. > Please see the comment #6. > Also, this patch has style errors that the bot didn't find. Would you mind pointing me where it is ? (In reply to comment #5) > Created an attachment (id=90568) [details] > Patch The extra touch information are changed to comply with the draft spec http://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html Now the WebCore::Touch is augmented with 3 fields : m_radiusX m_radiusY m_rotationAngle Comment on attachment 90568 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90568&action=review I was also going to complain about text alignment (we normally use one space), but those are all pre-exisintg issues in this code. I don't object to landing this, given the kind of support there already is for touch events. > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) The patch can't be landed with this line. You should add tests. > Source/WebKit/chromium/public/WebTouchPoint.h:52 > + , rotationAngle(0.0) { } Just "0", and the braces should go on separate lines. Created attachment 91147 [details]
Patch
Comment on attachment 90568 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=90568&action=review >> Source/WebCore/ChangeLog:8 >> + No new tests. (OOPS!) > > The patch can't be landed with this line. You should add tests. done. >> Source/WebKit/chromium/public/WebTouchPoint.h:52 >> + , rotationAngle(0.0) { } > > Just "0", and the braces should go on separate lines. done tests added. (In reply to comment #10) > Created an attachment (id=91147) [details] > Patch Created attachment 92277 [details]
Patch
(In reply to comment #13) > Created an attachment (id=92277) [details] > Patch prefix the three fields of WebCore::Touch radiusX radiusY rotationAngle to be webkitRadiusX webkitRadiusY webkitRotationAngle for now and will change to the standard name when the draft http://dvcs.w3.org/hg/webevents/raw-file/tip/touchevents.html#idl-def-Touch is approved. Well, hmm... It looks like that spec also covers the other pre-existing fields of Touch. It looks like the spec is co-edited by folks from Mozilla and Opera. Do you know the state of their implementations? Have they vendor prefixed these fields? Have they been shipping these fields for some time already? Comment #6 makes me concerned that there may be considerable divergence between the browsers. (In reply to comment #15) > Well, hmm... > > It looks like that spec also covers the other pre-existing fields of Touch. It looks like the spec is co-edited by folks from Mozilla and Opera. Do you know the state of their implementations? Have they vendor prefixed these fields? Have they been shipping these fields for some time already? > > Comment #6 makes me concerned that there may be considerable divergence between the browsers. According to this thread http://lists.w3.org/Archives/Public/public-webevents/2011AprJun/0083.html Matt Brubeck from Mozilla, also one of the editors of the draft, has an experimental implementation in the nightly build of mobile Firefox. Also Mozilla has vendor prefixed MozTouchEvent and related APIs which are not compatible with WebKit. Opera's implementation is based on iphone (and so the same as WebKit) The draft is based on WebKit's implementation but some divergence is developing. As for the radius and angle information, since they are now available from many touch devices, it is good to expose them to web developers. And as the draft is still evolving, vendor-prefixing the fields seems the safe way to go for now. Hi Darin, more thoughts on this patch ? Comment on attachment 92277 [details]
Patch
Agreed. Sorry for the high latency here. I was out on vacation.
Comment on attachment 92277 [details] Patch Clearing flags on attachment: 92277 Committed r86700: <http://trac.webkit.org/changeset/86700> All reviewed patches have been landed. Closing bug. |