Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
799953002:
[WebView] Move external video surface across container views. (Closed)
|
Created:
6 years ago by Ignacio Solla Modified:
5 years, 11 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org, qinmin, Hugo Holgersson Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[WebView] Move external video surface across container views.
When transitioning from/to fullscreen the WebView replaces the container
view. We need to move the ExternalVideoSurfaceContainer across container
views when this happens. Note that in fullscreen the video may still
be embedded in the page if the element in fullscreen is, for example, a <div>
containing the <video>.
BUG=440774, 398485
TEST=Manual
1. Force use of external surface (set needs_external_surface_ to true
in webmediaplayer_android.cc)
2. Load the following url on a test app using a WebView:
http://robnyman.github.io/fullscreen/
3. Test with 2 different actions here:
a. Go fullscreen on the <video> element by clicking on the fullscreen
video control
b. Go fullscreen on the <div> element by clicking on the “Fullscreen” button
4. Notice: video continues in fullscreen without frozen video and in the
case of b the video still scrolls with the page
Committed: https://crrev.com/5eba22cd252a34432d26649b06fa4e574fc84528
Cr-Commit-Position: refs/heads/master@{#309000}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address review comments #Patch Set 3 : Rebase #
Total comments: 4
Patch Set 4 : Added tests #
Total comments: 8
Patch Set 5 : Removed unnecessary polling and nits. #
Total comments: 2
Patch Set 6 : Nit #
Total comments: 6
Patch Set 7 : Run containsChildOfType on UI thread #
Messages
Total messages: 33 (8 generated)
igsolla@chromium.org changed reviewers: + benm@chromium.org, boliu@chromium.org
boliu@chromium.org: please review all changes. benm@chromium.org: for owners LGTM.
And test pls? https://codereview.chromium.org/799953002/diff/1/android_webview/java/src/org... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/799953002/diff/1/android_webview/java/src/org... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:18: import org.chromium.content.browser.ContentViewCore.ContainerViewChangedListener; There is an identical copy of this file under chromecast/ that needs to be fixed too (probably) Feel free to add a comment to point that out until someone componentizes this. https://codereview.chromium.org/799953002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/799953002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:366: public interface ContainerViewChangedListener { Class with default no-op implementation. Calls this a BlahObserver. And pull this out into a separate top level class. (all chrome patterns) https://codereview.chromium.org/799953002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:367: public void onContainerViewChanged(ViewGroup oldContainerView, ViewGroup newContainerView); I hate having two ways to access the same thing. Can we drop one of these arguments, and guarantee in the document that ContentViewCore.getContainerView() returns it? I prefer it returning the new one. https://codereview.chromium.org/799953002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:496: private List<ContainerViewChangedListener> mContainerViewChangedListeners; Use the ObserverList class in base https://codereview.chromium.org/799953002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:822: if (mContainerView != null) { Why skip null? https://codereview.chromium.org/799953002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:835: mContainerViewChangedListeners.add(listener); should we call listener.onContainerViewChanged right here?
gunsch@chromium.org changed reviewers: + gunsch@chromium.org
https://codereview.chromium.org/799953002/diff/1/android_webview/java/src/org... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/799953002/diff/1/android_webview/java/src/org... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:18: import org.chromium.content.browser.ContentViewCore.ContainerViewChangedListener; On 2014/12/12 17:24:16, boliu wrote: > There is an identical copy of this file under chromecast/ that needs to be fixed > too (probably) > > Feel free to add a comment to point that out until someone componentizes this. +1, I'm happy to componentize it after the current round of CLs touching this files are done
I'd like to submit this ASAP to unblock sony and fix it in the next release. I'm planning to add tests in a follow-up change, but that will require a bit more time. I need to investigate whether there is some easy way to test with encrypted video (my guess is no) and otherwise perhaps just add testing hooks to force external surface with clear video https://codereview.chromium.org/799953002/diff/1/android_webview/java/src/org... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/799953002/diff/1/android_webview/java/src/org... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:18: import org.chromium.content.browser.ContentViewCore.ContainerViewChangedListener; On 2014/12/12 17:32:12, gunsch wrote: > On 2014/12/12 17:24:16, boliu wrote: > > There is an identical copy of this file under chromecast/ that needs to be > fixed > > too (probably) > > > > Feel free to add a comment to point that out until someone componentizes this. > > +1, I'm happy to componentize it after the current round of CLs touching this > files are done Thanks Jesse, I'd like this class to exist in a share location too. Bo, I know that you have opposed to moving this class to content/ before, so just to be sure does not componentizing it have the same issues? Strictly speaking the class in chromecast does not need to be updated. Replacing the container view is not something that content embedders can get for free, and the Android WebView is the only embedder that supports this (it requires a non-trivial amount of work to support it). Chromecast does not need it and as far as I can tell they will never do because they own the view hierarchy. The Android WebView does not own it and that is one of its peculiarities and why we need to replace container views when transitioning from/to fullscreen. https://codereview.chromium.org/799953002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/799953002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:366: public interface ContainerViewChangedListener { On 2014/12/12 17:24:16, boliu wrote: > Class with default no-op implementation. Calls this a BlahObserver. And pull > this out into a separate top level class. (all chrome patterns) I have changed it but where are chrome patterns documented? I can see a lot of Listener interfaces in the code base, see for example SmartClipDataListener above. I have also made this a class with default no-op implementation, but in this case I'd prefer an interface (only one method, and I don't see this observer growing and for that matter letting implementations choosing which methods to overrride..) https://codereview.chromium.org/799953002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:367: public void onContainerViewChanged(ViewGroup oldContainerView, ViewGroup newContainerView); On 2014/12/12 17:24:16, boliu wrote: > I hate having two ways to access the same thing. Can we drop one of these > arguments, and guarantee in the document that ContentViewCore.getContainerView() > returns it? I prefer it returning the new one. Done. I pass the new container view as a param because: 1. We don't need to access the old container view in that way 2. and calling onContainerViewChanged with just the old value seems wrong to me. https://codereview.chromium.org/799953002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:496: private List<ContainerViewChangedListener> mContainerViewChangedListeners; On 2014/12/12 17:24:16, boliu wrote: > Use the ObserverList class in base Done. https://codereview.chromium.org/799953002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:822: if (mContainerView != null) { On 2014/12/12 17:24:17, boliu wrote: > Why skip null? Removed null check, don't need it. https://codereview.chromium.org/799953002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:835: mContainerViewChangedListeners.add(listener); On 2014/12/12 17:24:17, boliu wrote: > should we call listener.onContainerViewChanged right here? That's an anti-pattern due to the possibility of re-entrance.
boliu@chromium.org changed reviewers: + ycheo@chromium.org
On 2014/12/15 14:12:12, Ignacio Solla wrote: > I'd like to submit this ASAP to unblock sony and fix it in the next release. > > I'm planning to add tests in a follow-up change, but that will require a bit > more time. I need to investigate whether there is some easy way to test with > encrypted video (my guess is no) and otherwise perhaps just add testing hooks to > force external surface with clear video +ycheo I believe there is a command line flag to set to force all videos to use external surfaces just look at the existing tests. Keep chromecast/browser/android/apk/src/org/chromium/chromecast/shell/ExternalVideoSurfaceContainer.java idential to the one under webview https://codereview.chromium.org/799953002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/799953002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:196: mContentViewCore.removeContainerViewObserver(mContainerViewObserver); should assert mSurfaceView and this bunch of variables aren't null here https://codereview.chromium.org/799953002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/799953002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:548: mContainerViewObservers = new ObserverList<ContainerViewObserver>(); clear this in destroy() (just like mGestureStateListeners)
igsolla@chromium.org changed reviewers: - ycheo@chromium.org
igsolla@chromium.org changed reviewers: - ycheo@chromium.org
Added tests now forcing the use of the external surface. https://codereview.chromium.org/799953002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/799953002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:196: mContentViewCore.removeContainerViewObserver(mContainerViewObserver); On 2014/12/15 16:23:09, boliu wrote: > should assert mSurfaceView and this bunch of variables aren't null here Done. https://codereview.chromium.org/799953002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/799953002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:548: mContainerViewObservers = new ObserverList<ContainerViewObserver>(); On 2014/12/15 16:23:09, boliu wrote: > clear this in destroy() (just like mGestureStateListeners) Done.
Added tests now forcing the use of the external surface. https://codereview.chromium.org/799953002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java (right): https://codereview.chromium.org/799953002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:196: mContentViewCore.removeContainerViewObserver(mContainerViewObserver); On 2014/12/15 16:23:09, boliu wrote: > should assert mSurfaceView and this bunch of variables aren't null here Done. https://codereview.chromium.org/799953002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/799953002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:548: mContainerViewObservers = new ObserverList<ContainerViewObserver>(); On 2014/12/15 16:23:09, boliu wrote: > clear this in destroy() (just like mGestureStateListeners) Done.
On 2014/12/15 16:23:09, boliu wrote: > On 2014/12/15 14:12:12, Ignacio Solla wrote: > > I'd like to submit this ASAP to unblock sony and fix it in the next release. > > > > I'm planning to add tests in a follow-up change, but that will require a bit > > more time. I need to investigate whether there is some easy way to test with > > encrypted video (my guess is no) and otherwise perhaps just add testing hooks > to > > force external surface with clear video > > +ycheo > > I believe there is a command line flag to set to force all videos to use > external surfaces just look at the existing tests. > What Bo mention is kForceUseOverlayEmbeddedVideo, and plz check this: http://goo.gl/WGWn9l > > Keep > chromecast/browser/android/apk/src/org/chromium/chromecast/shell/ExternalVideoSurfaceContainer.java > idential to the one under webview > > https://codereview.chromium.org/799953002/diff/40001/android_webview/java/src... > File > android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java > (right): > > https://codereview.chromium.org/799953002/diff/40001/android_webview/java/src... > android_webview/java/src/org/chromium/android_webview/ExternalVideoSurfaceContainer.java:196: > mContentViewCore.removeContainerViewObserver(mContainerViewObserver); > should assert mSurfaceView and this bunch of variables aren't null here > > https://codereview.chromium.org/799953002/diff/40001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/799953002/diff/40001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:548: > mContainerViewObservers = new ObserverList<ContainerViewObserver>(); > clear this in destroy() (just like mGestureStateListeners)
On 2014/12/16 15:58:56, Yuncheol Heo wrote: > On 2014/12/15 16:23:09, boliu wrote: > > On 2014/12/15 14:12:12, Ignacio Solla wrote: > > > I'd like to submit this ASAP to unblock sony and fix it in the next release. > > > > > > I'm planning to add tests in a follow-up change, but that will require a bit > > > more time. I need to investigate whether there is some easy way to test with > > > encrypted video (my guess is no) and otherwise perhaps just add testing > hooks > > to > > > force external surface with clear video > > > > +ycheo > > > > I believe there is a command line flag to set to force all videos to use > > external surfaces just look at the existing tests. > > > What Bo mention is kForceUseOverlayEmbeddedVideo, and plz check this: > http://goo.gl/WGWn9l Thanks. That's what I'm using already.
chromecast/ rs lgtm
Thanks for the tests :D https://codereview.chromium.org/799953002/diff/60001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java (right): https://codereview.chromium.org/799953002/diff/60001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:196: assertContainsHolePunchingSurfaceView(mTestContainerView, false); polling doesn't quite work here when expected == false polling works by detecting a change, and this is trying to assert there is no change (ie no surface view added to view tree), the best you can do is poll and wait for a timeout and actually expect the timeout, which is super ugly. How about just get rid of this test? https://codereview.chromium.org/799953002/diff/60001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:455: private void assertContainsHolePunchingSurfaceView(final View view, final boolean expected) nit, call these functions pollFoo instead of assertFoo Any reason params here are final? https://codereview.chromium.org/799953002/diff/60001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:460: private void assertViewContainsChildOfType(final View view, final Class<?> childType, Class<? extends View> https://codereview.chromium.org/799953002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/799953002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:821: observer.onContainerViewChanged(mContainerView); So onContainerViewChanged and getContainerView return the same thing now. Can we just get rid of the parameter to onContainerViewChanged altogether? Sorry for not catching this last in the last round of review
https://codereview.chromium.org/799953002/diff/60001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java (right): https://codereview.chromium.org/799953002/diff/60001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:196: assertContainsHolePunchingSurfaceView(mTestContainerView, false); On 2014/12/16 16:44:19, boliu wrote: > polling doesn't quite work here when expected == false > > polling works by detecting a change, and this is trying to assert there is no > change (ie no surface view added to view tree), the best you can do is poll and > wait for a timeout and actually expect the timeout, which is super ugly. > > How about just get rid of this test? This test is useful. If you forceUseHolePunchingExternalSurface() before then it fails consistently so it does catch the potential problem that we're trying to catch. I have removed the unneeded polling part now here, and I have also performed an audit and removed other unnecessary polling from this class. https://codereview.chromium.org/799953002/diff/60001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:455: private void assertContainsHolePunchingSurfaceView(final View view, final boolean expected) On 2014/12/16 16:44:19, boliu wrote: > nit, call these functions pollFoo instead of assertFoo > > Any reason params here are final? Renamed assertBlah to assertWaitForBlah throughout for those asserts that use polling. As said above, I removed other unnecessary polling. Removed final. https://codereview.chromium.org/799953002/diff/60001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:460: private void assertViewContainsChildOfType(final View view, final Class<?> childType, On 2014/12/16 16:44:19, boliu wrote: > Class<? extends View> Done. https://codereview.chromium.org/799953002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/799953002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:821: observer.onContainerViewChanged(mContainerView); On 2014/12/16 16:44:19, boliu wrote: > So onContainerViewChanged and getContainerView return the same thing now. That's right. > Can we just get rid of the parameter to onContainerViewChanged altogether? > > Sorry for not catching this last in the last round of review We could, but I prefer to pass it. Otherwise observers will need to call getContainerView. The extra step seems unnecessary to me.
aw lgtm The parameter question is for the content owner to decide. https://codereview.chromium.org/799953002/diff/80001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java (right): https://codereview.chromium.org/799953002/diff/80001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:478: private boolean containsChildOfType(View view, final Class<?> childType) { Class<? extends View> here too?
https://codereview.chromium.org/799953002/diff/80001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java (right): https://codereview.chromium.org/799953002/diff/80001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:478: private boolean containsChildOfType(View view, final Class<?> childType) { On 2014/12/16 19:06:51, boliu wrote: > Class<? extends View> here too? Done.
On 2014/12/16 19:09:16, Ignacio Solla wrote: > https://codereview.chromium.org/799953002/diff/80001/android_webview/javatest... > File > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java > (right): > > https://codereview.chromium.org/799953002/diff/80001/android_webview/javatest... > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:478: > private boolean containsChildOfType(View view, final Class<?> childType) { > On 2014/12/16 19:06:51, boliu wrote: > > Class<? extends View> here too? > > Done. I need to fix one of the tests. It is flaky, it consistently passes locally, but fails remotely. I suspect the flakiness is due to a wait (but there is no easy way around it), so I may need to introduce polling.
On 2014/12/17 14:17:21, Ignacio Solla wrote: > I need to fix one of the tests. It is flaky, it consistently passes locally, but > fails remotely. I suspect the flakiness is due to a wait (but there is no easy > way around it), so I may need to introduce polling. The devices on the bot are mako running KOT49H, in case that's the difference run_tests.py does not support a repeat parameter, so I had this hacky patch: https://codereview.chromium.org/701943002/ just filter it to a single test, and it will repeat the test until failure. Probably should try this on all the new tests :)
hugo.holgersson@sonymobile.com changed reviewers: + hugo.holgersson@sonymobile.com
https://codereview.chromium.org/799953002/diff/100001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java (right): https://codereview.chromium.org/799953002/diff/100001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:197: DOMUtils.waitForVideoPlay(getWebContentsOnUiThread(), VIDEO_ID); How about assertTrue(DOMUtils.waitForVideoPlay...)? I mean, before we go on looking for a "hole punching surface", we first assert that video playback actually started (otherwise fail early). https://codereview.chromium.org/799953002/diff/100001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:212: DOMUtils.waitForVideoPlay(getWebContentsOnUiThread(), VIDEO_ID); Same as above. https://codereview.chromium.org/799953002/diff/100001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:240: DOMUtils.waitForVideoPlay(getWebContentsOnUiThread(), VIDEO_ID); Same as above.
On 2014/12/17 14:45:35, boliu wrote: > On 2014/12/17 14:17:21, Ignacio Solla wrote: > > I need to fix one of the tests. It is flaky, it consistently passes locally, > but > > fails remotely. I suspect the flakiness is due to a wait (but there is no easy > > way around it), so I may need to introduce polling. > > The devices on the bot are mako running KOT49H, in case that's the difference > > run_tests.py does not support a repeat parameter, so I had this hacky patch: > https://codereview.chromium.org/701943002/ just filter it to a single test, and > it will repeat the test until failure. Probably should try this on all the new > tests :) Thanks for the hacky patch Bo! I was able to reproduce the flakiness locally with the script. I believe that the problem was that we were running containsChildOfType in the instrumentation thread. Once I run it on the UI thread the flakiness seems to go away, so let's try again remotely and check that that is indeed the case.
https://codereview.chromium.org/799953002/diff/100001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java (right): https://codereview.chromium.org/799953002/diff/100001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:197: DOMUtils.waitForVideoPlay(getWebContentsOnUiThread(), VIDEO_ID); On 2014/12/17 15:05:13, Hugo Holgersson wrote: > How about assertTrue(DOMUtils.waitForVideoPlay...)? I mean, before we go on > looking for a "hole punching surface", we first assert that video playback > actually started (otherwise fail early). Done. https://codereview.chromium.org/799953002/diff/100001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:212: DOMUtils.waitForVideoPlay(getWebContentsOnUiThread(), VIDEO_ID); On 2014/12/17 15:05:13, Hugo Holgersson wrote: > Same as above. Done. https://codereview.chromium.org/799953002/diff/100001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:240: DOMUtils.waitForVideoPlay(getWebContentsOnUiThread(), VIDEO_ID); On 2014/12/17 15:05:13, Hugo Holgersson wrote: > Same as above. Done.
On 2014/12/17 17:34:56, Ignacio Solla wrote: > https://codereview.chromium.org/799953002/diff/100001/android_webview/javates... > File > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java > (right): > > https://codereview.chromium.org/799953002/diff/100001/android_webview/javates... > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:197: > DOMUtils.waitForVideoPlay(getWebContentsOnUiThread(), VIDEO_ID); > On 2014/12/17 15:05:13, Hugo Holgersson wrote: > > How about assertTrue(DOMUtils.waitForVideoPlay...)? I mean, before we go on > > looking for a "hole punching surface", we first assert that video playback > > actually started (otherwise fail early). > > Done. > > https://codereview.chromium.org/799953002/diff/100001/android_webview/javates... > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:212: > DOMUtils.waitForVideoPlay(getWebContentsOnUiThread(), VIDEO_ID); > On 2014/12/17 15:05:13, Hugo Holgersson wrote: > > Same as above. > > Done. > > https://codereview.chromium.org/799953002/diff/100001/android_webview/javates... > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:240: > DOMUtils.waitForVideoPlay(getWebContentsOnUiThread(), VIDEO_ID); > On 2014/12/17 15:05:13, Hugo Holgersson wrote: > > Same as above. > > Done. It seems it worked, but there was some other unreleated failure somewhere so let's try again (AndroidWebViewTest tests passed this time!, but there was an unreleated flake in ChromeShellTest).
On 2014/12/17 18:46:41, Ignacio Solla wrote: > It seems it worked, but there was some other unreleated failure somewhere so > let's try again (AndroidWebViewTest tests passed this time!, but there was an > unreleated flake in ChromeShellTest). Yeah it's probably unrelated. But oh god ChromeShell instrumentation tests are flaky. So the test runner will retry a test up to 2 times (so 3 runs in total) before actually reporting the test as failing. (Just search for "retry" in the stdio of the test step) But that far is way too low for tests. We should disable any test that gets retried, and file a bug to some owner. Flaky tests are worse than no tests imo. Webview tests does much better here :)
lgtm I like the pattern of passing the new container in the observer callback rather than having the callback invoke getContainerView.
The CQ bit was checked by igsolla@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/799953002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/5eba22cd252a34432d26649b06fa4e574fc84528 Cr-Commit-Position: refs/heads/master@{#309000}
Message was sent while issue was closed.
Patchset #8 (id:140001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
