Bug 51258

I have started work on a patch to address this.

Created attachment 77097 [details] Proposed fix 1

(In reply to comment #2) > Created an attachment (id=77097) [details] > Proposed fix 1 forgot to mention: tested using the Qt test runner: $ ./WebKitTools/Scripts/run-webkit-tests --qt --repeat-each 50 --timeout 5 fast/events/touch/ Running build-dumprendertree Running tests from /usr/local/google/home/joth/dev/b.clank/src/third_party/WebKit/LayoutTests Testing 13 test cases, repeating each test 50 times. fast/events/touch ................................................................................................. 6.72s total testing time all 650 test cases succeeded

Comment on attachment 77097 [details] Proposed fix 1 View in context: https://bugs.webkit.org/attachment.cgi?id=77097&action=review Nice cleanup, thanks! Just a few comments. > WebCore/page/EventHandler.cpp:2789 > +static const AtomicString& eventNameForTouchPointState(int state) PlatformTouchPoint::State instead of int? > WebCore/page/EventHandler.cpp:2808 > + // The complete set of touches in this event, for the event touches list. I think this comment is a little confusing. How about something like "Holds the complete set of touches on the screen and will be used as the 'touches' list in the JS event." > WebCore/page/EventHandler.cpp:2811 > + // For each target holds the complete set of currently active touches, for the targetTouches list. Again, a bit confusing. Maybe be more explicit and mention that it forms the 'targetTouches' list in the JS event. I think the touch code needs as many clear comments as possible :) > WebCore/page/EventHandler.cpp:2815 > + // Array of touches per state, used to assembe the the changedTouches list. assembe -> assemble and too many 'the's :), changedTouches list in JS event. > WebCore/page/EventHandler.cpp:2820 > + HashSet<EventTarget*> m_targets; Should the EventTargets be RefPtrs? > WebCore/page/EventHandler.cpp:2854 > + ASSERT(node); node.get()? > WebCore/page/EventHandler.cpp:2891 > + if (!touchTarget.get()) Why is this moved? Seemed better in it's old place so that we didn't bother creating a Touch without a target. > WebCore/page/EventHandler.cpp:2899 > + // touches and targetTouches should contain information about every touch currently on the screen. Maybe expand this comment to say that if it is a TouchRelease or Cancel then as it's not on the screen it won't be in touches or targetTouches, but will be in changedTouches. > WebCore/page/EventHandler.cpp:2916 > + changedTouches[point.state()].m_targets.add(touchTarget.get()); Perhaps point.state() could go into a local, we seem to be calling it a lot. > WebCore/page/EventHandler.cpp:2932 > + const AtomicString& stateName(eventNameForTouchPointState(state)); Can we move this into the TouchEvent::create call directly? > WebCore/page/EventHandler.cpp:2938 > + ASSERT(targetTouches); targetTouches.get()?

Comment on attachment 77097 [details] Proposed fix 1 View in context: https://bugs.webkit.org/attachment.cgi?id=77097&action=review Thanks for the comments, think I covered them all. Just preparing a new patch... >> WebCore/page/EventHandler.cpp:2789 >> +static const AtomicString& eventNameForTouchPointState(int state) > > PlatformTouchPoint::State instead of int? Done >> WebCore/page/EventHandler.cpp:2808 >> + // The complete set of touches in this event, for the event touches list. > > I think this comment is a little confusing. How about something like > "Holds the complete set of touches on the screen and will be used as the 'touches' list in the JS event." Done. Thanks. >> WebCore/page/EventHandler.cpp:2811 >> + // For each target holds the complete set of currently active touches, for the targetTouches list. > > Again, a bit confusing. Maybe be more explicit and mention that it forms the 'targetTouches' list in the JS event. I think the touch code needs as many clear comments as possible :) How about: // Another different on the 'touches' list above: filtered and grouped by event target. Used for the // 'targetTouches' list in the JS event. >> WebCore/page/EventHandler.cpp:2815 >> + // Array of touches per state, used to assembe the the changedTouches list. > > assembe -> assemble and too many 'the's :), changedTouches list in JS event. Done >> WebCore/page/EventHandler.cpp:2820 >> + HashSet<EventTarget*> m_targets; > > Should the EventTargets be RefPtrs? Done. + added a typedef EventTargetSet >> WebCore/page/EventHandler.cpp:2854 >> + ASSERT(node); > > node.get()? Apparently not, jorlow says to rely on the conversion operator. >> WebCore/page/EventHandler.cpp:2891 >> + if (!touchTarget.get()) > > Why is this moved? Seemed better in it's old place so that we didn't bother creating a Touch without a target. Mistake, fixed. Good spot! >> WebCore/page/EventHandler.cpp:2899 >> + // touches and targetTouches should contain information about every touch currently on the screen. > > Maybe expand this comment to say that if it is a TouchRelease or Cancel then as it's not on the screen it won't be in touches or targetTouches, but will be in changedTouches. Changed to: // touches and targetTouches should only contain information about touches still on the screen, so if this point is // released or cancelled it will only appear in the changedTouches list. I also added a link to http://www.sitepen.com/blog/2008/07/10/touching-and-gesturing-on-the-iphone/ upto, which hopefully helps spell this lot out. >> WebCore/page/EventHandler.cpp:2916 >> + changedTouches[point.state()].m_targets.add(touchTarget.get()); > > Perhaps point.state() could go into a local, we seem to be calling it a lot. Done >> WebCore/page/EventHandler.cpp:2932 >> + const AtomicString& stateName(eventNameForTouchPointState(state)); > > Can we move this into the TouchEvent::create call directly? Minor benefit it doing it outside the loop, and also it now has a static_cast (see your comment #1) so probably clearer as it too. >> WebCore/page/EventHandler.cpp:2938 >> + ASSERT(targetTouches); > > targetTouches.get()? Apparently not

Created attachment 77116 [details] Patch 2 - benm comments.

Comment on attachment 77116 [details] Patch 2 - benm comments. View in context: https://bugs.webkit.org/attachment.cgi?id=77116&action=review > WebCore/page/EventHandler.cpp:2801 > + ASSERT(false); ASSERT_NOT_REACHED()

Created attachment 77200 [details] Patch 3 - tonikitoo comment and fix comment typo Thanks.

Comment on attachment 77200 [details] Patch 3 - tonikitoo comment and fix comment typo View in context: https://bugs.webkit.org/attachment.cgi?id=77200&action=review WebCore changes look good. Had a quick look over the layout tests ... To any other folks participating in the review: I don't have review rights so I cannot r+ this patch when it's ready. > LayoutTests/fast/events/touch/touch-target-expected.txt:41 > +TEST COMPLETE We now have two "test complete" messages in this test - doesn't seem right? > LayoutTests/fast/events/touch/touch-target-limited-expected.txt:45 > +TEST COMPLETE ditto

Created attachment 77201 [details] Patch 4 - corrects expected results to only have one TEST COMPLETE

Comment on attachment 77201 [details] Patch 4 - corrects expected results to only have one TEST COMPLETE View in context: https://bugs.webkit.org/attachment.cgi?id=77201&action=review > WebCore/page/EventHandler.cpp:2860 > + RefPtr<Node> node = result.innerNode(); Why do we now take a reference? Is this required? > WebCore/page/EventHandler.cpp:2943 > + RefPtr<TouchList> targetTouches(state == PlatformTouchPoint::TouchCancelled ? emptyList : touchesByTarget.get(touchEventTarget)); Would it be clearer to have a isTouchCancelEvent boolean set up outside this inner loop?

Created attachment 77219 [details] Patch 5 - steveblock comments Thanks, both comments done. Could you commit-queue+ it too? Cheers.

Comment on attachment 77219 [details] Patch 5 - steveblock comments r=me

The commit-queue encountered the following flaky tests while processing attachment 77219 [details]: inspector/debugger-pause-on-breakpoint.html bug 51320 (author: podivilov@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 77219 [details] Patch 5 - steveblock comments Clearing flags on attachment: 77219 Committed r74553: <http://trac.webkit.org/changeset/74553>

All reviewed patches have been landed. Closing bug.

Comment on attachment 77219 [details] Patch 5 - steveblock comments View in context: https://bugs.webkit.org/attachment.cgi?id=77219&action=review > LayoutTests/fast/events/touch/script-tests/multi-touch-grouped-targets.js:75 > + eventSender.addTouchPoint(50, 150); Did you intend to add two identical touch points here? Since you really only add two unique test points, a correct implementation should fail this test (event.changedTouches.length should never be 3, for instance).