Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2533493002:
VR: Fix click during scrolling (Closed)
DescriptionVR: Fix click during scrolling
Now we can click while scrolling. The scrolling stops and Tap is sent.
BUG=668518
Committed: https://crrev.com/7f51e89599e3a75c42ad82d1d65b815fd6be3e96
Cr-Commit-Position: refs/heads/master@{#435051}
Patch Set 1 #Patch Set 2 : VR: Fix click during scrolling #
Total comments: 6
Patch Set 3 : VR: Fix click during scrolling #
Total comments: 4
Patch Set 4 : Rebase + Address Comments #
Messages
Total messages: 16 (7 generated)
Description was changed from ========== VR: Fix click during scrolling Now we can click while scrolling. The scrolling stops and Tap is sent. BUG=668518 ========== to ========== VR: Fix click during scrolling Now we can click while scrolling. The scrolling stops and Tap is sent. BUG=668518 ==========
asimjour@chromium.org changed reviewers: + bshe@chromium.org, mthiesse@chromium.org
On 2016/11/28 15:49:38, asimjour1 wrote: > mailto:asimjour@chromium.org changed reviewers: > + mailto:bshe@chromium.org, mailto:mthiesse@chromium.org PTAL
lgtm https://codereview.chromium.org/2533493002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2533493002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:206: if (state_ == SCROLLING && IsButtonDown(gvr::kControllerButtonClick)) nit: extract "state_ == SCROLLING && IsButtonDown(gvr::kControllerButtonClick)" out to bool to avoid duplication.
https://codereview.chromium.org/2533493002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2533493002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:206: if (state_ == SCROLLING && IsButtonDown(gvr::kControllerButtonClick)) can this logic move to HandleScrollingState ? It probably makes more sense in that function. https://codereview.chromium.org/2533493002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:284: // Touch position is changed and the touch point moves outside of slop. update the comment here to include buttondown detection here.
https://codereview.chromium.org/2533493002/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2533493002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:206: if (state_ == SCROLLING && IsButtonDown(gvr::kControllerButtonClick)) On 2016/11/28 16:33:57, bshe wrote: > can this logic move to HandleScrollingState ? It probably makes more sense in > that function. Done. https://codereview.chromium.org/2533493002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:206: if (state_ == SCROLLING && IsButtonDown(gvr::kControllerButtonClick)) On 2016/11/28 16:06:57, mthiesse wrote: > nit: extract "state_ == SCROLLING && IsButtonDown(gvr::kControllerButtonClick)" > out to bool to avoid duplication. This part is moved and there is no duplication anymore. https://codereview.chromium.org/2533493002/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:284: // Touch position is changed and the touch point moves outside of slop. On 2016/11/28 16:33:57, bshe wrote: > update the comment here to include buttondown detection here. Done.
lgtm with nits https://codereview.chromium.org/2533493002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2533493002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:205: nit: remove the empty line here. https://codereview.chromium.org/2533493002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:209: if (state_ == SCROLLING && IsButtonDown(gvr::kControllerButtonClick)) { nit: does it make sense to move this check into: if (gesture_list.back()->type == WebInputEvent::GestureScrollEnd) statement? It looks like with the new code, the type should be GestrueScrollEndfor both Fling and TapDown? It would be easier for me to read the code.
https://codereview.chromium.org/2533493002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_controller.cc (right): https://codereview.chromium.org/2533493002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:205: On 2016/11/28 21:52:36, bshe wrote: > nit: remove the empty line here. Done. https://codereview.chromium.org/2533493002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_controller.cc:209: if (state_ == SCROLLING && IsButtonDown(gvr::kControllerButtonClick)) { On 2016/11/28 21:52:36, bshe wrote: > nit: does it make sense to move this check into: > if (gesture_list.back()->type == WebInputEvent::GestureScrollEnd) statement? > It looks like with the new code, the type should be GestrueScrollEndfor both > Fling and TapDown? It would be easier for me to read the code. Done.
The CQ bit was checked by asimjour@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, bshe@chromium.org Link to the patchset: https://codereview.chromium.org/2533493002/#ps60001 (title: "Rebase + Address Comments")
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": 60001, "attempt_start_ts": 1480443999209300,
"parent_rev": "2e040717dfa19c2422f7d0f5007bf920b769b71e", "commit_rev":
"73c884c1afc6d94bdd09e6dd8bb4cf2c3271642b"}
Message was sent while issue was closed.
Description was changed from ========== VR: Fix click during scrolling Now we can click while scrolling. The scrolling stops and Tap is sent. BUG=668518 ========== to ========== VR: Fix click during scrolling Now we can click while scrolling. The scrolling stops and Tap is sent. BUG=668518 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== VR: Fix click during scrolling Now we can click while scrolling. The scrolling stops and Tap is sent. BUG=668518 ========== to ========== VR: Fix click during scrolling Now we can click while scrolling. The scrolling stops and Tap is sent. BUG=668518 Committed: https://crrev.com/7f51e89599e3a75c42ad82d1d65b815fd6be3e96 Cr-Commit-Position: refs/heads/master@{#435051} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7f51e89599e3a75c42ad82d1d65b815fd6be3e96 Cr-Commit-Position: refs/heads/master@{#435051} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
