Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2668003006:
Add restriction for running tests only with Daydream View headset (Closed)
DescriptionAdd restriction for running tests only with Daydream View headset
Adds an @Restriction option for only running a test if the current VR viewer is
set to Daydream View. One example use case of this is for VR tests using the
Daydream View NFC scan - since scanning sets the current viewer to Daydream
View, we don't want to run an NFC test unless the viewer is already set to
Daydream View. Otherwise, the viewer will switch in the middle of running
tests, which may affect the results of later tests within the same APK.
BUG=671373
Review-Url: https://codereview.chromium.org/2668003006
Cr-Commit-Position: refs/heads/master@{#448511}
Committed: https://chromium.googlesource.com/chromium/src/+/3014682836a4792d50abfa27456fce6a2dceaa0d
Patch Set 1 #
Total comments: 14
Patch Set 2 : Switch to VrClassesWrapper to remove excess reflection #
Total comments: 4
Patch Set 3 : Address mthiesse@ comments #
Total comments: 2
Patch Set 4 : Context.class -> Activity.class #
Total comments: 4
Patch Set 5 : Address tedchoc@ comments #
Messages
Total messages: 27 (8 generated)
bsheedy@chromium.org changed reviewers: + aelias@chromium.org, mthiesse@chromium.org
+mthiesse for VrShellTest.java OWNER and making sure there isn't a better way to get the current viewer. +aelias for chrome/test/android OWNER
bsheedy@chromium.org changed reviewers: + bauerb@chromium.org
Apparently mthiesse isn't actually an OWNER of the test file. +bauerb for OWNER instead.
https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java (right): https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:99: private boolean isDaydreamViewPaired() { You should probably either use VrShellDelegate#createVrClassesWrapper(), or copy the function to create an instance of the VrClassesWrapper, which can give you an instance of the DaydreamApi class wrapped in an API that doesn't require reflection to use.
https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java (right): https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:59: // in the Runnable I think this only applies to |mConcreteApiInstance|, as the other members are used for lazy initialization? And you might be able to make |mConcreteApiInstance| a local variable anyway, see below. https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:68: private void getApiClassIfNecessary() { Nit: It would feel a bit more natural to me to have this method return the API class (or null) and have all accesses go through it. https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:76: mDaydreamApiClass = null; Nit: This isn't even necessary -- if an exception is thrown above, it would happen before something is assigned to |mDaydreamApiClass|, so it would still have the default value of null. https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:109: FutureTask<Void> createApiInstance = new FutureTask<Void>(new Runnable() { You could make this a FutureTask<Object> and return the API instance, then you don't need the member (which isn't used outside of this method, right?)
Addressed comments - waiting on reply to comment on using VrClassesWrapper before uploading a new patch. https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java (right): https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:59: // in the Runnable On 2017/02/02 10:16:48, Bernhard Bauer wrote: > I think this only applies to |mConcreteApiInstance|, as the other members are > used for lazy initialization? And you might be able to make > |mConcreteApiInstance| a local variable anyway, see below. Done. https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:68: private void getApiClassIfNecessary() { On 2017/02/02 10:16:48, Bernhard Bauer wrote: > Nit: It would feel a bit more natural to me to have this method return the API > class (or null) and have all accesses go through it. Done. https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:76: mDaydreamApiClass = null; On 2017/02/02 10:16:48, Bernhard Bauer wrote: > Nit: This isn't even necessary -- if an exception is thrown above, it would > happen before something is assigned to |mDaydreamApiClass|, so it would still > have the default value of null. Done. https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:99: private boolean isDaydreamViewPaired() { On 2017/02/01 23:04:18, mthiesse wrote: > You should probably either use VrShellDelegate#createVrClassesWrapper(), or copy > the function to create an instance of the VrClassesWrapper, which can give you > an instance of the DaydreamApi class wrapped in an API that doesn't require > reflection to use. Spent some time trying to make this work, but not sure if it's (currently) possible. VrClassesWrapperImpl needs an Activity, but getTargetContext() returns a ChromeApplication, and AFAIK, this part of the code is run before any activities are actually started up. Passing in a new Activity allows us to create a VrClassesWrapperImpl and a VrDaydreamApi, but then calling any of its functions that use the activity (e.g. isDaydreamReadyDevice()) cause the test to silently fail. https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:109: FutureTask<Void> createApiInstance = new FutureTask<Void>(new Runnable() { On 2017/02/02 10:16:48, Bernhard Bauer wrote: > You could make this a FutureTask<Object> and return the API instance, then you > don't need the member (which isn't used outside of this method, right?) Done.
https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java (right): https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:99: private boolean isDaydreamViewPaired() { On 2017/02/02 20:11:24, bsheedy wrote: > On 2017/02/01 23:04:18, mthiesse wrote: > > You should probably either use VrShellDelegate#createVrClassesWrapper(), or > copy > > the function to create an instance of the VrClassesWrapper, which can give you > > an instance of the DaydreamApi class wrapped in an API that doesn't require > > reflection to use. > > Spent some time trying to make this work, but not sure if it's (currently) > possible. VrClassesWrapperImpl needs an Activity, but getTargetContext() returns > a ChromeApplication, and AFAIK, this part of the code is run before any > activities are actually started up. Passing in a new Activity allows us to > create a VrClassesWrapperImpl and a VrDaydreamApi, but then calling any of its > functions that use the activity (e.g. isDaydreamReadyDevice()) cause the test to > silently fail. I don't think I understand why this currently works, but using VrClassesWrapperImpl doesn't. Aren't you using the same Context in both cases?
https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java (right): https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:99: private boolean isDaydreamViewPaired() { On 2017/02/02 20:25:18, mthiesse wrote: > On 2017/02/02 20:11:24, bsheedy wrote: > > On 2017/02/01 23:04:18, mthiesse wrote: > > > You should probably either use VrShellDelegate#createVrClassesWrapper(), or > > copy > > > the function to create an instance of the VrClassesWrapper, which can give > you > > > an instance of the DaydreamApi class wrapped in an API that doesn't require > > > reflection to use. > > > > Spent some time trying to make this work, but not sure if it's (currently) > > possible. VrClassesWrapperImpl needs an Activity, but getTargetContext() > returns > > a ChromeApplication, and AFAIK, this part of the code is run before any > > activities are actually started up. Passing in a new Activity allows us to > > create a VrClassesWrapperImpl and a VrDaydreamApi, but then calling any of its > > functions that use the activity (e.g. isDaydreamReadyDevice()) cause the test > to > > silently fail. > > I don't think I understand why this currently works, but using > VrClassesWrapperImpl doesn't. Aren't you using the same Context in both cases? Currently, I'm directly using the NDK's DaydreamApi, which only requires a Context, at least for all the functions I'm using. VrClassesWrapperImpl requires an Activity (which we don't have), and we can't switch it to require a Context since exitFromVr requires an Activity.
https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java (right): https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:99: private boolean isDaydreamViewPaired() { On 2017/02/02 21:01:38, bsheedy wrote: > On 2017/02/02 20:25:18, mthiesse wrote: > > On 2017/02/02 20:11:24, bsheedy wrote: > > > On 2017/02/01 23:04:18, mthiesse wrote: > > > > You should probably either use VrShellDelegate#createVrClassesWrapper(), > or > > > copy > > > > the function to create an instance of the VrClassesWrapper, which can give > > you > > > > an instance of the DaydreamApi class wrapped in an API that doesn't > require > > > > reflection to use. > > > > > > Spent some time trying to make this work, but not sure if it's (currently) > > > possible. VrClassesWrapperImpl needs an Activity, but getTargetContext() > > returns > > > a ChromeApplication, and AFAIK, this part of the code is run before any > > > activities are actually started up. Passing in a new Activity allows us to > > > create a VrClassesWrapperImpl and a VrDaydreamApi, but then calling any of > its > > > functions that use the activity (e.g. isDaydreamReadyDevice()) cause the > test > > to > > > silently fail. > > > > I don't think I understand why this currently works, but using > > VrClassesWrapperImpl doesn't. Aren't you using the same Context in both cases? > > Currently, I'm directly using the NDK's DaydreamApi, which only requires a > Context, at least for all the functions I'm using. VrClassesWrapperImpl requires > an Activity (which we don't have), and we can't switch it to require a Context > since exitFromVr requires an Activity. You could make VrClassWrapperImpl take in a Context, rather than Activity, and cast the Context into an Activity if it is one, but otherwise just use it as a context. (See https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/base...)
Switched over to use VrClassesWrapper - PTAL. https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java (right): https://codereview.chromium.org/2668003006/diff/1/chrome/test/android/javates... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:99: private boolean isDaydreamViewPaired() { On 2017/02/02 21:49:46, mthiesse wrote: > On 2017/02/02 21:01:38, bsheedy wrote: > > On 2017/02/02 20:25:18, mthiesse wrote: > > > On 2017/02/02 20:11:24, bsheedy wrote: > > > > On 2017/02/01 23:04:18, mthiesse wrote: > > > > > You should probably either use VrShellDelegate#createVrClassesWrapper(), > > or > > > > copy > > > > > the function to create an instance of the VrClassesWrapper, which can > give > > > you > > > > > an instance of the DaydreamApi class wrapped in an API that doesn't > > require > > > > > reflection to use. > > > > > > > > Spent some time trying to make this work, but not sure if it's (currently) > > > > possible. VrClassesWrapperImpl needs an Activity, but getTargetContext() > > > returns > > > > a ChromeApplication, and AFAIK, this part of the code is run before any > > > > activities are actually started up. Passing in a new Activity allows us to > > > > create a VrClassesWrapperImpl and a VrDaydreamApi, but then calling any of > > its > > > > functions that use the activity (e.g. isDaydreamReadyDevice()) cause the > > test > > > to > > > > silently fail. > > > > > > I don't think I understand why this currently works, but using > > > VrClassesWrapperImpl doesn't. Aren't you using the same Context in both > cases? > > > > Currently, I'm directly using the NDK's DaydreamApi, which only requires a > > Context, at least for all the functions I'm using. VrClassesWrapperImpl > requires > > an Activity (which we don't have), and we can't switch it to require a Context > > since exitFromVr requires an Activity. > > You could make VrClassWrapperImpl take in a Context, rather than Activity, and > cast the Context into an Activity if it is one, but otherwise just use it as a > context. (See > https://cs.chromium.org/chromium/src/ui/android/java/src/org/chromium/ui/base...) > Done.
lgtm
https://codereview.chromium.org/2668003006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrClassesWrapperImpl.java (right): https://codereview.chromium.org/2668003006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrClassesWrapperImpl.java:29: public VrClassesWrapperImpl(Context context) { Might be better if you expose a second constructor and note that it's only for testing. You can then store only the Context type, because Activity can be cast to Context. https://codereview.chromium.org/2668003006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDaydreamApiImpl.java (right): https://codereview.chromium.org/2668003006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDaydreamApiImpl.java:73: if (activity != null) { Can we instead assert that activity is not null here? Or do tests have to call this function for some reason?
https://codereview.chromium.org/2668003006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrClassesWrapperImpl.java (right): https://codereview.chromium.org/2668003006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrClassesWrapperImpl.java:29: public VrClassesWrapperImpl(Context context) { On 2017/02/03 15:24:47, mthiesse wrote: > Might be better if you expose a second constructor and note that it's only for > testing. > > You can then store only the Context type, because Activity can be cast to > Context. Done. https://codereview.chromium.org/2668003006/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDaydreamApiImpl.java (right): https://codereview.chromium.org/2668003006/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrDaydreamApiImpl.java:73: if (activity != null) { On 2017/02/03 15:24:47, mthiesse wrote: > Can we instead assert that activity is not null here? Or do tests have to call > this function for some reason? Should never have to be called without a valid Activity. Done.
lgtm https://codereview.chromium.org/2668003006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2668003006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:221: vrClassesBuilderClass.getConstructor(Context.class); change this back to Activity.class
https://codereview.chromium.org/2668003006/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2668003006/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:221: vrClassesBuilderClass.getConstructor(Context.class); On 2017/02/03 18:23:52, mthiesse wrote: > change this back to Activity.class Done.
Description was changed from ========== Add restriction for running tests only with Daydream View headset Adds an @Restriction option for only running a test if the current VR viewer is set to Daydream View. One example use case of this is for VR tests using the Daydream View NFC scan - since scanning sets the current viewer to Daydream View, we don't want to run an NFC test unless the viewer is already set to Daydream View. Otherwise, the viewer will switch in the middle of running tests, which may affect the results of later tests within the same APK. BUG=671373 ========== to ========== Add restriction for running tests only with Daydream View headset Adds an @Restriction option for only running a test if the current VR viewer is set to Daydream View. One example use case of this is for VR tests using the Daydream View NFC scan - since scanning sets the current viewer to Daydream View, we don't want to run an NFC test unless the viewer is already set to Daydream View. Otherwise, the viewer will switch in the middle of running tests, which may affect the results of later tests within the same APK. BUG=671373 ==========
bsheedy@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc for chrome/test/android OWNER
chrome/test/android - lgtm w/ comments https://codereview.chromium.org/2668003006/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java (right): https://codereview.chromium.org/2668003006/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:61: private boolean mAttemptedToGetApi = false; = false isn't needed in java (false, 0, and null are defaults) for class level vars. https://codereview.chromium.org/2668003006/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:108: mainThreadHandler.post(checker); use ThreadUtils.runOnUiThreadBlocking that takes a Callable.
https://codereview.chromium.org/2668003006/diff/60001/chrome/test/android/jav... File chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java (right): https://codereview.chromium.org/2668003006/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:61: private boolean mAttemptedToGetApi = false; On 2017/02/06 17:50:44, Ted C wrote: > = false isn't needed in java (false, 0, and null are defaults) for class level > vars. Done. https://codereview.chromium.org/2668003006/diff/60001/chrome/test/android/jav... chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeInstrumentationTestRunner.java:108: mainThreadHandler.post(checker); On 2017/02/06 17:50:44, Ted C wrote: > use ThreadUtils.runOnUiThreadBlocking that takes a Callable. Done. Wasn't aware that the UI thread and main thread were the same, will remember in the future.
The CQ bit was checked by bsheedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, mthiesse@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2668003006/#ps80001 (title: "Address tedchoc@ 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": 80001, "attempt_start_ts": 1486430886034580,
"parent_rev": "10e5fc3598c9f2e1015458c814a1163680465b3e", "commit_rev":
"3014682836a4792d50abfa27456fce6a2dceaa0d"}
Message was sent while issue was closed.
Description was changed from ========== Add restriction for running tests only with Daydream View headset Adds an @Restriction option for only running a test if the current VR viewer is set to Daydream View. One example use case of this is for VR tests using the Daydream View NFC scan - since scanning sets the current viewer to Daydream View, we don't want to run an NFC test unless the viewer is already set to Daydream View. Otherwise, the viewer will switch in the middle of running tests, which may affect the results of later tests within the same APK. BUG=671373 ========== to ========== Add restriction for running tests only with Daydream View headset Adds an @Restriction option for only running a test if the current VR viewer is set to Daydream View. One example use case of this is for VR tests using the Daydream View NFC scan - since scanning sets the current viewer to Daydream View, we don't want to run an NFC test unless the viewer is already set to Daydream View. Otherwise, the viewer will switch in the middle of running tests, which may affect the results of later tests within the same APK. BUG=671373 Review-Url: https://codereview.chromium.org/2668003006 Cr-Commit-Position: refs/heads/master@{#448511} Committed: https://chromium.googlesource.com/chromium/src/+/3014682836a4792d50abfa27456f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/3014682836a4792d50abfa27456f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
