Bug 66800

Created attachment 104901 [details] Patch

Comment on attachment 104901 [details] Patch Attachment 104901 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9477520

Created attachment 104911 [details] Patch

Comment on attachment 104911 [details] Patch Attachment 104911 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9479541

Created attachment 104914 [details] Patch

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

Created attachment 104988 [details] Patch

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

Created attachment 104995 [details] Patch

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'

Created attachment 104998 [details] Patch

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

Created attachment 105161 [details] Patch

Created attachment 105358 [details] Patch

*** Bug 67615 has been marked as a duplicate of this bug. ***

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.

I would be happy to.

Comment on attachment 105358 [details] Patch Looks like there are just a couple style things left to clean up, right?

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

Created attachment 106462 [details] Patch

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.

Created attachment 106482 [details] Patch for landing

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

Created attachment 106538 [details] Patch for landing

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

Created attachment 106541 [details] Patch for landing

Comment on attachment 106541 [details] Patch for landing Clearing flags on attachment: 106541 Committed r94635: <http://trac.webkit.org/changeset/94635>

All reviewed patches have been landed. Closing bug.

The build fix from the future is in http://codereview.chromium.org/7840035/

Comment on attachment 106541 [details] Patch for landing Clearing flags on attachment: 106541 Committed r94692: <http://trac.webkit.org/changeset/94692>

All reviewed patches have been landed. Closing bug.

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.

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