Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2851313002:
Adds end-to-end tests for VrShell navigation transitions. (Closed)
DescriptionAdds end-to-end tests for VrShell navigation transitions.
- Adds a new test class, which tests all navigation transitions (e.g. clicking a link) in VrShell end-to-end.
BUG=715663
Review-Url: https://codereview.chromium.org/2851313002
Cr-Commit-Position: refs/heads/master@{#469143}
Committed: https://chromium.googlesource.com/chromium/src/+/f72a7d6c0adfaf509f04b2b307f016e71ce68578
Patch Set 1 #
Total comments: 34
Patch Set 2 : Incorporated review feedback #
Total comments: 4
Patch Set 3 : Incorporated review feedback 2 #
Total comments: 12
Patch Set 4 : Rebased on ToT, incorporated review feedback 3 #Messages
Total messages: 21 (7 generated)
tiborg@chromium.org changed reviewers: + bsheedy@chromium.org
Hi Brian, Please take a look at these end-to-end tests. I tried to reuse your prior work as much as possible. Your testing framework is really very helpful! If there is code we should refactor out to utils so that we can reuse it in other tests I'm happy to do this. Thank you! Cheers, Tibor
Overall, looks pretty good. Most comments are fairly minor and/or notes to myself to fix things on my end. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java (right): https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:28: @CommandLineFlags.Add("enable-features=VrShell") FYI, I think the enabling of VR Shell won't automatically enable WebVR like it does currently (seem to remember seeing a bug to separate the two), in which case you'll need to add the "enable-webvr" flag either here or to individual tests that use it. Can wait until they're split to do that, though. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:54: runJavaScriptOrFail("window.location.href = '" + testPageUrl + "';", POLL_TIMEOUT_SHORT_MS, Was there an issue using loadUrl() for this? It seemed to work while in VR when I tested it, and it should handle waiting for the page to load, etc. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:73: private void goFullscreened() throws InterruptedException, TimeoutException { nit: goFullscreened sounds a little odd to me - maybe enterFullscreen instead? https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:74: DOMUtils.clickNode(getActivity().getActivityTab().getContentViewCore(), "fullscreen"); I would suggest changing goFullscreened() to goFullscreened(ContentViewCore cvc), then: DomUtils.clickNode(cvc, "fullscreen"); waitOnJavaScriptStep(cvc.getWebContents()); assertTrue(DOMUtils.isFullscreen(cvc.getWebContents())); Simplifies code a bit, and makes the function usable in any tab (current use of mWebContents means it'd only work in the tab that's first opened when Chrome starts). As a note to myself, I should probably change the name of mWebContents to be more descriptive, e.g. mFirstTabWebContents. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:80: private void present() throws InterruptedException, TimeoutException { There's already a function to do this in VrTestBase (enterVrTap), although I didn't know that clickNode existed when I wrote it. It might be better to use clickNode instead of the current implementation (might solve some of the flakiness I've seen with it). Leave this as-is for now, but can you add a TODO for me here to investigate which solution is better? https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:81: DOMUtils.clickNode(getActivity().getActivityTab().getContentViewCore(), "webgl-canvas"); Same comment as above - pass in a CVC and use that. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:101: "Browser is on correct web site", url, getActivity().getActivityTab().getUrl()); Consider making assertState take a WebContents object, then using its getVisibleUrl() for the URL comparison and pass it directly to DOMUtils.isFullscreen(). https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:111: public int loadUrl(String url, long secondsToWait) I think the base loadUrl blocks until the page is fully loaded, in which case this override looks unnecessary. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:123: assertTrue(VrShellDelegate.isInVr()); nit: This assert doesn't really do anything since VrUtils.waitForVrSupported will throw an assertion error if VrShellDelegate.isInVr() is not true within the given timeout. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:127: public void startMainActivity() throws InterruptedException { Unnecessary override since VrTestBase's startMainActivity does the same thing. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:131: @Restriction(RESTRICTION_TYPE_VIEWER_DAYDREAM) Since every test here seems to have the same restriction of using Daydream View, you can put the restriction at the class level. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:133: public void test2dTo2d() throws InterruptedException, TimeoutException { I think test cases are supposed to have comments before them even if the names are self-explanatory. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:134: // Arrange. nit: The arrange/act/assert comments seem unnecessary since the test cases are pretty self-documenting. https://codereview.chromium.org/2851313002/diff/1/chrome/test/data/android/we... File chrome/test/data/android/webvr_instrumentation/html/test_navigation_2d_page.html (right): https://codereview.chromium.org/2851313002/diff/1/chrome/test/data/android/we... chrome/test/data/android/webvr_instrumentation/html/test_navigation_2d_page.html:6: <script src="../../../../../../third_party/WebKit/LayoutTests/resources/testharness.js"></script> Since you aren't doing any actual JavaScript-side testing, you should be able to remove this. https://codereview.chromium.org/2851313002/diff/1/chrome/test/data/android/we... File chrome/test/data/android/webvr_instrumentation/html/test_navigation_webvr_page.html (right): https://codereview.chromium.org/2851313002/diff/1/chrome/test/data/android/we... chrome/test/data/android/webvr_instrumentation/html/test_navigation_webvr_page.html:7: <script src="../../../../../../third_party/WebKit/LayoutTests/resources/testharness.js"></script> Should be able to remove here, too.
https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java (right): https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:28: @CommandLineFlags.Add("enable-features=VrShell") On 2017/05/01 21:48:17, bsheedy wrote: > FYI, I think the enabling of VR Shell won't automatically enable WebVR like it > does currently (seem to remember seeing a bug to separate the two), in which > case you'll need to add the "enable-webvr" flag either here or to individual > tests that use it. > > Can wait until they're split to do that, though. Done. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:54: runJavaScriptOrFail("window.location.href = '" + testPageUrl + "';", POLL_TIMEOUT_SHORT_MS, On 2017/05/01 21:48:16, bsheedy wrote: > Was there an issue using loadUrl() for this? It seemed to work while in VR when > I tested it, and it should handle waiting for the page to load, etc. I was not sure how many steps of the transition loadUrl skips. And since these are navigation e2e tests I wanted to make sure that the page initiates the transition. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:73: private void goFullscreened() throws InterruptedException, TimeoutException { On 2017/05/01 21:48:16, bsheedy wrote: > nit: goFullscreened sounds a little odd to me - maybe enterFullscreen instead? Done. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:74: DOMUtils.clickNode(getActivity().getActivityTab().getContentViewCore(), "fullscreen"); On 2017/05/01 21:48:16, bsheedy wrote: > I would suggest changing goFullscreened() to goFullscreened(ContentViewCore > cvc), then: > > DomUtils.clickNode(cvc, "fullscreen"); > waitOnJavaScriptStep(cvc.getWebContents()); > assertTrue(DOMUtils.isFullscreen(cvc.getWebContents())); > > Simplifies code a bit, and makes the function usable in any tab (current use of > mWebContents means it'd only work in the tab that's first opened when Chrome > starts). > > As a note to myself, I should probably change the name of mWebContents to be > more descriptive, e.g. mFirstTabWebContents. Done. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:80: private void present() throws InterruptedException, TimeoutException { On 2017/05/01 21:48:16, bsheedy wrote: > There's already a function to do this in VrTestBase (enterVrTap), although I > didn't know that clickNode existed when I wrote it. It might be better to use > clickNode instead of the current implementation (might solve some of the > flakiness I've seen with it). Leave this as-is for now, but can you add a TODO > for me here to investigate which solution is better? I considered changing enterVrTap. However, then WebVrTest#testScreenTapsNotRegisteredOnDaydream failed since this is using the function to tap on the screen. clickNode does not tap on the screen as far as I know but rather triggers the click handler of a DOM node. Might be wrong though. In any case, added a TODO for you. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:81: DOMUtils.clickNode(getActivity().getActivityTab().getContentViewCore(), "webgl-canvas"); On 2017/05/01 21:48:16, bsheedy wrote: > Same comment as above - pass in a CVC and use that. Done. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:101: "Browser is on correct web site", url, getActivity().getActivityTab().getUrl()); On 2017/05/01 21:48:17, bsheedy wrote: > Consider making assertState take a WebContents object, then using its > getVisibleUrl() for the URL comparison and pass it directly to > DOMUtils.isFullscreen(). Done. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:111: public int loadUrl(String url, long secondsToWait) On 2017/05/01 21:48:16, bsheedy wrote: > I think the base loadUrl blocks until the page is fully loaded, in which case > this override looks unnecessary. I'm not sure if loadUrl waits until all resources are loaded as well. I had problems with not being able to use function from navigation_e2e.js or webvr_e2e.js. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:123: assertTrue(VrShellDelegate.isInVr()); On 2017/05/01 21:48:16, bsheedy wrote: > nit: This assert doesn't really do anything since VrUtils.waitForVrSupported > will throw an assertion error if VrShellDelegate.isInVr() is not true within the > given timeout. Done. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:127: public void startMainActivity() throws InterruptedException { On 2017/05/01 21:48:17, bsheedy wrote: > Unnecessary override since VrTestBase's startMainActivity does the same thing. Done. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:131: @Restriction(RESTRICTION_TYPE_VIEWER_DAYDREAM) On 2017/05/01 21:48:16, bsheedy wrote: > Since every test here seems to have the same restriction of using Daydream View, > you can put the restriction at the class level. Done. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:133: public void test2dTo2d() throws InterruptedException, TimeoutException { On 2017/05/01 21:48:17, bsheedy wrote: > I think test cases are supposed to have comments before them even if the names > are self-explanatory. Done. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:134: // Arrange. On 2017/05/01 21:48:17, bsheedy wrote: > nit: The arrange/act/assert comments seem unnecessary since the test cases are > pretty self-documenting. Done. https://codereview.chromium.org/2851313002/diff/1/chrome/test/data/android/we... File chrome/test/data/android/webvr_instrumentation/html/test_navigation_2d_page.html (right): https://codereview.chromium.org/2851313002/diff/1/chrome/test/data/android/we... chrome/test/data/android/webvr_instrumentation/html/test_navigation_2d_page.html:6: <script src="../../../../../../third_party/WebKit/LayoutTests/resources/testharness.js"></script> On 2017/05/01 21:48:17, bsheedy wrote: > Since you aren't doing any actual JavaScript-side testing, you should be able to > remove this. Done. https://codereview.chromium.org/2851313002/diff/1/chrome/test/data/android/we... File chrome/test/data/android/webvr_instrumentation/html/test_navigation_webvr_page.html (right): https://codereview.chromium.org/2851313002/diff/1/chrome/test/data/android/we... chrome/test/data/android/webvr_instrumentation/html/test_navigation_webvr_page.html:7: <script src="../../../../../../third_party/WebKit/LayoutTests/resources/testharness.js"></script> On 2017/05/01 21:48:17, bsheedy wrote: > Should be able to remove here, too. But I need it for the add_completion_callback in webvr_e2e.js.
tiborg@chromium.org changed reviewers: + mthiesse@chromium.org
mthiesse@chromium.org: Please take a look at the VrShell code. Thanks!
LGTM with the comments addressed. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java (right): https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:54: runJavaScriptOrFail("window.location.href = '" + testPageUrl + "';", POLL_TIMEOUT_SHORT_MS, On 2017/05/01 23:09:45, tiborg wrote: > On 2017/05/01 21:48:16, bsheedy wrote: > > Was there an issue using loadUrl() for this? It seemed to work while in VR > when > > I tested it, and it should handle waiting for the page to load, etc. > > I was not sure how many steps of the transition loadUrl skips. And since these > are navigation e2e tests I wanted to make sure that the page initiates the > transition. Makes sense, although I'd add a comment explaining why you're doing it this way instead of using loadUrl. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:111: public int loadUrl(String url, long secondsToWait) On 2017/05/01 23:09:46, tiborg wrote: > On 2017/05/01 21:48:16, bsheedy wrote: > > I think the base loadUrl blocks until the page is fully loaded, in which case > > this override looks unnecessary. > > I'm not sure if loadUrl waits until all resources are loaded as well. I had > problems with not being able to use function from navigation_e2e.js or > webvr_e2e.js. In that case, I'll consider moving this into VrTestBase at some point - I haven't had issues in WebVrTest.java yet since I was always explicitly waiting on the synchronization booleans to be set during a test, but it'd be good to not have to rely on that. Leave this as-is for now. https://codereview.chromium.org/2851313002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java (right): https://codereview.chromium.org/2851313002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:100: throw new UnsupportedOperationException(); nit: Add a message to the exception https://codereview.chromium.org/2851313002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:115: waitOnJavaScriptStep(mWebContents); Change this to getActivity().getActivityTab().getWebContents() so that this works for whatever the currently displayed tab is instead of just the first tab.
https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java (right): https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:54: runJavaScriptOrFail("window.location.href = '" + testPageUrl + "';", POLL_TIMEOUT_SHORT_MS, On 2017/05/01 23:36:31, bsheedy wrote: > On 2017/05/01 23:09:45, tiborg wrote: > > On 2017/05/01 21:48:16, bsheedy wrote: > > > Was there an issue using loadUrl() for this? It seemed to work while in VR > > when > > > I tested it, and it should handle waiting for the page to load, etc. > > > > I was not sure how many steps of the transition loadUrl skips. And since these > > are navigation e2e tests I wanted to make sure that the page initiates the > > transition. > > Makes sense, although I'd add a comment explaining why you're doing it this way > instead of using loadUrl. Done. https://codereview.chromium.org/2851313002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:111: public int loadUrl(String url, long secondsToWait) On 2017/05/01 23:36:31, bsheedy wrote: > On 2017/05/01 23:09:46, tiborg wrote: > > On 2017/05/01 21:48:16, bsheedy wrote: > > > I think the base loadUrl blocks until the page is fully loaded, in which > case > > > this override looks unnecessary. > > > > I'm not sure if loadUrl waits until all resources are loaded as well. I had > > problems with not being able to use function from navigation_e2e.js or > > webvr_e2e.js. > > In that case, I'll consider moving this into VrTestBase at some point - I > haven't had issues in WebVrTest.java yet since I was always explicitly waiting > on the synchronization booleans to be set during a test, but it'd be good to not > have to rely on that. > > Leave this as-is for now. Sounds good. https://codereview.chromium.org/2851313002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java (right): https://codereview.chromium.org/2851313002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:100: throw new UnsupportedOperationException(); On 2017/05/01 23:36:33, bsheedy wrote: > nit: Add a message to the exception Done. https://codereview.chromium.org/2851313002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:115: waitOnJavaScriptStep(mWebContents); On 2017/05/01 23:36:32, bsheedy wrote: > Change this to getActivity().getActivityTab().getWebContents() so that this > works for whatever the currently displayed tab is instead of just the first tab. Done.
tiborg@chromium.org changed reviewers: + dtrainor@chromium.org
dtrainor@chromium.org: Hi David, Could you please review changes in BUILD.gn, VrShellNavigationTest.java and VrTestBase.java? Thank you! Cheers, Tibor
vr_shell/ lgtm
https://codereview.chromium.org/2851313002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2851313002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:280: const base::android::JavaParamRef<jobject>& obj) { nit: const JavaParamRef
https://codereview.chromium.org/2851313002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java (right): https://codereview.chromium.org/2851313002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:60: runJavaScriptOrFail("window.location.href = '" + testPageUrl + "';", POLL_TIMEOUT_SHORT_MS, Should this just load the URL on the WebContents' NavigationController? https://codereview.chromium.org/2851313002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:70: pageLoadedLatch.countDown(); I think this trigger action is supposed to actually load the URL (maybe put the runJavaScriptOrFail in here?)? https://codereview.chromium.org/2851313002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:76: "Page loaded", pageLoadedLatch.await(POLL_TIMEOUT_LONG_MS, TimeUnit.MILLISECONDS)); Might not need this. The waitForTabPageLoaded is supposed to block. https://codereview.chromium.org/2851313002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:97: switch (page) { Maybe pull this and the code above out to a getUrl(Page page) call? https://codereview.chromium.org/2851313002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:138: navigateTo(Page.PAGE_2D); Should these just be loadUrl() calls?
https://codereview.chromium.org/2851313002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java (right): https://codereview.chromium.org/2851313002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:60: runJavaScriptOrFail("window.location.href = '" + testPageUrl + "';", POLL_TIMEOUT_SHORT_MS, On 2017/05/02 15:56:40, David Trainor-ping if over 24h wrote: > Should this just load the URL on the WebContents' NavigationController? bsheedy@ and I discussed this. I chose to let the page initiate the transition because I was not sure how many steps are skipped by interacting with the controller directly or use loadUrl. Since these are navigation end-to-end tests I wanted it to be as comprehensive as possible. https://codereview.chromium.org/2851313002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:70: pageLoadedLatch.countDown(); On 2017/05/02 15:56:40, David Trainor-ping if over 24h wrote: > I think this trigger action is supposed to actually load the URL (maybe put the > runJavaScriptOrFail in here?)? Ohh, that makes a lot of sense! Changed it. Works like a charm now. Thanks! https://codereview.chromium.org/2851313002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:76: "Page loaded", pageLoadedLatch.await(POLL_TIMEOUT_LONG_MS, TimeUnit.MILLISECONDS)); On 2017/05/02 15:56:40, David Trainor-ping if over 24h wrote: > Might not need this. The waitForTabPageLoaded is supposed to block. Done. https://codereview.chromium.org/2851313002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:97: switch (page) { On 2017/05/02 15:56:40, David Trainor-ping if over 24h wrote: > Maybe pull this and the code above out to a getUrl(Page page) call? Done. https://codereview.chromium.org/2851313002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:138: navigateTo(Page.PAGE_2D); On 2017/05/02 15:56:40, David Trainor-ping if over 24h wrote: > Should these just be loadUrl() calls? As mentioned in my comment above, I wanted to make the tests as end-to-end-like as possible. https://codereview.chromium.org/2851313002/diff/40001/chrome/browser/android/... File chrome/browser/android/vr_shell/vr_shell.cc (right): https://codereview.chromium.org/2851313002/diff/40001/chrome/browser/android/... chrome/browser/android/vr_shell/vr_shell.cc:280: const base::android::JavaParamRef<jobject>& obj) { On 2017/05/02 14:52:13, mthiesse wrote: > nit: const JavaParamRef Done. Also removed namespace at other instances of JavaParamRef in this file.
lgtm thanks!
The CQ bit was checked by tiborg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsheedy@chromium.org, mthiesse@chromium.org Link to the patchset: https://codereview.chromium.org/2851313002/#ps60001 (title: "Rebased on ToT, incorporated review feedback 3")
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": 1493843264476780,
"parent_rev": "3dc8ae2e46895de7953b0427e40817887135270b", "commit_rev":
"f72a7d6c0adfaf509f04b2b307f016e71ce68578"}
Message was sent while issue was closed.
Description was changed from ========== Adds end-to-end tests for VrShell navigation transitions. - Adds a new test class, which tests all navigation transitions (e.g. clicking a link) in VrShell end-to-end. BUG=715663 ========== to ========== Adds end-to-end tests for VrShell navigation transitions. - Adds a new test class, which tests all navigation transitions (e.g. clicking a link) in VrShell end-to-end. BUG=715663 Review-Url: https://codereview.chromium.org/2851313002 Cr-Commit-Position: refs/heads/master@{#469143} Committed: https://chromium.googlesource.com/chromium/src/+/f72a7d6c0adfaf509f04b2b307f0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f72a7d6c0adfaf509f04b2b307f0... |
