Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2392943005:
Fix Cardboard trigger (broken by GVR update) (Closed)
DescriptionFix Cardboard trigger (broken by GVR update)
BUG=
Committed: https://crrev.com/036de9caf90696539a03f9259b98aab4a69dd96b
Cr-Commit-Position: refs/heads/master@{#423861}
Patch Set 1 #Patch Set 2 : Add Comment #
Total comments: 4
Patch Set 3 : Address comment #
Messages
Total messages: 17 (6 generated)
mthiesse@chromium.org changed reviewers: + dtrainor@chromium.org
PTAL
asimjour@chromium.org changed reviewers: + asimjour@chromium.org
https://codereview.chromium.org/2392943005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2392943005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:103: public boolean dispatchTouchEvent(MotionEvent event) { is it necessary to override this method?
https://codereview.chromium.org/2392943005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2392943005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:103: public boolean dispatchTouchEvent(MotionEvent event) { On 2016/10/06 15:12:16, asimjour1 wrote: > is it necessary to override this method? Well, the answer is conceptually yes, but maybe no in practice. GVR usually consumes touch events, but we have no guarantee of that, and they may just decide not to at times. We definitely don't want these CVCs receiving touch input no matter what, so I think this should stay.
On 2016/10/06 15:16:15, mthiesse wrote: > https://codereview.chromium.org/2392943005/diff/20001/chrome/android/java/src... > File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java > (right): > > https://codereview.chromium.org/2392943005/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:103: > public boolean dispatchTouchEvent(MotionEvent event) { > On 2016/10/06 15:12:16, asimjour1 wrote: > > is it necessary to override this method? > > Well, the answer is conceptually yes, but maybe no in practice. GVR usually > consumes touch events, but we have no guarantee of that, and they may just > decide not to at times. > > We definitely don't want these CVCs receiving touch input no matter what, so I > think this should stay. lgtm
lgtm % nit https://codereview.chromium.org/2392943005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2392943005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:89: public boolean dispatchTouchEvent(MotionEvent event) { @Override? Can this go below the constructor (probably closer to where the other overridden methods are)?
mthiesse@chromium.org changed reviewers: + bshe@chromium.org
bshe@chromium.org: Please review changes in chrome/browser
https://codereview.chromium.org/2392943005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java (right): https://codereview.chromium.org/2392943005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:89: public boolean dispatchTouchEvent(MotionEvent event) { On 2016/10/07 06:26:30, David Trainor wrote: > @Override? Can this go below the constructor (probably closer to where the > other overridden methods are)? Done.
On 2016/10/07 14:14:46, mthiesse wrote: > https://codereview.chromium.org/2392943005/diff/20001/chrome/android/java/src... > File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java > (right): > > https://codereview.chromium.org/2392943005/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShell.java:89: > public boolean dispatchTouchEvent(MotionEvent event) { > On 2016/10/07 06:26:30, David Trainor wrote: > > @Override? Can this go below the constructor (probably closer to where the > > other overridden methods are)? > > Done. lgtm
The CQ bit was checked by mthiesse@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asimjour@google.com, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2392943005/#ps40001 (title: "Address comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix Cardboard trigger (broken by GVR update) BUG= ========== to ========== Fix Cardboard trigger (broken by GVR update) BUG= Committed: https://crrev.com/036de9caf90696539a03f9259b98aab4a69dd96b Cr-Commit-Position: refs/heads/master@{#423861} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/036de9caf90696539a03f9259b98aab4a69dd96b Cr-Commit-Position: refs/heads/master@{#423861} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
