Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2478423002:
Rename SyntheticPointer to SyntheticPointerDriver (Closed)
DescriptionRename SyntheticPointer to SyntheticPointerDriver
Rename SyntheticPointer to SyntheticPointerDriver, SyntheticTouchPointer to
SyntheticTouchDriver, and SyntheticMousePointer to SyntheticMouseDriver,
because these classes do not represent the synthetic pointers. They actually create
synthetic pointers for each pointer types and their actions.
BUG=525187
Committed: https://crrev.com/1060f1f73a8498e324d05fcfd2fdca621bc9b15e
Cr-Commit-Position: refs/heads/master@{#434763}
Patch Set 1 : rename #
Total comments: 4
Patch Set 2 : Move valid user input in SyntheticPointerDriver #
Total comments: 2
Patch Set 3 : rename #
Total comments: 20
Patch Set 4 : rename #
Total comments: 12
Patch Set 5 : rename #
Total comments: 7
Patch Set 6 : rename #Messages
Total messages: 102 (80 generated)
Description was changed from ========== rename pointer controller controller controller controller controller BUG= ========== to ========== Rename SyntheticPointer to SyntheticPointerDriver BUG=525187 ==========
Description was changed from ========== Rename SyntheticPointer to SyntheticPointerDriver BUG=525187 ========== to ========== Rename SyntheticPointer to SyntheticPointerDriver Rename SyntheticPointer to SyntheticPointerDriver, SyntheticTouchPointer to SyntheticTouchDriver, and SyntheticMousePointer to SyntheticMouseDriver BUG=525187 ==========
Description was changed from ========== Rename SyntheticPointer to SyntheticPointerDriver Rename SyntheticPointer to SyntheticPointerDriver, SyntheticTouchPointer to SyntheticTouchDriver, and SyntheticMousePointer to SyntheticMouseDriver BUG=525187 ========== to ========== Rename SyntheticPointer to SyntheticPointerDriver Rename SyntheticPointer to SyntheticPointerDriver, SyntheticTouchPointer to SyntheticTouchDriver, and SyntheticMousePointer to SyntheticMouseDriver, because these classes do not represent the synthetic pointers. They actually create synthetic pointers for each pointer types and their actions. BUG=525187 ==========
Description was changed from ========== Rename SyntheticPointer to SyntheticPointerDriver Rename SyntheticPointer to SyntheticPointerDriver, SyntheticTouchPointer to SyntheticTouchDriver, and SyntheticMousePointer to SyntheticMouseDriver, because these classes do not represent the synthetic pointers. They actually create synthetic pointers for each pointer types and their actions. BUG=525187 ========== to ========== Rename SyntheticPointer to SyntheticPointerDriver Rename SyntheticPointer to SyntheticPointerDriver, SyntheticTouchPointer to SyntheticTouchDriver, and SyntheticMousePointer to SyntheticMouseDriver, because these classes do not represent the synthetic pointers. They actually create synthetic pointers for each pointer types and their actions. BUG=525187 ==========
Description was changed from ========== Rename SyntheticPointer to SyntheticPointerDriver Rename SyntheticPointer to SyntheticPointerDriver, SyntheticTouchPointer to SyntheticTouchDriver, and SyntheticMousePointer to SyntheticMouseDriver, because these classes do not represent the synthetic pointers. They actually create synthetic pointers for each pointer types and their actions. BUG=525187 ========== to ========== Rename SyntheticPointer to SyntheticPointerDriver Rename SyntheticPointer to SyntheticPointerDriver, SyntheticTouchPointer to SyntheticTouchDriver, and SyntheticMousePointer to SyntheticMouseDriver, because these classes do not represent the synthetic pointers. They actually create synthetic pointers for each pointer types and their actions. BUG=525187 ==========
lanwei@chromium.org changed reviewers: + tdresser@chromium.org
Description was changed from ========== Rename SyntheticPointer to SyntheticPointerDriver Rename SyntheticPointer to SyntheticPointerDriver, SyntheticTouchPointer to SyntheticTouchDriver, and SyntheticMousePointer to SyntheticMouseDriver, because these classes do not represent the synthetic pointers. They actually create synthetic pointers for each pointer types and their actions. BUG=525187 ========== to ========== Rename SyntheticPointer to SyntheticPointerDriver Rename SyntheticPointer to SyntheticPointerDriver, SyntheticTouchPointer to SyntheticTouchDriver, and SyntheticMousePointer to SyntheticMouseDriver, because these classes do not represent the synthetic pointers. They actually create synthetic pointers for each pointer types and their actions. BUG=525187 ==========
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 #1 (id:1) 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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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 #1 (id:20001) 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 #2 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:40001) has been deleted
https://codereview.chromium.org/2478423002/diff/80001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/2478423002/diff/80001/content/browser/rendere... content/browser/renderer_host/input/synthetic_pointer_action.cc:66: params.index() >= WebTouchEvent::kTouchesLengthCap)) { I'd split this up to make it a bit more legible. bool touch_index_exceeds_bounds = ...; https://codereview.chromium.org/2478423002/diff/80001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_pointer_driver.h (right): https://codereview.chromium.org/2478423002/diff/80001/content/browser/rendere... content/browser/renderer_host/input/synthetic_pointer_driver.h:30: virtual int Press(int index, Why do we need to pass index now? If we do, why do we also return an int?
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/2478423002/diff/80001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/2478423002/diff/80001/content/browser/rendere... content/browser/renderer_host/input/synthetic_pointer_action.cc:66: params.index() >= WebTouchEvent::kTouchesLengthCap)) { On 2016/11/07 17:44:11, tdresser wrote: > I'd split this up to make it a bit more legible. > > bool touch_index_exceeds_bounds = ...; Done. https://codereview.chromium.org/2478423002/diff/80001/content/browser/rendere... File content/browser/renderer_host/input/synthetic_pointer_driver.h (right): https://codereview.chromium.org/2478423002/diff/80001/content/browser/rendere... content/browser/renderer_host/input/synthetic_pointer_driver.h:30: virtual int Press(int index, On 2016/11/07 17:44:11, tdresser wrote: > Why do we need to pass index now? > > If we do, why do we also return an int? I added the index because we keep index_map_ in SyntheticPointerDriver. You are right, I should remove return index, and I also removed unused arguments.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2478423002/diff/100001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.cc (right): https://codereview.chromium.org/2478423002/diff/100001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:21: void SyntheticMouseDriver::Press(float x, float y, int index) { Can we just get rid of the index completely, and use the first unavailable index for the SyntheticTouchDriver? If not, we should DCHECK that the index is 0, in case consumers miss calling UserInputCheck. https://codereview.chromium.org/2478423002/diff/100001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.h (right): https://codereview.chromium.org/2478423002/diff/100001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.h:36: bool UserInputCheck(const SyntheticPointerActionParams& params); Why did we get rid of this comment?
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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_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 #3 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.cc (right): https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:28: void SyntheticMouseDriver::Move(float x, float y, int index) { DCHECK that index is -1? https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:37: void SyntheticMouseDriver::Release(int index) { DCHECK that index is -1? https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:44: const SyntheticPointerActionParams& params) const { Ensure index is -1? https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:35: for (size_t i = 0; i < param_list_->size(); ++i) { Why did we switch away from a range based for loop? https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:55: default: Should we explicitly handle the other cases here? https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_tap_gesture.cc (right): https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_tap_gesture.cc:35: if (!synthetic_pointer_driver_) Add {} https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_touch_driver.cc (right): https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_touch_driver.cc:38: const SyntheticPointerActionParams& params) const { Should we check that the index is valid (we have a pointer for any move / release, and don't have a pointer for press?) https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_touch_driver.h (right): https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_touch_driver.h:33: SyntheticWebTouchEvent touch_event_; I'm a bit worried that the implementation of SyntheticWebTouchEvent is robust enough to expose to developers, but that's a separate patch. I'm not convinced it actually manages the index correctly in cases such as: Press finger 1. Press finger 2. Release finger 2. Press finger 2. Release finger 2. Press finger 2. Release finger 2. Can you take a look at how we behave in that case, and make sure it works correctly? https://codereview.chromium.org/2478423002/diff/140001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/2478423002/diff/140001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:32: IDLE, Can we add a comment here? Why did this name change? https://codereview.chromium.org/2478423002/diff/140001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:53: // We only set index for source types which are not mouse. We shouldn't silently not do anything here. if (gesture_source_type == MOUSE_INPUT) DCHECK(index == -1);
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.
https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.cc (right): https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:28: void SyntheticMouseDriver::Move(float x, float y, int index) { On 2016/11/10 14:58:31, tdresser wrote: > DCHECK that index is -1? I added a check in the params.index() for mouse, and we do not use index here, so I did not add the DCHECK. Now I added the check here so make sure that the index is always -1 for mouse. https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:37: void SyntheticMouseDriver::Release(int index) { On 2016/11/10 14:58:31, tdresser wrote: > DCHECK that index is -1? Done. https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:44: const SyntheticPointerActionParams& params) const { On 2016/11/10 14:58:31, tdresser wrote: > Ensure index is -1? Done. https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action.cc (right): https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:35: for (size_t i = 0; i < param_list_->size(); ++i) { On 2016/11/10 14:58:32, tdresser wrote: > Why did we switch away from a range based for loop? I thought I could not change params's index in its caller, I thought it is a copy. I was wrong, it works the same. https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action.cc:55: default: On 2016/11/10 14:58:32, tdresser wrote: > Should we explicitly handle the other cases here? The case for finish may change later, but 'return GESTURE_FINISHED' makes sense right now. https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_tap_gesture.cc (right): https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_tap_gesture.cc:35: if (!synthetic_pointer_driver_) On 2016/11/10 14:58:32, tdresser wrote: > Add {} Done. https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_touch_driver.cc (right): https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_touch_driver.cc:38: const SyntheticPointerActionParams& params) const { On 2016/11/10 14:58:32, tdresser wrote: > Should we check that the index is valid (we have a pointer for any move / > release, and don't have a pointer for press?) I added a DCHECK, because the params.index() is setted when we call press, so I was not sure if we need to check it. https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... File content/browser/renderer_host/input/synthetic_touch_driver.h (right): https://codereview.chromium.org/2478423002/diff/140001/content/browser/render... content/browser/renderer_host/input/synthetic_touch_driver.h:33: SyntheticWebTouchEvent touch_event_; On 2016/11/10 14:58:32, tdresser wrote: > I'm a bit worried that the implementation of SyntheticWebTouchEvent is robust > enough to expose to developers, but that's a separate patch. > > I'm not convinced it actually manages the index correctly in cases such as: > Press finger 1. > Press finger 2. > Release finger 2. > Press finger 2. > Release finger 2. > Press finger 2. > Release finger 2. > > Can you take a look at how we behave in that case, and make sure it works > correctly? It works fine, I added a test. https://codereview.chromium.org/2478423002/diff/140001/content/common/input/s... File content/common/input/synthetic_pointer_action_params.h (right): https://codereview.chromium.org/2478423002/diff/140001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:32: IDLE, On 2016/11/10 14:58:32, tdresser wrote: > Can we add a comment here? Why did this name change? Done. https://codereview.chromium.org/2478423002/diff/140001/content/common/input/s... content/common/input/synthetic_pointer_action_params.h:53: // We only set index for source types which are not mouse. On 2016/11/10 14:58:32, tdresser wrote: > We shouldn't silently not do anything here. > > if (gesture_source_type == MOUSE_INPUT) > DCHECK(index == -1); Done.
https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.cc (right): https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:24: mouse_event_.clickCount = 1; Currently, if you double click, we'll continue setting the click count to 1. Is that okay? https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:239: TEST_F(SyntheticPointerActionTest, PointerTouchActionWithIdle) { It's difficult to tell what this test is doing because it's so long. Can we shorten it by using some helper methods or a loop? https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:303: EXPECT_EQ(pointer_touch_target->type(), WebInputEvent::TouchStart); Should we add some helpers to make this cleaner? https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:379: EXPECT_EQ(action_param_list_->at(0).index(), 0); What is the index here? We should make sure that we reuse indices when appropriate. https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:215: MovePoint(target, delta, event_timestamp, -1); I don't think we should explicitly pass -1 here, we should probably just change the default instead. https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.h (right): https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:84: int index = 0); Should the default here be -1? https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... File content/browser/renderer_host/input/synthetic_tap_gesture.cc (right): https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... content/browser/renderer_host/input/synthetic_tap_gesture.cc:52: gesture_source_type_ == SyntheticGestureParams::MOUSE_INPUT ? -1 : 0; Hmmm, this is a bit ugly. Maybe the index for mouse should always be 0, instead of -1? Sorry for going back and forth here. If we consistently use 0 as the index, and 0 as the default, then this will be a lot cleaner...
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
Patchset #5 (id:180001) 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/2478423002/diff/160001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.cc (right): https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:24: mouse_event_.clickCount = 1; On 2016/11/12 19:50:55, tdresser wrote: > Currently, if you double click, we'll continue setting the click count to 1. > > Is that okay? I thought if you create a mouse press, release, press, release, and if they are in the double-click time range, then a mouse double click will be generated, same for double tap. I have not checked if they will be corrected created, I need to add the gesture controller to test it. Do you think I can add a todo and check it later once we have all the code to generate a gesture? https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:239: TEST_F(SyntheticPointerActionTest, PointerTouchActionWithIdle) { On 2016/11/12 19:50:55, tdresser wrote: > It's difficult to tell what this test is doing because it's so long. Can we > shorten it by using some helper methods or a loop? Done. https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc (right): https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... content/browser/renderer_host/input/synthetic_smooth_move_gesture.cc:215: MovePoint(target, delta, event_timestamp, -1); On 2016/11/12 19:50:55, tdresser wrote: > I don't think we should explicitly pass -1 here, we should probably just change > the default instead. Done. https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... File content/browser/renderer_host/input/synthetic_smooth_move_gesture.h (right): https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... content/browser/renderer_host/input/synthetic_smooth_move_gesture.h:84: int index = 0); On 2016/11/12 19:50:55, tdresser wrote: > Should the default here be -1? Done. https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... File content/browser/renderer_host/input/synthetic_tap_gesture.cc (right): https://codereview.chromium.org/2478423002/diff/160001/content/browser/render... content/browser/renderer_host/input/synthetic_tap_gesture.cc:52: gesture_source_type_ == SyntheticGestureParams::MOUSE_INPUT ? -1 : 0; On 2016/11/12 19:50:56, tdresser wrote: > Hmmm, this is a bit ugly. > > Maybe the index for mouse should always be 0, instead of -1? > > Sorry for going back and forth here. > > If we consistently use 0 as the index, and 0 as the default, then this will be a > lot cleaner... 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 checked by lanwei@chromium.org to run a CQ dry run
Patchset #6 (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: 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
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:240001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
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 #6 (id:260001) has been deleted
Patchset #5 (id:200001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lanwei@chromium.org changed reviewers: + nzolghadr@chromium.org
lgtm https://codereview.chromium.org/2478423002/diff/280001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.h (right): https://codereview.chromium.org/2478423002/diff/280001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.h:24: void Move(float x, float y, int index) override; nit: Nobody is using this class directly right now or at least pointer to this class except via its parent pointer. But if we have a pointer to this class with the type SyntheticMouseDriver* and we want to call Move function we need to pass all 3 parameters. So maybe just add the default value here as well for the index? The same happens for Release. https://codereview.chromium.org/2478423002/diff/280001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/2478423002/diff/280001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:263: action_param_list_->push_back(params1); Not related to your patch and change here. Just want to confirm something here. If we have one finger down and keep pressing and releasing the second finger we are increasing the id of the second finger and eventually we hit the maximum id cap (kTouchesLengthCap) and we fail the press action. Right?
https://codereview.chromium.org/2478423002/diff/280001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/2478423002/diff/280001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:263: action_param_list_->push_back(params1); On 2016/11/21 17:56:46, Navid Zolghadr wrote: > Not related to your patch and change here. Just want to confirm something here. > If we have one finger down and keep pressing and releasing the second finger we > are increasing the id of the second finger and eventually we hit the maximum id > cap (kTouchesLengthCap) and we fail the press action. Right? Yes, you are right. The implementation is here content/common/input/synthetic_web_input_event_builders.cc If it is not reasonable, we can change it.
https://codereview.chromium.org/2478423002/diff/280001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/2478423002/diff/280001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:263: action_param_list_->push_back(params1); On 2016/11/23 19:59:23, lanwei wrote: > On 2016/11/21 17:56:46, Navid Zolghadr wrote: > > Not related to your patch and change here. Just want to confirm something > here. > > If we have one finger down and keep pressing and releasing the second finger > we > > are increasing the id of the second finger and eventually we hit the maximum > id > > cap (kTouchesLengthCap) and we fail the press action. Right? > > Yes, you are right. The implementation is here > content/common/input/synthetic_web_input_event_builders.cc > If it is not reasonable, we can change it. Nope. That should be fine for now. This patch didn't change that behavior anyway.
LGTM https://codereview.chromium.org/2478423002/diff/280001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/2478423002/diff/280001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:263: action_param_list_->push_back(params1); On 2016/11/23 20:08:21, Navid Zolghadr wrote: > On 2016/11/23 19:59:23, lanwei wrote: > > On 2016/11/21 17:56:46, Navid Zolghadr wrote: > > > Not related to your patch and change here. Just want to confirm something > > here. > > > If we have one finger down and keep pressing and releasing the second finger > > we > > > are increasing the id of the second finger and eventually we hit the maximum > > id > > > cap (kTouchesLengthCap) and we fail the press action. Right? > > > > Yes, you are right. The implementation is here > > content/common/input/synthetic_web_input_event_builders.cc > > If it is not reasonable, we can change it. > > Nope. That should be fine for now. This patch didn't change that behavior > anyway. We do need to fix this before we try to get external consumers using this API. Lan, can you file a bug?
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 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 tdresser@chromium.org, nzolghadr@chromium.org Link to the patchset: https://codereview.chromium.org/2478423002/#ps300001 (title: "rename")
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 lanwei@chromium.org
lanwei@chromium.org changed reviewers: + mbarbella@chromium.org
lanwei@chromium.org changed reviewers: + avi@chromium.org
avi@ could you please take a look at content/browser/BUILD.gn? Martin, could you please take a look at tools/ipc_fuzzer/fuzzer/fuzzer.cc? Thank you! https://codereview.chromium.org/2478423002/diff/280001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.h (right): https://codereview.chromium.org/2478423002/diff/280001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.h:24: void Move(float x, float y, int index) override; On 2016/11/21 17:56:46, Navid Zolghadr wrote: > nit: Nobody is using this class directly right now or at least pointer to this > class except via its parent pointer. But if we have a pointer to this class with > the type SyntheticMouseDriver* and we want to call Move function we need to pass > all 3 parameters. So maybe just add the default value here as well for the > index? The same happens for Release. Acknowledged. https://codereview.chromium.org/2478423002/diff/280001/content/browser/render... File content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc (right): https://codereview.chromium.org/2478423002/diff/280001/content/browser/render... content/browser/renderer_host/input/synthetic_pointer_action_unittest.cc:263: action_param_list_->push_back(params1); On 2016/11/28 15:12:03, tdresser wrote: > On 2016/11/23 20:08:21, Navid Zolghadr wrote: > > On 2016/11/23 19:59:23, lanwei wrote: > > > On 2016/11/21 17:56:46, Navid Zolghadr wrote: > > > > Not related to your patch and change here. Just want to confirm something > > > here. > > > > If we have one finger down and keep pressing and releasing the second > finger > > > we > > > > are increasing the id of the second finger and eventually we hit the > maximum > > > id > > > > cap (kTouchesLengthCap) and we fail the press action. Right? > > > > > > Yes, you are right. The implementation is here > > > content/common/input/synthetic_web_input_event_builders.cc > > > If it is not reasonable, we can change it. > > > > Nope. That should be fine for now. This patch didn't change that behavior > > anyway. > > We do need to fix this before we try to get external consumers using this API. > Lan, can you file a bug? Done.
gn file lgtm
ipc lgtm
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": 300001, "attempt_start_ts": 1480364174869580,
"parent_rev": "de4ae8bd1d25f6201c9804f564ee9f675380f8b2", "commit_rev":
"ca09181bcbf30e99dbdf39ddf2f17eca90b0822c"}
Message was sent while issue was closed.
Description was changed from ========== Rename SyntheticPointer to SyntheticPointerDriver Rename SyntheticPointer to SyntheticPointerDriver, SyntheticTouchPointer to SyntheticTouchDriver, and SyntheticMousePointer to SyntheticMouseDriver, because these classes do not represent the synthetic pointers. They actually create synthetic pointers for each pointer types and their actions. BUG=525187 ========== to ========== Rename SyntheticPointer to SyntheticPointerDriver Rename SyntheticPointer to SyntheticPointerDriver, SyntheticTouchPointer to SyntheticTouchDriver, and SyntheticMousePointer to SyntheticMouseDriver, because these classes do not represent the synthetic pointers. They actually create synthetic pointers for each pointer types and their actions. BUG=525187 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Rename SyntheticPointer to SyntheticPointerDriver Rename SyntheticPointer to SyntheticPointerDriver, SyntheticTouchPointer to SyntheticTouchDriver, and SyntheticMousePointer to SyntheticMouseDriver, because these classes do not represent the synthetic pointers. They actually create synthetic pointers for each pointer types and their actions. BUG=525187 ========== to ========== Rename SyntheticPointer to SyntheticPointerDriver Rename SyntheticPointer to SyntheticPointerDriver, SyntheticTouchPointer to SyntheticTouchDriver, and SyntheticMousePointer to SyntheticMouseDriver, because these classes do not represent the synthetic pointers. They actually create synthetic pointers for each pointer types and their actions. BUG=525187 Committed: https://crrev.com/1060f1f73a8498e324d05fcfd2fdca621bc9b15e Cr-Commit-Position: refs/heads/master@{#434763} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1060f1f73a8498e324d05fcfd2fdca621bc9b15e Cr-Commit-Position: refs/heads/master@{#434763} |
