Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2336803003:
Make SyntheticPointerAction to flush the pointer action sequence (Closed)
|
Created:
4 years, 3 months ago by lanwei Modified:
4 years ago CC:
chromium-reviews, extensions-reviews_chromium.org, mlamouri+watch-content_chromium.org, jam, dtapuska+chromiumwatch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, chromium-apps-reviews_chromium.org, dtapuska Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake SyntheticPointerAction to flush the pointer action sequence
After we receive the action sequence of multiple pointers from users, we will parse
them and group them into a two-dimensional array. The inner array will be the actions
that happen at the same time, so SyntheticPointerAction will dispatch them in the
same frame, until it process the whole sequence.
Previous patch: https://codereview.chromium.org/1884883005/
https://codereview.chromium.org/2178153002/
BUG=525187
Committed: https://crrev.com/3df2fbbf66d2f680128f655508a4817076d08ae2
Cr-Commit-Position: refs/heads/master@{#440237}
Patch Set 1 : controller #
Total comments: 4
Patch Set 2 : controller #
Total comments: 12
Patch Set 3 : controller #
Total comments: 14
Patch Set 4 : pointer controller #
Total comments: 16
Patch Set 5 : controller #
Total comments: 16
Patch Set 6 : controller #
Total comments: 1
Patch Set 7 : return invaid when index is out bound #
Total comments: 24
Patch Set 8 : synthetic action #
Total comments: 7
Patch Set 9 : controller #
Total comments: 13
Patch Set 10 : controller #Messages
Total messages: 179 (138 generated)
Description was changed from ========== controller controller BUG=525187 ========== to ========== Add SyntheticPointerActionController to flush the pointer action sequence. BUG=525187 ==========
lanwei@chromium.org changed reviewers: + dtapuska@chromium.org, samuong@chromium.org, tdresser@chromium.org
Patchset #1 (id:1) has been deleted
Could you please give me some feedbacks of the design and structures in this rough patch, thanks?
Description was changed from ========== Add SyntheticPointerActionController to flush the pointer action sequence. BUG=525187 ========== to ========== Add SyntheticPointerActionController to flush the pointer action sequence. After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so we will dispatch them in the same frame. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 ==========
lanwei@chromium.org changed reviewers: + nzolghadr@chromium.org
The code in GpuBenchmarking::TouchActionSequence is not correct, we receive a 2-D array, but I pretend it is a just one action. I put the file in patch is just to give an idea of the code flow. The real implementation will be in the next patch. https://codereview.chromium.org/2336803003/diff/20001/content/renderer/gpu/gp... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://codereview.chromium.org/2336803003/diff/20001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:948: if (!GetArg(args, &pointer_action_type_int)) { This is a simple version of getting just one action, I think Sam has patch to process a 2-D array.
I'm still looking into this but at a high level I think it's getting complicated having both a SyntheticGestureController and a SyntheticPointerActionController. After this lands, we should probably look at removing support for the high-level gestures and modifying gpu benchmarking and devtools to use the low-level pointer actions instead. Also, I think you need to rebase? https://codereview.chromium.org/2336803003/diff/20001/content/content_browser... File content/content_browser.gypi (right): https://codereview.chromium.org/2336803003/diff/20001/content/content_browser... content/content_browser.gypi:1205: 'browser/renderer_host/input/synthetic_pointer_action_controller.h', Is this still needed now that we're off of GYP and onto GN? https://codereview.chromium.org/2336803003/diff/20001/content/renderer/gpu/gp... File content/renderer/gpu/gpu_benchmarking_extension.cc (right): https://codereview.chromium.org/2336803003/diff/20001/content/renderer/gpu/gp... content/renderer/gpu/gpu_benchmarking_extension.cc:948: if (!GetArg(args, &pointer_action_type_int)) { On 2016/09/15 20:26:50, lanwei wrote: > This is a simple version of getting just one action, I think Sam has patch to > process a 2-D array. Since this is just sample code, maybe this should go into a test?
On 2016/09/23 12:53:40, samuong wrote: > I'm still looking into this but at a high level I think it's getting complicated > having both a SyntheticGestureController and a SyntheticPointerActionController. > After this lands, we should probably look at removing support for the high-level > gestures and modifying gpu benchmarking and devtools to use the low-level > pointer actions instead. Yes, I was thinking about this, but I am not sure if we really want to remove all the high level gestures, because they are easy to use for telemetry or devtool to simulate the common actions, like tap, scroll, or pinch. But we rewrite them by creating a sequence of synthetic pointer actions and then we can remove SyntheticGestureController. > > Also, I think you need to rebase? > > https://codereview.chromium.org/2336803003/diff/20001/content/content_browser... > File content/content_browser.gypi (right): > > https://codereview.chromium.org/2336803003/diff/20001/content/content_browser... > content/content_browser.gypi:1205: > 'browser/renderer_host/input/synthetic_pointer_action_controller.h', > Is this still needed now that we're off of GYP and onto GN? > > https://codereview.chromium.org/2336803003/diff/20001/content/renderer/gpu/gp... > File content/renderer/gpu/gpu_benchmarking_extension.cc (right): > > https://codereview.chromium.org/2336803003/diff/20001/content/renderer/gpu/gp... > content/renderer/gpu/gpu_benchmarking_extension.cc:948: if (!GetArg(args, > &pointer_action_type_int)) { > On 2016/09/15 20:26:50, lanwei wrote: > > This is a simple version of getting just one action, I think Sam has patch to > > process a 2-D array. > > Since this is just sample code, maybe this should go into a test?
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Tim, can you please take a first look at this one? I am passing a vector of actions through the IPC now, thanks! https://codereview.chromium.org/2336803003/diff/20001/content/content_browser... File content/content_browser.gypi (right): https://codereview.chromium.org/2336803003/diff/20001/content/content_browser... content/content_browser.gypi:1205: 'browser/renderer_host/input/synthetic_pointer_action_controller.h', On 2016/09/23 12:53:40, samuong wrote: > Is this still needed now that we're off of GYP and onto GN? I am not sure if the gyp is depreciated completely, or we can still use both?
Sorry, I haven't gotten through much of this yet. A few early thoughts. https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller.cc (right): https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller.cc:48: synthetic_gesture = SyntheticGesture::Create(gesture_params); Use {} for all clauses. https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller.cc:48: synthetic_gesture = SyntheticGesture::Create(gesture_params); I'm not sure this logic (converting to a SyntheticGesture) belongs here. Perhaps in RWHI? https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller.h (right): https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller.h:43: const OnGestureCompleteCallback& completion_callback); Why are we passing raw Params here? Couldn't we make a SyntheticGesture out of the params and use the existing method? https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (left): https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1749: weak_factory_.GetWeakPtr())); Why did we stop doing things this way? Perhaps the logic in SyntheticGestureController::QueueSyntheticPointerAction that creates a SyntheticGesture should be moved here? https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2036: << (int)result; Remove log. https://codereview.chromium.org/2336803003/diff/60001/content/common/input/in... File content/common/input/input_param_traits.cc (right): https://codereview.chromium.org/2336803003/diff/60001/content/common/input/in... content/common/input/input_param_traits.cc:89: case content::SyntheticGestureParams::POINTER_ACTION: Is this still used? Can we just send a list of length 1?
Ping - what's the state of this?
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lanwei@chromium.org changed reviewers: + mbarbella@chromium.org
https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller.cc (right): https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller.cc:48: synthetic_gesture = SyntheticGesture::Create(gesture_params); On 2016/09/30 13:24:30, tdresser wrote: > Use {} for all clauses. Done. https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller.cc:48: synthetic_gesture = SyntheticGesture::Create(gesture_params); On 2016/09/30 13:24:30, tdresser wrote: > I'm not sure this logic (converting to a SyntheticGesture) belongs here. Perhaps > in RWHI? Done. https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_gesture_controller.h (right): https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/synthetic_gesture_controller.h:43: const OnGestureCompleteCallback& completion_callback); On 2016/09/30 13:24:30, tdresser wrote: > Why are we passing raw Params here? Couldn't we make a SyntheticGesture out of > the params and use the existing method? Done. https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2336803003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:2036: << (int)result; On 2016/09/30 13:24:30, tdresser wrote: > Remove log. Done. https://codereview.chromium.org/2336803003/diff/60001/content/common/input/in... File content/common/input/input_param_traits.cc (right): https://codereview.chromium.org/2336803003/diff/60001/content/common/input/in... content/common/input/input_param_traits.cc:89: case content::SyntheticGestureParams::POINTER_ACTION: On 2016/09/30 13:24:30, tdresser wrote: > Is this still used? Can we just send a list of length 1? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2336803003/diff/120001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.cc (right): https://codereview.chromium.org/2336803003/diff/120001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:28: DCHECK_GE(gesture_params.param_list.size(), 0u); I might be missing something, but it seems like it may actually be possible to call this with an empty list (at least in the compromised renderer case). Could you either account for this or point me to where the validation is done? https://codereview.chromium.org/2336803003/diff/120001/tools/ipc_fuzzer/fuzze... File tools/ipc_fuzzer/fuzzer/fuzzer.cc (right): https://codereview.chromium.org/2336803003/diff/120001/tools/ipc_fuzzer/fuzze... tools/ipc_fuzzer/fuzzer/fuzzer.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. I tried patching these changes in to try it out, but it doesn't seem to build. Have you tried it locally? See https://chromium.googlesource.com/chromium/src/+/master/docs/ipc_fuzzer.md for instructions. (There seems to be an unrelated CHECK failure if you end up trying to run it locally, I'm just concerned with building at this point. I'll fix the CHECK failure soon, haven't looked at this code in a while.) https://codereview.chromium.org/2336803003/diff/120001/tools/ipc_fuzzer/fuzze... tools/ipc_fuzzer/fuzzer/fuzzer.cc:957: content::SyntheticPointerActionListParams::PointerActionType Should this be content::SyntheticPointerActionParams::PointerActionType? The compile failure mentioned above was from this line. https://codereview.chromium.org/2336803003/diff/120001/tools/ipc_fuzzer/fuzze... tools/ipc_fuzzer/fuzzer/fuzzer.cc:972: new content::SyntheticPointerActionListParams(action_params); Ideally you should use the constructor that takes a vector. See line 862 for an example.
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #4 (id:120001) has been deleted
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:140001) has been deleted
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/21 22:47:08, Martin Barbella wrote: > https://codereview.chromium.org/2336803003/diff/120001/content/browser/render... > File content/browser/renderer_host/input/synthetic_pointer_action_controller.cc > (right): > > https://codereview.chromium.org/2336803003/diff/120001/content/browser/render... > content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:28: > DCHECK_GE(gesture_params.param_list.size(), 0u); > I might be missing something, but it seems like it may actually be possible to > call this with an empty list (at least in the compromised renderer case). Could > you either account for this or point me to where the validation is done? The check is not necessary, I delete it. The code that prepares the SyntheticPointerActionListParams object is not in this patch, it is in gpu_benchmarking_extension.cc. You are take a look at this patch https://codereview.chromium.org/2348183003/diff/1/content/renderer/gpu/gpu_be.... I will make sure the list is not empty. > > https://codereview.chromium.org/2336803003/diff/120001/tools/ipc_fuzzer/fuzze... > File tools/ipc_fuzzer/fuzzer/fuzzer.cc (right): > > https://codereview.chromium.org/2336803003/diff/120001/tools/ipc_fuzzer/fuzze... > tools/ipc_fuzzer/fuzzer/fuzzer.cc:1: // Copyright 2015 The Chromium Authors. All > rights reserved. > I tried patching these changes in to try it out, but it doesn't seem to build. > Have you tried it locally? See > https://chromium.googlesource.com/chromium/src/+/master/docs/ipc_fuzzer.md for > instructions. > > (There seems to be an unrelated CHECK failure if you end up trying to run it > locally, I'm just concerned with building at this point. I'll fix the CHECK > failure soon, haven't looked at this code in a while.) Thank you! > https://codereview.chromium.org/2336803003/diff/120001/tools/ipc_fuzzer/fuzze... > tools/ipc_fuzzer/fuzzer/fuzzer.cc:957: > content::SyntheticPointerActionListParams::PointerActionType > Should this be content::SyntheticPointerActionParams::PointerActionType? The > compile failure mentioned above was from this line. Yes, Thank you! > https://codereview.chromium.org/2336803003/diff/120001/tools/ipc_fuzzer/fuzze... > tools/ipc_fuzzer/fuzzer/fuzzer.cc:972: new > content::SyntheticPointerActionListParams(action_params); > Ideally you should use the constructor that takes a vector. See line 862 for an > example. Done.
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:160001) has been deleted
Patchset #3 (id:180001) has been deleted
On 2016/10/23 21:48:38, lanwei wrote: > On 2016/10/21 22:47:08, Martin Barbella wrote: > > > https://codereview.chromium.org/2336803003/diff/120001/content/browser/render... > > File > content/browser/renderer_host/input/synthetic_pointer_action_controller.cc > > (right): > > > > > https://codereview.chromium.org/2336803003/diff/120001/content/browser/render... > > content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:28: > > DCHECK_GE(gesture_params.param_list.size(), 0u); > > I might be missing something, but it seems like it may actually be possible to > > call this with an empty list (at least in the compromised renderer case). > Could > > you either account for this or point me to where the validation is done? > > > The check is not necessary, I delete it. The code that prepares the > SyntheticPointerActionListParams object is not in this patch, it is in > gpu_benchmarking_extension.cc. > You are take a look at this patch > https://codereview.chromium.org/2348183003/diff/1/content/renderer/gpu/gpu_be.... > I will make sure the list is not empty. The problem is that the creation is happening renderer side, and the browser process can't trust the renderer to do validation. Are there any other cases where having an empty param_list would be problematic?
On 2016/10/24 17:10:52, Martin Barbella wrote: > On 2016/10/23 21:48:38, lanwei wrote: > > On 2016/10/21 22:47:08, Martin Barbella wrote: > > > > > > https://codereview.chromium.org/2336803003/diff/120001/content/browser/render... > > > File > > content/browser/renderer_host/input/synthetic_pointer_action_controller.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2336803003/diff/120001/content/browser/render... > > > > content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:28: > > > DCHECK_GE(gesture_params.param_list.size(), 0u); > > > I might be missing something, but it seems like it may actually be possible > to > > > call this with an empty list (at least in the compromised renderer case). > > Could > > > you either account for this or point me to where the validation is done? > > > > > > The check is not necessary, I delete it. The code that prepares the > > SyntheticPointerActionListParams object is not in this patch, it is in > > gpu_benchmarking_extension.cc. > > You are take a look at this patch > > > https://codereview.chromium.org/2348183003/diff/1/content/renderer/gpu/gpu_be.... > > I will make sure the list is not empty. > > The problem is that the creation is happening renderer side, and the browser > process can't trust the renderer to do validation. Are there any other cases > where having an empty param_list would be problematic? First, we would not pass an empty list. If we did, it would not be a problem, we just would not do anything, no action created.
On 2016/10/24 18:00:29, lanwei wrote: > On 2016/10/24 17:10:52, Martin Barbella wrote: > > On 2016/10/23 21:48:38, lanwei wrote: > > > On 2016/10/21 22:47:08, Martin Barbella wrote: > > > > > > > > > > https://codereview.chromium.org/2336803003/diff/120001/content/browser/render... > > > > File > > > content/browser/renderer_host/input/synthetic_pointer_action_controller.cc > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2336803003/diff/120001/content/browser/render... > > > > > > content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:28: > > > > DCHECK_GE(gesture_params.param_list.size(), 0u); > > > > I might be missing something, but it seems like it may actually be > possible > > to > > > > call this with an empty list (at least in the compromised renderer case). > > > Could > > > > you either account for this or point me to where the validation is done? > > > > > > > > > The check is not necessary, I delete it. The code that prepares the > > > SyntheticPointerActionListParams object is not in this patch, it is in > > > gpu_benchmarking_extension.cc. > > > You are take a look at this patch > > > > > > https://codereview.chromium.org/2348183003/diff/1/content/renderer/gpu/gpu_be.... > > > I will make sure the list is not empty. > > > > The problem is that the creation is happening renderer side, and the browser > > process can't trust the renderer to do validation. Are there any other cases > > where having an empty param_list would be problematic? > > First, we would not pass an empty list. If we did, it would not be a problem, we > just would not do anything, no action created. IPC and IPC fuzzer changes LGTM.
https://codereview.chromium.org/2336803003/diff/200001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.cc (right): https://codereview.chromium.org/2336803003/diff/200001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:30: if (gesture_source_type == SyntheticGestureParams::DEFAULT_INPUT) { If the synthetic pointer was already set, this silently does nothing. Should there be a DCHECK here? https://codereview.chromium.org/2336803003/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2336803003/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1187: SetSyntheticGestureController(); We call SetSyntheticGestureController before we ever call QueueSyntheticGesture, don't we? Is this needed? https://codereview.chromium.org/2336803003/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:2205: void RenderWidgetHostImpl::SetSyntheticGestureController() { This method is confusing. Should it be called CreateSyntheticGestureController? You aren't setting it to anything in particular. If so, we might want to DCHECK that this hasn't been called yet. https://codereview.chromium.org/2336803003/diff/200001/content/common/input/i... File content/common/input/input_param_traits_unittest.cc (left): https://codereview.chromium.org/2336803003/diff/200001/content/common/input/i... content/common/input/input_param_traits_unittest.cc:294: SyntheticPointerActionParams::PointerActionType::RELEASE); Should we still have release specific coverage, or are release and move similar enough that it isn't warranted? https://codereview.chromium.org/2336803003/diff/200001/content/common/input/s... File content/common/input/synthetic_pointer_action_list_params.h (right): https://codereview.chromium.org/2336803003/diff/200001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.h:22: SyntheticPointerActionListParams(SyntheticPointerActionParams param); Do we need this, or could we just pass in a vector of length 1? https://codereview.chromium.org/2336803003/diff/200001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.h:26: const SyntheticPointerActionListParams& other); Do we need to be able to copy these?
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Patchset #4 (id:220001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2336803003/diff/200001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.cc (right): https://codereview.chromium.org/2336803003/diff/200001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:30: if (gesture_source_type == SyntheticGestureParams::DEFAULT_INPUT) { On 2016/10/26 18:28:40, tdresser wrote: > If the synthetic pointer was already set, this silently does nothing. Should > there be a DCHECK here? You mean adding a DCHECK here for gesture_source_type? https://codereview.chromium.org/2336803003/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2336803003/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1187: SetSyntheticGestureController(); On 2016/10/26 18:28:40, tdresser wrote: > We call SetSyntheticGestureController before we ever call QueueSyntheticGesture, > don't we? > > Is this needed? Yes, there are some other places which also call this method without calling RenderWidgetHostImpl::OnQueueSyntheticGesture first, such as in InputHandler::SynthesizeTapGesture https://codesearch.chromium.org/chromium/src/content/browser/devtools/protoco... so we do need set SyntheticGestureControlle here. https://codereview.chromium.org/2336803003/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:2205: void RenderWidgetHostImpl::SetSyntheticGestureController() { On 2016/10/26 18:28:40, tdresser wrote: > This method is confusing. Should it be called CreateSyntheticGestureController? > You aren't setting it to anything in particular. > > If so, we might want to DCHECK that this hasn't been called yet. Done. https://codereview.chromium.org/2336803003/diff/200001/content/common/input/i... File content/common/input/input_param_traits_unittest.cc (left): https://codereview.chromium.org/2336803003/diff/200001/content/common/input/i... content/common/input/input_param_traits_unittest.cc:294: SyntheticPointerActionParams::PointerActionType::RELEASE); On 2016/10/26 18:28:40, tdresser wrote: > Should we still have release specific coverage, or are release and move similar > enough that it isn't warranted? Thanks for catching this, I delete by accident. https://codereview.chromium.org/2336803003/diff/200001/content/common/input/s... File content/common/input/synthetic_pointer_action_list_params.h (right): https://codereview.chromium.org/2336803003/diff/200001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.h:22: SyntheticPointerActionListParams(SyntheticPointerActionParams param); On 2016/10/26 18:28:40, tdresser wrote: > Do we need this, or could we just pass in a vector of length 1? Because most of the cases that we will only have one action_params, so I delete the constructor of std::vector<SyntheticPointerActionParams>. Is it ok? https://codereview.chromium.org/2336803003/diff/200001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.h:26: const SyntheticPointerActionListParams& other); On 2016/10/26 18:28:40, tdresser wrote: > Do we need to be able to copy these? Yes, because SyntheticGestureParams has it, all of its sub-classes have it
https://codereview.chromium.org/2336803003/diff/200001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.cc (right): https://codereview.chromium.org/2336803003/diff/200001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:30: if (gesture_source_type == SyntheticGestureParams::DEFAULT_INPUT) { On 2016/10/30 23:23:13, lanwei wrote: > On 2016/10/26 18:28:40, tdresser wrote: > > If the synthetic pointer was already set, this silently does nothing. Should > > there be a DCHECK here? > > You mean adding a DCHECK here for gesture_source_type? I'm proposing we add a DCHECK in the case where gesture_source_type was already set. https://codereview.chromium.org/2336803003/diff/200001/content/common/input/s... File content/common/input/synthetic_pointer_action_list_params.h (right): https://codereview.chromium.org/2336803003/diff/200001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.h:22: SyntheticPointerActionListParams(SyntheticPointerActionParams param); On 2016/10/30 23:23:13, lanwei wrote: > On 2016/10/26 18:28:40, tdresser wrote: > > Do we need this, or could we just pass in a vector of length 1? > > Because most of the cases that we will only have one action_params, so I delete > the constructor of std::vector<SyntheticPointerActionParams>. Is it ok? Acknowledged. https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1525: action_list_params, synthetic_pointer.get(), &index_map)); We use a single SyntheticPointer for two separate presses? That's a bit confusing. Should SyntheticPointer be SyntheticPointerType? Should we have a test which mixes touch and mouse input? https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:23: using IndexMap = std::array<int, WebTouchEvent::kTouchesLengthCap>; It might make sense to wrap this up in a class, so you can avoid exposing the initialization logic to the client. Clients shouldn't need to initialize the IndexMap before using this. https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:23: using IndexMap = std::array<int, WebTouchEvent::kTouchesLengthCap>; The way IndexMap is exposed to consumers is a bit clumsy. Perhaps it should be called something like "PointerState"? https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.cc (right): https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:20: std::unique_ptr<SyntheticGesture> synthetic_gesture = Use MakeUniq https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.h (right): https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.h:38: // object, and keep the states for all touch points. This comment is touch specific, but we use synthetic_pointer_ for mouse too. Could IndexMap live inside the SyntheticPointer? https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1189: return; Should this ever happen? Should there be a CHECK or DCHECK? https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1750: return; Should this ever happen?
https://codereview.chromium.org/2336803003/diff/240001/content/common/input/s... File content/common/input/synthetic_pointer_action_list_params.cc (right): https://codereview.chromium.org/2336803003/diff/240001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.cc:17: const SyntheticPointerActionListParams& other) Don't we need to call the cctor of the parent class here to copy gesture_source_type as well? I believe by default it just calls the default constructor of parent. Also now that the copy constructor is implemented should we also implement the = operator (and maybe move constructors)? https://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming) I can see it is not the case but I was just wondering if that was just missing that or was it intentional. On the other hand, do we even need the copy constructor here? It doesn't seem you have done anything particular here.
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:260001) has been deleted
Patchset #5 (id:280001) has been deleted
https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1525: action_list_params, synthetic_pointer.get(), &index_map)); On 2016/10/31 14:22:57, tdresser wrote: > We use a single SyntheticPointer for two separate presses? That's a bit > confusing. Should SyntheticPointer be SyntheticPointerType? > > Should we have a test which mixes touch and mouse input? We are planing to support multiple pointer types in one action sequence, but the implementation will be in the later patch. This patch will only support one pointer type. https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1525: action_list_params, synthetic_pointer.get(), &index_map)); On 2016/10/31 14:22:57, tdresser wrote: > We use a single SyntheticPointer for two separate presses? That's a bit > confusing. Should SyntheticPointer be SyntheticPointerType? > > Should we have a test which mixes touch and mouse input? Done. https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:23: using IndexMap = std::array<int, WebTouchEvent::kTouchesLengthCap>; On 2016/10/31 14:22:57, tdresser wrote: > It might make sense to wrap this up in a class, so you can avoid exposing the > initialization logic to the client. Clients shouldn't need to initialize the > IndexMap before using this. Done. https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.cc (right): https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.cc:20: std::unique_ptr<SyntheticGesture> synthetic_gesture = On 2016/10/31 14:22:58, tdresser wrote: > Use MakeUniq Acknowledged. https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_controller.h (right): https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_controller.h:38: // object, and keep the states for all touch points. On 2016/10/31 14:22:58, tdresser wrote: > This comment is touch specific, but we use synthetic_pointer_ for mouse too. > > Could IndexMap live inside the SyntheticPointer? Done. https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1189: return; On 2016/10/31 14:22:58, tdresser wrote: > Should this ever happen? Should there be a CHECK or DCHECK? Done. https://codereview.chromium.org/2336803003/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_impl.cc:1750: return; On 2016/10/31 14:22:58, tdresser wrote: > Should this ever happen? Done. https://codereview.chromium.org/2336803003/diff/240001/content/common/input/s... File content/common/input/synthetic_pointer_action_list_params.cc (right): https://codereview.chromium.org/2336803003/diff/240001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.cc:17: const SyntheticPointerActionListParams& other) On 2016/11/01 19:08:42, Navid Zolghadr wrote: > Don't we need to call the cctor of the parent class here to copy > gesture_source_type as well? I believe by default it just calls the default > constructor of parent. Also now that the copy constructor is implemented should > we also implement the = operator (and maybe move constructors)? > https://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming) > I can see it is not the case but I was just wondering if that was just missing > that or was it intentional. > > On the other hand, do we even need the copy constructor here? It doesn't seem > you have done anything particular here. Acknowledged. We have to keep this copy constructor here, in CreateGesture, https://cs.chromium.org/chromium/src/content/browser/renderer_host/input/synt... it calls all its subclasses' copy constructors.
Please update the CL description/title as well. https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:25: if (state_ == SETUP) { I assume this function needs to be called multiple times to flush all the vectors. Where do we use this function beside the tests? Should there be some sort of for loop or something? https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... File content/browser/renderer_host/input/synthetic_touch_driver.cc (right): https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... content/browser/renderer_host/input/synthetic_touch_driver.cc:32: index_map_[index] = touch_index; We are using an array and passing the index directly to this map array. We should validate the index input in all these functions as well as CheckInputUser. https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... File content/browser/renderer_host/input/synthetic_touch_driver.h (right): https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... content/browser/renderer_host/input/synthetic_touch_driver.h:20: using IndexMap = std::array<int, WebTouchEvent::kTouchesLengthCap>; Can we make this a private definition? https://codereview.chromium.org/2336803003/diff/300001/content/common/input/i... File content/common/input/input_param_traits_unittest.cc (right): https://codereview.chromium.org/2336803003/diff/300001/content/common/input/i... content/common/input/input_param_traits_unittest.cc:295: base::MakeUnique<SyntheticPointerActionListParams>(params); Could we just pass the "action_params" to MakeUnique and not create "params" at all as SyntheticPointerActionListParams has also a single argument constructor which takes care of this. We are already testing the push functionality in the other tests anyway. https://codereview.chromium.org/2336803003/diff/300001/content/common/input/s... File content/common/input/synthetic_pointer_action_list_params.cc (right): https://codereview.chromium.org/2336803003/diff/300001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.cc:45: const SyntheticPointerActionParams& param) { On way we can get away with this index is to just have this function get the whole list at once and push it on top. Right now it is not quite clear what we should do with invalid indices here. WDYT? https://codereview.chromium.org/2336803003/diff/300001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.cc:50: if (size > index) { We might also need to add index >=0. https://codereview.chromium.org/2336803003/diff/300001/content/common/input/s... File content/common/input/synthetic_pointer_action_list_params.h (right): https://codereview.chromium.org/2336803003/diff/300001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.h:23: SyntheticPointerActionListParams(ParamList& param_list); Could this be const &? https://codereview.chromium.org/2336803003/diff/300001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.h:34: std::vector<ParamList> params; Can we make this private?
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Navid, can you please take another look, thanks? https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:25: if (state_ == SETUP) { On 2016/12/06 17:05:38, Navid Zolghadr wrote: > I assume this function needs to be called multiple times to flush all the > vectors. Where do we use this function beside the tests? Should there be some > sort of for loop or something? https://codesearch.chromium.org/chromium/src/content/browser/renderer_host/in... The flush function in the SyntheticGestureController will check the result of this function and decide should flush again or call the callback. https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... File content/browser/renderer_host/input/synthetic_touch_driver.cc (right): https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... content/browser/renderer_host/input/synthetic_touch_driver.cc:32: index_map_[index] = touch_index; On 2016/12/06 17:05:38, Navid Zolghadr wrote: > We are using an array and passing the index directly to this map array. We > should validate the index input in all these functions as well as > CheckInputUser. Done. https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... File content/browser/renderer_host/input/synthetic_touch_driver.h (right): https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... content/browser/renderer_host/input/synthetic_touch_driver.h:20: using IndexMap = std::array<int, WebTouchEvent::kTouchesLengthCap>; On 2016/12/06 17:05:38, Navid Zolghadr wrote: > Can we make this a private definition? Done. https://codereview.chromium.org/2336803003/diff/300001/content/common/input/i... File content/common/input/input_param_traits_unittest.cc (right): https://codereview.chromium.org/2336803003/diff/300001/content/common/input/i... content/common/input/input_param_traits_unittest.cc:295: base::MakeUnique<SyntheticPointerActionListParams>(params); On 2016/12/06 17:05:38, Navid Zolghadr wrote: > Could we just pass the "action_params" to MakeUnique and not create "params" at > all as SyntheticPointerActionListParams has also a single argument constructor > which takes care of this. We are already testing the push functionality in the > other tests anyway. Done. https://codereview.chromium.org/2336803003/diff/300001/content/common/input/s... File content/common/input/synthetic_pointer_action_list_params.cc (right): https://codereview.chromium.org/2336803003/diff/300001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.cc:45: const SyntheticPointerActionParams& param) { On 2016/12/06 17:05:38, Navid Zolghadr wrote: > On way we can get away with this index is to just have this function get the > whole list at once and push it on top. Right now it is not quite clear what we > should do with invalid indices here. WDYT? You are right, exposing the index is not really safe. I will pass a list instead. https://codereview.chromium.org/2336803003/diff/300001/content/common/input/s... File content/common/input/synthetic_pointer_action_list_params.h (right): https://codereview.chromium.org/2336803003/diff/300001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.h:23: SyntheticPointerActionListParams(ParamList& param_list); On 2016/12/06 17:05:38, Navid Zolghadr wrote: > Could this be const &? Done. https://codereview.chromium.org/2336803003/diff/300001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.h:34: std::vector<ParamList> params; On 2016/12/06 17:05:38, Navid Zolghadr wrote: > Can we make this private? Because this has to pass through IPC channel, it is easy if it is public, and other synthetic gestures' parameters are all public.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add SyntheticPointerActionController to flush the pointer action sequence. After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so we will dispatch them in the same frame. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 ========== to ========== Make SyntheticPointerAction to flush the pointer action sequence After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so SyntheticPointerAction will dispatch them in the same frame, until it process the whole sequence. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 ==========
Description was changed from ========== Make SyntheticPointerAction to flush the pointer action sequence After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so SyntheticPointerAction will dispatch them in the same frame, until it process the whole sequence. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 ========== to ========== Make SyntheticPointerAction to flush the pointer action sequence After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so SyntheticPointerAction will dispatch them in the same frame, until it process the whole sequence. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 ==========
Description was changed from ========== Make SyntheticPointerAction to flush the pointer action sequence After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so SyntheticPointerAction will dispatch them in the same frame, until it process the whole sequence. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 ========== to ========== Make SyntheticPointerAction to flush the pointer action sequence After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so SyntheticPointerAction will dispatch them in the same frame, until it process the whole sequence. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 ==========
https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:25: if (state_ == SETUP) { On 2016/12/07 19:04:27, lanwei wrote: > On 2016/12/06 17:05:38, Navid Zolghadr wrote: > > I assume this function needs to be called multiple times to flush all the > > vectors. Where do we use this function beside the tests? Should there be some > > sort of for loop or something? > > https://codesearch.chromium.org/chromium/src/content/browser/renderer_host/in... > > The flush function in the SyntheticGestureController will check the result of > this function and decide should flush again or call the callback. So should that change be part of this patch or are you planning to update it later? https://codereview.chromium.org/2336803003/diff/320001/content/browser/render... File content/browser/renderer_host/input/synthetic_touch_driver.cc (right): https://codereview.chromium.org/2336803003/diff/320001/content/browser/render... content/browser/renderer_host/input/synthetic_touch_driver.cc:34: index_map_[index] = touch_index; I wonder if the debug check is enough. When users use the release builds and they pass an index less than 0 this could cause crash. Right? Do we need to worry about this case?
On 2016/12/09 16:04:47, Navid Zolghadr wrote: > https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... > File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): > > https://codereview.chromium.org/2336803003/diff/300001/content/browser/render... > content/browser/renderer_host/input/synthetic_pointer_action.cc:25: if (state_ > == SETUP) { > On 2016/12/07 19:04:27, lanwei wrote: > > On 2016/12/06 17:05:38, Navid Zolghadr wrote: > > > I assume this function needs to be called multiple times to flush all the > > > vectors. Where do we use this function beside the tests? Should there be > some > > > sort of for loop or something? > > > > > https://codesearch.chromium.org/chromium/src/content/browser/renderer_host/in... > > > > The flush function in the SyntheticGestureController will check the result of > > this function and decide should flush again or call the callback. > > So should that change be part of this patch or are you planning to update it > later? > > https://codereview.chromium.org/2336803003/diff/320001/content/browser/render... > File content/browser/renderer_host/input/synthetic_touch_driver.cc (right): > > https://codereview.chromium.org/2336803003/diff/320001/content/browser/render... > content/browser/renderer_host/input/synthetic_touch_driver.cc:34: > index_map_[index] = touch_index; > I wonder if the debug check is enough. When users use the release builds and > they pass an index less than 0 this could cause crash. Right? Do we need to > worry about this case? I now change it to check the index in SyntheticTouchDriver::UserInputCheck, and return invalid if the index is out of bound, then later when we receive the invalid, we will handler it in the callback function, or throw an error. What do you think?
lgtm
https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.cc (right): https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:22: mouse_event_ = SyntheticWebMouseEventBuilder::Build( Why did we switch from returning the index to passing it in? Doesn't that make the API more difficult to use?
https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:486: int count_; Be more specific in the variable name here. https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:1540: EXPECT_EQ(pointer_touch_target->states(0), WebTouchPoint::StatePressed); Can we add a helper to shorten this up? Maybe something that takes the pointer_touch_target, along with an index, position, and state? Take a look at https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid... https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.cc (right): https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:22: mouse_event_ = SyntheticWebMouseEventBuilder::Build( On 2016/12/12 15:32:49, tdresser wrote: > Why did we switch from returning the index to passing it in? Doesn't that make > the API more difficult to use? Acknowledged. https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:37: SyntheticPointerDriver::Create(gesture_source_type_); Should this happen only when the state is SETUP? https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:44: return SyntheticGesture::GESTURE_SOURCE_TYPE_NOT_IMPLEMENTED; Could we move this return up by the DCHECK that we aren't DEFAULT_INPUT? Then we don't need to check the gesture_source_type here. https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:64: param.set_gesture_source_type(gesture_source_type_); I know we've talked about this before, but I can't remember. Why do we need the gesture_source_type in each param as well as in the SyntheticPointerAction? Can't we only put it in one or the other, ideally in the SyntheticPointerAction? Do we allow mixing input types? https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:31: enum GestureState { SETUP, RUNNING, INVALID, DONE }; SETUP -> UNINITIALIZED? The rest of these, you could say: "Foo is GestureState" as in "Foo is RUNNING" "Foo is INVALID" "Foo is DONE" But SETUP is different. If you say "Foo is SETUP", it means something completely different! https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:42: size_t count_; Rename something like |params_dispatched_count_| https://codereview.chromium.org/2336803003/diff/340001/content/common/input/s... File content/common/input/synthetic_pointer_action_list_params.h (right): https://codereview.chromium.org/2336803003/diff/340001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.h:25: const SyntheticPointerActionListParams& other); Is this copy constructor required? Could this object be DISALLOW_COPY_AND_ASSIGN'ed? https://codereview.chromium.org/2336803003/diff/340001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/2336803003/diff/340001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:49: // For mouse pointers, the index should always be 0. Given the comment, let's switch the condition here, to check for mouse first. https://codereview.chromium.org/2336803003/diff/340001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:73: if (gesture_source_type_ != SyntheticGestureParams::MOUSE_INPUT) { Same as above, switch the clause order. https://codereview.chromium.org/2336803003/diff/340001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:96: // Pass an index value which is the index of the pointer in the list. This is a bit confusing. Maybe something like: // The index of the pointer in the list. Except that we should make it clear what list we're referring to.
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:360001) has been deleted
Patchset #8 (id:380001) has been deleted
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:400001) has been deleted
Patchset #8 (id:420001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Make SyntheticPointerAction to flush the pointer action sequence After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so SyntheticPointerAction will dispatch them in the same frame, until it process the whole sequence. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 ========== to ========== Make SyntheticPointerAction to flush the pointer action sequence After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so SyntheticPointerAction will dispatch them in the same frame, until it process the whole sequence. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:460001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:480001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #9 (id:500001) has been deleted
Patchset #8 (id:440001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Tim, I rebase the code, can you please take another look at the new patch, thanks? https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:37: SyntheticPointerDriver::Create(gesture_source_type_); On 2016/12/13 14:37:43, tdresser wrote: > Should this happen only when the state is SETUP? Done. https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:44: return SyntheticGesture::GESTURE_SOURCE_TYPE_NOT_IMPLEMENTED; On 2016/12/13 14:37:43, tdresser wrote: > Could we move this return up by the DCHECK that we aren't DEFAULT_INPUT? > Then we don't need to check the gesture_source_type here. Done. https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:64: param.set_gesture_source_type(gesture_source_type_); On 2016/12/13 14:37:43, tdresser wrote: > I know we've talked about this before, but I can't remember. Why do we need the > gesture_source_type in each param as well as in the SyntheticPointerAction? > Can't we only put it in one or the other, ideally in the SyntheticPointerAction? > > Do we allow mixing input types? Yes, the reason we have it in both SyntheticPointerActionParams and SyntheticPointerAction, is we want to support mixing input types later, so each SyntheticPointerActionParams should specify its own source type. But now we only support one type, so I make the SyntheticPointerAction has one source type. Should I remove the type from SyntheticPointerActionParams, and add it later, when we start to support multi-type. https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:31: enum GestureState { SETUP, RUNNING, INVALID, DONE }; On 2016/12/13 14:37:43, tdresser wrote: > SETUP -> UNINITIALIZED? > > The rest of these, you could say: > "Foo is GestureState" > as in > "Foo is RUNNING" > "Foo is INVALID" > "Foo is DONE" > > But SETUP is different. If you say > "Foo is SETUP", it means something completely different! Acknowledged. https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:42: size_t count_; On 2016/12/13 14:37:43, tdresser wrote: > Rename something like |params_dispatched_count_| Done. https://codereview.chromium.org/2336803003/diff/340001/content/common/input/s... File content/common/input/synthetic_pointer_action_list_params.h (right): https://codereview.chromium.org/2336803003/diff/340001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.h:25: const SyntheticPointerActionListParams& other); On 2016/12/13 14:37:43, tdresser wrote: > Is this copy constructor required? Could this object be > DISALLOW_COPY_AND_ASSIGN'ed? This is required for all the subclass of SyntheticGestureParams to create a gesture. https://codesearch.chromium.org/chromium/src/content/browser/renderer_host/in... https://codereview.chromium.org/2336803003/diff/340001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/2336803003/diff/340001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:49: // For mouse pointers, the index should always be 0. On 2016/12/13 14:37:44, tdresser wrote: > Given the comment, let's switch the condition here, to check for mouse first. Done. https://codereview.chromium.org/2336803003/diff/340001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:73: if (gesture_source_type_ != SyntheticGestureParams::MOUSE_INPUT) { On 2016/12/13 14:37:43, tdresser wrote: > Same as above, switch the clause order. Done. https://codereview.chromium.org/2336803003/diff/340001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:96: // Pass an index value which is the index of the pointer in the list. On 2016/12/13 14:37:44, tdresser wrote: > This is a bit confusing. > Maybe something like: > > // The index of the pointer in the list. > > Except that we should make it clear what list we're referring to. Done.
LGTM! https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/2336803003/diff/340001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:64: param.set_gesture_source_type(gesture_source_type_); On 2016/12/18 17:35:55, lanwei wrote: > On 2016/12/13 14:37:43, tdresser wrote: > > I know we've talked about this before, but I can't remember. Why do we need > the > > gesture_source_type in each param as well as in the SyntheticPointerAction? > > Can't we only put it in one or the other, ideally in the > SyntheticPointerAction? > > > > Do we allow mixing input types? > > Yes, the reason we have it in both SyntheticPointerActionParams and > SyntheticPointerAction, is we want to support mixing input types later, so each > SyntheticPointerActionParams should specify its own source type. But now we only > support one type, so I make the SyntheticPointerAction has one source type. > Should I remove the type from SyntheticPointerActionParams, and add it later, > when we start to support multi-type. Yeah, let's remove it for now. https://codereview.chromium.org/2336803003/diff/340001/content/common/input/s... File content/common/input/synthetic_pointer_action_list_params.h (right): https://codereview.chromium.org/2336803003/diff/340001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.h:25: const SyntheticPointerActionListParams& other); On 2016/12/18 17:35:55, lanwei wrote: > On 2016/12/13 14:37:43, tdresser wrote: > > Is this copy constructor required? Could this object be > > DISALLOW_COPY_AND_ASSIGN'ed? > This is required for all the subclass of SyntheticGestureParams to create a > gesture. > https://codesearch.chromium.org/chromium/src/content/browser/renderer_host/in... Acknowledged. https://codereview.chromium.org/2336803003/diff/520001/content/common/input/s... File content/common/input/synthetic_pointer_action_list_params.h (right): https://codereview.chromium.org/2336803003/diff/520001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.h:13: using blink::WebTouchEvent; Is this |using| needed?
Description was changed from ========== Make SyntheticPointerAction to flush the pointer action sequence After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so SyntheticPointerAction will dispatch them in the same frame, until it process the whole sequence. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Make SyntheticPointerAction to flush the pointer action sequence After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so SyntheticPointerAction will dispatch them in the same frame, until it process the whole sequence. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
lanwei@chromium.org changed reviewers: + avi@chromium.org
avi@ could you please take a look at content/common/BUILD.gn, thank you?
BUILD.gn LGTM Looks like you need an IPC review.
I'm still looking at this, but just a few small comments so far... https://codereview.chromium.org/2336803003/diff/60001/content/common/input/sy... File content/common/input/synthetic_pointer_action_list_params.h (right): https://codereview.chromium.org/2336803003/diff/60001/content/common/input/sy... content/common/input/synthetic_pointer_action_list_params.h:14: using blink::WebTouchEvent; is this needed? https://codereview.chromium.org/2336803003/diff/520001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/2336803003/diff/520001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:535: DCHECK(WebInputEvent::isTouchEventType(event.type)); why DCHECK and not ASSERT_TRUE? https://codereview.chromium.org/2336803003/diff/520001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.cc (right): https://codereview.chromium.org/2336803003/diff/520001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:22: DCHECK_EQ(index, 0); is this because only one synthetic mouse pointer is supported? is this documented anywhere?
https://codereview.chromium.org/2336803003/diff/520001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/2336803003/diff/520001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:535: DCHECK(WebInputEvent::isTouchEventType(event.type)); On 2016/12/20 05:13:26, samuong wrote: > why DCHECK and not ASSERT_TRUE? This isn't a test failure, it's improperly written test code (which is a bit subtle, but I think a distinction can be made). In one case, the code under test is wrong, in the other, the test code is broken. In general, I think it's best for test assertions to only show up in the main body of the test, so that they get reasonable line numbers.
lanwei@chromium.org changed reviewers: - dtapuska@chromium.org
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2336803003/diff/520001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/2336803003/diff/520001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:535: DCHECK(WebInputEvent::isTouchEventType(event.type)); On 2016/12/20 14:16:24, tdresser - OOO until Jan 3 wrote: > On 2016/12/20 05:13:26, samuong wrote: > > why DCHECK and not ASSERT_TRUE? > > This isn't a test failure, it's improperly written test code (which is a bit > subtle, but I think a distinction can be made). In one case, the code under test > is wrong, in the other, the test code is broken. > > In general, I think it's best for test assertions to only show up in the main > body of the test, so that they get reasonable line numbers. Acknowledged. https://codereview.chromium.org/2336803003/diff/520001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.cc (right): https://codereview.chromium.org/2336803003/diff/520001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:22: DCHECK_EQ(index, 0); On 2016/12/20 05:13:26, samuong wrote: > is this because only one synthetic mouse pointer is supported? is this > documented anywhere? Yes, because there is just one mouse cursor, supporting more than one mouses does not have any point. I will update the doc to state this. Also we only support one source type in one sequence right now. Will support multi-input later. I will also include this in the doc. https://codereview.chromium.org/2336803003/diff/520001/content/common/input/s... File content/common/input/synthetic_pointer_action_list_params.h (right): https://codereview.chromium.org/2336803003/diff/520001/content/common/input/s... content/common/input/synthetic_pointer_action_list_params.h:13: using blink::WebTouchEvent; On 2016/12/19 15:11:53, tdresser - OOO until Jan 3 wrote: > Is this |using| needed? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:75: return WebTouchPoint::StateUndefined; should this last check be handled by a default: case? https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:525: int actions_dispatched_times_; This name is a bit confusing. What do you think of "num_actions_dispatched_"? https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.cc (right): https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:47: DCHECK_EQ(params.index(), 0); If a WebDriver client sends a command to synthesize a pointer action, and sets the mouse index to something other than 0, we need to catch this somehow. If this function is the place where this check is supposed to happen, then we wouldn't want a DCHECK here. Should we just return false instead? https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:54: DCHECK_GE(actions_dispatched_count_, 0U); this is a size_t, which is unsigned, so this is unnecessary https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:40: size_t actions_dispatched_count_; i was initially a bit confused by the meaning of this - what do you think of "num_actions_dispatched_"? https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... File content/browser/renderer_host/input/synthetic_touch_driver.h (right): https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... content/browser/renderer_host/input/synthetic_touch_driver.h:35: using IndexMap = std::array<int, WebTouchEvent::kTouchesLengthCap>; I think you should just write blink::WebTouchEvent::kTouchesLengthCap here, and remove the using statement from the top. Otherwise any files which #include this header will pull in the using statement.
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:560001) has been deleted
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #10 (id:580001) has been deleted
Sam, can you please take another look, thanks? https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:75: return WebTouchPoint::StateUndefined; On 2016/12/21 00:02:26, samuong wrote: > should this last check be handled by a default: case? We already covered all the cased in the switch statements, so do not need a default, but the compiler does not know, so need a return outside the switch statements. https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:525: int actions_dispatched_times_; On 2016/12/21 00:02:26, samuong wrote: > This name is a bit confusing. What do you think of "num_actions_dispatched_"? Done. https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.cc (right): https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:47: DCHECK_EQ(params.index(), 0); On 2016/12/21 00:02:26, samuong wrote: > If a WebDriver client sends a command to synthesize a pointer action, and sets > the mouse index to something other than 0, we need to catch this somehow. > > If this function is the place where this check is supposed to happen, then we > wouldn't want a DCHECK here. Should we just return false instead? Because we do not use the index for mouse type, so I use DCHECK as a warning. But since we will put this requirement in the doc, and do not support multi-mouse devices, return false is better. Done. https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:54: DCHECK_GE(actions_dispatched_count_, 0U); On 2016/12/21 00:02:26, samuong wrote: > this is a size_t, which is unsigned, so this is unnecessary Done. https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:40: size_t actions_dispatched_count_; On 2016/12/21 00:02:27, samuong wrote: > i was initially a bit confused by the meaning of this - what do you think of > "num_actions_dispatched_"? Done. https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... File content/browser/renderer_host/input/synthetic_touch_driver.h (right): https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... content/browser/renderer_host/input/synthetic_touch_driver.h:35: using IndexMap = std::array<int, WebTouchEvent::kTouchesLengthCap>; On 2016/12/21 00:02:27, samuong wrote: > I think you should just write blink::WebTouchEvent::kTouchesLengthCap here, and > remove the using statement from the top. Otherwise any files which #include this > header will pull in the using statement. Done.
lgtm https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... File content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc (right): https://codereview.chromium.org/2336803003/diff/540001/content/browser/render... content/browser/renderer_host/input/synthetic_gesture_controller_unittest.cc:75: return WebTouchPoint::StateUndefined; On 2016/12/21 20:01:13, lanwei wrote: > On 2016/12/21 00:02:26, samuong wrote: > > should this last check be handled by a default: case? > > We already covered all the cased in the switch statements, so do not need a > default, but the compiler does not know, so need a return outside the switch > statements. Hmm ok. It works fine for me with clang on linux, but I'm not sure about the other platforms. If you're getting errors, I guess you'll have to handle this outside the switch.
The CQ bit was unchecked by lanwei@chromium.org
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mbarbella@chromium.org, nzolghadr@chromium.org, avi@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2336803003/#ps600001 (title: "controller")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Make SyntheticPointerAction to flush the pointer action sequence After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so SyntheticPointerAction will dispatch them in the same frame, until it process the whole sequence. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Make SyntheticPointerAction to flush the pointer action sequence After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so SyntheticPointerAction will dispatch them in the same frame, until it process the whole sequence. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 ==========
The CQ bit was checked by lanwei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 600001, "attempt_start_ts": 1482355658476640,
"parent_rev": "124a044a38ad8d2a93d0bca106acbef7a8c91543", "commit_rev":
"22b17e2a2fc3731b63966fc670a6477dc64298ba"}
Message was sent while issue was closed.
Description was changed from ========== Make SyntheticPointerAction to flush the pointer action sequence After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so SyntheticPointerAction will dispatch them in the same frame, until it process the whole sequence. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 ========== to ========== Make SyntheticPointerAction to flush the pointer action sequence After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so SyntheticPointerAction will dispatch them in the same frame, until it process the whole sequence. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 Review-Url: https://codereview.chromium.org/2336803003 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:600001)
Message was sent while issue was closed.
Description was changed from ========== Make SyntheticPointerAction to flush the pointer action sequence After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so SyntheticPointerAction will dispatch them in the same frame, until it process the whole sequence. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 Review-Url: https://codereview.chromium.org/2336803003 ========== to ========== Make SyntheticPointerAction to flush the pointer action sequence After we receive the action sequence of multiple pointers from users, we will parse them and group them into a two-dimensional array. The inner array will be the actions that happen at the same time, so SyntheticPointerAction will dispatch them in the same frame, until it process the whole sequence. Previous patch: https://codereview.chromium.org/1884883005/ https://codereview.chromium.org/2178153002/ BUG=525187 Committed: https://crrev.com/3df2fbbf66d2f680128f655508a4817076d08ae2 Cr-Commit-Position: refs/heads/master@{#440237} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/3df2fbbf66d2f680128f655508a4817076d08ae2 Cr-Commit-Position: refs/heads/master@{#440237} |
