|
|
DescriptionMake 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@chromium.org changed reviewers: + dtrainor@chromium.org
+dtrainor for OWNERS
https://codereview.chromium.org/2768583002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java (right): https://codereview.chromium.org/2768583002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:263: for (int i = 0; i < 10; i++) { Is there any way to detect when vr is entered and just send taps then?
On 2017/03/22 03:01:18, David Trainor-ping if over 24h wrote: > https://codereview.chromium.org/2768583002/diff/1/chrome/android/javatests/sr... > File > chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java > (right): > > https://codereview.chromium.org/2768583002/diff/1/chrome/android/javatests/sr... > chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:263: > for (int i = 0; i < 10; i++) { > Is there any way to detect when vr is entered and just send taps then? There is, and we're using it (we're waiting on Javascript to receive the 'vrdisplaypresentchange' event, which is only fired upon entering/exiting VR). There is also checking VrShellDelegate.isInVR(), but that gets set to true slightly before vrdisplaypresentchange is fired, so waiting on that shouldn't do anything. The problem seems to be that when we send a single tap and immediately check if it's registered, we sometimes check before the click event gets sent to Javascript and processed. Lowering the period over which clicks are sent (to say, 250 ms) should work just as well if that's preferable. For testScreenTapsRegisteredOnCardboard, it could be changed so that only a single tap is sent, and the Java code waits for Javascript to say that it's received a click event. Sending multiple taps over a period (or sending one and waiting, but that seems like a worse approach to me) is still necessary for checking that taps aren't registered when the viewer is Daydream View, though, as we can't wait for the click event in Javascript and checking immediately could result in the test falsely passing even if the tap would have actually been registered a few milliseconds later.
dtrainor@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc@ in case he's aware of a test utility method that will wait for the event to get processed. It looks like RWHV supports ProcessAckedTouchEvent()? But not on Android and it would need to get pushed all the way to the test layer. My main concern is these things generally result in flaky tests that just get disabled (especially depending on the type of device).
On 2017/03/23 04:35:17, David Trainor-ping if over 24h wrote: > +tedchoc@ in case he's aware of a test utility method that will wait for the > event to get processed. It looks like RWHV supports ProcessAckedTouchEvent()? > But not on Android and it would need to get pushed all the way to the test > layer. > > My main concern is these things generally result in flaky tests that just get > disabled (especially depending on the type of device). I'd look at GestureStateListener from ContentViewCore. That "might" be what we could listen to?
On 2017/03/23 19:09:35, Ted C wrote: > On 2017/03/23 04:35:17, David Trainor-ping if over 24h wrote: > > +tedchoc@ in case he's aware of a test utility method that will wait for the > > event to get processed. It looks like RWHV supports ProcessAckedTouchEvent()? > > > But not on Android and it would need to get pushed all the way to the test > > layer. > > > > My main concern is these things generally result in flaky tests that just get > > disabled (especially depending on the type of device). > > I'd look at GestureStateListener from ContentViewCore. That "might" be what > we could listen to? Unfortunately, it looks like using GestureStateListener just gives us redundant information. I tried adding a GestureStateListener that logs anytime onSingleTap() is called. In the Daydream case, it only logged once when entering VR. In the cardboard case, it logged twice, once when entering VR and once when tapping after entering VR. In all cases, the given "consumed" boolean was false.
On 2017/03/23 20:18:55, bsheedy wrote: > On 2017/03/23 19:09:35, Ted C wrote: > > On 2017/03/23 04:35:17, David Trainor-ping if over 24h wrote: > > > +tedchoc@ in case he's aware of a test utility method that will wait for the > > > event to get processed. It looks like RWHV supports > ProcessAckedTouchEvent()? > > > > > But not on Android and it would need to get pushed all the way to the test > > > layer. > > > > > > My main concern is these things generally result in flaky tests that just > get > > > disabled (especially depending on the type of device). > > > > I'd look at GestureStateListener from ContentViewCore. That "might" be what > > we could listen to? > > Unfortunately, it looks like using GestureStateListener just gives us redundant > information. I tried adding a GestureStateListener that logs anytime > onSingleTap() is called. In the Daydream case, it only logged once when entering > VR. In the cardboard case, it logged twice, once when entering VR and once when > tapping after entering VR. In all cases, the given "consumed" boolean was false. Any other ideas about what we could possibly listen to?
https://codereview.chromium.org/2768583002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java (right): https://codereview.chromium.org/2768583002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:253: @SmallTest This should likely be a LargeTest (and probably should have been a MediumTest before IMO). https://codereview.chromium.org/2768583002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:265: for (int i = 0; i < 5; i++) { What is the goal of the test? Are we trying to send multiple click events and see that nothing happens? In particular, would we want to have this in a loop, but we don't have a reliable way to delay the next event until the previous one was sent to the renderer? If we did have such a mechanism, would we want to send only two events and ensure the second event wasn't registered? I don't have a good suggestion for receiving acks that a event has been processed (aside from what we suggested and that didn't work for you). dtrainor@ suggested there was some thing in content/ that did receive ACKs so we could look at piping that out. I'm fine if we want to file a new crbug and add a TODO here though to make forward progress.
https://codereview.chromium.org/2768583002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java (right): https://codereview.chromium.org/2768583002/diff/20001/chrome/android/javatest... 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 should likely be a LargeTest (and probably should have been a MediumTest > before IMO). Done. https://codereview.chromium.org/2768583002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:265: for (int i = 0; i < 5; i++) { On 2017/03/27 15:44:06, Ted C wrote: > What is the goal of the test? Are we trying to send multiple click events and > see that nothing happens? In particular, would we want to have this in a loop, > but we don't have a reliable way to delay the next event until the previous one > was sent to the renderer? > > If we did have such a mechanism, would we want to send only two events and > ensure the second event wasn't registered? > > I don't have a good suggestion for receiving acks that a event has been > processed (aside from what we suggested and that didn't work for you). > dtrainor@ suggested there was some thing in content/ that did receive ACKs so we > could look at piping that out. I'm fine if we want to file a new crbug and add > a TODO here though to make forward progress. The purpose is to test that tapping on the screen when we're presenting in WebVR doesn't fire any click events if the VR viewer is set to Daydream View. Ideally, we send the first event (before we're presenting in WebVR) to enter VR (we need a user gesture in order to do so). Then, we send a second event, somehow wait until we know for sure that the event would have been fired in Javascript by now, and make sure that it hasn't actually been fired. I've talked to a couple SWEs working on the VR stuff for Chrome, and it looks like the touch event is actually being consumed by Daydream's DaydreamTouchListener when the viewer is set to Daydream View. I'm not exactly familiar with what exactly that ends up affecting, but I think it doesn't even make it to content/ in that case. If there's some way to be notified whenever events get consumed, that could work.
Description was changed from ========== Send multiple taps during VR tap tests 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, so instead send multiple taps over a one second period. BUG= ========== to ========== 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= ==========
bsheedy@chromium.org changed reviewers: + mthiesse@chromium.org
+mthiesse for the VR changes. I've added a way for the Daydream case to wait on VrShellImpl's parent to report that it's consumed the event, which should (hopefully?) mean we don't check that no clicks were registered in Javascript until they actually would have had a chance to.
Seems like a good approach overall, minor comments. https://codereview.chromium.org/2768583002/diff/80001/chrome/android/java/src... 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... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:451: nativeOnTriggerEvent(mNativeVrShell); Don't we also want to make sure this wasn't called? https://codereview.chromium.org/2768583002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java (right): https://codereview.chromium.org/2768583002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:277: touchRegisteredLatch.await(); What's the timeout on this? Will it wait forever if this test is going to fail? Would it be better to run a callback with a boolean parameter for whether or not daydream consumed the touch event? We can fail early if daydream didn't consume it, or if we generated a cardboard trigger event from the input.
Addressed mthiesse@ comments https://codereview.chromium.org/2768583002/diff/80001/chrome/android/java/src... 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... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellImpl.java:451: nativeOnTriggerEvent(mNativeVrShell); On 2017/03/28 18:08:01, mthiesse wrote: > Don't we also want to make sure this wasn't called? Done. https://codereview.chromium.org/2768583002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java (right): https://codereview.chromium.org/2768583002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:277: touchRegisteredLatch.await(); On 2017/03/28 18:08:01, mthiesse wrote: > What's the timeout on this? Will it wait forever if this test is going to fail? > > Would it be better to run a callback with a boolean parameter for whether or not > daydream consumed the touch event? We can fail early if daydream didn't consume > it, or if we generated a cardboard trigger event from the input. Done, and made the await() have a one second timeout.
lgtm
Can you try one last thing? Maybe override the OnClickListener for the CompositorViewHolder and validate the event is getting stolen before that? If that doesn't work with your flow or won't get you what you need, that's ok. lgtm either way.
On 2017/03/28 23:55:24, David Trainor-ping if over 24h wrote: > Can you try one last thing? Maybe override the OnClickListener for the > CompositorViewHolder and validate the event is getting stolen before that? If > that doesn't work with your flow or won't get you what you need, that's ok. > > lgtm either way. Doesn't look like that works. I tried getActivity().getTabsView().setOnClickListener( ... ), where getTabsView() returns ChromeTabbedActivity's CompositorViewHolder (or rather, whatever getCompositorViewHolder() returns). It looks like the OnClickListener's onClick function isn't called even in the Cardboard case where clicks are being received in Javascript.
The CQ bit was checked by bsheedy@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by bsheedy@chromium.org
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": 100001, "attempt_start_ts": 1490804393181420, "parent_rev": "1c59cb32b65444780200c5147fe3d53d4b03ac37", "commit_rev": "3e2ce51e88f19f0cc5196284f580583ddd4dbd9a"}
Message was sent while issue was closed.
Description was changed from ========== 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= ========== to ========== 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-Commit-Position: refs/heads/master@{#460452} Committed: https://chromium.googlesource.com/chromium/src/+/3e2ce51e88f19f0cc5196284f580... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/3e2ce51e88f19f0cc5196284f580...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2782963002/ by mdjones@chromium.org. The reason for reverting is: Breaking compile on several bots including Android x64 builder: symbol: class OnDispatchTouchEventCallback location: class WebVrTest ../../chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:269: error: cannot find symbol ((VrShellImpl) VrShellDelegate.getVrShellForTesting().
Message was sent while issue was closed.
Description was changed from ========== 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-Commit-Position: refs/heads/master@{#460452} Committed: https://chromium.googlesource.com/chromium/src/+/3e2ce51e88f19f0cc5196284f580... ========== to ========== 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-Commit-Position: refs/heads/master@{#460452} Committed: https://chromium.googlesource.com/chromium/src/+/3e2ce51e88f19f0cc5196284f580... ==========
On 2017/03/29 19:20:43, mdjones wrote: > A revert of this CL (patchset #6 id:100001) has been created in > https://codereview.chromium.org/2782963002/ by mailto:mdjones@chromium.org. > > The reason for reverting is: Breaking compile on several bots including Android > x64 builder: > symbol: class OnDispatchTouchEventCallback > location: class WebVrTest > ../../chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:269: > error: cannot find symbol > ((VrShellImpl) VrShellDelegate.getVrShellForTesting(). It looks like the issue was that the VR test APK and the VR source inclusions were behind the enable_vr flag, but the VR test library wasn't. With this CL, one of the files in the test library (WebVrTest.java) started depending on a file that was only included when enable_vr was set. I've moved the library behind the flag now, as it should only be built when VR is enabled anyways.
The CQ bit was checked by bsheedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2768583002/#ps120001 (title: "Move VR test library behind flag")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
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/bui...)
The CQ bit was checked by bsheedy@chromium.org
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": 120001, "attempt_start_ts": 1490837306755970, "parent_rev": "b9058f285cf483b7c49fe46e554f55f54ffeb956", "commit_rev": "bc288f4389ba798aa9c3ad2f593a2b008d0b1a6a"}
Message was sent while issue was closed.
Description was changed from ========== 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-Commit-Position: refs/heads/master@{#460452} Committed: https://chromium.googlesource.com/chromium/src/+/3e2ce51e88f19f0cc5196284f580... ========== to ========== 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/+/3e2ce51e88f19f0cc5196284f580... Review-Url: https://codereview.chromium.org/2768583002 Cr-Commit-Position: refs/heads/master@{#460623} Committed: https://chromium.googlesource.com/chromium/src/+/bc288f4389ba798aa9c3ad2f593a... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/bc288f4389ba798aa9c3ad2f593a... |