Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808723003:
VR: Sending the fling event to the right target (Closed)
DescriptionVR: Sending the fling event to the right target
Fling event is sent to the same target that the last scroll event
was sent to.
BUG=698260
Review-Url: https://codereview.chromium.org/2808723003
Cr-Commit-Position: refs/heads/master@{#464419}
Committed: https://chromium.googlesource.com/chromium/src/+/7887732326e9a545585ece8147cd2fe88a080f01
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add current_fling_target_ #
Total comments: 4
Patch Set 3 : VR: Sending the fling event to the right target #Patch Set 4 : rebase #
Messages
Total messages: 24 (10 generated)
asimjour@chromium.org changed reviewers: + mthiesse@chromium.org, tiborg@chromium.org
PTAL
https://codereview.chromium.org/2808723003/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_gl.cc (left): https://codereview.chromium.org/2808723003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:665: current_scroll_target = InputTarget::NONE; Don't we know have confusing state where we have a scroll target but the scroll has ended? Can we add a fling_target_ member instead, and set that here?
https://codereview.chromium.org/2808723003/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_gl.cc (left): https://codereview.chromium.org/2808723003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:665: current_scroll_target = InputTarget::NONE; On 2017/04/10 15:54:17, mthiesse wrote: > Don't we know have confusing state where we have a scroll target but the scroll > has ended? > > Can we add a fling_target_ member instead, and set that here? s/know/now
https://codereview.chromium.org/2808723003/diff/1/chrome/browser/android/vr_s... File chrome/browser/android/vr_shell/vr_shell_gl.cc (left): https://codereview.chromium.org/2808723003/diff/1/chrome/browser/android/vr_s... chrome/browser/android/vr_shell/vr_shell_gl.cc:665: current_scroll_target = InputTarget::NONE; current_fling_target_ is added.
https://codereview.chromium.org/2808723003/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2808723003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:679: case WebInputEvent::GestureFlingCancel: Shouldn't FlingCancel do something like sending a cancel gesture to the current_fling_target_? Why is it falling through to GestureTapDown?
asimjour@google.com changed reviewers: + asimjour@google.com
https://codereview.chromium.org/2808723003/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2808723003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:679: case WebInputEvent::GestureFlingCancel: On 2017/04/12 17:46:20, mthiesse wrote: > Shouldn't FlingCancel do something like sending a cancel gesture to the > current_fling_target_? Why is it falling through to GestureTapDown? Fling cancel happens when user start to touch the touchpad during a fling. I think it should only stop the fling if the laser is pointing to the same target that fling was sent to.
https://codereview.chromium.org/2808723003/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2808723003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:679: case WebInputEvent::GestureFlingCancel: On 2017/04/12 18:00:58, asimjour wrote: > On 2017/04/12 17:46:20, mthiesse wrote: > > Shouldn't FlingCancel do something like sending a cancel gesture to the > > current_fling_target_? Why is it falling through to GestureTapDown? > > Fling cancel happens when user start to touch the touchpad during a fling. I > think it should only stop the fling if the laser is pointing to the same target > that fling was sent to. Sure, but now we're sending a FlingCancel gesture to something that was probably not in a fling. Just for clarity sake, even if we use the exact same logic in the FlingCancel and TapDown cases, we shouldn't have it fall through because they should be they're distinct events that might diverge in behaviour.
https://codereview.chromium.org/2808723003/diff/20001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell_gl.cc (right): https://codereview.chromium.org/2808723003/diff/20001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell_gl.cc:679: case WebInputEvent::GestureFlingCancel: On 2017/04/12 18:05:28, mthiesse wrote: > On 2017/04/12 18:00:58, asimjour wrote: > > On 2017/04/12 17:46:20, mthiesse wrote: > > > Shouldn't FlingCancel do something like sending a cancel gesture to the > > > current_fling_target_? Why is it falling through to GestureTapDown? > > > > Fling cancel happens when user start to touch the touchpad during a fling. I > > think it should only stop the fling if the laser is pointing to the same > target > > that fling was sent to. > > Sure, but now we're sending a FlingCancel gesture to something that was probably > not in a fling. Just for clarity sake, even if we use the exact same logic in > the FlingCancel and TapDown cases, we shouldn't have it fall through because > they should be they're distinct events that might diverge in behaviour. I repeated the code under FlingCancel and TapDown, and made the separate. It is ok to send FlingCancel to a content that has not received a FlingStart before.
lgtm
The CQ bit was checked by asimjour@chromium.org
The CQ bit was unchecked by asimjour@chromium.org
The CQ bit was checked by asimjour@chromium.org
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: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm
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, tiborg@chromium.org Link to the patchset: https://codereview.chromium.org/2808723003/#ps60001 (title: "rebase")
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": 1492095956392490,
"parent_rev": "6ea1b0eef2438b8365c3b762b6e545b08e1a27ea", "commit_rev":
"7887732326e9a545585ece8147cd2fe88a080f01"}
Message was sent while issue was closed.
Description was changed from ========== VR: Sending the fling event to the right target Fling event is sent to the same target that the last scroll event was sent to. BUG=698260 ========== to ========== VR: Sending the fling event to the right target Fling event is sent to the same target that the last scroll event was sent to. BUG=698260 Review-Url: https://codereview.chromium.org/2808723003 Cr-Commit-Position: refs/heads/master@{#464419} Committed: https://chromium.googlesource.com/chromium/src/+/7887732326e9a545585ece8147cd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/7887732326e9a545585ece8147cd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
