Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2754203005:
touchpad swipe support added.
Descriptiontouchpad swipe support added.
I also changed the swipe directions to be consistent with smoothScrollBy.
'down' direction with positive scroll pixels scrolls down the page.
testSwipe is temprorarily disabled to land the chaning cl:
https://codereview.chromium.org/2742473002/
BUG=catapult:#3415
Patch Set 1 #
Total comments: 4
Patch Set 2 : Comments on scroll and swipe directions. #
Total comments: 8
Patch Set 3 : comments modified and js new style violations fixed. #
Total comments: 5
Messages
Total messages: 48 (25 generated)
The CQ bit was checked by sahel@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.
Description was changed from ========== touchpad swipe support added. I also changed the swipe directions to be consistent with smoothScrollBy. 'down' direction with positive scroll pixels scrolls down the page. testSwipe is temprorarily disabled to land the chaning cl: https://codereview.chromium.org/2742473002/ BUG=catapult:#3415 ========== to ========== touchpad swipe support added. I also changed the swipe directions to be consistent with smoothScrollBy. 'down' direction with positive scroll pixels scrolls down the page. testSwipe is temprorarily disabled to land the chaning cl: https://codereview.chromium.org/2742473002/ BUG=catapult:#3415 ==========
sahel@chromium.org changed reviewers: + bokan@chromium.org, dtapuska@chromium.org, tdresser@chromium.org
This is the catapult part of the cl for swipe touchpad support: https://codereview.chromium.org/2742473002/
LGTM, but you'll need to get a catapult reviewer with context in python too. https://codereview.chromium.org/2754203005/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/actions/action_runner_unittest.py (right): https://codereview.chromium.org/2754203005/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/actions/action_runner_unittest.py:380: # https://github.com/catapult-project/catapult/issues/3415 Normally this comment would go above the code it refers to, wouldn't it? https://codereview.chromium.org/2754203005/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/actions/swipe.js (right): https://codereview.chromium.org/2754203005/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/actions/swipe.js:62: var startLeft = Why change this?
On 2017/03/17 18:55:51, sahel wrote: > This is the catapult part of the cl for swipe touchpad support: > https://codereview.chromium.org/2742473002/ I guess my comment would be that we should add some comments as to what left/right means. Likely this inverted direction came from which way the finger is moving. (ie; see touch-action; https://developer.mozilla.org/en-US/docs/Web/CSS/touch-action) I'm fine either way we just need to document what a swipe left means.
On 2017/03/17 19:05:14, dtapuska wrote: > On 2017/03/17 18:55:51, sahel wrote: > > This is the catapult part of the cl for swipe touchpad support: > > https://codereview.chromium.org/2742473002/ > > I guess my comment would be that we should add some comments as to what > left/right means. Likely this inverted direction came from which way the finger > is moving. (ie; see touch-action; > https://developer.mozilla.org/en-US/docs/Web/CSS/touch-action) I'm fine either > way we just need to document what a swipe left means. I changed swipe so that both swipe and scroll, increase scrollTop/scrollLeft with scrolling with direction='down/right' and positive distance to scroll.
https://codereview.chromium.org/2754203005/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/actions/action_runner_unittest.py (right): https://codereview.chromium.org/2754203005/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/actions/action_runner_unittest.py:380: # https://github.com/catapult-project/catapult/issues/3415 On 2017/03/17 19:03:40, tdresser wrote: > Normally this comment would go above the code it refers to, wouldn't it? Done. https://codereview.chromium.org/2754203005/diff/1/telemetry/telemetry/interna... File telemetry/telemetry/internal/actions/swipe.js (right): https://codereview.chromium.org/2754203005/diff/1/telemetry/telemetry/interna... telemetry/telemetry/internal/actions/swipe.js:62: var startLeft = On 2017/03/17 19:03:40, tdresser wrote: > Why change this? I got js style errors while submitting the cl.
The CQ bit was checked by sahel@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.
sahel@chromium.org changed reviewers: + dtu@chromium.org
https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/swipe.py (right): https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/swipe.py:43: if self._synthetic_gesture_source == 'chrome.gpuBenchmarking.MOUSE_INPUT': Why is 0 velocity for touch input not a problem?
https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/swipe.py (right): https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/swipe.py:43: if self._synthetic_gesture_source == 'chrome.gpuBenchmarking.MOUSE_INPUT': On 2017/03/22 19:32:31, bokan wrote: > Why is 0 velocity for touch input not a problem? For touch fling we don't get the velocity from the user, instead keep track of touch move events to calculate the velocity. We only need it for simulating ChromeOS touchpad fling.
Ok lgtm from me, just have a question for my own understanding. https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/swipe.py (right): https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/swipe.py:43: if self._synthetic_gesture_source == 'chrome.gpuBenchmarking.MOUSE_INPUT': On 2017/03/23 17:10:26, sahel wrote: > On 2017/03/22 19:32:31, bokan wrote: > > Why is 0 velocity for touch input not a problem? > > For touch fling we don't get the velocity from the user, instead keep track of > touch move events to calculate the velocity. We only need it for simulating > ChromeOS touchpad fling. So for a touchpad fling, the browser calculates the velocity and sets the velocity in the FlingStart? While in touchscreen the renderer calculates the velocity based on the touch moves so when the finger lifts it generates the fling there? Is my understanding correct?
On 2017/03/23 21:36:47, bokan wrote: > Ok lgtm from me, just have a question for my own understanding. > > https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... > File telemetry/telemetry/internal/actions/swipe.py (right): > > https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... > telemetry/telemetry/internal/actions/swipe.py:43: if > self._synthetic_gesture_source == 'chrome.gpuBenchmarking.MOUSE_INPUT': > On 2017/03/23 17:10:26, sahel wrote: > > On 2017/03/22 19:32:31, bokan wrote: > > > Why is 0 velocity for touch input not a problem? > > > > For touch fling we don't get the velocity from the user, instead keep track of > > touch move events to calculate the velocity. We only need it for simulating > > ChromeOS touchpad fling. > > So for a touchpad fling, the browser calculates the velocity and sets the > velocity in the FlingStart? While in touchscreen the renderer calculates the > velocity based on the touch moves so when the finger lifts it generates the > fling there? Is my understanding correct? They both happen in browser side but differently: For touchpad fling the OS provides the velocity information. Velocity info gets read when a scroll event is generated from a native event: https://cs.chromium.org/chromium/src/ui/events/event.cc?q=scrollevent&l=1390 For touchscreen fling, in case of a motion event with ACTION_UP, the gesture_detector calls OnFling from gesture_provider to generate a GFS with the velocity that is recorded by velocity_tracker. https://cs.chromium.org/chromium/src/ui/events/gesture_detection/gesture_dete...
On 2017/03/24 15:13:12, sahel wrote: > On 2017/03/23 21:36:47, bokan wrote: > > Ok lgtm from me, just have a question for my own understanding. > > > > > https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... > > File telemetry/telemetry/internal/actions/swipe.py (right): > > > > > https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... > > telemetry/telemetry/internal/actions/swipe.py:43: if > > self._synthetic_gesture_source == 'chrome.gpuBenchmarking.MOUSE_INPUT': > > On 2017/03/23 17:10:26, sahel wrote: > > > On 2017/03/22 19:32:31, bokan wrote: > > > > Why is 0 velocity for touch input not a problem? > > > > > > For touch fling we don't get the velocity from the user, instead keep track > of > > > touch move events to calculate the velocity. We only need it for simulating > > > ChromeOS touchpad fling. > > > > So for a touchpad fling, the browser calculates the velocity and sets the > > velocity in the FlingStart? While in touchscreen the renderer calculates the > > velocity based on the touch moves so when the finger lifts it generates the > > fling there? Is my understanding correct? > > They both happen in browser side but differently: > For touchpad fling the OS provides the velocity information. Velocity info gets > read when a scroll event is generated from a native event: > https://cs.chromium.org/chromium/src/ui/events/event.cc?q=scrollevent&l=1390 > > For touchscreen fling, in case of a motion event with ACTION_UP, the > gesture_detector calls OnFling from gesture_provider to generate a GFS with the > velocity that is recorded by velocity_tracker. > https://cs.chromium.org/chromium/src/ui/events/gesture_detection/gesture_dete...
On 2017/03/24 15:13:12, sahel wrote: > On 2017/03/23 21:36:47, bokan wrote: > > Ok lgtm from me, just have a question for my own understanding. > > > > > https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... > > File telemetry/telemetry/internal/actions/swipe.py (right): > > > > > https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... > > telemetry/telemetry/internal/actions/swipe.py:43: if > > self._synthetic_gesture_source == 'chrome.gpuBenchmarking.MOUSE_INPUT': > > On 2017/03/23 17:10:26, sahel wrote: > > > On 2017/03/22 19:32:31, bokan wrote: > > > > Why is 0 velocity for touch input not a problem? > > > > > > For touch fling we don't get the velocity from the user, instead keep track > of > > > touch move events to calculate the velocity. We only need it for simulating > > > ChromeOS touchpad fling. > > > > So for a touchpad fling, the browser calculates the velocity and sets the > > velocity in the FlingStart? While in touchscreen the renderer calculates the > > velocity based on the touch moves so when the finger lifts it generates the > > fling there? Is my understanding correct? > > They both happen in browser side but differently: > For touchpad fling the OS provides the velocity information. Velocity info gets > read when a scroll event is generated from a native event: > https://cs.chromium.org/chromium/src/ui/events/event.cc?q=scrollevent&l=1390 > > For touchscreen fling, in case of a motion event with ACTION_UP, the > gesture_detector calls OnFling from gesture_provider to generate a GFS with the > velocity that is recorded by velocity_tracker. > https://cs.chromium.org/chromium/src/ui/events/gesture_detection/gesture_dete...
On 2017/03/24 15:13:12, sahel wrote: > On 2017/03/23 21:36:47, bokan wrote: > > Ok lgtm from me, just have a question for my own understanding. > > > > > https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... > > File telemetry/telemetry/internal/actions/swipe.py (right): > > > > > https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... > > telemetry/telemetry/internal/actions/swipe.py:43: if > > self._synthetic_gesture_source == 'chrome.gpuBenchmarking.MOUSE_INPUT': > > On 2017/03/23 17:10:26, sahel wrote: > > > On 2017/03/22 19:32:31, bokan wrote: > > > > Why is 0 velocity for touch input not a problem? > > > > > > For touch fling we don't get the velocity from the user, instead keep track > of > > > touch move events to calculate the velocity. We only need it for simulating > > > ChromeOS touchpad fling. > > > > So for a touchpad fling, the browser calculates the velocity and sets the > > velocity in the FlingStart? While in touchscreen the renderer calculates the > > velocity based on the touch moves so when the finger lifts it generates the > > fling there? Is my understanding correct? > > They both happen in browser side but differently: > For touchpad fling the OS provides the velocity information. Velocity info gets > read when a scroll event is generated from a native event: > https://cs.chromium.org/chromium/src/ui/events/event.cc?q=scrollevent&l=1390 > > For touchscreen fling, in case of a motion event with ACTION_UP, the > gesture_detector calls OnFling from gesture_provider to generate a GFS with the > velocity that is recorded by velocity_tracker. > https://cs.chromium.org/chromium/src/ui/events/gesture_detection/gesture_dete... Got it, thanks.
lgtm
The CQ bit was checked by sahel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2754203005/#ps20001 (title: "Comments on scroll and swipe directions.")
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: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
sahel@chromium.org changed reviewers: + sullivan@chromium.org
This is the catapult part of the cl for swipe touchpad support: https://codereview.chromium.org/2742473002/
sullivan@chromium.org changed reviewers: + nednguyen@google.com - dtu@chromium.org
nednguyen@google.com changed reviewers: + rnephew@chromium.org
On 2017/03/27 18:44:41, sullivan wrote: Randy: can you take a pass at reviewing this?
lgtm with a supernit. https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/action_runner.py (right): https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/action_runner.py:421: Direction = 'down' with positive distance to scroll increases Supernit: Direction -> direction So that its consistent with how they would pass it to ScrollPage. Same comment on other instances.
https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/action_runner.py (right): https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/action_runner.py:421: Direction = 'down' with positive distance to scroll increases On 2017/03/28 20:04:18, rnephew (Reviews Here) wrote: > Supernit: Direction -> direction > > So that its consistent with how they would pass it to ScrollPage. > > Same comment on other instances. The documentation is also very out of place. Please consider rephrasing it. https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/action_runner.py:633: velocity_y=0): this is an API change, please: 1) Announce this change to https://groups.google.com/a/chromium.org/forum/#!forum/telemetry-announce & give people around 1 or two weeks to prepare for it. 2) Make sure that there is no client code in https://cs.chromium.org/search/?q=SwipeElement that use the default value that would have changed behavior once you land this.
The CQ bit was checked by sahel@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: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
The CQ bit was checked by sahel@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.
I had to remove the unused self alias for this in swipe.js due to js style violation. https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/action_runner.py (right): https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/action_runner.py:421: Direction = 'down' with positive distance to scroll increases On 2017/03/28 21:15:15, nednguyen wrote: > On 2017/03/28 20:04:18, rnephew (Reviews Here) wrote: > > Supernit: Direction -> direction > > > > So that its consistent with how they would pass it to ScrollPage. > > > > Same comment on other instances. > > The documentation is also very out of place. Please consider rephrasing it. Done. https://codereview.chromium.org/2754203005/diff/20001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/action_runner.py:633: velocity_y=0): On 2017/03/28 21:15:16, nednguyen wrote: > this is an API change, please: > 1) Announce this change to > https://groups.google.com/a/chromium.org/forum/#!forum/telemetry-announce & give > people around 1 or two weeks to prepare for it. > 2) Make sure that there is no client code in > https://cs.chromium.org/search/?q=SwipeElement that use the default value that > would have changed behavior once you land this. 1)Done. 2) The default behavior is not changed (it increases scrollLeft) but the label for that is changed from left to right. This is for making swipe direction labels consistent with scroll. I checked the code both for swipeElement and swipePage to make sure that all the tests that are using these two functions maintain the same behavior as before. (The dependent cl is updated for this: https://codereview.chromium.org/2742473002/)
https://codereview.chromium.org/2754203005/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/action_runner.py (right): https://codereview.chromium.org/2754203005/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/action_runner.py:650: velocity_x, velocity_y: Fling velocities for touchpad swipe Hmhh, could this cause conflict with speed_in_pixels_per_second? What happens to speed_in_pixels_per_second when both of these are set? Also please add units in the variable name as the convention in this file: fling_speed_x_in_pixels_per_second, fling_speed_y_in_pixels_per_second https://codereview.chromium.org/2754203005/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/action_runner_unittest.py (right): https://codereview.chromium.org/2754203005/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/action_runner_unittest.py:377: Please also add tests that exercise the new code path. https://codereview.chromium.org/2754203005/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/swipe.js (right): https://codereview.chromium.org/2754203005/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/swipe.js:74: if (this.callback_) { please don't change this in this CL. Style change should be consistent, so should be applied to other files as well. https://codereview.chromium.org/2754203005/diff/40001/telemetry/telemetry/int... File telemetry/telemetry/internal/actions/swipe.py (right): https://codereview.chromium.org/2754203005/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/swipe.py:43: if self._synthetic_gesture_source == 'chrome.gpuBenchmarking.MOUSE_INPUT': Instead of "if a:; if b and c:", can you change this to "if a and b and c"? Nested code usually make it harder to follow the flow. https://codereview.chromium.org/2754203005/diff/40001/telemetry/telemetry/int... telemetry/telemetry/internal/actions/swipe.py:46: 'Touchpad fling must have non-zero velocity.') If self._synthetic_gesture_source != 'chrome.gpuBenchmarking.MOUSE_INPUT, should we raise exception if either of velocity_x or velocity_y is non zero? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
