Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(427)

Issue 2768583002: Make VR screen tap tests more stable (Closed)

Created:
3 years, 9 months ago by bsheedy
Modified:
3 years, 8 months ago
CC:
chromium-reviews, feature-vr-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Make VR screen tap tests more stable The single tap sent during the VR screen tap tests wasn't always getting registered since device response can be a bit slow during the VR transition. For the Cardboard case, wait on Javascript to say it received another click event. For the Daydream case, wait on VrShell saying its parent has consumed the event before checking. BUG= Review-Url: https://codereview.chromium.org/2768583002 Cr-Original-Commit-Position: refs/heads/master@{#460452} Committed: https://chromium.googlesource.com/chromium/src/+/3e2ce51e88f19f0cc5196284f580583ddd4dbd9a Review-Url: https://codereview.chromium.org/2768583002 Cr-Commit-Position: refs/heads/master@{#460623} Committed: https://chromium.googlesource.com/chromium/src/+/bc288f4389ba798aa9c3ad2f593a2b008d0b1a6a

Patch Set 1 #

Total comments: 1

Patch Set 2 : Lower duration for DD, change behavior for cardboard #

Total comments: 4

Patch Set 3 : Rebase + change annotations #

Patch Set 4 : Add way to wait on touch events being consumed #

Patch Set 5 : Change assert description #

Total comments: 4

Patch Set 6 : Switch to callback, add timeout #

Patch Set 7 : Move VR test library behind flag #

Messages

Total messages: 42 (16 generated)
bsheedy
+dtrainor for OWNERS
3 years, 9 months ago (2017-03-21 18:54:44 UTC) #2
David Trainor- moved to gerrit
https://codereview.chromium.org/2768583002/diff/1/chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java (right): https://codereview.chromium.org/2768583002/diff/1/chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java#newcode263 chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:263: for (int i = 0; i < 10; i++) ...
3 years, 9 months ago (2017-03-22 03:01:18 UTC) #3
bsheedy
On 2017/03/22 03:01:18, David Trainor-ping if over 24h wrote: > https://codereview.chromium.org/2768583002/diff/1/chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java > File > chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java ...
3 years, 9 months ago (2017-03-22 16:43:09 UTC) #4
David Trainor- moved to gerrit
+tedchoc@ in case he's aware of a test utility method that will wait for the ...
3 years, 9 months ago (2017-03-23 04:35:17 UTC) #6
Ted C
On 2017/03/23 04:35:17, David Trainor-ping if over 24h wrote: > +tedchoc@ in case he's aware ...
3 years, 9 months ago (2017-03-23 19:09:35 UTC) #7
bsheedy
On 2017/03/23 19:09:35, Ted C wrote: > On 2017/03/23 04:35:17, David Trainor-ping if over 24h ...
3 years, 9 months ago (2017-03-23 20:18:55 UTC) #8
bsheedy
On 2017/03/23 20:18:55, bsheedy wrote: > On 2017/03/23 19:09:35, Ted C wrote: > > On ...
3 years, 9 months ago (2017-03-24 19:49:49 UTC) #9
Ted C
https://codereview.chromium.org/2768583002/diff/20001/chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java (right): https://codereview.chromium.org/2768583002/diff/20001/chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java#newcode253 chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:253: @SmallTest This should likely be a LargeTest (and probably ...
3 years, 9 months ago (2017-03-27 15:44:06 UTC) #10
bsheedy
https://codereview.chromium.org/2768583002/diff/20001/chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java (right): https://codereview.chromium.org/2768583002/diff/20001/chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java#newcode253 chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:253: @SmallTest On 2017/03/27 15:44:06, Ted C wrote: > This ...
3 years, 9 months ago (2017-03-27 20:19:12 UTC) #11
bsheedy
+mthiesse for the VR changes. I've added a way for the Daydream case to wait ...
3 years, 8 months ago (2017-03-28 17:40:58 UTC) #14
mthiesse
Seems like a good approach overall, minor comments. https://codereview.chromium.org/2768583002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java (right): https://codereview.chromium.org/2768583002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java#newcode451 chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:451: nativeOnTriggerEvent(mNativeVrShell); ...
3 years, 8 months ago (2017-03-28 18:08:02 UTC) #15
bsheedy
Addressed mthiesse@ comments https://codereview.chromium.org/2768583002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java (right): https://codereview.chromium.org/2768583002/diff/80001/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java#newcode451 chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:451: nativeOnTriggerEvent(mNativeVrShell); On 2017/03/28 18:08:01, mthiesse wrote: ...
3 years, 8 months ago (2017-03-28 20:50:35 UTC) #16
mthiesse
lgtm
3 years, 8 months ago (2017-03-28 21:06:30 UTC) #17
David Trainor- moved to gerrit
Can you try one last thing? Maybe override the OnClickListener for the CompositorViewHolder and validate ...
3 years, 8 months ago (2017-03-28 23:55:24 UTC) #18
bsheedy
On 2017/03/28 23:55:24, David Trainor-ping if over 24h wrote: > Can you try one last ...
3 years, 8 months ago (2017-03-29 00:20:19 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2768583002/100001
3 years, 8 months ago (2017-03-29 00:30:55 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/259839) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, ...
3 years, 8 months ago (2017-03-29 02:14:44 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2768583002/100001
3 years, 8 months ago (2017-03-29 16:20:43 UTC) #25
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/3e2ce51e88f19f0cc5196284f580583ddd4dbd9a
3 years, 8 months ago (2017-03-29 18:00:59 UTC) #28
mdjones
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2782963002/ by mdjones@chromium.org. ...
3 years, 8 months ago (2017-03-29 19:20:43 UTC) #29
bsheedy
On 2017/03/29 19:20:43, mdjones wrote: > A revert of this CL (patchset #6 id:100001) has ...
3 years, 8 months ago (2017-03-29 20:37:01 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2768583002/120001
3 years, 8 months ago (2017-03-29 20:38:18 UTC) #34
mthiesse
lgtm
3 years, 8 months ago (2017-03-29 20:39:34 UTC) #35
commit-bot: I haz the power
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/182731)
3 years, 8 months ago (2017-03-30 01:27:58 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2768583002/120001
3 years, 8 months ago (2017-03-30 01:29:20 UTC) #39
commit-bot: I haz the power
3 years, 8 months ago (2017-03-30 01:59:08 UTC) #42
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/bc288f4389ba798aa9c3ad2f593a...

Powered by Google App Engine
This is Rietveld 408576698