Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
997283002:
Coalesce async touch move events until the ack back from render (Closed)
|
Created:
5 years, 9 months ago by lanwei Modified:
5 years, 7 months ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, jdduke+watch_chromium.org, jam, mlamouri+watch-content_chromium.org, Rick Byers Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCoalesce async touch move events until the ack back from render.
In order to avoid the touch event queue keep growing when render handles
async touchmove events longer than 200ms, which is the async touchmove
interval, we keep coalesce touch moves and send the next touchmove
until passing the async touchmove interval and receive the ACK from render.
BUG=448760
Committed: https://crrev.com/f1a2283b2d64a09d0d0a6a4468eccf30805b069d
Cr-Commit-Position: refs/heads/master@{#330548}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add an ack type for async touch move #
Total comments: 16
Patch Set 3 : rename variable #Patch Set 4 : update count #Patch Set 5 : Unit tests #
Total comments: 22
Patch Set 6 : Made changes based on comments #
Total comments: 21
Patch Set 7 : #
Total comments: 19
Patch Set 8 : #
Total comments: 11
Patch Set 9 : #
Total comments: 12
Patch Set 10 : #
Total comments: 22
Patch Set 11 : #Patch Set 12 : Use queue to store the IDs for sent async touchmove #
Total comments: 6
Patch Set 13 : Ack struct and unittest #
Total comments: 37
Patch Set 14 : Fixing unittests #
Total comments: 16
Patch Set 15 : #
Total comments: 17
Patch Set 16 : Change the unittests #
Total comments: 1
Patch Set 17 : Modify unittest #
Total comments: 4
Patch Set 18 : Format input_message #Messages
Total messages: 61 (19 generated)
lanwei@chromium.org changed reviewers: + jdduke@chromium.org, rbyers@chromium.org, tdresser@chromium.org
Thanks Lan! https://codereview.chromium.org/997283002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/997283002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/input_router_impl.cc:428: if (ack.type == WebInputEvent::TouchMove && This seems like it could be racy. When a scroll sequence transitions from being consumed to unconsumed (changing the value of IsSendingTouchEventsAsync()), it seems like we could incorrectly mark a touchmove ack as sync/async when that may not have been the case? https://codereview.chromium.org/997283002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/input_router_impl.cc:430: touch_event_queue_.ReceiveAsyncTouchMoveAck(); I'm a little concerned about the special interaction here between the TouchEventQueue and the InputRouter. I wonder if there's any way we could achieve the same result without changing the InputRouter at all? This would probably mean that, whenever we forward the async event, we put it in a special async queue separate from the regular queue (in the TouchEventQueue). That way, it doesn't block the regular queue, and we can use the async queue to filter out acks as they arrive. The downside there is that we'll need a way to map between acks and touches, as it's possible to get out-of-order acks. Sadly, I think we may need something like that anyway if we're to achieve nonblocking behavior.
Patchset #2 (id:20001) has been deleted
Could you please take a first look at this, meanwhile I am fixing and writing the unittests? Thank you.
This is preliminary, I haven't taken a particularly close look yet. https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... File content/browser/renderer_host/input/input_router_impl.h (right): https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/input_router_impl.h:22: struct InputHostMsg_HandleAsyncTouchEvent_ACK_Params; This should be HandleUncancelableEvent_ACK_Params, as this won't be sent for all async touch events. https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:537: // Once we flish the pending async touch move, we ignore all the acks flish -> flush https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:538: // from the aysnc touch moves that we sent out and wait for acks back. Fix aysnc spelling. https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:539: ignore_ack_ = ++sent_async_touch_move_count_; Split into two statements. https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:69: void ProcessAsyncTouchAck(); This should probably be ProcessUncancelableTouchAck. https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:208: bool receive_async_touch_move_ack_; Can you clarify this comment? When wouldn't we receive an ack for an async touch move event? Also, do you mean async, or uncancelable? https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:213: int ignore_ack_; The variable name here should indicate that this is a number. Perhaps: pending_uncancelable_event_ack_count_, or something along those lines. I'm not clear on the difference between this and |sent_async_touch_move_count_|. https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:215: // Count the number of aysnc touch moves sent out are waiting for their ack Fix spelling "aysnc"
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... File content/browser/renderer_host/input/input_router_impl.h (right): https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/input_router_impl.h:22: struct InputHostMsg_HandleAsyncTouchEvent_ACK_Params; On 2015/03/23 19:21:44, tdresser wrote: > This should be HandleUncancelableEvent_ACK_Params, as this won't be sent for all > async touch events. Done. https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:537: // Once we flish the pending async touch move, we ignore all the acks On 2015/03/23 19:21:44, tdresser wrote: > flish -> flush Done. https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:538: // from the aysnc touch moves that we sent out and wait for acks back. On 2015/03/23 19:21:44, tdresser wrote: > Fix aysnc spelling. Done. https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:539: ignore_ack_ = ++sent_async_touch_move_count_; On 2015/03/23 19:21:44, tdresser wrote: > Split into two statements. Done. https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:69: void ProcessAsyncTouchAck(); On 2015/03/23 19:21:44, tdresser wrote: > This should probably be ProcessUncancelableTouchAck. Done. https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:208: bool receive_async_touch_move_ack_; On 2015/03/23 19:21:44, tdresser wrote: > Can you clarify this comment? When wouldn't we receive an ack for an async touch > move event? Also, do you mean async, or uncancelable? Done. https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:213: int ignore_ack_; On 2015/03/23 19:21:44, tdresser wrote: > The variable name here should indicate that this is a number. > > Perhaps: pending_uncancelable_event_ack_count_, or something along those lines. > > I'm not clear on the difference between this and |sent_async_touch_move_count_|. Done. https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:215: // Count the number of aysnc touch moves sent out are waiting for their ack On 2015/03/23 19:21:44, tdresser wrote: > Fix spelling "aysnc" Done.
rbyers@chromium.org changed reviewers: - rbyers@chromium.org
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:100001) has been deleted
On 2015/03/24 18:46:38, lanwei wrote: > https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... > File content/browser/renderer_host/input/input_router_impl.h (right): > > https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... > content/browser/renderer_host/input/input_router_impl.h:22: struct > InputHostMsg_HandleAsyncTouchEvent_ACK_Params; > On 2015/03/23 19:21:44, tdresser wrote: > > This should be HandleUncancelableEvent_ACK_Params, as this won't be sent for > all > > async touch events. > > Done. > > https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... > File content/browser/renderer_host/input/touch_event_queue.cc (right): > > https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.cc:537: // Once we flish > the pending async touch move, we ignore all the acks > On 2015/03/23 19:21:44, tdresser wrote: > > flish -> flush > > Done. > > https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.cc:538: // from the aysnc > touch moves that we sent out and wait for acks back. > On 2015/03/23 19:21:44, tdresser wrote: > > Fix aysnc spelling. > > Done. > > https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.cc:539: ignore_ack_ = > ++sent_async_touch_move_count_; > On 2015/03/23 19:21:44, tdresser wrote: > > Split into two statements. > > Done. > > https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... > File content/browser/renderer_host/input/touch_event_queue.h (right): > > https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.h:69: void > ProcessAsyncTouchAck(); > On 2015/03/23 19:21:44, tdresser wrote: > > This should probably be ProcessUncancelableTouchAck. > > Done. > > https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.h:208: bool > receive_async_touch_move_ack_; > On 2015/03/23 19:21:44, tdresser wrote: > > Can you clarify this comment? When wouldn't we receive an ack for an async > touch > > move event? Also, do you mean async, or uncancelable? > > Done. > > https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.h:213: int ignore_ack_; > On 2015/03/23 19:21:44, tdresser wrote: > > The variable name here should indicate that this is a number. > > > > Perhaps: pending_uncancelable_event_ack_count_, or something along those > lines. > > > > I'm not clear on the difference between this and > |sent_async_touch_move_count_|. > > Done. > > https://codereview.chromium.org/997283002/diff/40001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.h:215: // Count the number > of aysnc touch moves sent out are waiting for their ack > On 2015/03/23 19:21:44, tdresser wrote: > > Fix spelling "aysnc" > > Done. I'd prefer to wait to do a more thorough review until you've got the unit tests up.
https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:391: TRACE_EVENT0("input", "TouchEventQueue::QueueEvent"); Might as well leave that whitespace there. https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:433: touchmove_slop_suppressor_->ConfirmTouchEvent(ack_result); Might as well leave that whitespace there. https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:226: int sent_uncancelable_touch_move_count_; I'm still not clear on why we need both pending_uncancelable_event_ack_count_ and sent_uncancelable_touch_move_count_. Let's talk it over in person. https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:1987: TEST_F(TouchEventQueueTest, SendNextThrottledAsyncTouchMoveAfterAck) { Give a brief summary of what scenario the test covers. https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2011: // Now queue a second touchmove and verify it's not (yet) dispatched. Clarify that it's not yet dispatched because we've now started scrolling, so we throttle the touchmoves. https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2016: EXPECT_EQ(1U, GetAndResetAckedEventCount()); Add a comment indicating that this is a fake ack. https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2018: // Coalease the subsequent touchmove with pending_async_touch_move_ and not "and not ready for dispatching" -> "but don't dispatch it yet" https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2031: EXPECT_EQ(1U, queued_event_count()); Why is there still something in the queue? Shouldn't it be empty? https://codereview.chromium.org/997283002/diff/140001/content/common/input/we... File content/common/input/web_input_event_traits.cc (right): https://codereview.chromium.org/997283002/diff/140001/content/common/input/we... content/common/input/web_input_event_traits.cc:465: // touch move, we will also send a fake ACK. I'd move this comment down to where we deal with TouchStart and TouchEnd events. https://codereview.chromium.org/997283002/diff/140001/content/common/input_me... File content/common/input_messages.h (right): https://codereview.chromium.org/997283002/diff/140001/content/common/input_me... content/common/input_messages.h:118: IPC_STRUCT_MEMBER(blink::WebInputEvent::Type, type) If the name includes "TouchMove" we don't need to pass a WebInputEvent::Type. https://codereview.chromium.org/997283002/diff/140001/content/renderer/input/... File content/renderer/input/input_event_filter.cc (right): https://codereview.chromium.org/997283002/diff/140001/content/renderer/input/... content/renderer/input/input_event_filter.cc:185: if (touch.type == WebInputEvent::TouchMove && !touch.cancelable) { We don't need the outer "if", do we? https://codereview.chromium.org/997283002/diff/140001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/997283002/diff/140001/content/renderer/render... content/renderer/render_widget.cc:1235: if (touch.type == WebInputEvent::TouchMove && !touch.cancelable) Can't we just: if(touch.type == WebInputEvent::TouchMove && !touch.cancelable) {...}
Patchset #6 (id:160001) has been deleted
https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:391: TRACE_EVENT0("input", "TouchEventQueue::QueueEvent"); On 2015/03/25 14:16:18, tdresser wrote: > Might as well leave that whitespace there. Done. https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:433: touchmove_slop_suppressor_->ConfirmTouchEvent(ack_result); On 2015/03/25 14:16:18, tdresser wrote: > Might as well leave that whitespace there. Done. https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:1987: TEST_F(TouchEventQueueTest, SendNextThrottledAsyncTouchMoveAfterAck) { On 2015/03/25 14:16:18, tdresser wrote: > Give a brief summary of what scenario the test covers. Done. https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2011: // Now queue a second touchmove and verify it's not (yet) dispatched. On 2015/03/25 14:16:18, tdresser wrote: > Clarify that it's not yet dispatched because we've now started scrolling, so we > throttle the touchmoves. Done. https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2018: // Coalease the subsequent touchmove with pending_async_touch_move_ and not On 2015/03/25 14:16:18, tdresser wrote: > "and not ready for dispatching" -> "but don't dispatch it yet" Done. https://codereview.chromium.org/997283002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2031: EXPECT_EQ(1U, queued_event_count()); On 2015/03/25 14:16:18, tdresser wrote: > Why is there still something in the queue? Shouldn't it be empty? When the touch move is sent but the fake ack is not sent, the touch move is still in the queue. https://codereview.chromium.org/997283002/diff/140001/content/common/input/we... File content/common/input/web_input_event_traits.cc (right): https://codereview.chromium.org/997283002/diff/140001/content/common/input/we... content/common/input/web_input_event_traits.cc:465: // touch move, we will also send a fake ACK. On 2015/03/25 14:16:18, tdresser wrote: > I'd move this comment down to where we deal with TouchStart and TouchEnd events. Done. https://codereview.chromium.org/997283002/diff/140001/content/common/input_me... File content/common/input_messages.h (right): https://codereview.chromium.org/997283002/diff/140001/content/common/input_me... content/common/input_messages.h:118: IPC_STRUCT_MEMBER(blink::WebInputEvent::Type, type) On 2015/03/25 14:16:18, tdresser wrote: > If the name includes "TouchMove" we don't need to pass a WebInputEvent::Type. Done. https://codereview.chromium.org/997283002/diff/140001/content/renderer/input/... File content/renderer/input/input_event_filter.cc (right): https://codereview.chromium.org/997283002/diff/140001/content/renderer/input/... content/renderer/input/input_event_filter.cc:185: if (touch.type == WebInputEvent::TouchMove && !touch.cancelable) { On 2015/03/25 14:16:18, tdresser wrote: > We don't need the outer "if", do we? Done. https://codereview.chromium.org/997283002/diff/140001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/997283002/diff/140001/content/renderer/render... content/renderer/render_widget.cc:1235: if (touch.type == WebInputEvent::TouchMove && !touch.cancelable) On 2015/03/25 14:16:18, tdresser wrote: > Can't we just: > if(touch.type == WebInputEvent::TouchMove && !touch.cancelable) {...} We need to cast to WebTouchEvent.
https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:362: // If we don't care about the ack disposition, or it is an async touchmove If we identify that uncancelable touchmoves ignore their ack again (in web_input_event_traits.h), then we don't need this special case. https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:617: void InputRouterImpl::ProcessUncancelableTouchMoveAck() { Do we need a separate method for this? https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (left): https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:436: Put back whitespace. https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:468: Put back whitespace. https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:452: if (pending_async_touchmove_) { Shouldn't we only dispatch the pending async touchmove if sent_uncancelable_touch_move_count_ is 0? I would expect a test of how flushing works to fail with this logic. https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:536: ++sent_uncancelable_touch_move_count_; Could we increase the count inside SendTouchEventImmediately? It would be nice if we only had to increment it in one place. https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:213: // ack from render, which we use to decide when to send the next touch move. when to send the next async touch move move. https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:218: int sent_uncancelable_touch_move_count_; How about uncancelable_touch_moves_pending_ack_count_. It's a bit long, but it's clearer than "sent". https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2197: // Send touchstart will flush pending_async_touch_move_, and increse the increse -> increase https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2206: EXPECT_EQ(i+2, sent_uncancelable_touch_move_count()); Nice test, thanks! https://codereview.chromium.org/997283002/diff/180001/content/common/input/we... File content/common/input/web_input_event_traits.cc (right): https://codereview.chromium.org/997283002/diff/180001/content/common/input/we... content/common/input/web_input_event_traits.cc:482: // for uncancelable touch move, we will also send a fake ACK. Hmmm, this is a bit confusing. (Sorry I didn't catch this earlier) We do ignore the disposition of an uncancelable touchmove, but we still care about when the ack is sent. Based on the name of this method, I'd expect TouchMove to be in this list. Can we put it back?
https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:362: // If we don't care about the ack disposition, or it is an async touchmove On 2015/03/26 15:25:28, tdresser wrote: > If we identify that uncancelable touchmoves ignore their ack again (in > web_input_event_traits.h), then we don't need this special case. Done. https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:617: void InputRouterImpl::ProcessUncancelableTouchMoveAck() { On 2015/03/26 15:25:28, tdresser wrote: > Do we need a separate method for this? Done. https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (left): https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:436: On 2015/03/26 15:25:28, tdresser wrote: > Put back whitespace. Done. https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:468: On 2015/03/26 15:25:28, tdresser wrote: > Put back whitespace. Done. https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:452: if (pending_async_touchmove_) { On 2015/03/26 15:25:28, tdresser wrote: > Shouldn't we only dispatch the pending async touchmove if > sent_uncancelable_touch_move_count_ is 0? I would expect a test of how flushing > works to fail with this logic. Done. https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:536: ++sent_uncancelable_touch_move_count_; On 2015/03/26 15:25:28, tdresser wrote: > Could we increase the count inside SendTouchEventImmediately? It would be nice > if we only had to increment it in one place. Done. https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:213: // ack from render, which we use to decide when to send the next touch move. On 2015/03/26 15:25:29, tdresser wrote: > when to send the next async touch move move. Done. https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:218: int sent_uncancelable_touch_move_count_; On 2015/03/26 15:25:29, tdresser wrote: > How about uncancelable_touch_moves_pending_ack_count_. > > It's a bit long, but it's clearer than "sent". Done. https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/997283002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2197: // Send touchstart will flush pending_async_touch_move_, and increse the On 2015/03/26 15:25:29, tdresser wrote: > increse -> increase Done. https://codereview.chromium.org/997283002/diff/180001/content/common/input/we... File content/common/input/web_input_event_traits.cc (right): https://codereview.chromium.org/997283002/diff/180001/content/common/input/we... content/common/input/web_input_event_traits.cc:482: // for uncancelable touch move, we will also send a fake ACK. On 2015/03/26 15:25:29, tdresser wrote: > Hmmm, this is a bit confusing. (Sorry I didn't catch this earlier) > > We do ignore the disposition of an uncancelable touchmove, but we still care > about when the ack is sent. > > Based on the name of this method, I'd expect TouchMove to be in this list. Can > we put it back? Done.
https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:446: Source<void>(this), Details<int>(&type)); As far as I can tell, this notification is never used. The comments state that its only used in testing, but I don't see any references to it in any tests. I think that you can safely get rid of the notification here. Perhaps it would be worth trying to get rid of NOTIFICATION_RENDER_WIDGET_HOST_DID_RECEIVE_INPUT_EVENT_ACK entirely in a follow up patch. https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:452: // Send the next pending async touch move once we receive the ack. What if the timer is still running? https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:1679: MoveTouchPoint(0, 10, 25); Why the change in location? https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2017: EXPECT_EQ(1U, GetAndResetAckedEventCount()); Add a comment that this was a fake ack. It's probably worth clarifying this for the first fake ack per test. https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2027: Add a comment describing what happens below. https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2028: AdvanceTouchTime(kMinSecondsBetweenThrottledTouchmoves + 0.1); It might be clearer what's happening if we had a set of assertions between advancing the time and moving the pointer. https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2037: // We receive the fake ack for touchmove and clear the queue. Which touchmove is this the fake ack for? The fake acks will always be dispatched synchronously, so we won't be able to get: TM1 - TM2 - sync ack for TM1
https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:446: Source<void>(this), Details<int>(&type)); On 2015/03/27 13:34:44, tdresser wrote: > As far as I can tell, this notification is never used. > > The comments state that its only used in testing, but I don't see any references > to it in any tests. > > I think that you can safely get rid of the notification here. Perhaps it would > be worth trying to get rid of > NOTIFICATION_RENDER_WIDGET_HOST_DID_RECEIVE_INPUT_EVENT_ACK entirely in a follow > up patch. Yup, agreed. https://codereview.chromium.org/997283002/diff/200001/content/common/input_me... File content/common/input_messages.h (right): https://codereview.chromium.org/997283002/diff/200001/content/common/input_me... content/common/input_messages.h:247: IPC_MESSAGE_ROUTED1(InputHostMsg_HandleUncancelableTouchMoveEvent_ACK, Can we just make this IPC_MESSAGE_ROUTED0(InputHostMsg_HandleUncancelableTouchMoveEvent_ACK); https://codereview.chromium.org/997283002/diff/200001/content/renderer/input/... File content/renderer/input/input_event_filter.cc (right): https://codereview.chromium.org/997283002/diff/200001/content/renderer/input/... content/renderer/input/input_event_filter.cc:181: const WebTouchEvent& touch = static_cast<const WebTouchEvent&>(*event); It sounds like we should consolidate this logic. What if we have a scoped_ptr<IPC::Message> WebInputEventTraits::CreateAckIfNecessary(event) method which does the right thing? Then both InputEventFilter and RenderWidget can just call that.
https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:446: Source<void>(this), Details<int>(&type)); On 2015/03/27 13:34:44, tdresser wrote: > As far as I can tell, this notification is never used. > > The comments state that its only used in testing, but I don't see any references > to it in any tests. > > I think that you can safely get rid of the notification here. Perhaps it would > be worth trying to get rid of > NOTIFICATION_RENDER_WIDGET_HOST_DID_RECEIVE_INPUT_EVENT_ACK entirely in a follow > up patch. Done. https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:452: // Send the next pending async touch move once we receive the ack. On 2015/03/27 13:34:44, tdresser wrote: > What if the timer is still running? Done. https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:1679: MoveTouchPoint(0, 10, 25); On 2015/03/27 13:34:44, tdresser wrote: > Why the change in location? Done. https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2017: EXPECT_EQ(1U, GetAndResetAckedEventCount()); On 2015/03/27 13:34:44, tdresser wrote: > Add a comment that this was a fake ack. It's probably worth clarifying this for > the first fake ack per test. Done. https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2027: On 2015/03/27 13:34:44, tdresser wrote: > Add a comment describing what happens below. Done. https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2028: AdvanceTouchTime(kMinSecondsBetweenThrottledTouchmoves + 0.1); On 2015/03/27 13:34:44, tdresser wrote: > It might be clearer what's happening if we had a set of assertions between > advancing the time and moving the pointer. Done. https://codereview.chromium.org/997283002/diff/200001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2028: AdvanceTouchTime(kMinSecondsBetweenThrottledTouchmoves + 0.1); On 2015/03/27 13:34:44, tdresser wrote: > It might be clearer what's happening if we had a set of assertions between > advancing the time and moving the pointer. Done. https://codereview.chromium.org/997283002/diff/200001/content/common/input_me... File content/common/input_messages.h (right): https://codereview.chromium.org/997283002/diff/200001/content/common/input_me... content/common/input_messages.h:247: IPC_MESSAGE_ROUTED1(InputHostMsg_HandleUncancelableTouchMoveEvent_ACK, On 2015/03/30 18:04:15, jdduke wrote: > Can we just make this > > IPC_MESSAGE_ROUTED0(InputHostMsg_HandleUncancelableTouchMoveEvent_ACK); Done. https://codereview.chromium.org/997283002/diff/200001/content/renderer/input/... File content/renderer/input/input_event_filter.cc (right): https://codereview.chromium.org/997283002/diff/200001/content/renderer/input/... content/renderer/input/input_event_filter.cc:181: const WebTouchEvent& touch = static_cast<const WebTouchEvent&>(*event); On 2015/03/30 18:04:15, jdduke wrote: > It sounds like we should consolidate this logic. > > What if we have a > > scoped_ptr<IPC::Message> WebInputEventTraits::CreateAckIfNecessary(event) > > method which does the right thing? Then both InputEventFilter and RenderWidget > can just call that. Done.
https://codereview.chromium.org/997283002/diff/220001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/220001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:460: touch.event.cancelable = !send_touch_events_async_; send_touch_events_async_ should never be false here, should it? It would be strange to send the pending_async_touchmove synchronously. https://codereview.chromium.org/997283002/diff/220001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/997283002/diff/220001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2011: // Now queue a second touchmove and verify it's not (yet) dispatched, because nit: a second -> another It's not actually the second touchmove. https://codereview.chromium.org/997283002/diff/220001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2022: // dispatch it yet. I think we can make this test more focused, and move some of the logic from this test into another test. Testing that coalescing works should probably go in a separate test. This test is supposed to verify that we wait for an async ack before dispatching the pending async touch move. Everything other than testing that should be removed if possible. I'd set the timestamp of the first async touchmove to after the throttle limit, so the logic is something like: - touchstart - touchmove (scrollbegin) - touchmove (scrollupdate) - async touchmove (dispatched) - async touchmove (passed throttle timeout) - ack first async touchmove I think you can get rid of two events, which will make the test easier to read. https://codereview.chromium.org/997283002/diff/220001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2031: AdvanceTouchTime(kMinSecondsBetweenThrottledTouchmoves + 0.1); Oops, I completely misinterpreted what this method does. The expectations which I got you to add below are completely unnecessary. Sorry! I was thinking that this method advanced the global clock, not the timestamp of future touch events. https://codereview.chromium.org/997283002/diff/220001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2099: MoveTouchPoint(0, 0, 20); Same as above: I think you can get rid of this touch move and the next. https://codereview.chromium.org/997283002/diff/220001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2177: MoveTouchPoint(0, 0, 20); We should be able to get rid of this touchmove. https://codereview.chromium.org/997283002/diff/220001/content/common/input/we... File content/common/input/web_input_event_traits.cc (right): https://codereview.chromium.org/997283002/diff/220001/content/common/input/we... content/common/input/web_input_event_traits.cc:507: if (send_ack) { Could we end up overwriting response here? We should at least: DCHECK(!response);
https://codereview.chromium.org/997283002/diff/220001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/220001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:460: touch.event.cancelable = !send_touch_events_async_; On 2015/03/31 14:44:07, tdresser wrote: > send_touch_events_async_ should never be false here, should it? It would be > strange to send the pending_async_touchmove synchronously. You are right, it will always be async, I thought we may coalesce a sync touchmove, and in this case, we dispatch it right away, so pending_async_touchmove is only async touchmove. Done. https://codereview.chromium.org/997283002/diff/220001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/997283002/diff/220001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2011: // Now queue a second touchmove and verify it's not (yet) dispatched, because On 2015/03/31 14:44:07, tdresser wrote: > nit: a second -> another > > It's not actually the second touchmove. Done. https://codereview.chromium.org/997283002/diff/220001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2022: // dispatch it yet. On 2015/03/31 14:44:07, tdresser wrote: > I think we can make this test more focused, and move some of the logic from this > test into another test. > > Testing that coalescing works should probably go in a separate test. > > This test is supposed to verify that we wait for an async ack before dispatching > the pending async touch move. > > Everything other than testing that should be removed if possible. I'd set the > timestamp of the first async touchmove to after the throttle limit, so the logic > is something like: > - touchstart > - touchmove (scrollbegin) > - touchmove (scrollupdate) > - async touchmove (dispatched) > - async touchmove (passed throttle timeout) > - ack first async touchmove > > I think you can get rid of two events, which will make the test easier to read. Done. https://codereview.chromium.org/997283002/diff/220001/content/common/input/we... File content/common/input/web_input_event_traits.cc (right): https://codereview.chromium.org/997283002/diff/220001/content/common/input/we... content/common/input/web_input_event_traits.cc:507: if (send_ack) { On 2015/03/31 14:44:07, tdresser wrote: > Could we end up overwriting response here? > > We should at least: > DCHECK(!response); Done.
LGTM, with the addition of one more test. Make sure to get an LGTM from jdduke before landing. https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:1983: Add a new test that specifically tests the coalescing of async touchmove events. (The logic I had you remove from the other tests).
https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.h (right): https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.h:22: struct InputHostMsg_HandleUncancelableTouchMoveEvent_ACK_Params; Nit: This forward decl is no longer needed. https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:449: // uncancelable touchmoves. DCHECK_GT(uncancelable_touch_moves_pending_ack_count_, 0); https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:463: SendTouchEventImmediately(&touch); I can't say I'm thrilled with needing two acks for async touchmoves. Can we change the semantics of IgnoresAckDisposition so that we can expect just one? Right now, the TouchEventQueue isn't really aware of the fake ack system, and it would be nice to keep it that way. Hmm, if we renamed |IgnoresAckDisposition| to |WillReceiveAckFromRenderer|, would that be sufficient? Then the InputRouter only generates synthetic events if that is false (avoiding the uncancelable touchmove case), and we no longer need to insert the uncancelable touchmove into the front of the queue. Thoughts? https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:511: uncancelable_touch_moves_pending_ack_count_ > 0) { Maybe move this into the above condition: send_touchmove_now |= (uncancelable_touch_moves_pending_ack_count == 0 && touch.event.timeStampSeconds >= last_sent_touch_timestamp_sec_ + kAsyncTouchMoveIntervalSec); https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:69: void ProcessUncancelableTouchMoveAck(); A brief comment here is probably appropriate. https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:107: int UncancelableTouchMovesPendingAckCount() const { nit: linux_hacker_case https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:220: double last_sent_touch_timestamp_sec_; Nit: line break before last_sent_touch_timestamp_sec_
Patchset #10 (id:260001) has been deleted
Patchset #10 (id:280001) has been deleted
https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.h (right): https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.h:22: struct InputHostMsg_HandleUncancelableTouchMoveEvent_ACK_Params; On 2015/04/02 19:36:00, jdduke wrote: > Nit: This forward decl is no longer needed. Done. https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:449: // uncancelable touchmoves. On 2015/04/02 19:36:00, jdduke wrote: > DCHECK_GT(uncancelable_touch_moves_pending_ack_count_, 0); Done. https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:69: void ProcessUncancelableTouchMoveAck(); On 2015/04/02 19:36:00, jdduke wrote: > A brief comment here is probably appropriate. Done. https://codereview.chromium.org/997283002/diff/240001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:220: double last_sent_touch_timestamp_sec_; On 2015/04/02 19:36:00, jdduke wrote: > Nit: line break before last_sent_touch_timestamp_sec_ Done.
Patchset #10 (id:300001) has been deleted
I think we're getting close, thanks for sticking with this. https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:361: bool ignores_ack = Let's change this to read: |bool needs_synthetic_ack = WebInputEventTraits::WillReceiveAckFromRenderer(input_event);| Might be worth adding a brief router test to make sure we get balanced ack counts for both a cancelable and uncancelable touchmove? https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:442: // (ProcessInputEventAck) method, but not on other platforms; using Let's kill all of this NotificationService code, I don't think it's used any longer even for testing. https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:453: touch_event_queue_.ProcessUncancelableTouchMoveAck(); We also need: client_->DecrementInFlightEventCount(); UMA_HISTOGRAM_TIMES("MPArch.IIR_InputEventDelta", TimeTicks::Now() - input_event_start_time_); https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:506: send_touchmove_now &= ack_pending_async_touchmove_.empty(); So, I think we have to send the touchmove if we can't coalesce or there are events already in the queue. I think this condition should be: send_touchmove_now |= !pending_async_touchmove_ack_count && (touch.event.timeStampSeconds >= last_sent_touch_timestamp_sec_ + kAsyncTouchMoveIntervalSec); https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:555: PopTouchEvent(); Why can't we move this code next to the other async touchmove code we have in SendTouchEventImmediately? https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:704: if (touch->event.type == WebInputEvent::TouchMove && I think we may need a guard here in the event that we get a synchronous ack for whatever reason (e.g., the touch is filtered by WebView). Right now we have a |base::AutoReset<bool> dispatching_touch(&dispatching_touch_, true);| call to guard against that case. We might need to move that guard into this function, and then we only do this logic if we haven't yet received an ack. https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:709: ack_pending_async_touchmove_.push_front(*touch); A count should be sufficient. https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:267: const std::vector<WebTouchEvent> sent_all_events() const { maybe |all_sent_events|, and you can return by const ref. https://codereview.chromium.org/997283002/diff/320001/content/common/input/we... File content/common/input/web_input_event_traits.cc (right): https://codereview.chromium.org/997283002/diff/320001/content/common/input/we... content/common/input/web_input_event_traits.cc:501: response = scoped_ptr<IPC::Message>( I think we can just do: return make_scoped_ptr(new ...); https://codereview.chromium.org/997283002/diff/320001/content/common/input/we... content/common/input/web_input_event_traits.cc:515: response = scoped_ptr<IPC::Message>( Same here, |return make_scoped_ptr(new ...)|. https://codereview.chromium.org/997283002/diff/320001/content/common/input/we... content/common/input/web_input_event_traits.cc:518: return response.Pass(); Then this just becomes |return nullptr;|
https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:361: bool ignores_ack = On 2015/04/15 17:14:53, jdduke wrote: > Let's change this to read: > > |bool needs_synthetic_ack = > WebInputEventTraits::WillReceiveAckFromRenderer(input_event);| > > Might be worth adding a brief router test to make sure we get balanced ack > counts for both a cancelable and uncancelable touchmove? Done. https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:442: // (ProcessInputEventAck) method, but not on other platforms; using On 2015/04/15 17:14:53, jdduke wrote: > Let's kill all of this NotificationService code, I don't think it's used any > longer even for testing. Done. https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:453: touch_event_queue_.ProcessUncancelableTouchMoveAck(); On 2015/04/15 17:14:53, jdduke wrote: > We also need: > > client_->DecrementInFlightEventCount(); > UMA_HISTOGRAM_TIMES("MPArch.IIR_InputEventDelta", TimeTicks::Now() - > input_event_start_time_); Done. https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:506: send_touchmove_now &= ack_pending_async_touchmove_.empty(); On 2015/04/15 17:14:53, jdduke wrote: > So, I think we have to send the touchmove if we can't coalesce or there are > events already in the queue. I think this condition should be: > > send_touchmove_now |= > !pending_async_touchmove_ack_count && > (touch.event.timeStampSeconds >= > last_sent_touch_timestamp_sec_ + kAsyncTouchMoveIntervalSec); Done. https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:555: PopTouchEvent(); On 2015/04/15 17:14:53, jdduke wrote: > Why can't we move this code next to the other async touchmove code we have in > SendTouchEventImmediately? When pending_async_touchmove_ is not null and we have a no-async-touchmove at the front of the queue, we need to flush the pending_async_touchmove_, and we cannot PopTouchEvent (in this case, it is no-async-touchmove). https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:704: if (touch->event.type == WebInputEvent::TouchMove && On 2015/04/15 17:14:53, jdduke wrote: > I think we may need a guard here in the event that we get a synchronous ack for > whatever reason (e.g., the touch is filtered by WebView). > > Right now we have a |base::AutoReset<bool> > dispatching_touch(&dispatching_touch_, true);| call to guard against that case. > We might need to move that guard into this function, and then we only do this > logic if we haven't yet received an ack. Done. https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:709: ack_pending_async_touchmove_.push_front(*touch); On 2015/04/15 17:14:53, jdduke wrote: > A count should be sufficient. Done. https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/997283002/diff/320001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:267: const std::vector<WebTouchEvent> sent_all_events() const { On 2015/04/15 17:14:53, jdduke wrote: > maybe |all_sent_events|, and you can return by const ref. Done. https://codereview.chromium.org/997283002/diff/320001/content/common/input/we... File content/common/input/web_input_event_traits.cc (right): https://codereview.chromium.org/997283002/diff/320001/content/common/input/we... content/common/input/web_input_event_traits.cc:501: response = scoped_ptr<IPC::Message>( On 2015/04/15 17:14:53, jdduke wrote: > I think we can just do: > > return make_scoped_ptr(new ...); Done. https://codereview.chromium.org/997283002/diff/320001/content/common/input/we... content/common/input/web_input_event_traits.cc:515: response = scoped_ptr<IPC::Message>( On 2015/04/15 17:14:53, jdduke wrote: > Same here, |return make_scoped_ptr(new ...)|. Done. https://codereview.chromium.org/997283002/diff/320001/content/common/input/we... content/common/input/web_input_event_traits.cc:518: return response.Pass(); On 2015/04/15 17:14:53, jdduke wrote: > Then this just becomes |return nullptr;| Done.
I think the last high level point I have is regarding the other change you're making to bundle the touch event id. The intent of that change is to make the ack type implicit, yet in this patch we're actually moving in the opposite direction. Perhaps we should go ahead and do the same in this patch (use a unified ack type that includes the id, as you originally had) for the sake of continuity. I don't think you'll have to change much, as we already have the ack creation helpers, so it may just be switching to a queue instead of a count. Thoughts?
Patchset #12 (id:360001) has been deleted
https://codereview.chromium.org/997283002/diff/380001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.h (right): https://codereview.chromium.org/997283002/diff/380001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.h:145: void ProcessInputEventAck(blink::WebInputEvent::Type event_type, What if we add a struct InputEventAck { type, state, latency, overscroll, unique_touch; }; with a few helper constructors? We could put that in content/common/input. Thoughts? That would simplify a number of these functions, and eventually will let us generalize the ack quite simply. https://codereview.chromium.org/997283002/diff/380001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/380001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:461: if (touch_queue_.front()->coalesced_event().event.uniqueTouchEventId != This should probably be a DCHECK_EQ instead of an "if return". We spokea about this briefly, but I think I may not have been clear. We should only be getting spurious acks if we've flushed the queue, otherwise, we should be getting the right ack and I think it's safe to keep emptying the queue. https://codereview.chromium.org/997283002/diff/380001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/997283002/diff/380001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:504: uint32 last_send_event_id_; Nit: last_sent https://codereview.chromium.org/997283002/diff/380001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:708: last_send_event_id_ = touch_event_.uniqueTouchEventId; Hmm, but what if touches are still in the queue? Or do we not test that? This method gets called *before* a touch is sent to the queue, so if we then ack an event that has already been sent, we're acking the wrong event.
Patchset #13 (id:400001) has been deleted
https://codereview.chromium.org/997283002/diff/380001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/380001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:461: if (touch_queue_.front()->coalesced_event().event.uniqueTouchEventId != On 2015/05/01 19:11:09, jdduke wrote: > This should probably be a DCHECK_EQ instead of an "if return". We spokea about > this briefly, but I think I may not have been clear. We should only be getting > spurious acks if we've flushed the queue, otherwise, we should be getting the > right ack and I think it's safe to keep emptying the queue. Done. https://codereview.chromium.org/997283002/diff/380001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/997283002/diff/380001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:504: uint32 last_send_event_id_; On 2015/05/01 19:11:09, jdduke wrote: > Nit: last_sent Done.
https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (left): https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:506: WebInputEvent::Type event_type, For now let's keep this signature (adding a touch id argument) so we avoid creating the temporary ack structures that I guess aren't really necessary as we're not propagating them to the client. Maybe at some point we can switch, but for now let's avoid the overhead. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:361: WebInputEventTraits::WillReceiveAckFromRenderer(input_event); I think you're missing a "!" here. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:371: InputEventAck ack(input_event.type, INPUT_EVENT_ACK_STATE_IGNORED, Looking at this now, I can see why you wondered about why we need the new struct. I guess it doesn't actually save us any code. I had originally anticipated that we would send this ack throughout the pipeline (i.e., use it instead of InputEventAckState when acking to the router's client, and the touch queue, etc...), but maybe that refactoring should be done separately. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:602: const uint32 unique_touch_event_id) { Hmm, passing by const value is rare, let's just pass a regular uint32. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.h (right): https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.h:144: void ProcessInputEventAck(const InputEventAck& ack, AckSource ack_source); See comments in the .cc file, I guess until we propagate this to the external world let's keep the arguments separate. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:391: TRACE_EVENT0("input", "TouchEventQueue::QueueEvent"); What kind of touch id validation should be doing? Should we validate that this is always non-zero? Note that the queue will *mostly* just work if all touch IDs are zero, but then we may not have the right correctness guarantees. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:435: if (pending_async_touchmove_ && ack_pending_async_touchmove_.empty()) { Let's add a DCHECK(touch_queue_.empty()); https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:436: TouchEventWithLatencyInfo touch = *pending_async_touchmove_; See comment below, I think we can share some logic so we'll have: if (pending_async_touchmove_->timeStampSeconds >= last_sent_touch_timestamp_sec_ + kAsyncTouchMoveIntervalSec) { FlushPendingAsyncTouchmove(); } https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:541: scoped_ptr<TouchEventWithLatencyInfo> async_move = Perhaps add a |FlushPendingAsyncTouchMove()| helper that does this? Then we can call it here and in the ack? https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:712: client_->OnTouchEventAck(*touch, INPUT_EVENT_ACK_STATE_IGNORED); Can we make this PopTouchEventToClient(INPUT_EVENT_ACK_STATE_IGNORED):? https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:713: TryForwardNextEventToRenderer(); Let'd add a return here. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_unittest.cc (right): https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/render_widget_host_unittest.cc:462: InputEventAck ack(type, ack_result); Maybe DCHECK(!WebInputEventTraits::isTouchEventType(type)); https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/render_widget_host_unittest.cc:466: void SendTouchEventACK(WebInputEvent::Type type, This is used just once, I'd rather we either just create the ack where it is currently needed, or force the caller to supply the touch id. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/render_widget_host_unittest.cc:470: LOG(ERROR) << " count " << count; Not sure we need this log? https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:476: void SetLastSentEventID(uint32 event_id) { last_sent_event_id_ = event_id; } Instead of setting the id and trying to guess it, can we instead have a separate ACK method for touches? SendTouchEventAck(type, id, result); And the caller is explicitly responsible for associating the proper ID? https://codereview.chromium.org/997283002/diff/440001/content/common/input/in... File content/common/input/input_event_ack.cc (right): https://codereview.chromium.org/997283002/diff/440001/content/common/input/in... content/common/input/input_event_ack.cc:26: : InputEventAck(type, state, latency, NULL, unique_touch_event_id) { Nit: nullptr https://codereview.chromium.org/997283002/diff/440001/content/common/input/in... content/common/input/input_event_ack.cc:33: InputEventAck::InputEventAck() { We'll still want to "zero" initialize type/state/unique_touch_event_id here. https://codereview.chromium.org/997283002/diff/440001/content/common/input/in... content/common/input/input_event_ack.cc:35: InputEventAck::~InputEventAck() { Nit: line break between methods. https://codereview.chromium.org/997283002/diff/440001/content/common/input/in... File content/common/input/input_event_ack.h (right): https://codereview.chromium.org/997283002/diff/440001/content/common/input/in... content/common/input/input_event_ack.h:22: ui::LatencyInfo latency, Let's pass latency by const ref. https://codereview.chromium.org/997283002/diff/440001/content/common/input/we... File content/common/input/web_input_event_traits.cc (right): https://codereview.chromium.org/997283002/diff/440001/content/common/input/we... content/common/input/web_input_event_traits.cc:12: #include "content/common/input_messages.h" I think we can remove this include. https://codereview.chromium.org/997283002/diff/440001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/997283002/diff/440001/content/renderer/render... content/renderer/render_widget.cc:1244: InputEventAck ack(input_event->type, ack_result, swap_latency_info, Looking at this now, I wonder if this is really any more readable. I like that the assignment from before is nice and explicit, so maybe these constructors aren't actually buying us anything.
https://codereview.chromium.org/997283002/diff/440001/content/common/input/in... File content/common/input/input_event_ack.h (right): https://codereview.chromium.org/997283002/diff/440001/content/common/input/in... content/common/input/input_event_ack.h:20: InputEventAck(blink::WebInputEvent::Type type, Dang, looks like we'll need to align this to properly compile LatencyInfo. Sigh, maybe we should just go back to the old auto-generated struct, and make this change separately (I didn't realize it would be such a pain!).
Patchset #13 (id:420001) has been deleted
https://codereview.chromium.org/997283002/diff/440001/content/common/input/in... File content/common/input/input_event_ack.h (right): https://codereview.chromium.org/997283002/diff/440001/content/common/input/in... content/common/input/input_event_ack.h:20: InputEventAck(blink::WebInputEvent::Type type, On 2015/05/07 21:38:05, jdduke wrote: > Dang, looks like we'll need to align this to properly compile LatencyInfo. Sigh, > maybe we should just go back to the old auto-generated struct, and make this > change separately (I didn't realize it would be such a pain!). Per our offline discussion, I think we just need to pass the LatencyInfo by const ref. We can keep this structure for now.
https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (left): https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:506: WebInputEvent::Type event_type, On 2015/05/07 21:11:15, jdduke wrote: > For now let's keep this signature (adding a touch id argument) so we avoid > creating the temporary ack structures that I guess aren't really necessary as > we're not propagating them to the client. Maybe at some point we can switch, but > for now let's avoid the overhead. Done. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:361: WebInputEventTraits::WillReceiveAckFromRenderer(input_event); On 2015/05/07 21:11:15, jdduke wrote: > I think you're missing a "!" here. Not really, because we change both the function name and variable name, they mean the same thing right now, and I revert the condition below. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:602: const uint32 unique_touch_event_id) { On 2015/05/07 21:11:15, jdduke wrote: > Hmm, passing by const value is rare, let's just pass a regular uint32. Done. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:391: TRACE_EVENT0("input", "TouchEventQueue::QueueEvent"); On 2015/05/07 21:11:15, jdduke wrote: > What kind of touch id validation should be doing? Should we validate that this > is always non-zero? > > Note that the queue will *mostly* just work if all touch IDs are zero, but then > we may not have the right correctness guarantees. We cannot easily add the validation on this, I think we need to create a new issue to do it, because there are some webtouchevent (mostly cancel), created with a unique_event_id 0, we need to find out all these case and add a valid id to them https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:435: if (pending_async_touchmove_ && ack_pending_async_touchmove_.empty()) { On 2015/05/07 21:11:15, jdduke wrote: > Let's add a DCHECK(touch_queue_.empty()); Done. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:541: scoped_ptr<TouchEventWithLatencyInfo> async_move = On 2015/05/07 21:11:15, jdduke wrote: > Perhaps add a |FlushPendingAsyncTouchMove()| helper that does this? Then we can > call it here and in the ack? Done. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:712: client_->OnTouchEventAck(*touch, INPUT_EVENT_ACK_STATE_IGNORED); On 2015/05/07 21:11:15, jdduke wrote: > Can we make this PopTouchEventToClient(INPUT_EVENT_ACK_STATE_IGNORED):? Done. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:713: TryForwardNextEventToRenderer(); On 2015/05/07 21:11:15, jdduke wrote: > Let'd add a return here. Done. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_unittest.cc (right): https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/render_widget_host_unittest.cc:462: InputEventAck ack(type, ack_result); On 2015/05/07 21:11:15, jdduke wrote: > Maybe DCHECK(!WebInputEventTraits::isTouchEventType(type)); Done. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/render_widget_host_unittest.cc:466: void SendTouchEventACK(WebInputEvent::Type type, On 2015/05/07 21:11:15, jdduke wrote: > This is used just once, I'd rather we either just create the ack where it is > currently needed, or force the caller to supply the touch id. > Done. https://codereview.chromium.org/997283002/diff/440001/content/browser/rendere... content/browser/renderer_host/render_widget_host_unittest.cc:470: LOG(ERROR) << " count " << count; On 2015/05/07 21:11:15, jdduke wrote: > Not sure we need this log? Forgot to delete :) https://codereview.chromium.org/997283002/diff/440001/content/common/input/in... File content/common/input/input_event_ack.cc (right): https://codereview.chromium.org/997283002/diff/440001/content/common/input/in... content/common/input/input_event_ack.cc:26: : InputEventAck(type, state, latency, NULL, unique_touch_event_id) { On 2015/05/07 21:11:16, jdduke wrote: > Nit: nullptr Done. https://codereview.chromium.org/997283002/diff/440001/content/common/input/in... content/common/input/input_event_ack.cc:33: InputEventAck::InputEventAck() { On 2015/05/07 21:11:16, jdduke wrote: > We'll still want to "zero" initialize type/state/unique_touch_event_id here. Done. https://codereview.chromium.org/997283002/diff/440001/content/common/input/in... content/common/input/input_event_ack.cc:35: InputEventAck::~InputEventAck() { On 2015/05/07 21:11:16, jdduke wrote: > Nit: line break between methods. Done.
A few more nits, almost there I think. https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:370: if (!needs_synthetic_ack) { Hmm, synthetic means fake, so I think this should be if (needs_synthetic_ack) (so we need to reverse initialization of the variable). https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:389: InputEventAck ack( You can remove this unused variable. https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.h (right): https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.h:18: #include "content/common/input/input_event_ack.h" Nit: We can still forward declare the InputEventAck (struct InputEventAck;) instead of adding the include here. https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... File content/browser/renderer_host/input/mock_input_ack_handler.cc (right): https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... content/browser/renderer_host/input/mock_input_ack_handler.cc:50: LOG(ERROR) << " MockInputAckHandler::OnTouchEventAck "; Nit: Remove this. https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:552: TouchEventWithLatencyInfo touch = *pending_async_touchmove_; Instead of incurring a copy you can do: scoped_ptr<TouchEventWithLatencyInfo> touch = pending_async_touchmove_.Pass(); https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:76: void ProcessUncancelableTouchMoveAck(); I think we can remove this function. https://codereview.chromium.org/997283002/diff/460001/content/common/input/in... File content/common/input/input_event_ack.cc (right): https://codereview.chromium.org/997283002/diff/460001/content/common/input/in... content/common/input/input_event_ack.cc:35: : unique_touch_event_id(0) { We should also initialize type (to WebInputEvent::Unknown) and state (to INPUT_EVENT_ACK_STATE_UNKNOWN). https://codereview.chromium.org/997283002/diff/460001/content/common/input/we... File content/common/input/web_input_event_traits.h (right): https://codereview.chromium.org/997283002/diff/460001/content/common/input/we... content/common/input/web_input_event_traits.h:11: #include "content/common/input/did_overscroll_params.h" Hmm, I don't think we need these extra includes any longer.
Patchset #15 (id:480001) has been deleted
Patchset #15 (id:500001) has been deleted
https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.cc (right): https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:370: if (!needs_synthetic_ack) { On 2015/05/08 21:30:48, jdduke wrote: > Hmm, synthetic means fake, so I think this should be > > if (needs_synthetic_ack) > > (so we need to reverse initialization of the variable). Done. https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.cc:389: InputEventAck ack( On 2015/05/08 21:30:48, jdduke wrote: > You can remove this unused variable. Done. https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.h (right): https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.h:18: #include "content/common/input/input_event_ack.h" On 2015/05/08 21:30:48, jdduke wrote: > Nit: We can still forward declare the InputEventAck (struct InputEventAck;) > instead of adding the include here. Done. https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... File content/browser/renderer_host/input/mock_input_ack_handler.cc (right): https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... content/browser/renderer_host/input/mock_input_ack_handler.cc:50: LOG(ERROR) << " MockInputAckHandler::OnTouchEventAck "; On 2015/05/08 21:30:49, jdduke wrote: > Nit: Remove this. Done. https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:552: TouchEventWithLatencyInfo touch = *pending_async_touchmove_; On 2015/05/08 21:30:49, jdduke wrote: > Instead of incurring a copy you can do: > > scoped_ptr<TouchEventWithLatencyInfo> touch = pending_async_touchmove_.Pass(); Done. https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/997283002/diff/460001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:76: void ProcessUncancelableTouchMoveAck(); On 2015/05/08 21:30:49, jdduke wrote: > I think we can remove this function. Done. https://codereview.chromium.org/997283002/diff/460001/content/common/input/in... File content/common/input/input_event_ack.cc (right): https://codereview.chromium.org/997283002/diff/460001/content/common/input/in... content/common/input/input_event_ack.cc:35: : unique_touch_event_id(0) { On 2015/05/08 21:30:49, jdduke wrote: > We should also initialize type (to WebInputEvent::Unknown) and state (to > INPUT_EVENT_ACK_STATE_UNKNOWN). Done. https://codereview.chromium.org/997283002/diff/460001/content/common/input/we... File content/common/input/web_input_event_traits.h (right): https://codereview.chromium.org/997283002/diff/460001/content/common/input/we... content/common/input/web_input_event_traits.h:11: #include "content/common/input/did_overscroll_params.h" On 2015/05/08 21:30:49, jdduke wrote: > Hmm, I don't think we need these extra includes any longer. Done.
The core logic looks good, now just some nits and suggestion on the touch id testing pattern. https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.h (right): https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.h:34: struct InputEventAck; Nit: alphabetical order after |DidOverscrollParams|. https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl_unittest.cc (right): https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl_unittest.cc:329: size_t count = process_->sink().message_count(); Hmm, this probably works fine, but I wonder if it's worth being more explicit when we ack touches. What if we changed our touch forwarding to something like: uint32 SendTouchEvent(); and then each caller of SendTouchEvent would just store the ID and then include that in the ack? It's a bit more work for each test, but I feel like there may be less surprises. Then we'd have a method like: SendTouchEventAck(type, uint32 id, result); Looks like there are 4 or 5 tests where you might need to do this, but I think it's probably worth doing now. Alternatively, we could cheat and modify the touch id to always be some constant, say, kTestTouchId, before we send it, but that probably violates the spirit of the test :). https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:45: event.cancelable; It's probably worth keeping the WebInputEventTraits::WillReceiveAckFromRenderer(event) query here in addition to the event.cancelable bit, just for safety. https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:553: scoped_ptr<TouchEventWithLatencyInfo> touch = Let's add a |DCHECK(!dispatching_touch_);| here. https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:222: std::deque<uint32> ack_pending_async_touchmove_; Maybe |ack_pending_async_touchmove_ids_|? https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_unittest.cc (right): https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/render_widget_host_unittest.cc:467: uint32 LastSentEventID() { Again, I don't think we should do this, I think we should be explicit about the ack id. There's only 1 or 2 tests here that use touches so I'd rather we're explicit than relying on this logic. You can make |SendTouchEvent()| return the event id. https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:483: uint32 last_sent_event_id = 0; Again, I don't think we should use this logic. Let's either cache the id when we send it, or make it available somehow each time we send the event. https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:2802: PressTouchPoint(0, 1); Looks like only 2 methods use the |PressTouchPoint| helper methods. I'd be OK getting rid of them entirely and just storing the SyntheticWebTochEvent here, or have the helper methods return the id (you'll need to cache it before the points are reset). https://codereview.chromium.org/997283002/diff/520001/content/common/input/in... File content/common/input/input_event_ack.h (right): https://codereview.chromium.org/997283002/diff/520001/content/common/input/in... content/common/input/input_event_ack.h:20: InputEventAck(blink::WebInputEvent::Type type, Lots of constructors :) At some point in the future I might change this so we can do something like: SendInputEventAck(InputEventAck().set_type(...) .set_state(...) .set_latency(...) .set_touch_id(...) .validate()); But that can certainly come later and we don't have to do it now.
Patchset #16 (id:540001) has been deleted
https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl.h (right): https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl.h:34: struct InputEventAck; On 2015/05/12 16:07:11, jdduke wrote: > Nit: alphabetical order after |DidOverscrollParams|. Done. https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/input_router_impl_unittest.cc (right): https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/input/input_router_impl_unittest.cc:329: size_t count = process_->sink().message_count(); On 2015/05/12 16:07:11, jdduke wrote: > Hmm, this probably works fine, but I wonder if it's worth being more explicit > when we ack touches. What if we changed our touch forwarding to something like: > > uint32 SendTouchEvent(); > > and then each caller of SendTouchEvent would just store the ID and then include > that in the ack? It's a bit more work for each test, but I feel like there may > be less surprises. Then we'd have a method like: > > SendTouchEventAck(type, uint32 id, result); > > Looks like there are 4 or 5 tests where you might need to do this, but I think > it's probably worth doing now. > > Alternatively, we could cheat and modify the touch id to always be some > constant, say, kTestTouchId, before we send it, but that probably violates the > spirit of the test :). I think you are right, even though we have to add the additional event_id every time when we ack events, but it is much clearer, which event it acks. https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:45: event.cancelable; On 2015/05/12 16:07:11, jdduke wrote: > It's probably worth keeping the > WebInputEventTraits::WillReceiveAckFromRenderer(event) query here in addition to > the event.cancelable bit, just for safety. Done. https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:553: scoped_ptr<TouchEventWithLatencyInfo> touch = On 2015/05/12 16:07:11, jdduke wrote: > Let's add a |DCHECK(!dispatching_touch_);| here. Done. https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.h:222: std::deque<uint32> ack_pending_async_touchmove_; On 2015/05/12 16:07:11, jdduke wrote: > Maybe |ack_pending_async_touchmove_ids_|? Done. https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_unittest.cc (right): https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/render_widget_host_unittest.cc:467: uint32 LastSentEventID() { On 2015/05/12 16:07:11, jdduke wrote: > Again, I don't think we should do this, I think we should be explicit about the > ack id. There's only 1 or 2 tests here that use touches so I'd rather we're > explicit than relying on this logic. You can make |SendTouchEvent()| return the > event id. Done. https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:483: uint32 last_sent_event_id = 0; On 2015/05/12 16:07:11, jdduke wrote: > Again, I don't think we should use this logic. Let's either cache the id when we > send it, or make it available somehow each time we send the event. Done. https://codereview.chromium.org/997283002/diff/520001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:2802: PressTouchPoint(0, 1); On 2015/05/12 16:07:11, jdduke wrote: > Looks like only 2 methods use the |PressTouchPoint| helper methods. I'd be OK > getting rid of them entirely and just storing the SyntheticWebTochEvent here, or > have the helper methods return the id (you'll need to cache it before the points > are reset). Done.
lgtm https://codereview.chromium.org/997283002/diff/560001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/997283002/diff/560001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:483: SendTouchEventACK(get<0>(params)->type, ack_result, event_id); Since we're already extracting the event from the IPC here, you could probably just do: const blink::WebInputEvent* event = get<0>(params); SendTouchEventAck(event->type, ack_result, WebInputEventTraits::GetUniqueTouchEventId(*event)); then you don't need the additional argument for this specific ack method. I didn't realize this method was used by most of the touch-related acks.
lanwei@chromium.org changed reviewers: + nasko@chromium.org, sadrul@chromium.org
sadrul@chromium.org: Please review changes in Could you please take a look at ui/events/event.h? nasko@chromium.org: Please review changes in Could you please take a look at content/common/input_messages.h content/renderer/render_widget.cc? Thank you.
Some formatting comments for the IPC changes. Once fixed, LGTM. https://codereview.chromium.org/997283002/diff/580001/content/common/input_me... File content/common/input_messages.h (right): https://codereview.chromium.org/997283002/diff/580001/content/common/input_me... content/common/input_messages.h:111: IPC_STRUCT_TRAITS_MEMBER(type) Please indent struct members to match existing style in the file. https://codereview.chromium.org/997283002/diff/580001/content/common/input_me... content/common/input_messages.h:242: IPC_MESSAGE_ROUTED1(InputHostMsg_HandleInputEvent_ACK, content::InputEventAck) nit: Parameter name should follow in /* */ comment.
On 2015/05/15 03:01:53, lanwei wrote: > mailto:sadrul@chromium.org: Please review changes in > > Could you please take a look at ui/events/event.h? lgtm
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, jdduke@chromium.org, sadrul@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/997283002/#ps600001 (title: "Format input_message")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/997283002/600001
Message was sent while issue was closed.
Committed patchset #18 (id:600001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/f1a2283b2d64a09d0d0a6a4468eccf30805b069d Cr-Commit-Position: refs/heads/master@{#330548}
Message was sent while issue was closed.
https://codereview.chromium.org/997283002/diff/580001/content/common/input_me... File content/common/input_messages.h (right): https://codereview.chromium.org/997283002/diff/580001/content/common/input_me... content/common/input_messages.h:111: IPC_STRUCT_TRAITS_MEMBER(type) On 2015/05/18 17:20:07, nasko wrote: > Please indent struct members to match existing style in the file. Done. https://codereview.chromium.org/997283002/diff/580001/content/common/input_me... content/common/input_messages.h:242: IPC_MESSAGE_ROUTED1(InputHostMsg_HandleInputEvent_ACK, content::InputEventAck) On 2015/05/18 17:20:07, nasko wrote: > nit: Parameter name should follow in /* */ comment. Done. |
