Notify Blink about start of gesture scroll through a queued event.

Notify Blink about start of gesture scroll through a queued event.

Added WebIputEventL::TouchScrollStarted event to notify Blink when a
gesture scroll is starting.

We need to know from Blink when gesture scroll is about to start.
So far, the only use of this info is firing a pointercancel event
during scrolling. We had been using a bit in WebTouchEvent before,
which didn't really reflect the possibility of scrolling (only
indicates if the touch has moved beyond the slop region). To find
the correct info to pass to Blink, we have to consider the effect of
touch-action on gestures.

In this CL, we push a new event into the touch queue when gestures
passes touch-action filtering.

This CL also adds a hack to prevent unintended reentry to a dispatch
method in TouchEventQueue.

BUG=567740, 600773
Committed: https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b
Cr-Commit-Position: refs/heads/master@{#388258}

[mustaq, 2016-03-15 19:16:29]

mustaq@chromium.org changed reviewers:
+ rbyers@chromium.org, tdresser@chromium.org

[mustaq, 2016-03-15 19:16:30]

Any clue why the input_router_impl mod suppresses all touch events after
TouchScrollStarted event?

[mustaq, 2016-03-15 19:47:40]

On 2016/03/15 19:16:30, mustaq wrote:
> Any clue why the input_router_impl mod suppresses all touch events after
> TouchScrollStarted event?

I think I have got a suspect here: assuming an in-flight touchmove can
coalesce with upcoming touchmoves before it is popped off the queue,
the TouchScrollStarted event prevented coalescing two events by sitting
in the middle, which messed up an optimization in
TouchEventQueue::QueueEvent with FilterBeforeForwarding.

Is my assumption right to begin with?

[tdresser, 2016-03-16 14:56:31]

You're right that this will likely prevent some coalescing, but I don't
immediately see how this could cause the broken behavior.

https://codereview.chromium.org/1800143002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/1800143002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/touch_event_queue.cc:860: return
FORWARD_TO_RENDERER;
Are we returning before this for some TouchScrollStarted events?

[mustaq, 2016-03-16 15:15:40]

https://codereview.chromium.org/1800143002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/1800143002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/touch_event_queue.cc:860: return
FORWARD_TO_RENDERER;
On 2016/03/16 14:56:31, tdresser wrote:
> Are we returning before this for some TouchScrollStarted events?

That's a possibility of course, but not in the case I am looking at now. See
below.

https://codereview.chromium.org/1800143002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/touch_event_queue.cc:886: return
FORWARD_TO_RENDERER;
Here is a possible problem: for touchmoves as well as touchends, the filter
normally returns FORWARD_TO_RENDERER from within this loop. But after firing
TouchScrollStarted event, the filter doesn't return from within the loop,
instead returns ACK_WITH_NO_CONSUMER_EXISTS.

[tdresser, 2016-03-16 15:39:15]

https://codereview.chromium.org/1800143002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/1800143002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/touch_event_queue.cc:886: return
FORWARD_TO_RENDERER;
On 2016/03/16 15:15:40, mustaq wrote:
> Here is a possible problem: for touchmoves as well as touchends, the filter
> normally returns FORWARD_TO_RENDERER from within this loop. But after firing
> TouchScrollStarted event, the filter doesn't return from within the loop,
> instead returns ACK_WITH_NO_CONSUMER_EXISTS.

Yeah, that's causing the behavior you're seeing.

What's causing the TouchMove's not to trigger HasPointChanged? We might want to
check if the last_sent_touch_event_->type is different from the event.type, and
FORWARD_TO_RENDERER if that's the case.

[mustaq, 2016-03-16 15:44:11]

https://codereview.chromium.org/1800143002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/1800143002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/input/touch_event_queue.cc:886: return
FORWARD_TO_RENDERER;
On 2016/03/16 15:39:15, tdresser wrote:
> On 2016/03/16 15:15:40, mustaq wrote:
> > Here is a possible problem: for touchmoves as well as touchends, the filter
> > normally returns FORWARD_TO_RENDERER from within this loop. But after firing
> > TouchScrollStarted event, the filter doesn't return from within the loop,
> > instead returns ACK_WITH_NO_CONSUMER_EXISTS.
> 
> Yeah, that's causing the behavior you're seeing.
> 
> What's causing the TouchMove's not to trigger HasPointChanged? We might want
to
> check if the last_sent_touch_event_->type is different from the event.type,
and
> FORWARD_TO_RENDERER if that's the case.

Got the culprit: when sent_touch_event_ is the new event, the point.id
comparison above fails, so the loop doesn't effectively execute.

[mustaq, 2016-03-16 19:40:51]

Here is a patch that works but proves that it could be impossible to send
pointercancel after the first touchmove. In this patch, we encounter three
touchmoves before seeing the pointercancel. This is what is happening here:

When InputRouter::SendGestureEvent is called (which is the first point in time
we can be certain about touch-action filtering), the first two touchmoves have
been dispatched and the third one is in flight. Even if we are able to switch
the order of InputRouter::SendTouchEvent & InputRouter::SendGestureEvent calls
(from RenderWidgethostImpl), we would still get two touchmoves.

Looks like two-touchmoves is an acceptable solution then?

[tdresser, 2016-03-17 14:19:10]

On 2016/03/16 19:40:51, mustaq wrote:
> Here is a patch that works but proves that it could be impossible to send
> pointercancel after the first touchmove. In this patch, we encounter three
> touchmoves before seeing the pointercancel. This is what is happening here:
> 
> When InputRouter::SendGestureEvent is called (which is the first point in time
> we can be certain about touch-action filtering), the first two touchmoves have
> been dispatched and the third one is in flight. Even if we are able to switch
> the order of InputRouter::SendTouchEvent & InputRouter::SendGestureEvent calls
> (from RenderWidgethostImpl), we would still get two touchmoves.
> 
> Looks like two-touchmoves is an acceptable solution then?

Is this two touchmoves outside of the slop region?

[mustaq, 2016-03-22 15:55:36]

On 2016/03/17 14:19:10, tdresser wrote:
> On 2016/03/16 19:40:51, mustaq wrote:
> > Here is a patch that works but proves that it could be impossible to send
> > pointercancel after the first touchmove. In this patch, we encounter three
> > touchmoves before seeing the pointercancel. This is what is happening here:
> > 
> > When InputRouter::SendGestureEvent is called (which is the first point in
time
> > we can be certain about touch-action filtering), the first two touchmoves
have
> > been dispatched and the third one is in flight. Even if we are able to
switch
> > the order of InputRouter::SendTouchEvent & InputRouter::SendGestureEvent
calls
> > (from RenderWidgethostImpl), we would still get two touchmoves.
> > 
> > Looks like two-touchmoves is an acceptable solution then?
> 
> Is this two touchmoves outside of the slop region?

I am investigating the number again, but they were both outside the slop region,
visible from JS.

[mustaq, 2016-03-22 18:17:46]

Patchset #3 (id:40001) has been deleted

[mustaq, 2016-03-30 19:01:20]

It works perfectly now, woohoo!

ptal.

[tdresser, 2016-03-30 19:19:01]

What was going wrong previously?

https://codereview.chromium.org/1800143002/diff/100001/components/test_runner...
File components/test_runner/event_sender.cc (right):

https://codereview.chromium.org/1800143002/diff/100001/components/test_runner...
components/test_runner/event_sender.cc:514: void NotifyEndOfTouchScroll();
This appears to never be defined...

https://codereview.chromium.org/1800143002/diff/100001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/1800143002/diff/100001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.cc:760: // Skip ack for
TouchScrollStarted---it was synthesized within the queue.
I'd go with " - " instead of "---".

https://codereview.chromium.org/1800143002/diff/100001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/1800143002/diff/100001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.h:74: // Adds a
TouchScrollStarted event to the front of the queue.
This comment is a bit misleading, as it's often placed second in the queue.

https://codereview.chromium.org/1800143002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1800143002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:1158:
DCHECK(touch.event.type != blink::WebInputEvent::TouchScrollStarted);
Add comment.

https://codereview.chromium.org/1800143002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html
(right):

https://codereview.chromium.org/1800143002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html:164:
// TODO(mustaq): Scrolling is now independent of whether the TEs thru the
eventSender gets
thru -> from
gets -> get

I'm not sure what this comment means, can you clarify it?

https://codereview.chromium.org/1800143002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1800143002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.h:59: // When a
particular pointerId is removed that id is considered free even
removed, that

https://codereview.chromium.org/1800143002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/1800143002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:314: //
TODO(nzolghadr): crbug.com/579553 dealing with implicit touch capturing vs
pointer event capturing
Missing .

[mustaq, 2016-03-30 20:20:42]

> What was going wrong previously?

Had two main problems:
- The queue was occasionally empty even when a touch event was in flight.
  Delaying the TouchEventQueue pop_front() a bit fixed the problem.
- Acks for TouchScrollStarted was leaking upwards, towards the RWH level,
  which messed up TE release sequencing in TDGF. Fix: limited all
  "activities" related to TouchScrollStarted strictly within/below
  TouchEventQueue.

(Then there was an iterator/pointer mess-up triggered by the 2nd-place
push in the deque---my generously-added fprintfs only worsened the
situation!)

[mustaq, 2016-03-30 20:21:05]

https://codereview.chromium.org/1800143002/diff/100001/components/test_runner...
File components/test_runner/event_sender.cc (right):

https://codereview.chromium.org/1800143002/diff/100001/components/test_runner...
components/test_runner/event_sender.cc:514: void NotifyEndOfTouchScroll();
On 2016/03/30 19:19:01, tdresser wrote:
> This appears to never be defined...

Removed.

https://codereview.chromium.org/1800143002/diff/100001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/1800143002/diff/100001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.cc:760: // Skip ack for
TouchScrollStarted---it was synthesized within the queue.
On 2016/03/30 19:19:01, tdresser wrote:
> I'd go with " - " instead of "---".

Changed.

https://codereview.chromium.org/1800143002/diff/100001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/1800143002/diff/100001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.h:74: // Adds a
TouchScrollStarted event to the front of the queue.
On 2016/03/30 19:19:01, tdresser wrote:
> This comment is a bit misleading, as it's often placed second in the queue.

It is better? Or you want to see something like "1st or 2nd place"?

https://codereview.chromium.org/1800143002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/1800143002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:1158:
DCHECK(touch.event.type != blink::WebInputEvent::TouchScrollStarted);
On 2016/03/30 19:19:01, tdresser wrote:
> Add comment.

Done.

https://codereview.chromium.org/1800143002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1800143002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.h:59: // When a
particular pointerId is removed that id is considered free even
On 2016/03/30 19:19:01, tdresser wrote:
> removed, that

Done.

https://codereview.chromium.org/1800143002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/1800143002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:314: //
TODO(nzolghadr): crbug.com/579553 dealing with implicit touch capturing vs
pointer event capturing
On 2016/03/30 19:19:01, tdresser wrote:
> Missing .

Done.

[mustaq, 2016-04-01 14:26:56]

ptal: done adding the last unit test.

[tdresser, 2016-04-01 14:50:24]

https://codereview.chromium.org/1800143002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html
(right):

https://codereview.chromium.org/1800143002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html:164:
// TODO(mustaq): Scrolling is now independent of whether the TEs thru the
eventSender gets
On 2016/03/30 19:19:01, tdresser wrote:
> thru -> from
> gets -> get
> 
> I'm not sure what this comment means, can you clarify it?

Ping.

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
content/browser/renderer_host/input/input_router_impl.cc:162: if
(gesture_event.event.type == blink::WebInputEvent::GestureScrollBegin) {
This file doesn't appear to use {} for any other if statements with single line
bodies. Let's be consistent.

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
content/browser/renderer_host/input/input_router_impl_unittest.cc:277: void
ProcessSyntheticInputEventAck(WebInputEvent::Type type) {
Let's make this method more specific - something like
AckTouchScrollStartedNotification, and add a comment on why it's needed.

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
content/browser/renderer_host/input/input_router_impl_unittest.cc:1703:
ProcessSyntheticInputEventAck(WebInputEvent::TouchScrollStarted);
Shouldn't we assert somewhere that we output TouchScrollStarted events?

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.cc:499:
touch.event.touchesLength = 0;
The WebTouchEventConstructor zeroes these fields, so we shouldn't need to zero
them explicitly.

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.h:13: #include <map>
It looks like this include is unused. Can you remove it while you're here?

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html
(right):

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html:64:
eventSender.touchMove();
Why do we only notify after this touchMove, instead of the previous one?

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:320:
std::sort(mappedIds.begin(), mappedIds.end());
Why does this needs a stable ordering?

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.h:65:
WillBeHeapVector<int> getPointerIdsOfType(WebPointerProperties::PointerType);
We don't need WillBe types anymore, do we?
Might as well just say HeapVector.

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3721: bool
freshTouchEvents = true;
I'm not a big fan of the name of this variable. Could we rename it to something
like "startingNewTouchSequence"?

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:312:
RefPtrWillBeRawPtr<EventTarget> target =
m_nodeUnderPointer.get(pointerId).target;
Don't need WillBe types.

https://codereview.chromium.org/1800143002/diff/160001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/1800143002/diff/160001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:2626:
TEST_F(TouchEventQueueTest, TouchScrollNotificationOrder) {
Aren't there a few cases we should test?
1. Queue is empty when we receive the notification.
2. Queue has length one.
3. Queue has length > 1 (probably test with > 3)

Perhaps we don't need #2, but I suspect at least 1 and 3 should be tested.

[mustaq, 2016-04-01 18:31:05]

ptal

https://codereview.chromium.org/1800143002/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html
(right):

https://codereview.chromium.org/1800143002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html:164:
// TODO(mustaq): Scrolling is now independent of whether the TEs thru the
eventSender gets
On 2016/04/01 14:50:24, tdresser wrote:
> On 2016/03/30 19:19:01, tdresser wrote:
> > thru -> from
> > gets -> get
> > 
> > I'm not sure what this comment means, can you clarify it?
> 
> Ping.

Earlier, we used to decide about pointercancels from within EventHandler, so it
was important to check that after canceling a touchmove (implying no scrolling)
no pointercancel is fired.

In this CL, the pointercancel decision comes directly from the scrolling
decision in the input-router (which knows about canceled touchmoves both
before/after this CL). Since notifyStartOfTouchScroll() simulates the scrolling
decision at input-router, preventing touchmoves through eventSender has no
effect.

Anyway, just did the TODO here.

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
content/browser/renderer_host/input/input_router_impl.cc:162: if
(gesture_event.event.type == blink::WebInputEvent::GestureScrollBegin) {
On 2016/04/01 14:50:24, tdresser wrote:
> This file doesn't appear to use {} for any other if statements with single
line
> bodies. Let's be consistent.

Done.

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
content/browser/renderer_host/input/input_router_impl_unittest.cc:277: void
ProcessSyntheticInputEventAck(WebInputEvent::Type type) {
On 2016/04/01 14:50:24, tdresser wrote:
> Let's make this method more specific - something like
> AckTouchScrollStartedNotification, and add a comment on why it's needed.

Done.

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
content/browser/renderer_host/input/input_router_impl_unittest.cc:1703:
ProcessSyntheticInputEventAck(WebInputEvent::TouchScrollStarted);
On 2016/04/01 14:50:24, tdresser wrote:
> Shouldn't we assert somewhere that we output TouchScrollStarted events?

The existing tests here only care about events passed upstream of
input_event_router, between RH & the router. The TouchScrollStarted event is
strictly downstream, and I have the check already in TouchEventQueueTest.

Testing TouchScrollStarted here would require pulling TouchEventQueueTest logic
into this file, which violates the "unittest" concept.

I added queue size checks below wherever we have only the new event in the
queue.

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.cc:499:
touch.event.touchesLength = 0;
On 2016/04/01 14:50:24, tdresser wrote:
> The WebTouchEventConstructor zeroes these fields, so we shouldn't need to zero
> them explicitly.

Removed the length. But left the id as-is to emphasize the special value.

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue.h (right):

https://codereview.chromium.org/1800143002/diff/140001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.h:13: #include <map>
On 2016/04/01 14:50:24, tdresser wrote:
> It looks like this include is unused. Can you remove it while you're here?

Done.

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html
(right):

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html:64:
eventSender.touchMove();
On 2016/04/01 14:50:24, tdresser wrote:
> Why do we only notify after this touchMove, instead of the previous one?

I originally put it here to emphasize that pointercancel firing is triggered
specifically by touchMoveCausingScrollIfUncanceled and not by any side-effect of
touch-events. The same argument now applies to notifyStartOfTouchScroll.

If you still prefer the ideal sequence here, I will move the notification a step
above.

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:320:
std::sort(mappedIds.begin(), mappedIds.end());
On 2016/04/01 14:50:24, tdresser wrote:
> Why does this needs a stable ordering?

Removed.

I will bring it back if you don't like the apparent random ordering in the
layout test output.

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.h:65:
WillBeHeapVector<int> getPointerIdsOfType(WebPointerProperties::PointerType);
On 2016/04/01 14:50:24, tdresser wrote:
> We don't need WillBe types anymore, do we?
> Might as well just say HeapVector.

Yes, good catch. Let's save one search-replace from haraken's plate ;-)

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3721: bool
freshTouchEvents = true;
On 2016/04/01 14:50:24, tdresser wrote:
> I'm not a big fan of the name of this variable. Could we rename it to
something
> like "startingNewTouchSequence"?

Done.

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:312:
RefPtrWillBeRawPtr<EventTarget> target =
m_nodeUnderPointer.get(pointerId).target;
On 2016/04/01 14:50:24, tdresser wrote:
> Don't need WillBe types.

Done. Didn't touch old code though.

https://codereview.chromium.org/1800143002/diff/160001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/1800143002/diff/160001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:2626:
TEST_F(TouchEventQueueTest, TouchScrollNotificationOrder) {
On 2016/04/01 14:50:24, tdresser wrote:
> Aren't there a few cases we should test?
> 1. Queue is empty when we receive the notification.
> 2. Queue has length one.
> 3. Queue has length > 1 (probably test with > 3)
> 
> Perhaps we don't need #2, but I suspect at least 1 and 3 should be tested.

Added all, including #2 where the notification is at the end of the queue.

[tdresser, 2016-04-01 19:03:20]

LGTM, % nits, and figuring out if the pointer cancel order needs to be sane.

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html
(right):

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html:64:
eventSender.touchMove();
On 2016/04/01 18:31:04, mustaq wrote:
> On 2016/04/01 14:50:24, tdresser wrote:
> > Why do we only notify after this touchMove, instead of the previous one?
> 
> I originally put it here to emphasize that pointercancel firing is triggered
> specifically by touchMoveCausingScrollIfUncanceled and not by any side-effect
of
> touch-events. The same argument now applies to notifyStartOfTouchScroll.
> 
> If you still prefer the ideal sequence here, I will move the notification a
step
> above.

Let's move the notification up.

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:320:
std::sort(mappedIds.begin(), mappedIds.end());
On 2016/04/01 18:31:05, mustaq wrote:
> On 2016/04/01 14:50:24, tdresser wrote:
> > Why does this needs a stable ordering?
> 
> Removed.
> 
> I will bring it back if you don't like the apparent random ordering in the
> layout test output.

Hmmm, I'm actually not sure what's correct here. What do other browsers do?

https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
content/browser/renderer_host/input/input_router_impl_unittest.cc:280: //
result.
I think we can get rid of "like other touch events".

https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
content/browser/renderer_host/input/input_router_impl_unittest.cc:281: void
AckTouchScrollStartedNotification(WebInputEvent::Type type) {
We can hardcode the type in this method.

https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.cc:765:
WebInputEvent::TouchScrollStarted)
Multiline bodies require {}

https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:2627: // ===
Test 1: touch-scroll-notification in an empty queue ===
There isn't much code duplication incurred by splitting this into 3 separate
tests. I think it would be worthwhile.

https://codereview.chromium.org/1800143002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html
(right):

https://codereview.chromium.org/1800143002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html:52:
function testSingleDrag(notifyStartOfTOuchScroll) {
notifyStartOfTOuchScroll ->
notifyStartOfTouchScroll

[mustaq, 2016-04-01 20:21:09]

Patchset #9 (id:180001) has been deleted

[mustaq, 2016-04-01 21:49:53]

This stopped working from last patch! I have no clue!!

-lgtm

https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
File content/browser/renderer_host/input/input_router_impl_unittest.cc (right):

https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
content/browser/renderer_host/input/input_router_impl_unittest.cc:280: //
result.
On 2016/04/01 19:03:20, tdresser wrote:
> I think we can get rid of "like other touch events".

Done.

https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
content/browser/renderer_host/input/input_router_impl_unittest.cc:281: void
AckTouchScrollStartedNotification(WebInputEvent::Type type) {
On 2016/04/01 19:03:20, tdresser wrote:
> We can hardcode the type in this method.

Done.

https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.cc:765:
WebInputEvent::TouchScrollStarted)
On 2016/04/01 19:03:20, tdresser wrote:
> Multiline bodies require {}

Done.

https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right):

https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue_unittest.cc:2627: // ===
Test 1: touch-scroll-notification in an empty queue ===
On 2016/04/01 19:03:20, tdresser wrote:
> There isn't much code duplication incurred by splitting this into 3 separate
> tests. I think it would be worthwhile.

Done.

https://codereview.chromium.org/1800143002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html
(right):

https://codereview.chromium.org/1800143002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html:52:
function testSingleDrag(notifyStartOfTOuchScroll) {
On 2016/04/01 19:03:20, tdresser wrote:
> notifyStartOfTOuchScroll ->
> notifyStartOfTouchScroll

Done.

[tdresser, 2016-04-04 12:36:33]

On 2016/04/01 21:49:53, mustaq wrote:
> This stopped working from last patch! I have no clue!!
> 
> -lgtm
> 
>
https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
> File content/browser/renderer_host/input/input_router_impl_unittest.cc
(right):
> 
>
https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
> content/browser/renderer_host/input/input_router_impl_unittest.cc:280: //
> result.
> On 2016/04/01 19:03:20, tdresser wrote:
> > I think we can get rid of "like other touch events".
> 
> Done.
> 
>
https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
> content/browser/renderer_host/input/input_router_impl_unittest.cc:281: void
> AckTouchScrollStartedNotification(WebInputEvent::Type type) {
> On 2016/04/01 19:03:20, tdresser wrote:
> > We can hardcode the type in this method.
> 
> Done.
> 
>
https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
> File content/browser/renderer_host/input/touch_event_queue.cc (right):
> 
>
https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
> content/browser/renderer_host/input/touch_event_queue.cc:765:
> WebInputEvent::TouchScrollStarted)
> On 2016/04/01 19:03:20, tdresser wrote:
> > Multiline bodies require {}
> 
> Done.
> 
>
https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
> File content/browser/renderer_host/input/touch_event_queue_unittest.cc
(right):
> 
>
https://codereview.chromium.org/1800143002/diff/200001/content/browser/render...
> content/browser/renderer_host/input/touch_event_queue_unittest.cc:2627: // ===
> Test 1: touch-scroll-notification in an empty queue ===
> On 2016/04/01 19:03:20, tdresser wrote:
> > There isn't much code duplication incurred by splitting this into 3 separate
> > tests. I think it would be worthwhile.
> 
> Done.
> 
>
https://codereview.chromium.org/1800143002/diff/200001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html
> (right):
> 
>
https://codereview.chromium.org/1800143002/diff/200001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html:52:
> function testSingleDrag(notifyStartOfTOuchScroll) {
> On 2016/04/01 19:03:20, tdresser wrote:
> > notifyStartOfTOuchScroll ->
> > notifyStartOfTouchScroll
> 
> Done.

Just so I remember to take another look at this once it's working, not LGTM.

[mustaq, 2016-04-06 20:53:04]

Description was changed from

==========
Notify Blink about start of gesture scroll through a queued event.

Added WebIputEventL::TouchScrollStarted event to notify Blink when a
gesture scroll is starting.

BUG=567740
==========

to

==========
Notify Blink about start of gesture scroll through a queued event.

Added WebIputEventL::TouchScrollStarted event to notify Blink when a
gesture scroll is starting.

We need to know from Blink when gesture scroll is about to start.
So far, the only use of this info is firing a pointercancel event
during scrolling. We had been using a bit in WebTouchEvent before,
which didn't really reflect the possibility of scrolling (only
indicates if the touch has moved beyond the slop region). To find
the correct info to pass to Blink, we have to consider the effect of
touch-action on gestures.

In this CL, we push a new event into the touch queue when gestures
passes touch-action filtering.

BUG=567740
==========

[mustaq, 2016-04-06 20:55:27]

Description was changed from

==========
Notify Blink about start of gesture scroll through a queued event.

Added WebIputEventL::TouchScrollStarted event to notify Blink when a
gesture scroll is starting.

We need to know from Blink when gesture scroll is about to start.
So far, the only use of this info is firing a pointercancel event
during scrolling. We had been using a bit in WebTouchEvent before,
which didn't really reflect the possibility of scrolling (only
indicates if the touch has moved beyond the slop region). To find
the correct info to pass to Blink, we have to consider the effect of
touch-action on gestures.

In this CL, we push a new event into the touch queue when gestures
passes touch-action filtering.

BUG=567740
==========

to

==========
Notify Blink about start of gesture scroll through a queued event.

Added WebIputEventL::TouchScrollStarted event to notify Blink when a
gesture scroll is starting.

We need to know from Blink when gesture scroll is about to start.
So far, the only use of this info is firing a pointercancel event
during scrolling. We had been using a bit in WebTouchEvent before,
which didn't really reflect the possibility of scrolling (only
indicates if the touch has moved beyond the slop region). To find
the correct info to pass to Blink, we have to consider the effect of
touch-action on gestures.

In this CL, we push a new event into the touch queue when gestures
passes touch-action filtering.

This CL also adds a hack to prevent unintended reentry to a dispatch
method in TouchEventQueue.

BUG=567740,600773
==========

[mustaq, 2016-04-08 16:46:03]

ptal.

[tdresser, 2016-04-11 13:18:47]

What's the hack you referred to here:
https://bugs.chromium.org/p/chromium/issues/detail?id=600773#c11?

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:320:
std::sort(mappedIds.begin(), mappedIds.end());
On 2016/04/01 19:03:20, tdresser wrote:
> On 2016/04/01 18:31:05, mustaq wrote:
> > On 2016/04/01 14:50:24, tdresser wrote:
> > > Why does this needs a stable ordering?
> > 
> > Removed.
> > 
> > I will bring it back if you don't like the apparent random ordering in the
> > layout test output.
> 
> Hmmm, I'm actually not sure what's correct here. What do other browsers do?

I see you added this back - do others browsers sort this?

[mustaq, 2016-04-11 14:26:47]

On 2016/04/11 13:18:47, tdresser wrote:
> What's the hack you referred to here:
> https://bugs.chromium.org/p/chromium/issues/detail?id=600773#c11?

It's here: touch_event_queue.cc:779

>
https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):
> 
>
https://codereview.chromium.org/1800143002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/events/PointerEventFactory.cpp:320:
> std::sort(mappedIds.begin(), mappedIds.end());
> On 2016/04/01 19:03:20, tdresser wrote:
> > On 2016/04/01 18:31:05, mustaq wrote:
> > > On 2016/04/01 14:50:24, tdresser wrote:
> > > > Why does this needs a stable ordering?
> > > 
> > > Removed.
> > > 
> > > I will bring it back if you don't like the apparent random ordering in the
> > > layout test output.
> > 
> > Hmmm, I'm actually not sure what's correct here. What do other browsers do?
> 
> I see you added this back - do others browsers sort this?

Edge seems to be canceling pointers in pointerId order.

[tdresser, 2016-04-11 14:43:37]

https://codereview.chromium.org/1800143002/diff/240001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/1800143002/diff/240001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.cc:551: // "locally" in
InputRouter.
This should never happen.
Does this still happen now that we bail of of SendTouchEventImmediately?

Why?

https://codereview.chromium.org/1800143002/diff/240001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.cc:779: //
crbug.com(600773): Hack to avoid cyclic reentry to this method.
Let's format the bug as a url.

[mustaq, 2016-04-11 16:03:12]

ptal

https://codereview.chromium.org/1800143002/diff/240001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/1800143002/diff/240001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.cc:551: // "locally" in
InputRouter.
On 2016/04/11 14:43:37, tdresser wrote:
> This should never happen.
> Does this still happen now that we bail of of SendTouchEventImmediately?
> 
> Why?

Yes, this can happen even when we avoid reentry to SendTouchEventImmediately. My
interpretation is that acks are not the only trigger for dispatching, we are
dispatching even when a new event is getting queued.

This is a preexisting bug (crbug.com/600773), could be related to the missing
acks bug (crbug.com/601068) too. I added a ref to the first bug.

https://codereview.chromium.org/1800143002/diff/240001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.cc:779: //
crbug.com(600773): Hack to avoid cyclic reentry to this method.
On 2016/04/11 14:43:37, tdresser wrote:
> Let's format the bug as a url.

Done.

[tdresser, 2016-04-11 18:19:38]

LGTM, but let's make sure to follow up on the touch event queue issues promptly.

https://codereview.chromium.org/1800143002/diff/280001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/1800143002/diff/280001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.cc:505: // the in-flight
event.
Should we DCHECK this here?

[mustaq, 2016-04-11 18:40:41]

https://codereview.chromium.org/1800143002/diff/280001/content/browser/render...
File content/browser/renderer_host/input/touch_event_queue.cc (right):

https://codereview.chromium.org/1800143002/diff/280001/content/browser/render...
content/browser/renderer_host/input/touch_event_queue.cc:505: // the in-flight
event.
On 2016/04/11 18:19:38, tdresser wrote:
> Should we DCHECK this here?

Didn't add a DCHECK here because I am not 100% sure that my comment covers all
the cases. To be precise, can the InputRouterImpl::SendGestureEvent() call be
triggered by some "event" from a WebGestureDeviceTouchscreen other than a touch
ack? RenderWidgetHostImpl::ForwardGestureEventWithLatencyInfo has too many
callers to be certain about it.

[mustaq, 2016-04-11 18:45:08]

mustaq@chromium.org changed reviewers:
+ bokan@chromium.org

[mustaq, 2016-04-11 18:45:09]

rbyers@: Please review changes in Source/core/{input,events}

bokan@: Please review changes in Source/web/

[bokan, 2016-04-11 22:22:00]

Source/web lgtm

I'm a core/ ONWER now so I can take a look at that too (assuming Rick's busy)
but it'll take a me a little longer since I'll need to get a bit acquainted with
PointerEvents.

https://codereview.chromium.org/1800143002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/1800143002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:305: //
TODO(crbug.com/579553): This event follows implicit TE capture. The actual
target
Please keep the TODO format from before: |TODO(name): Description|.

[mustaq, 2016-04-12 17:16:14]

https://codereview.chromium.org/1800143002/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/1800143002/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:305: //
TODO(crbug.com/579553): This event follows implicit TE capture. The actual
target
On 2016/04/11 22:22:00, bokan wrote:
> Please keep the TODO format from before: |TODO(name): Description|.

My interpretation of the Blink style guide entry
(https://www.chromium.org/blink/coding-style) is that our cpp style
(https://google.github.io/styleguide/cppguide.html#TODO_Comments) overrides it.
This is because the Blink style points to the general cpp style, and the recent
FIXME-to-TODO change in documentation seems to be a search/replace w/o much
thought. Agree/disagree?

[bokan, 2016-04-12 17:22:21]

On 2016/04/12 17:16:14, mustaq wrote:
>
https://codereview.chromium.org/1800143002/diff/300001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):
> 
>
https://codereview.chromium.org/1800143002/diff/300001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/input/PointerEventManager.cpp:305: //
> TODO(crbug.com/579553): This event follows implicit TE capture. The actual
> target
> On 2016/04/11 22:22:00, bokan wrote:
> > Please keep the TODO format from before: |TODO(name): Description|.
> 
> My interpretation of the Blink style guide entry
> (https://www.chromium.org/blink/coding-style) is that our cpp style
> (https://google.github.io/styleguide/cppguide.html#TODO_Comments) overrides
it.
> This is because the Blink style points to the general cpp style, and the
recent
> FIXME-to-TODO change in documentation seems to be a search/replace w/o much
> thought. Agree/disagree?

The FIXME-to-TODO change was from FIXME to TODO(name) so the names weren't there
before. The Blink style guide says to use TODO(name) and links to the Google CPP
style (which does allow a few other styles). Generally, we use Google/Chromiuim
style and any Blink rules supersede them so I read this as use TODO(name). More
importantly, I did a code search and found <5 instances of TODO(###) in the
Blink code base so we should use the prevalent style.

[mustaq, 2016-04-12 18:04:26]

On 2016/04/12 17:22:21, bokan wrote:
> On 2016/04/12 17:16:14, mustaq wrote:
> >
>
https://codereview.chromium.org/1800143002/diff/300001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1800143002/diff/300001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/input/PointerEventManager.cpp:305: //
> > TODO(crbug.com/579553): This event follows implicit TE capture. The actual
> > target
> > On 2016/04/11 22:22:00, bokan wrote:
> > > Please keep the TODO format from before: |TODO(name): Description|.
> > 
> > My interpretation of the Blink style guide entry
> > (https://www.chromium.org/blink/coding-style) is that our cpp style
> > (https://google.github.io/styleguide/cppguide.html#TODO_Comments) overrides
> it.
> > This is because the Blink style points to the general cpp style, and the
> recent
> > FIXME-to-TODO change in documentation seems to be a search/replace w/o much
> > thought. Agree/disagree?
> 
> The FIXME-to-TODO change was from FIXME to TODO(name) so the names weren't
there
> before. The Blink style guide says to use TODO(name) and links to the Google
CPP
> style (which does allow a few other styles). Generally, we use
Google/Chromiuim
> style and any Blink rules supersede them so I read this as use TODO(name).
More
> importantly, I did a code search and found <5 instances of TODO(###) in the
> Blink code base so we should use the prevalent style.

Back to the original format since my preferred style is (still) a minority in
Blink: 23 TODO(crbug...) vs 2349 TODO(name) vs 3683 FIXME.
2349 for TODO(name))

[mustaq, 2016-04-12 18:24:54]

@rbyers: need approval for changes in event_sender, WebInputEvent,
PlatformEvent.

[Rick Byers, 2016-04-12 18:41:46]

On 2016/04/12 18:24:54, mustaq wrote:
> @rbyers: need approval for changes in event_sender, WebInputEvent,
> PlatformEvent.

components/test_runner/ and public/ LGTM

And obviously I think the overall design is sound (since we've discussed it lots
with Tim, Dave, etc off-list).

I'll try to get to core/ by EOD but if bokan@ beats me to it then go ahead.

A couple risk areas to consider:
 - Have you looked for code anywhere (eg. plugins) that will be surprised by
having a new type of touch event?  Maybe search for all uses of
WebInputEvent::TouchCancel - eg. are there any switch statements that need a new
case?
 - In particular, does this new TouchScrollStarted event need to get plumbed
down through browser plugin (via a new type of blink::TouchEvent)?  Maybe chat
with wjmaclean.

[mustaq, 2016-04-12 20:19:22]

mustaq@chromium.org changed reviewers:
+ dtrainor@chromium.org

[mustaq, 2016-04-12 20:19:24]

dtrainor@chromium.org: Please review changes in blimp/

[mustaq, 2016-04-12 20:48:06]

On 2016/04/12 18:41:46, Rick Byers wrote:
> On 2016/04/12 18:24:54, mustaq wrote:
> > @rbyers: need approval for changes in event_sender, WebInputEvent,
> > PlatformEvent.
> 
> components/test_runner/ and public/ LGTM
> 
> And obviously I think the overall design is sound (since we've discussed it
lots
> with Tim, Dave, etc off-list).
> 
> I'll try to get to core/ by EOD but if bokan@ beats me to it then go ahead.
> 
> A couple risk areas to consider:
>  - Have you looked for code anywhere (eg. plugins) that will be surprised by
> having a new type of touch event?  Maybe search for all uses of
> WebInputEvent::TouchCancel - eg. are there any switch statements that need a
new
> case?
>  - In particular, does this new TouchScrollStarted event need to get plumbed
> down through browser plugin (via a new type of blink::TouchEvent)?  Maybe chat
> with wjmaclean.

Checked all refs to TouchCancel and added the checks that are needed for the new
event. The other refs are handled correctly by the default case.

After a quick discussion with wjmaclean, it seems plumbing back to plugins is
not
applicable here since Blink EventHandler is the ultimate consumer of the new
event.

[mustaq, 2016-04-12 20:48:31]

The CQ bit was checked by mustaq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-12 20:48:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1800143002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1800143002/340001

[commit-bot: I haz the power, 2016-04-12 20:52:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-12 20:52:18]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bokan, 2016-04-12 22:42:46]

Source/core lgtm too % comments/suggestions/questions

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html
(left):

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html:180:
eventToPreventDefault = "touchmove";
Why do we no longer care about testing preventDefault on touchmove? And what are
the semantics/expectations when preventDefault is called with regard to getting
pointercancel?

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:177:
pointerEventInit.setPointerId(pointerId);
setIdTypeButtons used to add a bunch of data into some internal maps. That's no
longer needed because all of those were keeping track of Mouse/Touch events <->
Pointer event mappings and the cancel is no longer related to one. Is that
correct?

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3716: WebInputEventResult
EventHandler::handleTouchEvent(const PlatformTouchEvent& event)
This method is way too big. Not in this CL, but I'd make this a candidate for
breaking up into more manageable chunks.

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3727: bool
newTouchSequence = true;
Please also update the param name in dispatchTouchEvents

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3913:
m_pointerEventManager.unblockTouchPointers();
Does this have to be way down here or can it be moved up to where we determined
newTouchSequence? As it is, you have to wonder about all the code in between
like the early return for no-touch handlers (which I *think* is fine now, but
future changes could easily get this wrong).

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3924: return
dispatchTouchEvents(event, touchInfos, newTouchSequence, allTouchReleased);
Question: How are touch events canceled and blocked?

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/PageWidgetDelegate.cpp:256:
WEBINPUT_EVENT_CASE(TouchCancel)
You should probably add a conversion for TouchScrollStarted here.

[Navid Zolghadr, 2016-04-13 14:33:05]

nzolghadr@chromium.org changed reviewers:
+ nzolghadr@chromium.org

[Navid Zolghadr, 2016-04-13 14:33:07]

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3727: bool
newTouchSequence = true;
On 2016/04/12 22:42:46, bokan wrote:
> Please also update the param name in dispatchTouchEvents

I couldn't find any use for this variable in that function. I was going to
remove it altogether in my next change after this change.

[mustaq, 2016-04-13 14:56:27]

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html
(left):

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html:180:
eventToPreventDefault = "touchmove";
On 2016/04/12 22:42:45, bokan wrote:
> Why do we no longer care about testing preventDefault on touchmove? And what
are
> the semantics/expectations when preventDefault is called with regard to
getting
> pointercancel?

Quoting below the comment I made in patch#5 of this file. I was about to add the
comment in the html but then realized it won't make sense after the change. Let
me know if you think otherwise.

> Earlier, we used to decide about pointercancels from within EventHandler, so
it
> was important to check that after canceling a touchmove (implying no
scrolling)
> no pointercancel is fired.
> 
> In this CL, the pointercancel decision comes directly from the scrolling
> decision in the input-router (which knows about canceled touchmoves both
> before/after this CL). Since notifyStartOfTouchScroll() simulates the
scrolling
> decision at input-router, preventing touchmoves through eventSender has no
> effect.

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:177:
pointerEventInit.setPointerId(pointerId);
On 2016/04/12 22:42:45, bokan wrote:
> setIdTypeButtons used to add a bunch of data into some internal maps. That's
no
> longer needed because all of those were keeping track of Mouse/Touch events
<->
> Pointer event mappings and the cancel is no longer related to one. Is that
> correct?

You are right that we don't need those mappings: the mapping store pointer
specific states (JS-visible pointerId, incoming raw id, buttons state etc). Here
we have the |pointerId| specified directly, so no need to consult the maps. The
state |buttons| is irrelevant for pointercancel.

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3716: WebInputEventResult
EventHandler::handleTouchEvent(const PlatformTouchEvent& event)
On 2016/04/12 22:42:46, bokan wrote:
> This method is way too big. Not in this CL, but I'd make this a candidate for
> breaking up into more manageable chunks.

Agree, too big is too unhealthy/unreadable. Navid is currently looking at
releasing implicit touch capture through releasePointerCapture, which would put
the PE handler ahead of the TE handler, and would significantly shorten this
method.

I will start working on making the EventHandler modular (crbug.com/449649) this
quarter.

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3727: bool
newTouchSequence = true;
On 2016/04/13 14:33:06, Navid Zolghadr wrote:
> On 2016/04/12 22:42:46, bokan wrote:
> > Please also update the param name in dispatchTouchEvents
> 
> I couldn't find any use for this variable in that function. I was going to
> remove it altogether in my next change after this change.

Removed.

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3913:
m_pointerEventManager.unblockTouchPointers();
On 2016/04/12 22:42:46, bokan wrote:
> Does this have to be way down here or can it be moved up to where we
determined
> newTouchSequence? As it is, you have to wonder about all the code in between
> like the early return for no-touch handlers (which I *think* is fine now, but
> future changes could easily get this wrong). 

I kept it here because the code above is fairly unrelated to PEs. But
future-proofing seems to be a stronger argument. Done.

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3924: return
dispatchTouchEvents(event, touchInfos, newTouchSequence, allTouchReleased);
On 2016/04/12 22:42:46, bokan wrote:
> Question: How are touch events canceled and blocked?

Any PE that was canceled above effectively "hides" the corresponding touch point
in the TE, checked thru the |consumed| bits in touchInfos. I guess this is what
you meant by "blocking".

Not sure what you meant by "canceled" TEs. |dispatchTouchEvent| convey the
outcome upstream whenever a TE is canceled from JS. It doesn't affect PEs
directly. But it can switch the browser/GR away from the throttled touchmoves
mode, thus affect generation of future touchmoves for the same touch sequence
and hence the generation of future PEs. Does this answer your 2nd query?

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/PageWidgetDelegate.cpp:256:
WEBINPUT_EVENT_CASE(TouchCancel)
On 2016/04/12 22:42:46, bokan wrote:
> You should probably add a conversion for TouchScrollStarted here.

Done.

[mustaq, 2016-04-13 15:15:01]

The CQ bit was checked by mustaq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-13 15:15:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1800143002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1800143002/380001

[commit-bot: I haz the power, 2016-04-13 16:00:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-13 16:00:54]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[mustaq, 2016-04-13 20:06:37]

The CQ bit was checked by mustaq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-13 20:07:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1800143002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1800143002/400001

[commit-bot: I haz the power, 2016-04-13 20:20:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-13 20:20:47]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[bokan, 2016-04-13 22:59:48]

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html
(left):

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/events/pointerevents/touch-pointercancel.html:180:
eventToPreventDefault = "touchmove";
On 2016/04/13 14:56:27, mustaq wrote:
> On 2016/04/12 22:42:45, bokan wrote:
> > Why do we no longer care about testing preventDefault on touchmove? And what
> are
> > the semantics/expectations when preventDefault is called with regard to
> getting
> > pointercancel?
> 
> Quoting below the comment I made in patch#5 of this file. I was about to add
the
> comment in the html but then realized it won't make sense after the change.
Let
> me know if you think otherwise.
> 
> > Earlier, we used to decide about pointercancels from within EventHandler, so
> it
> > was important to check that after canceling a touchmove (implying no
> scrolling)
> > no pointercancel is fired.
> > 
> > In this CL, the pointercancel decision comes directly from the scrolling
> > decision in the input-router (which knows about canceled touchmoves both
> > before/after this CL). Since notifyStartOfTouchScroll() simulates the
> scrolling
> > decision at input-router, preventing touchmoves through eventSender has no
> > effect.

Ah, ok, it was testing an implementation detail that's gone now. This is fine.

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:177:
pointerEventInit.setPointerId(pointerId);
On 2016/04/13 14:56:27, mustaq wrote:
> On 2016/04/12 22:42:45, bokan wrote:
> > setIdTypeButtons used to add a bunch of data into some internal maps. That's
> no
> > longer needed because all of those were keeping track of Mouse/Touch events
> <->
> > Pointer event mappings and the cancel is no longer related to one. Is that
> > correct?
> 
> You are right that we don't need those mappings: the mapping store pointer
> specific states (JS-visible pointerId, incoming raw id, buttons state etc).
Here
> we have the |pointerId| specified directly, so no need to consult the maps.
The
> state |buttons| is irrelevant for pointercancel.

Acknowledged.

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3716: WebInputEventResult
EventHandler::handleTouchEvent(const PlatformTouchEvent& event)
On 2016/04/13 14:56:27, mustaq wrote:
> On 2016/04/12 22:42:46, bokan wrote:
> > This method is way too big. Not in this CL, but I'd make this a candidate
for
> > breaking up into more manageable chunks.
> 
> Agree, too big is too unhealthy/unreadable. Navid is currently looking at
> releasing implicit touch capture through releasePointerCapture, which would
put
> the PE handler ahead of the TE handler, and would significantly shorten this
> method.
> 
> I will start working on making the EventHandler modular (crbug.com/449649)
this
> quarter.

Awesome, thanks!

https://codereview.chromium.org/1800143002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:3924: return
dispatchTouchEvents(event, touchInfos, newTouchSequence, allTouchReleased);
On 2016/04/13 14:56:27, mustaq wrote:
> On 2016/04/12 22:42:46, bokan wrote:
> > Question: How are touch events canceled and blocked?
> 
> Any PE that was canceled above effectively "hides" the corresponding touch
point
> in the TE, checked thru the |consumed| bits in touchInfos. I guess this is
what
> you meant by "blocking".
Got it, thanks.

> 
> Not sure what you meant by "canceled" TEs. |dispatchTouchEvent| convey the
> outcome upstream whenever a TE is canceled from JS. It doesn't affect PEs
> directly. But it can switch the browser/GR away from the throttled touchmoves
> mode, thus affect generation of future touchmoves for the same touch sequence
> and hence the generation of future PEs. Does this answer your 2nd query?

Yah, my canceled question made no sense here :)

[David Trainor- moved to gerrit, 2016-04-15 16:05:00]

blimp/ lgtm

[mustaq, 2016-04-18 20:51:24]

@tdresser: Fixed the test failures triggered by the TouchScrollStarted event
after GSB.

[mustaq, 2016-04-18 20:51:36]

The CQ bit was checked by mustaq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-18 20:52:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1800143002/420001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1800143002/420001

[commit-bot: I haz the power, 2016-04-18 21:02:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-18 21:02:36]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mustaq, 2016-04-19 15:22:29]

The CQ bit was checked by mustaq@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-19 15:22:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1800143002/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1800143002/440001

[mustaq, 2016-04-19 17:15:21]

The CQ bit was checked by mustaq@chromium.org

[mustaq, 2016-04-19 17:15:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org, rbyers@chromium.org, bokan@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1800143002/#ps460001
(title: "Rebased")

[commit-bot: I haz the power, 2016-04-19 17:15:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1800143002/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1800143002/460001

[mustaq, 2016-04-19 17:19:47]

On 2016/04/18 20:51:24, mustaq wrote:
> @tdresser: Fixed the test failures triggered by the TouchScrollStarted event
> after GSB.

All tests are passing now, yayy!

[commit-bot: I haz the power, 2016-04-19 18:49:18]

Description was changed from

==========
Notify Blink about start of gesture scroll through a queued event.

Added WebIputEventL::TouchScrollStarted event to notify Blink when a
gesture scroll is starting.

We need to know from Blink when gesture scroll is about to start.
So far, the only use of this info is firing a pointercancel event
during scrolling. We had been using a bit in WebTouchEvent before,
which didn't really reflect the possibility of scrolling (only
indicates if the touch has moved beyond the slop region). To find
the correct info to pass to Blink, we have to consider the effect of
touch-action on gestures.

In this CL, we push a new event into the touch queue when gestures
passes touch-action filtering.

This CL also adds a hack to prevent unintended reentry to a dispatch
method in TouchEventQueue.

BUG=567740,600773
==========

to

==========
Notify Blink about start of gesture scroll through a queued event.

Added WebIputEventL::TouchScrollStarted event to notify Blink when a
gesture scroll is starting.

We need to know from Blink when gesture scroll is about to start.
So far, the only use of this info is firing a pointercancel event
during scrolling. We had been using a bit in WebTouchEvent before,
which didn't really reflect the possibility of scrolling (only
indicates if the touch has moved beyond the slop region). To find
the correct info to pass to Blink, we have to consider the effect of
touch-action on gestures.

In this CL, we push a new event into the touch queue when gestures
passes touch-action filtering.

This CL also adds a hack to prevent unintended reentry to a dispatch
method in TouchEventQueue.

BUG=567740,600773
==========

[commit-bot: I haz the power, 2016-04-19 18:49:20]

Committed patchset #22 (id:460001)

[commit-bot: I haz the power, 2016-04-22 19:14:19]

Description was changed from

==========
Notify Blink about start of gesture scroll through a queued event.

Added WebIputEventL::TouchScrollStarted event to notify Blink when a
gesture scroll is starting.

We need to know from Blink when gesture scroll is about to start.
So far, the only use of this info is firing a pointercancel event
during scrolling. We had been using a bit in WebTouchEvent before,
which didn't really reflect the possibility of scrolling (only
indicates if the touch has moved beyond the slop region). To find
the correct info to pass to Blink, we have to consider the effect of
touch-action on gestures.

In this CL, we push a new event into the touch queue when gestures
passes touch-action filtering.

This CL also adds a hack to prevent unintended reentry to a dispatch
method in TouchEventQueue.

BUG=567740,600773
==========

to

==========
Notify Blink about start of gesture scroll through a queued event.

Added WebIputEventL::TouchScrollStarted event to notify Blink when a
gesture scroll is starting.

We need to know from Blink when gesture scroll is about to start.
So far, the only use of this info is firing a pointercancel event
during scrolling. We had been using a bit in WebTouchEvent before,
which didn't really reflect the possibility of scrolling (only
indicates if the touch has moved beyond the slop region). To find
the correct info to pass to Blink, we have to consider the effect of
touch-action on gestures.

In this CL, we push a new event into the touch queue when gestures
passes touch-action filtering.

This CL also adds a hack to prevent unintended reentry to a dispatch
method in TouchEventQueue.

BUG=567740,600773

Committed: https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b
Cr-Commit-Position: refs/heads/master@{#388258}
==========

[commit-bot: I haz the power, 2016-04-22 19:14:21]

Patchset 22 (id:??) landed as
https://crrev.com/b3c44a0beb7e5a56fc78861c22b588380de3f49b
Cr-Commit-Position: refs/heads/master@{#388258}