Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23856016:
Send touch cancel to renderer when scrolling starts (Closed)
|
Created:
7 years, 3 months ago by Yufeng Shen (Slow to review) Modified:
7 years, 2 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionSend touch cancel to renderer when scrolling starts
We decide that Desktop Chrome should follow the Chrome on Android
mode for touch behaviour during scrolling: send touch cancel to
renderer when scrolling starts and then stop sending touch events
to renderer.
This CL implements the above model. When GestureScrollBegin is received,
we inject a synthesized touch cancel event into the touch event queue
and stop sending all the following touch events to renderer. When the
touch cancel event is acked, we drop it before sending it to view so it
won't interrupt normal gesture flow.
BUG=240735
TEST=Enable the flag #enable-no-touch-to-renderer-while-scrolling in chrome://flags
1. go to www.rbyers.net/eventTest.html
scroll the page, one should first see touch move events, then touch cancel events.
2. go to www.rbyers.net/janky-touch-scroll.html
scroll the page and one should see smooth scrolling once the scrolling starts.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226638
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fix the issue that TouchCancel may never be sent #
Total comments: 18
Patch Set 3 : always insert the touch cancel at the beginning of TEQ #
Total comments: 8
Patch Set 4 : more test coverage #Patch Set 5 : rebase #
Messages
Total messages: 29 (0 generated)
This is to unify Desktop & Android chrome touch behavior during scrolling. PTAL.
On 2013/09/20 00:16:42, Yufeng Shen wrote: > This is to unify Desktop & Android chrome touch behavior during scrolling. PTAL. There are some subtleties here that I wonder we might be missing. Suppose we start scrolling, and send the cancel... should we not then drain pending TouchMove's (ack with NO_CONSUMER_EXISTS) until we hit a TouchEnd? We'll also have to coordinate this adoption with the corresponding removal of Android's touch cancel logic, and we may even want to make it happen in the same patch. Would you mind if we delayed review until M31 branch?
On 2013/09/20 16:44:05, jdduke wrote: > On 2013/09/20 00:16:42, Yufeng Shen wrote: > > This is to unify Desktop & Android chrome touch behavior during scrolling. > PTAL. > > There are some subtleties here that I wonder we might be missing. Suppose we > start scrolling, and send the cancel... should we not then drain pending > TouchMove's (ack with NO_CONSUMER_EXISTS) until we hit a TouchEnd? > Once the touch cancel is inserted at the front of the touch event queue, all the following touches will not be sent to renderer (ShouldForwardToRender() returns false due to no_touch_to_renderer_ set to be true) and acked to view with NO_CONSUMER_EXISTS. And if TouchEnd comes, it will generate ScrollEnd or FlingStart, which will sets no_touch_to_renderer_ back to false. Originally I implemented this by calling FlushQueue() when scroll starts to drain the touch queue then I realized that force to flush the queue while in the process of dispatching touch ack is not good, so I switch to the above mentioned the model. > We'll also have to coordinate this adoption with the corresponding removal of > Android's touch cancel logic, and we may even want to make it happen in the same > patch. Would you mind if we delayed review until M31 branch? Sure. You guys can review now and I will hold pushing it until M31 branches but I doubt the review can be that quick.
On 2013/09/20 17:07:20, Yufeng Shen wrote: > On 2013/09/20 16:44:05, jdduke wrote: > > On 2013/09/20 00:16:42, Yufeng Shen wrote: > > > This is to unify Desktop & Android chrome touch behavior during scrolling. > > PTAL. > > > > There are some subtleties here that I wonder we might be missing. Suppose we > > start scrolling, and send the cancel... should we not then drain pending > > TouchMove's (ack with NO_CONSUMER_EXISTS) until we hit a TouchEnd? > > > > Once the touch cancel is inserted at the front of the touch event queue, all > the following touches will not be sent to renderer (ShouldForwardToRender() > returns > false due to no_touch_to_renderer_ set to be true) and acked to view with > NO_CONSUMER_EXISTS. And if TouchEnd comes, it will generate ScrollEnd or > FlingStart, > which will sets no_touch_to_renderer_ back to false. > > Originally I implemented this by calling FlushQueue() when scroll starts > to drain the touch queue then I realized that force to flush the queue while in > the > process of dispatching touch ack is not good, so I switch to the above mentioned > the model. > > > We'll also have to coordinate this adoption with the corresponding removal of > > Android's touch cancel logic, and we may even want to make it happen in the > same > > patch. Would you mind if we delayed review until M31 branch? > > Sure. You guys can review now and I will hold pushing it until M31 branches but > I doubt > the review can be that quick. Ahh, for whatever reason I was thinking "ShouldForwardToRenderer" was called externally (similar to the GEF), but it's internal. Right, on Android we ack any queued touch events until the TouchEnd only after the TouchCancel ack is received.
https://codereview.chromium.org/23856016/diff/1/content/browser/renderer_host... File content/browser/renderer_host/input/immediate_input_router_unittest.cc (right): https://codereview.chromium.org/23856016/diff/1/content/browser/renderer_host... content/browser/renderer_host/input/immediate_input_router_unittest.cc:1800: INPUT_EVENT_ACK_STATE_NOT_CONSUMED); The line in 1795 checks that only the scroll-begin event was sent. When is the touch-cancel event dispatched? Why do you send the ack for touch-cancel here? https://codereview.chromium.org/23856016/diff/1/content/browser/renderer_host... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/23856016/diff/1/content/browser/renderer_host... content/browser/renderer_host/input/touch_event_queue.cc:188: touch_queue_.push_front(synthesized_cancel_event_); When is this cancel event actually dispatched to the renderer? https://codereview.chromium.org/23856016/diff/1/content/browser/renderer_host... content/browser/renderer_host/input/touch_event_queue.cc:191: if (gesture_event.event.type == WebKit::WebInputEvent::GestureScrollEnd || else if https://codereview.chromium.org/23856016/diff/1/content/browser/renderer_host... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/23856016/diff/1/content/browser/renderer_host... content/browser/renderer_host/input/touch_event_queue.h:99: TouchEventWithLatencyInfo latest_event_; Why do you need this? https://codereview.chromium.org/23856016/diff/1/content/browser/renderer_host... content/browser/renderer_host/input/touch_event_queue.h:103: CoalescedWebTouchEvent* synthesized_cancel_event_; We shouldn't hold onto a raw pointer like this. Add some attribute on CoalescedWebTouchEvent that carries this information instead.
https://codereview.chromium.org/23856016/diff/1/content/browser/renderer_host... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/23856016/diff/1/content/browser/renderer_host... content/browser/renderer_host/input/touch_event_queue.cc:188: touch_queue_.push_front(synthesized_cancel_event_); Inserting the cancel at the front... The only way this will work is if GestureScrollBegin came synchronously from a PopTouchEventToClient (i.e., |dispatching_touch_ack_| is true). We cannot make that assumption... Otherwise, if the queue was previously empty, as sadrul@ suggests, the cancel will never be forwarded, and we'll just keep queueing events. If the queue was non-empty, and so a touch-event was in-flight, we'll be ack'ing the wrong event in ProcessTouchAck.
https://codereview.chromium.org/23856016/diff/1/content/browser/renderer_host... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/23856016/diff/1/content/browser/renderer_host... content/browser/renderer_host/input/touch_event_queue.cc:188: touch_queue_.push_front(synthesized_cancel_event_); On 2013/09/23 16:34:34, jdduke wrote: > Inserting the cancel at the front... The only way this will work is if > GestureScrollBegin came synchronously from a PopTouchEventToClient (i.e., > |dispatching_touch_ack_| is true). We cannot make that assumption... > > Otherwise, if the queue was previously empty, as sadrul@ suggests, the cancel > will never be forwarded, and we'll just keep queueing events. If the queue was > non-empty, and so a touch-event was in-flight, we'll be ack'ing the wrong event > in ProcessTouchAck. How about : 1) if the touch_queue_ is empty, we enqueue the cancel event and send it to renderer immediately 2) if the touch_queue is not empty, we insert the cancel event after the first event in the queue. Does this seem correct ? My only concern is that if the touch_queue_ is empty, and we are in dispatching ack, would sending the touch cancel cause any problem ?
> How about : > 1) if the touch_queue_ is empty, we enqueue the cancel event and send it to > renderer immediately > 2) if the touch_queue is not empty, we insert the cancel event after the first > event in the queue. Yeah, that sounds like the right approach. > Does this seem correct ? My only concern is that if the touch_queue_ is empty, > and we are in dispatching ack, would sending the touch cancel cause any problem > ? OK, with the patch from https://codereview.chromium.org/24312009/, if you're within |dispatching_touch_ack_|, you should be able to insert the cancel into the queue (like you've done), and it will be dispatched when the stack unwinds.
Update: 1. Only fake a touch cancel if the GestureScrollBegin is from dispatching a touch event, so that we can get the correct touch ids for touch cancel from that touch event. 2. When faking the touch cancel, if the touch event queue is empty, or the first touch event in the queue has not been sent to renderer, insert the touch cancel at the beginning of the queue. Otherwise, insert the touch cancel after the first touch event. PTAL, thanks.
https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... File content/browser/renderer_host/input/immediate_input_router_unittest.cc (right): https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/immediate_input_router_unittest.cc:1907: process_->sink().ClearMessages(); Can you check that the cancel event didn't go to the client? https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/immediate_input_router_unittest.cc:1957: } You should also add some test cases where the touch-queue has a number of events in the queue before scrolling starts to make sure the cancel event gets inserted in the right place. https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.cc:45: event_with_latency.event.touchesLength) { You should check that has_been_sent_to_renderer_ isn't set for either event, and that ignore_ack_ in the two events match. https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.cc:222: // for ack, we insert the touch cancel after that touch event. + "This cancel event will be dispatched to the renderer in response to the ACK for the earlier event." https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.cc:263: dispatching_touch_ack(&dispatching_touch_ack_, acked_event.get()); Hm, this would only work if the acked touch-event generates the scroll-gesture synchronously, which is the case on cros/win, but is it also always the case on android? If it is, then this would work. If not, we have to do this differently. jdduke@: Can you confirm the behaviour on android? (Sorry, I hadn't considered this during our earlier discussion). https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.h:55: void OnGestureScrollEvent(const GestureEventWithLatencyInfo& gesture_event); doc
https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.cc:178: if (touch_queue_.front()->has_been_sent_to_renderer()) You can remove this flag (or make it a DCHECK(!...), by restricting when you call TryForwardNextEventToRenderer(), as mentioned below. https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.cc:227: TryForwardNextEventToRenderer(); I believe you can remove this call. |dispatching_touch_ack_| is true means that there is no touch-event in-flight. So, the head of the queue has yet to be sent, and you can |.push_front()| regardless of whether or not the queue is empty. This also means that we're withing the scope of |ProcessTouchAck()|, which will make the TryForwardNextEventToRenderer() call for you at the end. https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.cc:263: dispatching_touch_ack(&dispatching_touch_ack_, acked_event.get()); On 2013/09/27 15:13:51, sadrul wrote: > Hm, this would only work if the acked touch-event generates the scroll-gesture > synchronously, which is the case on cros/win, but is it also always the case on > android? If it is, then this would work. If not, we have to do this differently. > jdduke@: Can you confirm the behaviour on android? > > (Sorry, I hadn't considered this during our earlier discussion). Yup, that's generally the case on Android, and otherwise we'll either send our own cancel via a timeout, or we never send touches to the TEQ. I'd like to move the timeout here (behind a flag or something like that) so we don't have to reason about it in multiple places.
https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.cc:227: TryForwardNextEventToRenderer(); On 2013/09/30 19:21:51, jdduke wrote: > I believe you can remove this call. > > |dispatching_touch_ack_| is true means that there is no touch-event in-flight. > So, the head of the queue has yet to be sent, and you can |.push_front()| > regardless of whether or not the queue is empty. > > This also means that we're withing the scope of |ProcessTouchAck()|, which will > make the TryForwardNextEventToRenderer() call for you at the end. Client::OnTouchEventAck can cause another touch-event to be added to the queue on android, right (i.e. the ACK for a touch-event can trigger another touch-event to be added to the queue), in which case the front isn't always the right place for the touch-cancel event?
https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... File content/browser/renderer_host/input/immediate_input_router_unittest.cc (right): https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/immediate_input_router_unittest.cc:1907: process_->sink().ClearMessages(); On 2013/09/27 15:13:51, sadrul wrote: > Can you check that the cancel event didn't go to the client? Done. https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/immediate_input_router_unittest.cc:1957: } On 2013/09/27 15:13:51, sadrul wrote: > You should also add some test cases where the touch-queue has a number of events > in the queue before scrolling starts to make sure the cancel event gets inserted > in the right place. now changed to that the touch cancel is always at the beginning. https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.cc:45: event_with_latency.event.touchesLength) { On 2013/09/27 15:13:51, sadrul wrote: > You should check that has_been_sent_to_renderer_ isn't set for either event, and > that ignore_ack_ in the two events match. Done. https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.cc:178: if (touch_queue_.front()->has_been_sent_to_renderer()) On 2013/09/30 19:21:51, jdduke wrote: > You can remove this flag (or make it a DCHECK(!...), by restricting when you > call TryForwardNextEventToRenderer(), as mentioned below. removed. https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.cc:222: // for ack, we insert the touch cancel after that touch event. On 2013/09/27 15:13:51, sadrul wrote: > + "This cancel event will be dispatched to the renderer in response to the ACK > for the earlier event." see the comments below. https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.cc:227: TryForwardNextEventToRenderer(); On 2013/09/30 19:21:51, jdduke wrote: > I believe you can remove this call. > > |dispatching_touch_ack_| is true means that there is no touch-event in-flight. > So, the head of the queue has yet to be sent, and you can |.push_front()| > regardless of whether or not the queue is empty. > > This also means that we're withing the scope of |ProcessTouchAck()|, which will > make the TryForwardNextEventToRenderer() call for you at the end. Right. So all the complication comes from that I was afraid the gesture scroll events can come async for chrome on android. If in most cases the gesture scroll events are generated synchronously from acking a touch event, then the flow is much simpler. As jdduke@ said, we can always insert the touch cancel at the beginning of the TEQ. A few exceptions of chrome on android that gesture scroll does not come from acking a touch event is: 1. No touch handler on the page at all. So touch events are directly sent to GR to generate touch scroll. In this case, |dispatching_touch_ack| would be false, and we don't fake the touch cancel, which makes sense since no touch is sent to renderer anyway. 2. There is timeout for getting touch ack, and android touch queue sends a touch cancel to TEQ and start sending its touch events directly to GR and generate touch scroll events. In this case, |dispatching_touch_ack_| is false, so we don't fake the touch cancel. And chrome on android 's own touch cancel optimization naturally kicks in. https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.h:55: void OnGestureScrollEvent(const GestureEventWithLatencyInfo& gesture_event); On 2013/09/27 15:13:51, sadrul wrote: > doc Done.
On 2013/09/30 20:33:24, sadrul wrote: > https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... > File content/browser/renderer_host/input/touch_event_queue.cc (right): > > https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... > content/browser/renderer_host/input/touch_event_queue.cc:227: > TryForwardNextEventToRenderer(); > On 2013/09/30 19:21:51, jdduke wrote: > > I believe you can remove this call. > > > > |dispatching_touch_ack_| is true means that there is no touch-event in-flight. > > > So, the head of the queue has yet to be sent, and you can |.push_front()| > > regardless of whether or not the queue is empty. > > > > This also means that we're withing the scope of |ProcessTouchAck()|, which > will > > make the TryForwardNextEventToRenderer() call for you at the end. > > Client::OnTouchEventAck can cause another touch-event to be added to the queue > on android, right (i.e. the ACK for a touch-event can trigger another > touch-event to be added to the queue), in which case the front isn't always the > right place for the touch-cancel event? So if client::OnTouchEventAck() is queuing another touch event by calling QueueEvent(), since |dispatching_touch_ack_| is true, the event will be just gets push_back() but not sent. So inserting touch cancel at the front is still valid ?
LGTM https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/23856016/diff/14001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.cc:227: TryForwardNextEventToRenderer(); On 2013/09/30 20:43:14, Yufeng Shen wrote: > On 2013/09/30 19:21:51, jdduke wrote: > > I believe you can remove this call. > > > > |dispatching_touch_ack_| is true means that there is no touch-event in-flight. > > > So, the head of the queue has yet to be sent, and you can |.push_front()| > > regardless of whether or not the queue is empty. > > > > This also means that we're withing the scope of |ProcessTouchAck()|, which > will > > make the TryForwardNextEventToRenderer() call for you at the end. > > > Right. So all the complication comes from that I was afraid the gesture scroll > events > can come async for chrome on android. If in most cases the gesture scroll events > are > generated synchronously from acking a touch event, then the flow is much > simpler. As > jdduke@ said, we can always insert the touch cancel at the beginning of the TEQ. > > A few exceptions of chrome on android that gesture scroll does not come from > acking a touch event is: > 1. No touch handler on the page at all. So touch events are directly sent to GR > to generate > touch scroll. In this case, |dispatching_touch_ack| would be false, and we don't > fake the > touch cancel, which makes sense since no touch is sent to renderer anyway. > 2. There is timeout for getting touch ack, and android touch queue sends a touch > cancel to TEQ and start sending its touch events directly to GR and generate > touch scroll events. In this case, |dispatching_touch_ack_| is false, so we > don't fake the touch cancel. And chrome on android 's own touch cancel > optimization naturally kicks in. Thanks for the detailed explanation. This makes sense. https://codereview.chromium.org/23856016/diff/23001/content/browser/renderer_... File content/browser/renderer_host/input/immediate_input_router_unittest.cc (right): https://codereview.chromium.org/23856016/diff/23001/content/browser/renderer_... content/browser/renderer_host/input/immediate_input_router_unittest.cc:1886: EXPECT_EQ(latest_event().type, WebInputEvent::TouchCancel); EXPECT_EQ(<expected value>, <actual value>), so this should be: EXPECT_EQ(TouchCancel, latest_event().type); (otherwise, the error message when this fails becomes a bit confusing). The same applies in the following lines too. https://codereview.chromium.org/23856016/diff/23001/content/browser/renderer_... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/23856016/diff/23001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.cc:203: // |dispatching_touch_ack_| must be true when we reach here, meaning we s/must be true/is non-null/
https://codereview.chromium.org/23856016/diff/23001/content/browser/renderer_... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/23856016/diff/23001/content/browser/renderer_... content/browser/renderer_host/input/touch_event_queue.cc:165: // If there are queued touch events, then try to forward them to the renderer Would you mind adding a DCHECK(!dispatching_touch_ack_) at the beginning of this method? I think we almost ran into this condition during one of the patch revisions, and we really only want to call this method *after* the stack has unwound from the initial ack to the client.
https://codereview.chromium.org/23856016/diff/23001/content/browser/renderer_... File content/browser/renderer_host/input/immediate_input_router_unittest.cc (right): https://codereview.chromium.org/23856016/diff/23001/content/browser/renderer_... content/browser/renderer_host/input/immediate_input_router_unittest.cc:1880: Would it be worthwhile to add another (non-coalesced) TouchMove event to the queue before we send the ScrollBegin? That way, we also handle the case where there are events in the queue when the cancel is sent, which should be dispatched on the cancel ack.
Jared, Do you want to send me the android side patch so I can land them together ? https://chromiumcodereview.appspot.com/23856016/diff/23001/content/browser/re... File content/browser/renderer_host/input/immediate_input_router_unittest.cc (right): https://chromiumcodereview.appspot.com/23856016/diff/23001/content/browser/re... content/browser/renderer_host/input/immediate_input_router_unittest.cc:1880: On 2013/10/01 16:27:42, jdduke wrote: > Would it be worthwhile to add another (non-coalesced) TouchMove event to the > queue before we send the ScrollBegin? That way, we also handle the case where > there are events in the queue when the cancel is sent, which should be > dispatched on the cancel ack. Done. Send another TouchStart to the queue (so it won't get coalesced with the TouchMove). And check that after the TouchCancel is acked, the TouchStart is dispatched to client. https://chromiumcodereview.appspot.com/23856016/diff/23001/content/browser/re... content/browser/renderer_host/input/immediate_input_router_unittest.cc:1886: EXPECT_EQ(latest_event().type, WebInputEvent::TouchCancel); On 2013/10/01 11:32:45, sadrul wrote: > EXPECT_EQ(<expected value>, <actual value>), so this should be: > EXPECT_EQ(TouchCancel, latest_event().type); > (otherwise, the error message when this fails becomes a bit confusing). The same > applies in the following lines too. Done. https://chromiumcodereview.appspot.com/23856016/diff/23001/content/browser/re... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://chromiumcodereview.appspot.com/23856016/diff/23001/content/browser/re... content/browser/renderer_host/input/touch_event_queue.cc:165: // If there are queued touch events, then try to forward them to the renderer On 2013/10/01 13:21:01, jdduke wrote: > > Would you mind adding a DCHECK(!dispatching_touch_ack_) at the beginning of this > method? I think we almost ran into this condition during one of the patch > revisions, and we really only want to call this method *after* the stack has > unwound from the initial ack to the client. Done. https://chromiumcodereview.appspot.com/23856016/diff/23001/content/browser/re... content/browser/renderer_host/input/touch_event_queue.cc:203: // |dispatching_touch_ack_| must be true when we reach here, meaning we On 2013/10/01 11:32:45, sadrul wrote: > s/must be true/is non-null/ Done.
Jared, Do you want to send me the android side patch so I can land them together ? https://chromiumcodereview.appspot.com/23856016/diff/23001/content/browser/re... File content/browser/renderer_host/input/immediate_input_router_unittest.cc (right): https://chromiumcodereview.appspot.com/23856016/diff/23001/content/browser/re... content/browser/renderer_host/input/immediate_input_router_unittest.cc:1880: On 2013/10/01 16:27:42, jdduke wrote: > Would it be worthwhile to add another (non-coalesced) TouchMove event to the > queue before we send the ScrollBegin? That way, we also handle the case where > there are events in the queue when the cancel is sent, which should be > dispatched on the cancel ack. Done. Send another TouchStart to the queue (so it won't get coalesced with the TouchMove). And check that after the TouchCancel is acked, the TouchStart is dispatched to client. https://chromiumcodereview.appspot.com/23856016/diff/23001/content/browser/re... content/browser/renderer_host/input/immediate_input_router_unittest.cc:1886: EXPECT_EQ(latest_event().type, WebInputEvent::TouchCancel); On 2013/10/01 11:32:45, sadrul wrote: > EXPECT_EQ(<expected value>, <actual value>), so this should be: > EXPECT_EQ(TouchCancel, latest_event().type); > (otherwise, the error message when this fails becomes a bit confusing). The same > applies in the following lines too. Done. https://chromiumcodereview.appspot.com/23856016/diff/23001/content/browser/re... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://chromiumcodereview.appspot.com/23856016/diff/23001/content/browser/re... content/browser/renderer_host/input/touch_event_queue.cc:165: // If there are queued touch events, then try to forward them to the renderer On 2013/10/01 13:21:01, jdduke wrote: > > Would you mind adding a DCHECK(!dispatching_touch_ack_) at the beginning of this > method? I think we almost ran into this condition during one of the patch > revisions, and we really only want to call this method *after* the stack has > unwound from the initial ack to the client. Done. https://chromiumcodereview.appspot.com/23856016/diff/23001/content/browser/re... content/browser/renderer_host/input/touch_event_queue.cc:203: // |dispatching_touch_ack_| must be true when we reach here, meaning we On 2013/10/01 11:32:45, sadrul wrote: > s/must be true/is non-null/ Done.
On 2013/10/01 20:38:48, Yufeng Shen wrote: > Jared, > > Do you want to send me the android side patch so I can land > them together ? I forgot that this is behind a flag, so that shouldn't be necessary? I have the corresponding Android patch here https://codereview.chromium.org/25552003/ I'll test that some more tomorrow, and then add the flag to the default set of Android flags in that commit. lgtm
On 2013/10/01 20:55:03, jdduke wrote: > On 2013/10/01 20:38:48, Yufeng Shen wrote: > > Jared, > > > > Do you want to send me the android side patch so I can land > > them together ? > > I forgot that this is behind a flag, so that shouldn't be necessary? I have the > corresponding Android patch here https://codereview.chromium.org/25552003/ > > I'll test that some more tomorrow, and then add the flag to the default set of > Android flags in that commit. > > lgtm Rick, Do you want to launch this CL under a flag or not ?
On 2013/10/01 21:30:19, Yufeng Shen wrote: > Rick, > > Do you want to launch this CL under a flag or not ? I don't think there's any harm in keeping this behind a flag and gradually (hopefully quickly) rolling out the behavior by default to platforms where it makes sense. Can you not add it to the set of default flags for Aura? I suppose we could change it to a "Disable" flag as well...
This doesn't seem like a good candidate for a flag. I think we should just change the behavior without flag.
On 2013/10/01 21:54:04, aelias wrote: > This doesn't seem like a good candidate for a flag. I think we should just > change the behavior without flag. It's already behind a flag. My recommendation is that we land this, land https://codereview.chromium.org/25552003/, then land a patch removing the flag entirely. Or do it all in one patch, either way.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miletus@chromium.org/23856016/42001
On 2013/10/01 22:03:02, jdduke wrote: > On 2013/10/01 21:54:04, aelias wrote: > > This doesn't seem like a good candidate for a flag. I think we should just > > change the behavior without flag. > > It's already behind a flag. My recommendation is that we land this, land > https://codereview.chromium.org/25552003/, then land a patch removing the flag > entirely. Or do it all in one patch, either way. Cool, I will push this first. And then lets have a single CL that has both Android side change and removes the flag.
On 2013/10/02 17:18:48, Yufeng Shen wrote: > Cool, I will push this first. And then lets have a single CL that has both > Android side > change and removes the flag. sgtm, thanks.
On 2013/10/02 19:05:34, aelias wrote: > On 2013/10/02 17:18:48, Yufeng Shen wrote: > > Cool, I will push this first. And then lets have a single CL that has both > > Android side > > change and removes the flag. > > sgtm, thanks. Sorry for the delay. Yes, this sounds good to me too. We just need to be prepared that we might break an important site and need to revert temporarily while we work with the site to fix their bug <grin>. A flag makes this slightly more convenient (just flip the state), but we can worry about it if/when it happens.
Message was sent while issue was closed.
Change committed as 226638 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
