Bug 66800 - [Chromium] Modify WebTouchEvent structure to match WebCore::TouchEvent
Summary: [Chromium] Modify WebTouchEvent structure to match WebCore::TouchEvent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
: 67615 (view as bug list)
Depends on: 67704
Blocks: 66687
  Show dependency treegraph
 
Reported: 2011-08-23 12:50 PDT by borenet
Modified: 2011-09-09 14:51 PDT (History)
7 users (show)

See Also:


Attachments
Patch (7.90 KB, patch)
2011-08-23 13:00 PDT, borenet
no flags Details | Formatted Diff | Diff
Patch (7.87 KB, patch)
2011-08-23 14:07 PDT, borenet
no flags Details | Formatted Diff | Diff
Patch (7.87 KB, patch)
2011-08-23 14:14 PDT, borenet
no flags Details | Formatted Diff | Diff
Patch (9.49 KB, patch)
2011-08-24 06:41 PDT, borenet
no flags Details | Formatted Diff | Diff
Patch (9.90 KB, patch)
2011-08-24 07:26 PDT, borenet
no flags Details | Formatted Diff | Diff
Patch (9.96 KB, patch)
2011-08-24 07:42 PDT, borenet
no flags Details | Formatted Diff | Diff
Patch (10.16 KB, patch)
2011-08-25 05:09 PDT, borenet
no flags Details | Formatted Diff | Diff
Patch (10.74 KB, patch)
2011-08-26 07:00 PDT, borenet
no flags Details | Formatted Diff | Diff
Patch (10.71 KB, patch)
2011-09-06 12:19 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (10.63 KB, patch)
2011-09-06 14:02 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (10.65 KB, patch)
2011-09-06 20:30 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (11.00 KB, patch)
2011-09-06 21:20 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description borenet 2011-08-23 12:50:51 PDT
Modify WebTouchEvent structure to match WebCore::TouchEvent
Comment 1 borenet 2011-08-23 13:00:00 PDT
Created attachment 104901 [details]
Patch
Comment 2 WebKit Review Bot 2011-08-23 13:38:41 PDT
Comment on attachment 104901 [details]
Patch

Attachment 104901 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9477520
Comment 3 borenet 2011-08-23 14:07:16 PDT
Created attachment 104911 [details]
Patch
Comment 4 WebKit Review Bot 2011-08-23 14:11:37 PDT
Comment on attachment 104911 [details]
Patch

Attachment 104911 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9479541
Comment 5 borenet 2011-08-23 14:14:52 PDT
Created attachment 104914 [details]
Patch
Comment 6 Steve Block 2011-08-24 02:44:22 PDT
Comment on attachment 104914 [details]
Patch

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

Mostly minor points.

Once these are fixed up, it would be good to get a Chromium person to take a look before I r+.

> Source/WebKit/chromium/ChangeLog:3
> +        Modify WebTouchEvent structure to match WebCore::TouchEvent

You should modify the bug title to include 'Chromium', as this touches only Chromium's WebKit layer. I think the pattern is to prefix the title with '[Chromium]'.

> Source/WebKit/chromium/public/WebInputEvent.h:347
> +    static const int touchPointsLengthCap = 8;

Should this be 'touchesLengthCap' to match the other variable names?

> Source/WebKit/chromium/public/WebTouchPoint.h:-45
> -    };

Can you explain the reasons for and the consequences of removing this enum?

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:46
> +#endif // ENABLE(TOUCH_EVENTS)

It looks like the contents of these files are already guarded with ENABLE(TOUCH_EVENTS), so there's no need for a guard here. WebKit policy is generally to use as few guards as possible.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:270
> +        m_touchPoints.append(PlatformTouchPointBuilder(widget, event.touches[i]));

Should you rename m_touchPoints to m_touches for consistency?

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:394
> +namespace {

It looks like other helpers in this file are static, rather than in an anonymous namespace. I think an anonymous namespace is now the preferred style, but I'm not sure whether you should fix all other occurrences here, stick to 'static' for consistency, or leave it as it is.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:397
> +    for (unsigned int i = 0; i < touches->length(); i++) {

What happens if touches->length() exceeds touchPointsLengthCap? Is touches->length() limited by WebCore? If so, is there a compile or run-time check to make sure that touchPointsLengthCap is at least this value?

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:398
> +        WebTouchPoint pt;

Use complete words for variable names.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:410
> +        (*touchPointsLength)++;

Is there any reason to increment this, rather than just setting it to touches->length()? It seems strange to rely on the fact that it's zero-initialized. If it's not zero-initialized, the value will be wrong, as we always set touchPoints from index 0 onwards.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:426
> +        type = Undefined;

Does WebCore use touch event types other than start/move/end/cancel? If so, mention in the header comment which ones we honour and the fallback case. If not, perhaps there should be an assert here?

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:431
> +    timeStampSeconds = event.timeStamp() * 1.0e-3;

Eeek! Surely there's an existing kMillisecondsToSeconds you could use? Or at least use a static constant in this file.

> Source/WebKit/chromium/src/WebInputEventConversion.h:48
> +#endif // ENABLE(TOUCH_EVENTS)

Again, avoid guards unless required to build.

> Source/WebKit/chromium/src/WebInputEventConversion.h:127
> +// Converts a WebCore::TouchEvent to a corresponding WebTouchEvent.

Might be nice to add a corresponding comment to PlatformTouchPointBuilder/PlatformTouchEventBuilder to make clear that these convert in the other direction.
Comment 7 borenet 2011-08-24 06:24:07 PDT
Comment on attachment 104914 [details]
Patch

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

>> Source/WebKit/chromium/public/WebTouchPoint.h:-45
>> -    };
> 
> Can you explain the reasons for and the consequences of removing this enum?

The enum limits the number of possible id's for WebTouchPoints.  While it could be expanded, it will become difficult to keep this in sync with WebTouchEvent::touchPointsLengthCap - since we now have an upper bound of eight touch points, we could see as many as eight different id's.  Since the enum is never actually used, it makes more sense to me to remove it.

>> Source/WebKit/chromium/src/WebInputEventConversion.cpp:270
>> +        m_touchPoints.append(PlatformTouchPointBuilder(widget, event.touches[i]));
> 
> Should you rename m_touchPoints to m_touches for consistency?

Probably, but that's a part of PlatformTouchEvent, and changing it would make this a much higher-impact change.

>> Source/WebKit/chromium/src/WebInputEventConversion.cpp:426
>> +        type = Undefined;
> 
> Does WebCore use touch event types other than start/move/end/cancel? If so, mention in the header comment which ones we honour and the fallback case. If not, perhaps there should be an assert here?

Those are the only touch event types.  What is the preferred way to do assertions?

>> Source/WebKit/chromium/src/WebInputEventConversion.cpp:431
>> +    timeStampSeconds = event.timeStamp() * 1.0e-3;
> 
> Eeek! Surely there's an existing kMillisecondsToSeconds you could use? Or at least use a static constant in this file.

The best I'm finding is in WebCore/icu/unicode/utypes.h.  That seems pretty far off - would it be better to define a constant here?
Comment 8 borenet 2011-08-24 06:41:24 PDT
Created attachment 104988 [details]
Patch
Comment 9 Steve Block 2011-08-24 06:49:39 PDT
Comment on attachment 104914 [details]
Patch

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

>>> Source/WebKit/chromium/public/WebTouchPoint.h:-45
>>> -    };
>> 
>> Can you explain the reasons for and the consequences of removing this enum?
> 
> The enum limits the number of possible id's for WebTouchPoints.  While it could be expanded, it will become difficult to keep this in sync with WebTouchEvent::touchPointsLengthCap - since we now have an upper bound of eight touch points, we could see as many as eight different id's.  Since the enum is never actually used, it makes more sense to me to remove it.

OK, if it's just to identify the ordered points in the list, removing it seems reasonable.

>>> Source/WebKit/chromium/src/WebInputEventConversion.cpp:270
>>> +        m_touchPoints.append(PlatformTouchPointBuilder(widget, event.touches[i]));
>> 
>> Should you rename m_touchPoints to m_touches for consistency?
> 
> Probably, but that's a part of PlatformTouchEvent, and changing it would make this a much higher-impact change.

OK

>>> Source/WebKit/chromium/src/WebInputEventConversion.cpp:426
>>> +        type = Undefined;
>> 
>> Does WebCore use touch event types other than start/move/end/cancel? If so, mention in the header comment which ones we honour and the fallback case. If not, perhaps there should be an assert here?
> 
> Those are the only touch event types.  What is the preferred way to do assertions?

I think you should probably use ASSERT_UNREACHED(), then set the type to undefined and return, as you do now.

>>> Source/WebKit/chromium/src/WebInputEventConversion.cpp:431
>>> +    timeStampSeconds = event.timeStamp() * 1.0e-3;
>> 
>> Eeek! Surely there's an existing kMillisecondsToSeconds you could use? Or at least use a static constant in this file.
> 
> The best I'm finding is in WebCore/icu/unicode/utypes.h.  That seems pretty far off - would it be better to define a constant here?

In DateMath.h there's WTF::msPerSecond, or a local constant is fine.
Comment 10 Steve Block 2011-08-24 06:58:31 PDT
Comment on attachment 104988 [details]
Patch

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

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:304
> +    timeStampSeconds = event.timeStamp() / MILLIS_PER_SECOND;

event.timeStamp() is an integer value, right, so you need to make MILLIS_PER_SECOND a floating-point value to force floating-point division.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:397
> +    unsigned int maxWebTouches = static_cast<unsigned int>(WebTouchEvent::touchesLengthCap);

Any reason WebTouchEvent::touchesLengthCap isn't unsigned? Also, maybe add a comment here or at WebTouchEvent::touchesLengthCap that we limit the max number of touches and simply drop extra touches on the floor.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:399
> +                                maxWebTouches : touches->length());

I think there's a min() macro you can use.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:429
> +        type = Undefined;

Add ASSERT_UNREACHED()
Comment 11 borenet 2011-08-24 07:26:17 PDT
Created attachment 104995 [details]
Patch
Comment 12 Steve Block 2011-08-24 07:34:59 PDT
Comment on attachment 104995 [details]
Patch

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

LGTM, but please get a Chromium reviewer to take a look before I r+.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:427
> +        ASSERT_NOT_REACHED();

I think it's Chromium policy to have code to gracefully fail after an ASSERT_NOT_REACHED(), as these don't cause a crash in release builds.

> Source/WebKit/chromium/src/WebInputEventConversion.h:126
> +// NOTE: WebTouchEvents have a cap on number of WebTouchPoints. Any points

'has a cap on the'
Comment 13 borenet 2011-08-24 07:42:58 PDT
Created attachment 104998 [details]
Patch
Comment 14 Darin Fisher (:fishd, Google) 2011-08-24 14:00:45 PDT
Comment on attachment 104998 [details]
Patch

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

> Source/WebKit/chromium/public/WebInputEvent.h:347
> +    static const unsigned int touchesLengthCap = 8;

nit: we use 'unsigned' instead of 'unsigned int' in the WebKit API (just like the rest of WebKit).

> Source/WebKit/chromium/public/WebInputEvent.h:349
> +    unsigned int touchesLength;

it seems like it would be helpful to define the difference between
touches, changedTouches and targetTouches.

> Source/WebKit/chromium/public/WebTouchPoint.h:42
> +        : id(0)

What values can "id" have if not one of the pre-defined Finger enum values?

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:53
> +#define MILLIS_PER_SECOND 1000.0

please use a C++ constant

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:269
> +    for (unsigned int i = 0; i < event.touchesLength; ++i)

nit: 'unsigned int' -> 'unsigned'

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:395
> +void AddTouchPoints(TouchList* touches, WebTouchPoint* touchPoints,

nit: it is more common place in WebKit to just use the 'static' keyword here.

can touches be declared as const?  'const TouchList*'?
or, can it be a 'const TouchList&' instead?  it doesn't look like this function
mutates the TouchList at all, so it should not be passed as a non-const pointer/
reference.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:396
> +                    unsigned int* touchPointsLength, WebCore::IntPoint offset) {

nit: no need for WebCore:: prefix

it is usually better to list out parameters last and in parameters first.  i would
therefore move offset before touchPoints.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:404
> +            touch->pageX() - offset.x(),

what is the offset used for?  does this represent scroll offset of the frame?  if so,
does this work properly with iframes?  we normally need to use one of the conversion
routines like convertToContainingWindow to map from widget coordinates and viewport
coordinates.

> Source/WebKit/chromium/src/WebInputEventConversion.h:130
> +    WebTouchEventBuilder(const WebCore::Widget*, const WebCore::TouchEvent&);

no need for WebCore:: prefix
Comment 15 borenet 2011-08-24 15:28:43 PDT
Comment on attachment 104998 [details]
Patch

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

>> Source/WebKit/chromium/public/WebTouchPoint.h:42
>> +        : id(0)
> 
> What values can "id" have if not one of the pre-defined Finger enum values?

'id' is just an integer in both WebCore::Touch and Pepper.  We never do any conversion of ids, and we don't require that the WebTouchPoint id is one of the enum values, so there really isn't much point in having it at all.

>> Source/WebKit/chromium/src/WebInputEventConversion.cpp:404
>> +            touch->pageX() - offset.x(),
> 
> what is the offset used for?  does this represent scroll offset of the frame?  if so,
> does this work properly with iframes?  we normally need to use one of the conversion
> routines like convertToContainingWindow to map from widget coordinates and viewport
> coordinates.

Offset is the DOM (x, y) of the plugin widget.  The expression then results in the touch coordinate being the DOM coordinate within the plugin.  Is there a better method for this?

>> Source/WebKit/chromium/src/WebInputEventConversion.h:130
>> +    WebTouchEventBuilder(const WebCore::Widget*, const WebCore::TouchEvent&);
> 
> no need for WebCore:: prefix

Removing this causes compile errors
Comment 16 borenet 2011-08-24 16:00:42 PDT
Comment on attachment 104998 [details]
Patch

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

>> Source/WebKit/chromium/src/WebInputEventConversion.cpp:395
>> +void AddTouchPoints(TouchList* touches, WebTouchPoint* touchPoints,
> 
> nit: it is more common place in WebKit to just use the 'static' keyword here.
> 
> can touches be declared as const?  'const TouchList*'?
> or, can it be a 'const TouchList&' instead?  it doesn't look like this function
> mutates the TouchList at all, so it should not be passed as a non-const pointer/
> reference.

This could be changed to be const TouchList&, but we get a TouchList* from WebCore::TouchEvent and would have to make local copies in WebTouchEventBuilder().  Compiler complains about const-correctness when calling touches->item(i) if const TouchList* is used.  Without modifying the TouchList class, I'm not sure if a const pointer can be used.
Comment 17 borenet 2011-08-25 05:09:49 PDT
Created attachment 105161 [details]
Patch
Comment 18 borenet 2011-08-26 07:00:55 PDT
Created attachment 105358 [details]
Patch
Comment 19 Steve Block 2011-09-05 13:18:28 PDT
*** Bug 67615 has been marked as a duplicate of this bug. ***
Comment 20 Steve Block 2011-09-06 02:45:48 PDT
Eric was an intern who I believe has now left, so I'm not sure whether he's still working on this bug. Adam, you might want to take over where he left off.
Comment 21 Adam Barth 2011-09-06 10:34:06 PDT
I would be happy to.
Comment 22 Adam Barth 2011-09-06 10:37:17 PDT
Comment on attachment 105358 [details]
Patch

Looks like there are just a couple style things left to clean up, right?
Comment 23 Steve Block 2011-09-06 10:44:39 PDT
I already gave an LGTM and it looks like Eric addressed all of Darin's comments, so I think the last patch is good and just needs Darin's r+.
Comment 24 Adam Barth 2011-09-06 10:58:47 PDT
Comment on attachment 105358 [details]
Patch

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

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:394
> +static void AddTouchPoints(TouchList* touches, const IntPoint& offset, 

AddTouchPoints => addTouchPoints

There are a couple of other minor things like that.  I'll re-spin the patch and then we'll probably be good to go.
Comment 25 Adam Barth 2011-09-06 12:19:15 PDT
Created attachment 106462 [details]
Patch
Comment 26 Darin Fisher (:fishd, Google) 2011-09-06 13:37:39 PDT
Comment on attachment 106462 [details]
Patch

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

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:53
> +static const double kMillisPerSecond = 1000.0;

nit: I don't think WebKit uses the k* prefix for constants.  this should just be millisPerSecond.

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:385
> +    unsigned numberOfCharacters = std::min(event.keyEvent()->text().length(), static_cast<unsigned>(WebKeyboardEvent::textLengthCap));

note: since WebKeyboardEventBuilder extends from WebKeyboardEvent, no need for
the WebKeyboardEvent:: prefix in front of textLengthCap.
Comment 27 Adam Barth 2011-09-06 14:02:18 PDT
Created attachment 106482 [details]
Patch for landing
Comment 28 WebKit Review Bot 2011-09-06 14:44:00 PDT
Comment on attachment 106482 [details]
Patch for landing

Rejecting attachment 106482 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
g/obj.target/Source/WebKit/chromium/libwebkit.a(out/Debug/obj.target/Source/WebKit/chromium/../../../webkit/Source/WebKit/chromium/src/WebInputEventConversion.o): in function WebKit::addTouchPoints(WebCore::TouchList*, WebCore::IntPoint const&, WebKit::WebTouchPoint*, unsigned int*):Source/WebKit/chromium/src/WebInputEventConversion.cpp:396: error: undefined reference to 'WebKit::WebTouchEvent::touchesLengthCap'
collect2: ld returned 1 exit status
make: *** [out/Debug/webkit_unit_tests] Error 1

Full output: http://queues.webkit.org/results/9599129
Comment 29 Adam Barth 2011-09-06 20:30:11 PDT
Created attachment 106538 [details]
Patch for landing
Comment 30 WebKit Review Bot 2011-09-06 20:47:36 PDT
Comment on attachment 106538 [details]
Patch for landing

Rejecting attachment 106538 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
roller.o
  CXX(target) out/Debug/obj.target/DumpRenderTree/Tools/DumpRenderTree/chromium/Task.o
Tools/DumpRenderTree/chromium/EventSender.cpp: In member function 'void EventSender::sendCurrentTouchEvent(WebKit::WebInputEvent::Type)':
Tools/DumpRenderTree/chromium/EventSender.cpp:951: error: 'touchPointsLengthCap' is not a member of 'WebKit::WebTouchEvent'
make: *** [out/Debug/obj.target/DumpRenderTree/Tools/DumpRenderTree/chromium/EventSender.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/9599334
Comment 31 Adam Barth 2011-09-06 21:20:37 PDT
Created attachment 106541 [details]
Patch for landing
Comment 32 WebKit Review Bot 2011-09-06 21:34:54 PDT
Comment on attachment 106541 [details]
Patch for landing

Clearing flags on attachment: 106541

Committed r94635: <http://trac.webkit.org/changeset/94635>
Comment 33 WebKit Review Bot 2011-09-06 21:35:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Adam Barth 2011-09-07 11:27:59 PDT
The build fix from the future is in http://codereview.chromium.org/7840035/
Comment 35 Adam Barth 2011-09-07 11:31:31 PDT
Comment on attachment 106541 [details]
Patch for landing

Clearing flags on attachment: 106541

Committed r94692: <http://trac.webkit.org/changeset/94692>
Comment 36 Adam Barth 2011-09-07 11:31:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 Robert Kroeger 2011-09-09 11:52:56 PDT
I don't understand why this change made. Would someone be willing to explain?

In particular: 

* changedTouches and targetTouches fields don't ever appear to be set in the browser

* they get dropped on the floor when the WebTouchEvent is made into a PlatformTouchEvent

* they get re-created from the touch event stream in EventHandler::handleTouchEvent from PlatformTouchEvent

* their addition would seem to nearly triple the size of a WebTouchEvent

* it breaks what I have understood to be a Chromium WebKit API principle that Web<*>Event should map closely to Platform<*>Event.

So while the blocked bug suggests some possible purposes, the changes that I would imagine would be needed to go forward to completion on this would seem to require non-trivial surgery to EventHandler::handleTouchEvent -- surgery that I'd like to discuss.
Comment 38 Adam Barth 2011-09-09 14:51:14 PDT
> I don't understand why this change made. Would someone be willing to explain?

This change is part of the landing the Android branch of the Chromium port.  I can connect you with the appropriate folks if you'd like to discuss more with them.