Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1884883005:
Prepare SyntheticPointerAction to handle touch actions for multiple fingers (Closed)
|
Created:
4 years, 8 months ago by lanwei Modified:
4 years, 5 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, extensions-reviews_chromium.org, jam, dtapuska+chromiumwatch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, chromium-apps-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrepare SyntheticPointerAction to handle touch actions for multiple fingers
This patch is to make SyntheticPointerAction handle touch actions for multiple fingers, which will be dispatched together in one frame.
Next patch: Modify SyntheticGestureController to queue all the actions in one SyntheticPointerAction until we see a PROCESS action, then pushing in a queue and flush.
Make sure mouse actions also work.
Put the TouchActionSequence in the GpuBenchmarking for now.
Please see the design doc here:
https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4
BUG=525187
Committed: https://crrev.com/a9d9a6721c939076b6c688b137378f2c4b939eb9
Cr-Commit-Position: refs/heads/master@{#405222}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Move logic to controller #
Total comments: 16
Patch Set 3 : Make a new class SyntheticPointerActionController #
Total comments: 21
Patch Set 4 : Modify comments #
Total comments: 19
Patch Set 5 : #
Total comments: 1
Patch Set 6 : Add tests for mouse and change type of params_list #
Total comments: 16
Patch Set 7 : Split the patch #
Total comments: 1
Patch Set 8 : Move synthetic pointer action tests in a seperate file #
Total comments: 8
Patch Set 9 : Create a test for SyntheticPointerAction #
Total comments: 2
Patch Set 10 : Refactor test #Patch Set 11 : #
Total comments: 6
Patch Set 12 : Refactor tests #
Total comments: 5
Patch Set 13 : pointer action #
Total comments: 6
Patch Set 14 : new pointer action #
Total comments: 14
Patch Set 15 : new pointer action #
Total comments: 4
Patch Set 16 : return nullptr #Messages
Total messages: 61 (22 generated)
Description was changed from ========== touchaction touchaction BUG=525187 ========== to ========== Handling TouchActionSequence in GpuBenchmarking This patch handles the low level touch actions in the browser side. While we receives actions through IPC one by one, we queue them until we see a PROCESS action, we will flush all the queued actions together in one frame. While chromedriver_extension is under implementation, I put the TouchActionSequence in the GpuBenchmarking for now. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ==========
lanwei@chromium.org changed reviewers: + samuong@chromium.org, tdresser@chromium.org
Tim and Sam, can you please take a look? This patch is pretty rough, please ignore the logs.
A bit of early feedback. https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/synthetic_pointer_action.cc:53: SyntheticPointerActionParams params = (*iter); () aren't needed. https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/synthetic_pointer_action.cc:60: index_map_[params.index()] = index; Should we use a vector for index_map_? Somewhere we should validate that the length is okay, which we can't do if we're passing a raw pointer. https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/synthetic_pointer_action.h:39: SyntheticPointer* synthetic_pointer_; What's the lifetime here? Should this be unique_ptr? https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/synthetic_pointer_action.h:40: int* index_map_; Should this be unique_ptr? https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_impl.cc:1719: void RenderWidgetHostImpl::QueueSyntheticPointerAction( We'd prefer to have this logic encapsulated within the SyntheticGestureController, if possible. Does this need to live in RWHI? https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_impl.h:639: void OnSyntheticPointerActionCompleted(SyntheticGesture::Result result); It isn't clear to me why we need this in addition to OnSyntheticGestureCompleted. Aren't SyntheticPointerActions SyntheticGestures? https://codereview.chromium.org/1884883005/diff/1/content/renderer/gpu/gpu_be... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://codereview.chromium.org/1884883005/diff/1/content/renderer/gpu/gpu_be... content/renderer/gpu/gpu_benchmarking_extension.cc:871: // pass a two-dimensional list of actions. pass -> Pass Is this supposed to be currently passing a 2D list of actions? If so, I'm a bit confused.
https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/synthetic_pointer_action.cc:60: index_map_[params.index()] = index; On 2016/04/18 15:24:39, tdresser wrote: > Should we use a vector for index_map_? Somewhere we should validate that the > length is okay, which we can't do if we're passing a raw pointer. Yes, I was thinking about which one to choose, I felt an int pointer is easy to use, but you are right, vector is safer. https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/synthetic_pointer_action.h:39: SyntheticPointer* synthetic_pointer_; On 2016/04/18 15:24:39, tdresser wrote: > What's the lifetime here? Should this be unique_ptr? synthetic_pointer_ and index_map_ are passed to this function as pointer, because this function will change the value of these two variables, but RenderWidgetHostImpl should keep the ownership of these two and manager their create and destroy, so I use raw pointer. https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_impl.cc:1719: void RenderWidgetHostImpl::QueueSyntheticPointerAction( On 2016/04/18 15:24:39, tdresser wrote: > We'd prefer to have this logic encapsulated within the > SyntheticGestureController, if possible. > > Does this need to live in RWHI? No, you are right, I will move this part related to queuing synthetic pointer actions to SyntheticGestureController. https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/1884883005/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_impl.h:639: void OnSyntheticPointerActionCompleted(SyntheticGesture::Result result); On 2016/04/18 15:24:39, tdresser wrote: > It isn't clear to me why we need this in addition to > OnSyntheticGestureCompleted. Aren't SyntheticPointerActions SyntheticGestures? Yes, but OnSyntheticGestureCompleted will send something back, but for our SyntheticPointerAction, it is not necessary to send thing back every time when we queue a SyntheticGesture. I am sending a message back when we finish processing all the actions. https://codereview.chromium.org/1884883005/diff/1/content/renderer/gpu/gpu_be... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://codereview.chromium.org/1884883005/diff/1/content/renderer/gpu/gpu_be... content/renderer/gpu/gpu_benchmarking_extension.cc:871: // pass a two-dimensional list of actions. On 2016/04/18 15:24:39, tdresser wrote: > pass -> Pass > > Is this supposed to be currently passing a 2D list of actions? If so, I'm a bit > confused. Yes, it should be 2D array. I am not sure how to carry this 2D array and extract it from gin::Arguments. I will confirm with Sam and add this part, and I will add a todo for this for now.
https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture.h (right): https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture.h:41: POINTER_ACTION_PROCESSED, Maybe add a comment describing what's different about finished and processed? Or give them more specific names. https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller.h (right): https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller.h:17: #include "content/browser/renderer_host/input/synthetic_pointer_action.h" Where is this used? https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller.h:19: #include "content/common/input/synthetic_gesture_packet.h" Where is this used? https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller.h:26: using blink::WebTouchEvent; Where is this used? https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller.h:57: int PointerIndex(int index) const; Add this method in the test file by making instantiating a subclass of SyntheticGestureController in the test, which adds this method. You'll need to make the index_map_ (or whatever owns it) protected, but that's fine. https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller.h:100: std::vector<SyntheticPointerActionParams> action_param_list_; We're going to be adding a bunch of state and logic to this class. Would it be better to make a SyntheticPointerGestureController, which is contained by this class? We would then just forward a few method calls on the SyntheticGestureController to the SyntheticPointerGestureController. I suspect in the long term, that will result in cleaner code. https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller.h:102: std::vector<int> index_map_; If this is fixed size, use std::array. Add a comment explaining what this is mapping from and to. https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_target_base.cc (right): https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_target_base.cc:62: << "Touch coordinates are not within content bounds on TouchStart."; Is dragging your finger outside the window reasonable? Also, the log text still references TouchStart. https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... content/browser/renderer_host/input/synthetic_pointer_action.h:40: SyntheticPointer* synthetic_pointer_; Add a comment about the lifetimes of these objects, and who owns them. https://codereview.chromium.org/1884883005/diff/20001/content/renderer/gpu/gp... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://codereview.chromium.org/1884883005/diff/20001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:888: bool GpuBenchmarking::TouchActionSequence(gin::Arguments* args) { This patch is getting a little big - maybe we could land the GpuBenchmarking extension part separately?
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture.h (right): https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture.h:41: POINTER_ACTION_PROCESSED, On 2016/05/13 13:58:04, tdresser wrote: > Maybe add a comment describing what's different about finished and processed? Or > give them more specific names. Done. https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller.h (right): https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller.h:17: #include "content/browser/renderer_host/input/synthetic_pointer_action.h" On 2016/05/13 13:58:04, tdresser wrote: > Where is this used? Deleted. https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller.h:102: std::vector<int> index_map_; On 2016/05/13 13:58:04, tdresser wrote: > If this is fixed size, use std::array. > Add a comment explaining what this is mapping from and to. Done. https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_target_base.cc (right): https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_target_base.cc:62: << "Touch coordinates are not within content bounds on TouchStart."; On 2016/05/13 13:58:04, tdresser wrote: > Is dragging your finger outside the window reasonable? > > Also, the log text still references TouchStart. Because when we combine actions together, sometimes touch event's type is not TouchStart, but it may have a touch point which is a press, so I need to check for all the touch points' state. https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/1884883005/diff/20001/content/browser/rendere... content/browser/renderer_host/input/synthetic_pointer_action.h:40: SyntheticPointer* synthetic_pointer_; On 2016/05/13 13:58:04, tdresser wrote: > Add a comment about the lifetimes of these objects, and who owns them. Done. https://codereview.chromium.org/1884883005/diff/20001/content/renderer/gpu/gp... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://codereview.chromium.org/1884883005/diff/20001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:888: bool GpuBenchmarking::TouchActionSequence(gin::Arguments* args) { On 2016/05/13 13:58:04, tdresser wrote: > This patch is getting a little big - maybe we could land the GpuBenchmarking > extension part separately? Sure. I will remove this file from this patch.
https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:49: class MockSyntheticGestureController : public SyntheticGestureController { Technically this isn't a Mock. I can never keep track of what all the correct uses for the terms fake, mock, stub, dummy etc are, but a mock implies that this object doesn't perform actual behavior. Let's call this TestSyntheticGestureController. https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:47: std::vector<SyntheticPointerActionParams> param_list_; This is currently being copied. Should this be a const ref? https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.cc (right): https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:25: if (gesture_params.pointer_action_type() == Move this outside of the outer if. I think reducing the amount of nesting here will make this easier to read. https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:31: // later. Add a bug link. https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:41: } else Add {} here. In general if one clause of an if statement requires {}, it makes sense to use them on all clauses. https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:42: action_param_list_.push_back(gesture_params); I'd invert this condition so the common, easy to follow case (pushing the gesture params) comes first, and the complicated case (process || finish), comes second. https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:65: int SyntheticPointerActionController::PointerIndex(int index) const { Do we need this method here? Could it be defined in synthetic_gesture_controller_unittest.cc? https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.h (right): https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.h:34: // A list keeps all the action paramters, which should be executed in the paramters -> parameters "A list of all" https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.h:41: // users define and the real indexes kept in the touch event. "users define." Let's be more specific about which indexes are being referred to here. Where do the "user defined" indexes come from? https://codereview.chromium.org/1884883005/diff/120001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1884883005/diff/120001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:25: // we'll dispatch all queued events. A FINISH action will be recevied when received https://codereview.chromium.org/1884883005/diff/120001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:26: // we reach to the end of the action sequence. to the end -> the end
Description was changed from ========== Handling TouchActionSequence in GpuBenchmarking This patch handles the low level touch actions in the browser side. While we receives actions through IPC one by one, we queue them until we see a PROCESS action, we will flush all the queued actions together in one frame. While chromedriver_extension is under implementation, I put the TouchActionSequence in the GpuBenchmarking for now. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ========== to ========== Handling TouchActionSequence in SyntheticGestureController This patch handles the low level touch actions in the browser side. While we receives actions through IPC one by one, we queue them until we see a PROCESS action, we will flush all the queued actions together in one frame. My next step is to put the TouchActionSequence in the GpuBenchmarking for now. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ==========
Description was changed from ========== Handling TouchActionSequence in SyntheticGestureController This patch handles the low level touch actions in the browser side. While we receives actions through IPC one by one, we queue them until we see a PROCESS action, we will flush all the queued actions together in one frame. My next step is to put the TouchActionSequence in the GpuBenchmarking for now. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ========== to ========== Handling TouchActionSequence in SyntheticGestureController This patch handles the low level touch actions in the browser side. While we receives actions through IPC one by one, we queue them until we see a PROCESS action, we will flush all the queued actions together in one frame. My next step is to put the TouchActionSequence in the GpuBenchmarking for now. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ==========
https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:49: class MockSyntheticGestureController : public SyntheticGestureController { On 2016/05/19 17:57:03, tdresser wrote: > Technically this isn't a Mock. I can never keep track of what all the correct > uses for the terms fake, mock, stub, dummy etc are, but a mock implies that this > object doesn't perform actual behavior. > > Let's call this TestSyntheticGestureController. Thanks for explaining what a mock is doing! https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:47: std::vector<SyntheticPointerActionParams> param_list_; On 2016/05/19 17:57:03, tdresser wrote: > This is currently being copied. Should this be a const ref? We cannot, because there is another constructor explicit SyntheticPointerAction(const SyntheticPointerActionParams& params) which does not initialize the value of param_list_. https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.cc (right): https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:25: if (gesture_params.pointer_action_type() == On 2016/05/19 17:57:04, tdresser wrote: > Move this outside of the outer if. I think reducing the amount of nesting here > will make this easier to read. Done. https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:31: // later. On 2016/05/19 17:57:04, tdresser wrote: > Add a bug link. Done. https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:42: action_param_list_.push_back(gesture_params); On 2016/05/19 17:57:03, tdresser wrote: > I'd invert this condition so the common, easy to follow case (pushing the > gesture params) comes first, and the complicated case (process || finish), comes > second. Done. https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:65: int SyntheticPointerActionController::PointerIndex(int index) const { On 2016/05/19 17:57:04, tdresser wrote: > Do we need this method here? Could it be defined in > synthetic_gesture_controller_unittest.cc? Done. https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.h (right): https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.h:34: // A list keeps all the action paramters, which should be executed in the On 2016/05/19 17:57:04, tdresser wrote: > paramters -> parameters > > "A list of all" Done. https://codereview.chromium.org/1884883005/diff/120001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1884883005/diff/120001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:25: // we'll dispatch all queued events. A FINISH action will be recevied when On 2016/05/19 17:57:04, tdresser wrote: > received Done. https://codereview.chromium.org/1884883005/diff/120001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:26: // we reach to the end of the action sequence. On 2016/05/19 17:57:04, tdresser wrote: > to the end -> the end Done.
https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/1884883005/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:47: std::vector<SyntheticPointerActionParams> param_list_; On 2016/05/20 01:48:16, lanwei wrote: > On 2016/05/19 17:57:03, tdresser wrote: > > This is currently being copied. Should this be a const ref? > > We cannot, because there is another constructor explicit > SyntheticPointerAction(const SyntheticPointerActionParams& params) which does > not initialize the value of param_list_. Then perhaps we should pass a pointer? Right now the comment about lifetimes is incorrect, because param_list_ is owned by the SyntheticPointerAction, because it's a copy of the data in SyntheticPointerActionController. Another option would be to have a backpointer to the SyntheticPointerActionController, but having 3 raw pointers here is probably cleaner. https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture.h (right): https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture.h:41: // Used when receiving SyntheticPointerAction of PROCESS or FINISH type. Is this comment referring to both of the values below? If so, it's probably redundant. https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (left): https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1492: EXPECT_EQ(pointer_touch_target->type(), WebInputEvent::TouchStart); Why remove the type comparison? https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:60: ; Remove extra ; https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:37: for (auto iter = param_list_.begin(); iter != param_list_.end(); ++iter) { Might as well use a range based for loop here. Also, can the iterator be a const ref? Can we write this something like: for (const SyntheticPointerActionParams& params : param_list_) {}? https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:46: // pointer action sequence and resets them when finish. resets them when it finishes. https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:49: IndexMap* index_map_; I'd add a private accessor for index_map_, const IndexMap& index_map(), so that you can use that accessor and avoid all the awkward dereferencing in the cc file. https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.cc (right): https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:11: index_map_[i] = -1; std::fill https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:22: SyntheticPointerActionParams::PointerActionType::PROCESS) This isn't technically required by the style guide, but I find it more readable to use {} if either the condition or the body is multi-line. https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:23: action_param_list_.push_back(gesture_params); Should we push back the gesture params if the type is Finish?
https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture.h (right): https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture.h:41: // Used when receiving SyntheticPointerAction of PROCESS or FINISH type. On 2016/05/20 14:24:30, tdresser wrote: > Is this comment referring to both of the values below? If so, it's probably > redundant. Done. https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (left): https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1492: EXPECT_EQ(pointer_touch_target->type(), WebInputEvent::TouchStart); On 2016/05/20 14:24:30, tdresser wrote: > Why remove the type comparison? Because when we have one finger move and one finger down, the type is not meaningful, so I remove this type, and compare the states for all finger points. https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:60: ; On 2016/05/20 14:24:30, tdresser wrote: > Remove extra ; Done. https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:37: for (auto iter = param_list_.begin(); iter != param_list_.end(); ++iter) { On 2016/05/20 14:24:31, tdresser wrote: > Might as well use a range based for loop here. > > Also, can the iterator be a const ref? > > Can we write this something like: > > for (const SyntheticPointerActionParams& params : param_list_) {}? Thank you for your suggestion, I will keep in mind putting const on the objects we do not need to change. https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:46: // pointer action sequence and resets them when finish. On 2016/05/20 14:24:31, tdresser wrote: > resets them when it finishes. Done. https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:49: IndexMap* index_map_; On 2016/05/20 14:24:31, tdresser wrote: > I'd add a private accessor for index_map_, > > const IndexMap& index_map(), so that you can use that accessor and avoid all the > awkward dereferencing in the cc file. I need to change the index_map_ in the case of press, so I add both set and get functions here. https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.cc (right): https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:11: index_map_[i] = -1; On 2016/05/20 14:24:31, tdresser wrote: > std::fill Done. https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:22: SyntheticPointerActionParams::PointerActionType::PROCESS) On 2016/05/20 14:24:31, tdresser wrote: > This isn't technically required by the style guide, but I find it more readable > to use {} if either the condition or the body is multi-line. Done. https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:23: action_param_list_.push_back(gesture_params); On 2016/05/20 14:24:31, tdresser wrote: > Should we push back the gesture params if the type is Finish? The reason I push the 'FINISH' action into the param_list is I use this action to return a POINTER_ACTION_FINISHED result in SyntheticPointerAction::ForwardInputEvents, then in RenderWidgetHostImpl::OnSyntheticPointerActionCompleted, I will reset synthetic pointer and send a message back. For 'process', I do not send anything back.
https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (left): https://codereview.chromium.org/1884883005/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1492: EXPECT_EQ(pointer_touch_target->type(), WebInputEvent::TouchStart); On 2016/05/23 16:40:50, lanwei wrote: > On 2016/05/20 14:24:30, tdresser wrote: > > Why remove the type comparison? > > Because when we have one finger move and one finger down, the type is not > meaningful, so I remove this type, and compare the states for all finger points. I wouldn't say it's not meaningful - it depends on the order that the events were dispatched in. I think it would be reasonable to test that. https://codereview.chromium.org/1884883005/diff/160001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/1884883005/diff/160001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:53: std::vector<SyntheticPointerActionParams> param_list_; The lifetime here is still confused - we shouldn't copy this, and if we need to copy this, we shouldn't claim it's owned by SyntheticPointerActionController.
https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1656: EXPECT_EQ(0, num_failure_); Should we test a case that does cause a failure? https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:44: case SyntheticPointerActionParams::PointerActionType::PRESS: { Let's use {} in these case statements consistently. In this case, they aren't necessary, so we might as well remove them. https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:76: return (*index_map_)[index]; GetPointIndex(index). https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:51: // value to push a new patch of action parameters. patch -> batch https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:56: // pointer action sequence and resets them when it finishes. resets -> resetting https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.cc (right): https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:37: synthetic_gesture = std::unique_ptr<SyntheticGesture>( You can replace = std::unique_ptr<SyntheticGesture> here with a .reset, can't you? https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1176: if (synthetic_gesture_controller_) { Should this ever run if !view_? It might be easier to write as: if (!view_) return; if (!synthetic_gesture_controller_) sgc.reset(...); sgc->Queue... https://codereview.chromium.org/1884883005/diff/180001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1884883005/diff/180001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:21: struct CONTENT_EXPORT SyntheticPointerActionParams Would we be able to DISALLOW_COPY_AND_ASSIGN on this?
Description was changed from ========== Handling TouchActionSequence in SyntheticGestureController This patch handles the low level touch actions in the browser side. While we receives actions through IPC one by one, we queue them until we see a PROCESS action, we will flush all the queued actions together in one frame. My next step is to put the TouchActionSequence in the GpuBenchmarking for now. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ========== to ========== Prepare SyntheticPointerAction to handle touch actions for multiple fingers This patch is to make SyntheticPointerAction handle touch actions for multiple fingers, which will be dispatched together in one frame. Next patch: Modify SyntheticGestureController to queue all the actions in one SyntheticPointerAction until we see a PROCESS action, then pushing in a queue and flush. Make sure mouse actions also work. Put the TouchActionSequence in the GpuBenchmarking for now. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ==========
Patchset #7 (id:200001) has been deleted
https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:44: case SyntheticPointerActionParams::PointerActionType::PRESS: { On 2016/05/31 13:44:26, tdresser wrote: > Let's use {} in these case statements consistently. In this case, they aren't > necessary, so we might as well remove them. Done. https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:51: // value to push a new patch of action parameters. On 2016/05/31 13:44:26, tdresser wrote: > patch -> batch Done. https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:56: // pointer action sequence and resets them when it finishes. On 2016/05/31 13:44:26, tdresser wrote: > resets -> resetting Done. https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.cc (right): https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:37: synthetic_gesture = std::unique_ptr<SyntheticGesture>( On 2016/05/31 13:44:26, tdresser wrote: > You can replace = std::unique_ptr<SyntheticGesture> here with a .reset, can't > you? Yes, thanks! https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1176: if (synthetic_gesture_controller_) { On 2016/05/31 13:44:26, tdresser wrote: > Should this ever run if !view_? > > It might be easier to write as: > > if (!view_) > return; > > if (!synthetic_gesture_controller_) > sgc.reset(...); > > sgc->Queue... I feel if the synthetic_gesture_controller is not null, but view is null then if (!view_) return; will be skip synthetic_gesture_controller_->QueueSyntheticPointerAction. https://codereview.chromium.org/1884883005/diff/180001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1884883005/diff/180001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:21: struct CONTENT_EXPORT SyntheticPointerActionParams On 2016/05/31 13:44:26, tdresser wrote: > Would we be able to DISALLOW_COPY_AND_ASSIGN on this? Because SyntheticGestureParams has SyntheticGestureParams(const SyntheticGestureParams& other) and all its subclasses have SyntheticXXXGestureParams(const SyntheticXXXGestureParams& other) as well, we cannot make it DISALLOW_COPY_AND_ASSIGN.
Thanks for splitting this up. Everything looks good - I just want to do another pass over the tests once you've split them out. https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1884883005/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1176: if (synthetic_gesture_controller_) { On 2016/06/02 13:26:27, lanwei wrote: > On 2016/05/31 13:44:26, tdresser wrote: > > Should this ever run if !view_? > > > > It might be easier to write as: > > > > if (!view_) > > return; > > > > if (!synthetic_gesture_controller_) > > sgc.reset(...); > > > > sgc->Queue... > > I feel if the synthetic_gesture_controller is not null, but view is null then > if (!view_) > return; > will be skip synthetic_gesture_controller_->QueueSyntheticPointerAction. Is there ever a case where the controller is non-null but the view is null? https://codereview.chromium.org/1884883005/diff/180001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/1884883005/diff/180001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:21: struct CONTENT_EXPORT SyntheticPointerActionParams On 2016/06/02 13:26:27, lanwei wrote: > On 2016/05/31 13:44:26, tdresser wrote: > > Would we be able to DISALLOW_COPY_AND_ASSIGN on this? > > Because SyntheticGestureParams has SyntheticGestureParams(const > SyntheticGestureParams& other) and all its subclasses have > SyntheticXXXGestureParams(const SyntheticXXXGestureParams& other) as well, we > cannot make it DISALLOW_COPY_AND_ASSIGN. Acknowledged. https://codereview.chromium.org/1884883005/diff/220001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/220001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1572: SyntheticPointerAction::IndexMap index_map; Can we move tests for the pointer action specifically to a separate file from the synthetic gesture controller tests?
Patchset #9 (id:260001) has been deleted
Patchset #8 (id:240001) has been deleted
https://codereview.chromium.org/1884883005/diff/280001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/280001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller_unittest.cc:47: void set_pointer_assumed_stopped_time_ms(int time_ms) { NOTIMPLEMENTED(); } Why is this here? It isn't overriding anything... https://codereview.chromium.org/1884883005/diff/280001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller_unittest.cc:104: class SyntheticPointerActionControllerTest : public testing::Test { Shouldn't this just be SyntheticPointerActionTest? Why is "Controller" here (and in the filename)? https://codereview.chromium.org/1884883005/diff/280001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller_unittest.cc:168: CreateControllerAndTarget<MockSyntheticPointerTouchActionTarget>(); I was thinking we'd have some low level tests which only relied on the SyntheticPointerAction, and not the controller, and then some separate tests which required the controller.
Patchset #9 (id:300001) has been deleted
Patchset #9 (id:320001) has been deleted
https://codereview.chromium.org/1884883005/diff/280001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/280001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller_unittest.cc:47: void set_pointer_assumed_stopped_time_ms(int time_ms) { NOTIMPLEMENTED(); } On 2016/06/06 18:36:08, tdresser wrote: > Why is this here? It isn't overriding anything... SyntheticGestureTarget class has these virtual methods. If we inherit from SyntheticGestureTargetBase, we have other virtual methods to implement. Is there a good way to do this with these virtual methods? https://codesearch.chromium.org/chromium/src/content/browser/renderer_host/in... https://codereview.chromium.org/1884883005/diff/280001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller_unittest.cc:168: CreateControllerAndTarget<MockSyntheticPointerTouchActionTarget>(); On 2016/06/06 18:36:08, tdresser wrote: > I was thinking we'd have some low level tests which only relied on the > SyntheticPointerAction, and not the controller, and then some separate tests > which required the controller. The test cases are very similar to SyntheticGestureControllerTest, do you think we should add these to SyntheticGestureControllerTest as well?
Patchset #9 (id:340001) has been deleted
https://codereview.chromium.org/1884883005/diff/280001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/280001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller_unittest.cc:47: void set_pointer_assumed_stopped_time_ms(int time_ms) { NOTIMPLEMENTED(); } On 2016/06/06 18:36:08, tdresser wrote: > Why is this here? It isn't overriding anything... Done. https://codereview.chromium.org/1884883005/diff/280001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller_unittest.cc:104: class SyntheticPointerActionControllerTest : public testing::Test { On 2016/06/06 18:36:08, tdresser wrote: > Shouldn't this just be SyntheticPointerActionTest? Why is "Controller" here (and > in the filename)? Done. https://codereview.chromium.org/1884883005/diff/280001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller_unittest.cc:168: CreateControllerAndTarget<MockSyntheticPointerTouchActionTarget>(); On 2016/06/06 18:36:08, tdresser wrote: > I was thinking we'd have some low level tests which only relied on the > SyntheticPointerAction, and not the controller, and then some separate tests > which required the controller. Done.
https://codereview.chromium.org/1884883005/diff/360001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/360001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:72: unsigned touch_length_; This has touch specific information in it, but then also has a touch specific subclass. Why isn't this a single class?
https://codereview.chromium.org/1884883005/diff/360001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/360001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:72: unsigned touch_length_; On 2016/06/09 16:50:22, tdresser wrote: > This has touch specific information in it, but then also has a touch specific > subclass. > > Why isn't this a single class? Sorry, I moved this into the MockSyntheticPointerTouchActionTarget, the reason I have a separate function for touch is I will have another one for mouse.
https://codereview.chromium.org/1884883005/diff/400001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/400001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:35: return SyntheticGestureParams::TOUCH_INPUT; Should this be pure virtual? https://codereview.chromium.org/1884883005/diff/400001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:163: EXPECT_EQ(pointer_touch_target->indexes(0), index_map_[0]); The value here is deterministic, right? Can we just use an integer literal here (and below)? https://codereview.chromium.org/1884883005/diff/400001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:239: TEST_F(SyntheticPointerActionTest, PointerTouchActionInvalid) { From this test, we can't tell if each individual way of failing is being tested, or if we're just getting into a bad state and then continuously failing. Maybe send a successful event between each pair of failing events?
https://codereview.chromium.org/1884883005/diff/400001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/400001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:35: return SyntheticGestureParams::TOUCH_INPUT; On 2016/06/10 18:33:29, tdresser wrote: > Should this be pure virtual? Done. https://codereview.chromium.org/1884883005/diff/400001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:163: EXPECT_EQ(pointer_touch_target->indexes(0), index_map_[0]); On 2016/06/10 18:33:28, tdresser wrote: > The value here is deterministic, right? > > Can we just use an integer literal here (and below)? Done. https://codereview.chromium.org/1884883005/diff/400001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:239: TEST_F(SyntheticPointerActionTest, PointerTouchActionInvalid) { On 2016/06/10 18:33:29, tdresser wrote: > From this test, we can't tell if each individual way of failing is being tested, > or if we're just getting into a bad state and then continuously failing. Maybe > send a successful event between each pair of failing events? Done. https://codereview.chromium.org/1884883005/diff/420001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/420001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:79: SyntheticGestureParams::GestureSourceType PointerSourceType() const override { I know it is confusing that these two functions are very similar, but they are different. One is a default source type and the other one is the actual type. Do you think it is fine to keep both. Or we can keep GetDefaultSyntheticGestureSourceType in the parent class. The default function is only called if the source type is not setup.
https://codereview.chromium.org/1884883005/diff/420001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/1884883005/diff/420001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:42: switch (params.pointer_action_type()) Are you missing a brace here? I'm a bit surprised this is working. https://codereview.chromium.org/1884883005/diff/420001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:43: case SyntheticPointerActionParams::PointerActionType::PRESS: { Maybe the brace from this line should be up after the switch? https://codereview.chromium.org/1884883005/diff/420001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/420001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:79: SyntheticGestureParams::GestureSourceType PointerSourceType() const override { On 2016/06/16 07:54:24, lanwei wrote: > I know it is confusing that these two functions are very similar, but they are > different. One is a default source type and the other one is the actual type. Do > you think it is fine to keep both. Or we can keep > GetDefaultSyntheticGestureSourceType in the parent class. The default function > is only called if the source type is not setup. Could we just only use the default?
https://codereview.chromium.org/1884883005/diff/420001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/1884883005/diff/420001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:42: switch (params.pointer_action_type()) On 2016/06/23 14:22:50, tdresser wrote: > Are you missing a brace here? > > I'm a bit surprised this is working. Thanks for catching this, sorry for this kind of mistake.
LGTM, but get a review from Sam and a security owner for the owners change. https://codereview.chromium.org/1884883005/diff/440001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/1884883005/diff/440001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:43: bool UserInputCheck(const SyntheticPointerActionParams& params); Let's add a comment here explaining what UserInputCheck does. Do we eventually want to determine an error string here too? If so, we might want a TODO for that here. https://codereview.chromium.org/1884883005/diff/440001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/440001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:95: void CreateSyntheticPointerAndTarget() { It looks like you're adding some complexity to make it easier to test mouse in the future. I'd say keep things simple for now, and add that complexity when it's actually going to be used. https://codereview.chromium.org/1884883005/diff/440001/content/common/input/O... File content/common/input/OWNERS (right): https://codereview.chromium.org/1884883005/diff/440001/content/common/input/O... content/common/input/OWNERS:5: per-file *_param_traits*.*=file://ipc/SECURITY_OWNERS Can you add one of the SECURITY_OWNERS to this review to review the change to this file?
Patchset #14 (id:460001) has been deleted
Patchset #14 (id:480001) has been deleted
https://codereview.chromium.org/1884883005/diff/440001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/1884883005/diff/440001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:43: bool UserInputCheck(const SyntheticPointerActionParams& params); On 2016/06/28 15:44:11, tdresser wrote: > Let's add a comment here explaining what UserInputCheck does. Do we eventually > want to determine an error string here too? If so, we might want a TODO for that > here. Maybe not, this function will just return true or false, but SyntheticGesture::Result will have more details. https://codereview.chromium.org/1884883005/diff/440001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/440001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:95: void CreateSyntheticPointerAndTarget() { On 2016/06/28 15:44:11, tdresser wrote: > It looks like you're adding some complexity to make it easier to test mouse in > the future. I'd say keep things simple for now, and add that complexity when > it's actually going to be used. Done. https://codereview.chromium.org/1884883005/diff/440001/content/common/input/O... File content/common/input/OWNERS (right): https://codereview.chromium.org/1884883005/diff/440001/content/common/input/O... content/common/input/OWNERS:5: per-file *_param_traits*.*=file://ipc/SECURITY_OWNERS On 2016/06/28 15:44:11, tdresser wrote: > Can you add one of the SECURITY_OWNERS to this review to review the change to > this file? Will do!
Still LGTM https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer.h (right): https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer.h:43: virtual SyntheticGestureParams::GestureSourceType PointerSourceType() We could just call this "SourceType", or even "Source". The fact that we're referring to the pointer is implicit, since it's in the SyntheticPointer class. https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:94: void SetUp() override { Prefer code in the constructor to code in SetUp. https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:108: pointer_action_.reset(); These resets shouldn't be necessary. https://codereview.chromium.org/1884883005/diff/500001/content/common/input/O... File content/common/input/OWNERS (right): https://codereview.chromium.org/1884883005/diff/500001/content/common/input/O... content/common/input/OWNERS:5: per-file *_param_traits*.*=file://ipc/SECURITY_OWNERS Add someone from SECURITY_OWNERS as a reviewer, to make sure this is correct. https://codereview.chromium.org/1884883005/diff/500001/content/common/input/i... File content/common/input/input_param_traits_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/500001/content/common/input/i... content/common/input/input_param_traits_unittest.cc:317: new SyntheticPointerActionParams( Apparently we're supposed to be using MakeUnique now: https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?rcl=0&l=48
https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:23: index_map_(index_map) {} In a previous discussion, we decided that the SyntheticGestureController should own the SyntheticMousePointer and SyntheticTouchPointer objects. I'm guessing it should also own the IndexMap? The SyntheticGesture/SyntheticPointerAction object gets created in RenderWidgetHostImpl::OnQueueSyntheticGesture, which runs before the SyntheticGestureController is created in RenderWidgetHostImpl::QueueSyntheticGesture. So I think that rather than have this second constructor, we should set the SyntheticPointer (and IndexMap?) via a setter. WDYT? https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:31: return ForwardTouchOrMouseInputEvents(timestamp, target); Does this still need to be a separate function, or can it be inlined?
https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:23: index_map_(index_map) {} On 2016/07/08 18:38:34, samuong wrote: > In a previous discussion, we decided that the SyntheticGestureController should > own the SyntheticMousePointer and SyntheticTouchPointer objects. I'm guessing it > should also own the IndexMap? Yes, it is commented in its head file. > The SyntheticGesture/SyntheticPointerAction object gets created in > RenderWidgetHostImpl::OnQueueSyntheticGesture, which runs before the > SyntheticGestureController is created in > RenderWidgetHostImpl::QueueSyntheticGesture. So I think that rather than have > this second constructor, we should set the SyntheticPointer (and IndexMap?) via > a setter. WDYT? I think it maybe not clear if you just look at this class. Once I create the SyntheticPointerActionController, it will be clear. SyntheticPointerActionController is always running, that is why I let it own and keep the synthetic_pointer and index_map. When we receive a sequence of pointer actions, we will create a new synthetic_pointer and index_map. We start queue and forward each action, and we will create a new SyntheticPointerAction for each action. That is why we need this constructor. https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:31: return ForwardTouchOrMouseInputEvents(timestamp, target); On 2016/07/08 18:38:35, samuong wrote: > Does this still need to be a separate function, or can it be inlined? This will be changed in the next patch, so I think it is better to leave like this.
OK I see, that's fine with me then. LGTM.
lanwei@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer.h (right): https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer.h:43: virtual SyntheticGestureParams::GestureSourceType PointerSourceType() On 2016/07/08 12:22:14, tdresser wrote: > We could just call this "SourceType", or even "Source". > > The fact that we're referring to the pointer is implicit, since it's in the > SyntheticPointer class. Done. https://codereview.chromium.org/1884883005/diff/500001/content/common/input/i... File content/common/input/input_param_traits_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/500001/content/common/input/i... content/common/input/input_param_traits_unittest.cc:317: new SyntheticPointerActionParams( On 2016/07/08 12:22:14, tdresser wrote: > Apparently we're supposed to be using MakeUnique now: > https://cs.chromium.org/chromium/src/base/memory/ptr_util.h?rcl=0&l=48 Acknowledged.
ipc lgtm https://codereview.chromium.org/1884883005/diff/520001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture.cc (right): https://codereview.chromium.org/1884883005/diff/520001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture.cc:48: return std::unique_ptr<SyntheticGesture>(); Nit: this can be written as return nullptr https://codereview.chromium.org/1884883005/diff/520001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/1884883005/diff/520001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:18: std::unique_ptr<std::vector<SyntheticPointerActionParams>> param_list, FWIW, vector is movable in C++11, so the unique_ptr indirection isn't really necessary. Unfortunately, Chromium style doesn't allow writing std::vector<>&&, so that means we can't force the input to be an efficient move. So if you think this is better and less prone to error, I'm fine with that.
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from samuong@chromium.org, tdresser@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1884883005/#ps540001 (title: "return nullptr")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Prepare SyntheticPointerAction to handle touch actions for multiple fingers This patch is to make SyntheticPointerAction handle touch actions for multiple fingers, which will be dispatched together in one frame. Next patch: Modify SyntheticGestureController to queue all the actions in one SyntheticPointerAction until we see a PROCESS action, then pushing in a queue and flush. Make sure mouse actions also work. Put the TouchActionSequence in the GpuBenchmarking for now. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ========== to ========== Prepare SyntheticPointerAction to handle touch actions for multiple fingers This patch is to make SyntheticPointerAction handle touch actions for multiple fingers, which will be dispatched together in one frame. Next patch: Modify SyntheticGestureController to queue all the actions in one SyntheticPointerAction until we see a PROCESS action, then pushing in a queue and flush. Make sure mouse actions also work. Put the TouchActionSequence in the GpuBenchmarking for now. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:540001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Prepare SyntheticPointerAction to handle touch actions for multiple fingers This patch is to make SyntheticPointerAction handle touch actions for multiple fingers, which will be dispatched together in one frame. Next patch: Modify SyntheticGestureController to queue all the actions in one SyntheticPointerAction until we see a PROCESS action, then pushing in a queue and flush. Make sure mouse actions also work. Put the TouchActionSequence in the GpuBenchmarking for now. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 ========== to ========== Prepare SyntheticPointerAction to handle touch actions for multiple fingers This patch is to make SyntheticPointerAction handle touch actions for multiple fingers, which will be dispatched together in one frame. Next patch: Modify SyntheticGestureController to queue all the actions in one SyntheticPointerAction until we see a PROCESS action, then pushing in a queue and flush. Make sure mouse actions also work. Put the TouchActionSequence in the GpuBenchmarking for now. Please see the design doc here: https://docs.google.com/document/d/1adz5Xb1hYt6SCRdMdhR2EXjhvF-AqshCIhZv7jVuZF4 BUG=525187 Committed: https://crrev.com/a9d9a6721c939076b6c688b137378f2c4b939eb9 Cr-Commit-Position: refs/heads/master@{#405222} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/a9d9a6721c939076b6c688b137378f2c4b939eb9 Cr-Commit-Position: refs/heads/master@{#405222}
Message was sent while issue was closed.
https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:94: void SetUp() override { On 2016/07/08 12:22:14, tdresser wrote: > Prefer code in the constructor to code in SetUp. Done. https://codereview.chromium.org/1884883005/diff/500001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:108: pointer_action_.reset(); On 2016/07/08 12:22:14, tdresser wrote: > These resets shouldn't be necessary. Done. https://codereview.chromium.org/1884883005/diff/500001/content/common/input/O... File content/common/input/OWNERS (right): https://codereview.chromium.org/1884883005/diff/500001/content/common/input/O... content/common/input/OWNERS:5: per-file *_param_traits*.*=file://ipc/SECURITY_OWNERS On 2016/07/08 12:22:14, tdresser wrote: > Add someone from SECURITY_OWNERS as a reviewer, to make sure this is correct. Done. https://codereview.chromium.org/1884883005/diff/520001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture.cc (right): https://codereview.chromium.org/1884883005/diff/520001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture.cc:48: return std::unique_ptr<SyntheticGesture>(); On 2016/07/13 12:18:12, dcheng wrote: > Nit: this can be written as return nullptr Acknowledged. https://codereview.chromium.org/1884883005/diff/520001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/1884883005/diff/520001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:18: std::unique_ptr<std::vector<SyntheticPointerActionParams>> param_list, On 2016/07/13 12:18:12, dcheng wrote: > FWIW, vector is movable in C++11, so the unique_ptr indirection isn't really > necessary. Unfortunately, Chromium style doesn't allow writing std::vector<>&&, > so that means we can't force the input to be an efficient move. So if you think > this is better and less prone to error, I'm fine with that. We think it is safe to use unique_ptr here to guarantee always moving the vector. |
