Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12225056:
Coalesce scrollUpdate and pinchUpdate on gesture_event queue (Closed)
|
Created:
7 years, 10 months ago by Yusuf Modified:
7 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, sadrul, klobag.chromium Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCoalesce scrollUpdate and pinchUpdate on gesture_event queue
Currently we are just coalescing scrollUpdates and we end up not utilizing
this when we have interleaved scrollUpdate and pinchUpdates. This change makes
that possible and makes sure the gesture_event_queue doesnt get flooded with
interleaved scroll and pinch updates.
BUG=174189
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182205
Patch Set 1 #
Total comments: 9
Patch Set 2 : Made loop clearer and fixed transform for pinch #Patch Set 3 : Renaming local variables for clarity #
Total comments: 5
Patch Set 4 : Added test and got rid of loop #
Total comments: 6
Patch Set 5 : Styling fixes and added anchor related expectations and fixed numerical error #Patch Set 6 : Fixed crash on processGestureAck on win trybots #Patch Set 7 : Rebased #Patch Set 8 : Fixed crash on win trybot #Patch Set 9 : Fix build error #
Total comments: 3
Messages
Total messages: 20 (0 generated)
This still needs some cleanup(I am doing something wrong with timeStamps and although this doesn't cause any visible problems I need to fix it.) and needs the tests added, but wanted to upload the general idea as a heads up to everyone. It now works, coalescing pinchUpdate and scrollUpdates and keeping the queue size at a maximum of 2 events while pinching. PTAL
https://codereview.chromium.org/12225056/diff/1/content/browser/renderer_host... File content/browser/renderer_host/gesture_event_filter.cc (right): https://codereview.chromium.org/12225056/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:283: GestureEventQueue::reverse_iterator gesture_it; if we have coalesced... we should only ever have a single scroll and pinch in the queue that we can coalesce with (recursively) in which case, you don't need to iterate? https://codereview.chromium.org/12225056/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:302: combined_transform.PreconcatTransform(GetTransformForEvent(*gesture_it)); Somewhat rhetorical question: if we are busily maintaining a unified scale / transform matrix, why do we bother with separate events? Always ship a single unified WebGestureTransform... ? Would simplify the code? https://codereview.chromium.org/12225056/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:307: first_pinch_update_in_queue->data.pinchUpdate.scale = combined_scale; I think that you need to update the position of the event too? Doesn't this end applying the combined scale around the oldest event's center? https://codereview.chromium.org/12225056/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:335: float scale = gesture_event.data.pinchUpdate.scale; I think this is might be wrong. AFAIK: a pinch event is a T^-1 S T where T translates the pinch center to the origin?
https://codereview.chromium.org/12225056/diff/1/content/browser/renderer_host... File content/browser/renderer_host/gesture_event_filter.cc (right): https://codereview.chromium.org/12225056/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:302: combined_transform.PreconcatTransform(GetTransformForEvent(*gesture_it)); I guess I don't know enough about how these gestures are handled to answer this completely, but doesn't that mean adding a dual-gesture that may or may not get used? On 2013/02/06 22:16:34, rjkroege wrote: > > Somewhat rhetorical question: if we are busily maintaining a unified scale / > transform matrix, why do we bother with separate events? Always ship a single > unified WebGestureTransform... ? Would simplify the code? https://codereview.chromium.org/12225056/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:307: first_pinch_update_in_queue->data.pinchUpdate.scale = combined_scale; Yes, you are right. I am now trying to figure out at what point and how to transform the anchor point. On 2013/02/06 22:16:34, rjkroege wrote: > I think that you need to update the position of the event too? Doesn't this end > applying the combined scale around the oldest event's center?
tests to follow. https://codereview.chromium.org/12225056/diff/1/content/browser/renderer_host... File content/browser/renderer_host/gesture_event_filter.cc (right): https://codereview.chromium.org/12225056/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:283: GestureEventQueue::reverse_iterator gesture_it; Done. Well, I kept the for but made it clearer that we loop twice at most. On 2013/02/06 22:16:34, rjkroege wrote: > if we have coalesced... > > we should only ever have a single scroll and pinch in the queue that we can > coalesce with (recursively) > > in which case, you don't need to iterate? > https://codereview.chromium.org/12225056/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:307: first_pinch_update_in_queue->data.pinchUpdate.scale = combined_scale; Still thinking. On 2013/02/06 22:34:09, Yusuf wrote: > Yes, you are right. I am now trying to figure out at what point and how to > transform the anchor point. > > > > On 2013/02/06 22:16:34, rjkroege wrote: > > I think that you need to update the position of the event too? Doesn't this > end > > applying the combined scale around the oldest event's center? > https://codereview.chromium.org/12225056/diff/1/content/browser/renderer_host... content/browser/renderer_host/gesture_event_filter.cc:335: float scale = gesture_event.data.pinchUpdate.scale; On 2013/02/06 22:16:34, rjkroege wrote: > I think this is might be wrong. AFAIK: a pinch event is a T^-1 S T where T > translates the pinch center to the origin? Done.
looks largely good minus the absence of tests. https://codereview.chromium.org/12225056/diff/8001/content/browser/renderer_h... File content/browser/renderer_host/gesture_event_filter.cc (right): https://codereview.chromium.org/12225056/diff/8001/content/browser/renderer_h... content/browser/renderer_host/gesture_event_filter.cc:192: VLOG(0)<<"ANCHOR FOR PINCH "<<gesture_event.x<<" : "<<gesture_event.y; you should remove before landing? https://codereview.chromium.org/12225056/diff/8001/content/browser/renderer_h... content/browser/renderer_host/gesture_event_filter.cc:292: for (gesture_it = coalesced_gesture_events_.rbegin(); I would have coded this without the loop. My reasoning is as follows: the presence of the loop suggests to me that if there are ever multiple mergeable events in the queue, weird stuff will happen because you will merge the new events in with oldest un-handled events yes? If you merge them with the newest unhandled event, it seems more recoverable. maybe. At a minimum, I think you should add some DCHECKs here. :-) https://codereview.chromium.org/12225056/diff/8001/content/browser/renderer_h... content/browser/renderer_host/gesture_event_filter.cc:305: float combined_scale = combined_transform.matrix().getDouble(0, 0); I am increasingly convinced that we need to have one kind of event for all kinds of scroll / pan. In the future. https://codereview.chromium.org/12225056/diff/8001/content/browser/renderer_h... content/browser/renderer_host/gesture_event_filter.cc:305: float combined_scale = combined_transform.matrix().getDouble(0, 0); DCHECK that 0,0 == 1,1 just in case? https://codereview.chromium.org/12225056/diff/8001/content/browser/renderer_h... File content/browser/renderer_host/gesture_event_filter.h (right): https://codereview.chromium.org/12225056/diff/8001/content/browser/renderer_h... content/browser/renderer_host/gesture_event_filter.h:95: bool ShouldTryMerging(const WebKit::WebGestureEvent& new_event, explanatory comment for method?
Added tests. Got rid of the loop. Corrected combined scroll so that we anchor to last anchor and that gets subtracted fromm combined value. Scroll and pinches now get sent together without waiting for ACK, added related logic to ignore ACKs. Added comments to methods, DCHECK and removed logs. PTAL
nice looking tests. https://codereview.chromium.org/12225056/diff/5003/content/browser/renderer_h... File content/browser/renderer_host/gesture_event_filter.cc (right): https://codereview.chromium.org/12225056/diff/5003/content/browser/renderer_h... content/browser/renderer_host/gesture_event_filter.cc:229: GestureEventQueue::iterator it_front = coalesced_gesture_events_.begin(); you could probably get rid of the iterator and just use the array accessor? https://codereview.chromium.org/12225056/diff/5003/content/browser/renderer_h... content/browser/renderer_host/gesture_event_filter.cc:232: if ((*it_front).type == WebInputEvent::GestureScrollUpdate && I understand why you have done this. I think though it's fragile that we sometimes merge an incoming event into a pair of events with an implicit attached state associated with both events. In the long term, I think we should move to a single unified WebPanScroll event. However, as an interim step: perhaps you could consider using a local subclass of a WebGestureEvent that is a "PanScroll" event. This gives us the most of the benefit of the unified event at least here. In particular: any incoming event merges with at most only one existing event. The PanScroll event would own the matrix. On Ack, you build an appropriate GSU | GPU from the first PanScroll and leave the remaining PanScroll in the queue if it translates to two events. On Merge, you merge with the PanScroll at the end of deque else push. I appreciate that there is schedule pressure here so if this sounds like too much work, I'd be open to you adding an appropriate TODO. https://codereview.chromium.org/12225056/diff/5003/content/browser/renderer_h... content/browser/renderer_host/gesture_event_filter.cc:294: GestureEventQueue::reverse_iterator it_back = don't need the iterator without the loop?
PTAL https://codereview.chromium.org/12225056/diff/5003/content/browser/renderer_h... File content/browser/renderer_host/gesture_event_filter.cc (right): https://codereview.chromium.org/12225056/diff/5003/content/browser/renderer_h... content/browser/renderer_host/gesture_event_filter.cc:229: GestureEventQueue::iterator it_front = coalesced_gesture_events_.begin(); On 2013/02/12 19:33:16, rjkroege wrote: > you could probably get rid of the iterator and just use the array accessor? Done. https://codereview.chromium.org/12225056/diff/5003/content/browser/renderer_h... content/browser/renderer_host/gesture_event_filter.cc:232: if ((*it_front).type == WebInputEvent::GestureScrollUpdate && Added TODO for now On 2013/02/12 19:33:16, rjkroege wrote: > I understand why you have done this. I think though it's fragile that we > sometimes merge an incoming event into a pair of events with an implicit > attached state associated with both events. > > In the long term, I think we should move to a single unified WebPanScroll event. > However, as an interim step: perhaps you could consider using a local subclass > of a WebGestureEvent that is a "PanScroll" event. This gives us the most of the > benefit of the unified event at least here. In particular: any incoming event > merges with at most only one existing event. > > The PanScroll event would own the matrix. > > On Ack, you build an appropriate GSU | GPU from the first PanScroll and leave > the remaining PanScroll in the queue if it translates to two events. > > On Merge, you merge with the PanScroll at the end of deque else push. > > I appreciate that there is schedule pressure here so if this sounds like too > much work, I'd be open to you adding an appropriate TODO. > https://codereview.chromium.org/12225056/diff/5003/content/browser/renderer_h... content/browser/renderer_host/gesture_event_filter.cc:294: GestureEventQueue::reverse_iterator it_back = On 2013/02/12 19:33:16, rjkroege wrote: > don't need the iterator without the loop? Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusufo@chromium.org/12225056/12002
Failed to trigger a try job on android_dbg HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusufo@chromium.org/12225056/7005
Failed to trigger a try job on ios_rel_device HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusufo@chromium.org/12225056/11004
Failed to trigger a try job on win_rel HTTP Error 400: Bad Request
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusufo@chromium.org/12225056/9009
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yusufo@chromium.org/12225056/2013
Message was sent while issue was closed.
Change committed as 182205
Message was sent while issue was closed.
Transforms seem sane at an initial glance. post-LGTM https://codereview.chromium.org/12225056/diff/2013/content/browser/renderer_h... File content/browser/renderer_host/gesture_event_filter.cc (right): https://codereview.chromium.org/12225056/diff/2013/content/browser/renderer_h... content/browser/renderer_host/gesture_event_filter.cc:232: // TODO(yusufo): Introduce GesturePanScroll so that these can be combined How is GesturePanScroll different from ordinary scroll? https://codereview.chromium.org/12225056/diff/2013/content/browser/renderer_h... content/browser/renderer_host/gesture_event_filter.cc:344: float combined_scale = combined_scroll_pinch_.matrix().getDouble(0, 0); Can you include some comments showing these computations in matrix notation? It's very difficult to confirm correctness of the transforms when they're in this form. In particular, making sure the subsequent translations get the correct scale applied. https://codereview.chromium.org/12225056/diff/2013/content/browser/renderer_h... content/browser/renderer_host/gesture_event_filter.cc:375: gesture_transform.Translate(-gesture_event.x, -gesture_event.y); Generally, looks OK. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
