Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2029053003:
Fix AW tests for Spitzer (Closed)
|
Created:
4 years, 6 months ago by Tima Vaisburd Modified:
4 years, 5 months ago CC:
android-webview-reviews_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix AW tests for Spitzer
In constast to the MediaPlayerBridge/MediaSourcePlayer
based pipeline Spitzer never creates an underlay SurfaceView
(ContentVideoView) when it works for WebView.
This ContentVideoView was required for some tests. this CL removes
this requirements for all full screen tests and disables
hole punching tests since we consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
BUG=597495
Committed: https://crrev.com/a08264d208ffa47ebadbdde57c8226f1ecee5114
Cr-Commit-Position: refs/heads/master@{#402950}
Patch Set 1 #
Total comments: 1
Patch Set 2 : rebased, disabled ExternalVideoSurfaceContainerTest #Patch Set 3 : Rebased again #Patch Set 4 : Set COPY_REQUIRED flag for WevView video frame and enabled deferred strategy for WebView #
Total comments: 3
Patch Set 5 : Disabled testGetVideoLoadingProgressView #Patch Set 6 : Split the actual enabling and COPY_REUIRED out, leaving just tests #Patch Set 7 : Rebased #
Total comments: 5
Patch Set 8 : Reenabled AwContentsClientGetVideoLoadingProgressViewTest #
Messages
Total messages: 63 (23 generated)
timav@chromium.org changed reviewers: + boliu@chromium.org
In the light of hole punching mail thread I disabled corresponding AW tests. PTAL.
Description was changed from ========== Fix AW tests for Spitzer and enable Spitzer in AW Spitzer does not always use ContentVideoView which was required for some tests. We removed this requirements for all full screen tests and disabled hole punching tests since we consider them obsolete. BUG=597495 To enable Spizter in WebView we did two kind ========== to ========== Fix AW tests for Spitzer and enable Spitzer in AW Spitzer does not always use ContentVideoView which was required for some tests. We removed this requirements for all full screen tests and disabled hole punching tests since we consider them obsolete. BUG=597495 ==========
two steps: 1) make setVideoOverlayForEmbeddedEncryptedVideoEnabled no-op, merge that to m52, wait for m52 to go to stable to make sure no one complains 2) enable spitzer, remove tests, etc you are jumping ahead
timav@chromium.org changed reviewers: + dalecurtis@google.com, sgurun@chromium.org
It is a good plan, only can we wait for M52 to go stable to activate Spitzer then?
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
If that's the plan we should do 1) now and land this for M53 now. We can revert later.
(Getting test coverage, etc early for Spitzer seems more important, hence landing this for M53 ASAP)
On 2016/06/01 20:32:29, DaleCurtis wrote: > (Getting test coverage, etc early for Spitzer seems more important, hence > landing this for M53 ASAP) I'd still ask to do 1) and merge it first. fwiw webview beta population is tiny to the point of negligible, so doing it early doesn't help that much unless something major breaks
On 2016/06/01 20:44:03, boliu wrote: > On 2016/06/01 20:32:29, DaleCurtis wrote: > > (Getting test coverage, etc early for Spitzer seems more important, hence > > landing this for M53 ASAP) > > I'd still ask to do 1) and merge it first. > > fwiw webview beta population is tiny to the point of negligible, so doing it > early doesn't help that much unless something major breaks agreed, webview use cases are a lot larger than Chrome but the feedback mechanisms are a lot limited. Going cautious is generally best path for making sure we don't break stuff. Let's disable the functionality and tests for M52 and enable Spitzer for M53.
https://codereview.chromium.org/2029053003/diff/1/android_webview/javatests/s... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java (right): https://codereview.chromium.org/2029053003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:524: private void assertContainsContentVideoView() throws Exception { remove this and assertContainsOneContentVideoView
Actually.. spitzer folks: probably a good idea to have a test that explicitly tests for ContentVideoView full screen, and one that tests full screen video without ContentVideoView? Is it easy to write tests like that and maintain them?
On 2016/06/02 20:31:39, boliu wrote: > Actually.. spitzer folks: probably a good idea to have a test that explicitly > tests for ContentVideoView full screen, and one that tests full screen video > without ContentVideoView? Is it easy to write tests like that and maintain them? Agreed that we should have both kinds of test if possible. Fullscreening a h264 <video> will always use ContentVideoView today, and that's a safe assumption to make for the medium term I'd say. We won't be shipping a software h264 decoder so the only reason it might change is if we find an issue with SurfaceView & MediaCodec on any devices (seems unlikely).
On 2016/06/02 20:40:13, watk wrote: > On 2016/06/02 20:31:39, boliu wrote: > > Actually.. spitzer folks: probably a good idea to have a test that explicitly > > tests for ContentVideoView full screen, and one that tests full screen video > > without ContentVideoView? Is it easy to write tests like that and maintain > them? > > Agreed that we should have both kinds of test if possible. Fullscreening a h264 > <video> will always use ContentVideoView today, and that's a safe assumption to > make for the medium term I'd say. We won't be shipping a software h264 decoder > so the only reason it might change is if we find an issue with SurfaceView & > MediaCodec on any devices (seems unlikely). Ok let's do that. Doesn't have to be this CL. But maybe don't remove assertContainsContentVideoView yet. so... lgtm
On 2016/06/02 20:44:30, boliu wrote: > On 2016/06/02 20:40:13, watk wrote: > > On 2016/06/02 20:31:39, boliu wrote: > > > Actually.. spitzer folks: probably a good idea to have a test that > explicitly > > > tests for ContentVideoView full screen, and one that tests full screen video > > > without ContentVideoView? Is it easy to write tests like that and maintain > > them? > > > > Agreed that we should have both kinds of test if possible. Fullscreening a > h264 > > <video> will always use ContentVideoView today, and that's a safe assumption > to > > make for the medium term I'd say. We won't be shipping a software h264 decoder > > so the only reason it might change is if we find an issue with SurfaceView & > > MediaCodec on any devices (seems unlikely). > Ah, my bad. This is wrong because we recently started blacklisting SurfaceView. But the tests could check this: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...
dalecurtis@chromium.org changed reviewers: - dalecurtis@google.com
lgtm
The CQ bit was checked by timav@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2029053003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2029053003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
let's switch to COPY_REQUIRED before enabling spitzer. context: crbug.com/582170
Description was changed from ========== Fix AW tests for Spitzer and enable Spitzer in AW Spitzer does not always use ContentVideoView which was required for some tests. We removed this requirements for all full screen tests and disabled hole punching tests since we consider them obsolete. BUG=597495 ========== to ========== Fix AW tests for Spitzer and enable Spitzer in AW Spitzer does not always use ContentVideoView which was required for some tests. We removed this requirements for all full screen tests and disabled hole punching tests since we consider them obsolete. BUG=597495 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
timav@chromium.org changed reviewers: + liberato@chromium.org, watk@chromium.org
Could you, please, take a look at this CL again and advise on the correct way to set COPY_REQUIRED flag? Thank you!
On 2016/06/09 01:51:50, Tima Vaisburd wrote: > Could you, please, take a look at this CL again and advise on the correct way to > set COPY_REQUIRED flag? Thank you! i don't see any reason why the mailbox manager should get upset about it. as an aside, you probably won't want to do this unconditionally. AVDA should tell you whether or not to do it. deferred strategy says yes, copying says no. it can be put into Capabilities, like |needs_all_picture_buffers_to_decode_|. it's good to keep the copying strategy up to date, even if we're not using it by default. it's been helpful to diagnose problems.
pull out COPY_REQUIRED into a separate CL? in case spitzer needs to be reverted actually in that spirit, maybe pull out disabling tests into a separate CL too.. https://codereview.chromium.org/2029053003/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/mailbox_manager_sync.cc (right): https://codereview.chromium.org/2029053003/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/mailbox_manager_sync.cc:297: // LOG(ERROR) << "MailboxSync: Incompatible attachment"; not ok https://codereview.chromium.org/2029053003/diff/60001/media/filters/gpu_video... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/2029053003/diff/60001/media/filters/gpu_video... media/filters/gpu_video_decoder.cc:180: switches::kEnableThreadedTextureMailboxes); you can't assume gpu switches are present in the renderer process do this: https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc... https://codereview.chromium.org/2029053003/diff/60001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2029053003/diff/60001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:482: // DCHECK(!gles_decoder->GetContextGroup()->mailbox_manager()->UsesSync()); remove it
Description was changed from
==========
Fix AW tests for Spitzer and enable Spitzer in AW
Spitzer does not always use ContentVideoView which was required
for some tests. We removed this requirements for all full screen
tests and disabled hole punching tests since we consider them
obsolete.
BUG=597495
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
==========
to
==========
Fix AW tests for Spitzer
Currently for Android Webview Spitzer never creates ContentVideoView
which was required for some tests. We removed this requirements for
all full screen tests and disabled hole punching tests since we
consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
Later we might restore the Spitzer ability to create ContentVideoView
in some cases, however, Spizter might still choose not to
create it for certain codecs and devices.
BUG=597495
==========
Description was changed from
==========
Fix AW tests for Spitzer
Currently for Android Webview Spitzer never creates ContentVideoView
which was required for some tests. We removed this requirements for
all full screen tests and disabled hole punching tests since we
consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
Later we might restore the Spitzer ability to create ContentVideoView
in some cases, however, Spizter might still choose not to
create it for certain codecs and devices.
BUG=597495
==========
to
==========
Fix AW tests for Spitzer
Currently for Android Webview Spitzer never creates ContentVideoView
which was required for some tests. We removed this requirements for
all full screen tests and disabled hole punching tests since we
consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
Later we might restore the Spitzer ability to create ContentVideoView
in some cases, however, Spitzer might still choose not to
create it for certain codecs and devices.
BUG=597495
==========
On 2016/06/09 15:37:15, boliu wrote: > pull out COPY_REQUIRED into a separate CL? in case spitzer needs to be reverted > > actually in that spirit, maybe pull out disabling tests into a separate CL too.. Keeping the tests in this CL, the enabling Spitzer is now a new dependent CL https://codereview.chromium.org/2057743002
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2029053003/#ps100001 (title: "Split the actual enabling and COPY_REUIRED out, leaving just tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2029053003/100001
The CQ bit was unchecked by boliu@chromium.org
unchecked cq disable tests when we are ready to enable spitzer, otherwise we are losing test coverage for nothing
On 2016/06/09 20:51:13, boliu wrote: > unchecked cq > > disable tests when we are ready to enable spitzer, otherwise we are losing test > coverage for nothing Also taking previous approval with massive new changes is generally not ok either.
On 2016/06/09 20:52:01, boliu wrote: > On 2016/06/09 20:51:13, boliu wrote: > > unchecked cq > > > > disable tests when we are ready to enable spitzer, otherwise we are losing > test > > coverage for nothing > > Also taking previous approval with massive new changes is generally not ok > either. My bad. I thought this would be acceptable (and even desired) because it constitutes a part that everybody seemed to agree with.
Description was changed from
==========
Fix AW tests for Spitzer
Currently for Android Webview Spitzer never creates ContentVideoView
which was required for some tests. We removed this requirements for
all full screen tests and disabled hole punching tests since we
consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
Later we might restore the Spitzer ability to create ContentVideoView
in some cases, however, Spitzer might still choose not to
create it for certain codecs and devices.
BUG=597495
==========
to
==========
Fix AW tests for Spitzer
Currently for Android Webview Spitzer never creates ContentVideoView
which was required for some tests. We removed this requirements for
all full screen tests and disabled hole punching tests since we
consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
Later we plan restore the Spitzer ability to create ContentVideoView in WebView
(https://codereview.chromium.org/2052103002),
however, Spitzer might still choose not to
do it for certain codecs and devices.
BUG=597495
==========
Description was changed from
==========
Fix AW tests for Spitzer
Currently for Android Webview Spitzer never creates ContentVideoView
which was required for some tests. We removed this requirements for
all full screen tests and disabled hole punching tests since we
consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
Later we plan restore the Spitzer ability to create ContentVideoView in WebView
(https://codereview.chromium.org/2052103002),
however, Spitzer might still choose not to
do it for certain codecs and devices.
BUG=597495
==========
to
==========
Fix AW tests for Spitzer
Currently for Android Webview Spitzer never creates ContentVideoView
which was required for some tests. We removed this requirements for
all full screen tests and disabled hole punching tests since we
consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
Later we plan restore the Spitzer ability to create ContentVideoView
in WebView (https://codereview.chromium.org/2052103002),
however, Spitzer might still choose not to do it for certain
codecs and devices.
BUG=597495
==========
Description was changed from
==========
Fix AW tests for Spitzer
Currently for Android Webview Spitzer never creates ContentVideoView
which was required for some tests. We removed this requirements for
all full screen tests and disabled hole punching tests since we
consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
Later we plan restore the Spitzer ability to create ContentVideoView
in WebView (https://codereview.chromium.org/2052103002),
however, Spitzer might still choose not to do it for certain
codecs and devices.
BUG=597495
==========
to
==========
Fix AW tests for Spitzer
In constast to the MediaPlayerBridge/MediaSourcePlayer
based pipeline Spitzer never creates an underlay SurfaceView (ContentVideoView)
when it works for WebView.
This ContentVideoView was required for some tests. this CL removes this
requirements for
all full screen tests and disables hole punching tests since we
consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
BUG=597495
==========
Description was changed from
==========
Fix AW tests for Spitzer
In constast to the MediaPlayerBridge/MediaSourcePlayer
based pipeline Spitzer never creates an underlay SurfaceView (ContentVideoView)
when it works for WebView.
This ContentVideoView was required for some tests. this CL removes this
requirements for
all full screen tests and disables hole punching tests since we
consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
BUG=597495
==========
to
==========
Fix AW tests for Spitzer
In constast to the MediaPlayerBridge/MediaSourcePlayer
based pipeline Spitzer never creates an underlay SurfaceView (ContentVideoView)
when it works for WebView.
This ContentVideoView was required for some tests. this CL removes
this requirements for all full screen tests and disables
hole punching tests since we consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
BUG=597495
==========
Description was changed from
==========
Fix AW tests for Spitzer
In constast to the MediaPlayerBridge/MediaSourcePlayer
based pipeline Spitzer never creates an underlay SurfaceView (ContentVideoView)
when it works for WebView.
This ContentVideoView was required for some tests. this CL removes
this requirements for all full screen tests and disables
hole punching tests since we consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
BUG=597495
==========
to
==========
Fix AW tests for Spitzer
In constast to the MediaPlayerBridge/MediaSourcePlayer
based pipeline Spitzer never creates an underlay SurfaceView
(ContentVideoView) when it works for WebView.
This ContentVideoView was required for some tests. this CL removes
this requirements for all full screen tests and disables
hole punching tests since we consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
BUG=597495
==========
PTAL. Most (but not all) tests have already been disabled because of flakiness, this CL disables them for another reason.
https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java (right): https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:55: public void testGetVideoLoadingProgressView() throws Throwable { why is this one failing? it doesn't call setForceVideoOverlayForTests afaict?
https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java (right): https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:55: public void testGetVideoLoadingProgressView() throws Throwable { On 2016/06/27 18:59:52, boliu wrote: > why is this one failing? it doesn't call setForceVideoOverlayForTests afaict? I can only find the call to getVideoLoadingProgressView() in ContentVideoView: https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... I think without ContentVideoView getVideoLoadingProgressView() is not called, state change listener is not attached etc.
https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java (right): https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:55: public void testGetVideoLoadingProgressView() throws Throwable { On 2016/06/27 19:38:38, Tima Vaisburd wrote: > On 2016/06/27 18:59:52, boliu wrote: > > why is this one failing? it doesn't call setForceVideoOverlayForTests afaict? > > I can only find the call to getVideoLoadingProgressView() in ContentVideoView: > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... > > I think without ContentVideoView getVideoLoadingProgressView() is not called, > state change listener is not attached etc. Hmm.. then this also breaks WebChromeClient.getVideoLoadingProgressView. Can we get some data on how often this is used? Should also to make sure to depcreate this method on the next version of android.
On 2016/06/27 19:57:58, boliu wrote: > https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... > File > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java > (right): > > https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:55: > public void testGetVideoLoadingProgressView() throws Throwable { > On 2016/06/27 19:38:38, Tima Vaisburd wrote: > > On 2016/06/27 18:59:52, boliu wrote: > > > why is this one failing? it doesn't call setForceVideoOverlayForTests > afaict? > > > > I can only find the call to getVideoLoadingProgressView() in ContentVideoView: > > > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... > > > > I think without ContentVideoView getVideoLoadingProgressView() is not called, > > state change listener is not attached etc. > > Hmm.. then this also breaks WebChromeClient.getVideoLoadingProgressView. > > Can we get some data on how often this is used? I "volunteered" sgurun to find out :p > Should also to make sure to > depcreate this method on the next version of android.
https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java (right): https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:55: public void testGetVideoLoadingProgressView() throws Throwable { On 2016/06/27 19:57:58, boliu wrote: > On 2016/06/27 19:38:38, Tima Vaisburd wrote: > > On 2016/06/27 18:59:52, boliu wrote: > > > why is this one failing? it doesn't call setForceVideoOverlayForTests > afaict? > > > > I can only find the call to getVideoLoadingProgressView() in ContentVideoView: > > > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... > > > > I think without ContentVideoView getVideoLoadingProgressView() is not called, > > state change listener is not attached etc. > > Hmm.. then this also breaks WebChromeClient.getVideoLoadingProgressView. > > Can we get some data on how often this is used? Should also to make sure to > depcreate this method on the next version of android. With the recent change https://codereview.chromium.org/2083123003/ ContentVideoView (CVV) is always required for full screen mode and this tests passes. The CVV has been reintroduced to improve the transition to full screen, it is not essential for video to be running.
https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java (right): https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:55: public void testGetVideoLoadingProgressView() throws Throwable { On 2016/06/29 01:11:12, Tima Vaisburd wrote: > On 2016/06/27 19:57:58, boliu wrote: > > On 2016/06/27 19:38:38, Tima Vaisburd wrote: > > > On 2016/06/27 18:59:52, boliu wrote: > > > > why is this one failing? it doesn't call setForceVideoOverlayForTests > > afaict? > > > > > > I can only find the call to getVideoLoadingProgressView() in > ContentVideoView: > > > > > > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... > > > > > > I think without ContentVideoView getVideoLoadingProgressView() is not > called, > > > state change listener is not attached etc. > > > > Hmm.. then this also breaks WebChromeClient.getVideoLoadingProgressView. > > > > Can we get some data on how often this is used? Should also to make sure to > > depcreate this method on the next version of android. > > With the recent change https://codereview.chromium.org/2083123003/ > ContentVideoView (CVV) is always required for full screen mode and this tests > passes. The CVV has been reintroduced to improve the transition to full screen, > it is not essential for video to be running. If that's behavior is going to stay long term, then don't need to disable this test?
On 2016/06/29 02:39:16, boliu wrote: > https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... > File > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java > (right): > > https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:55: > public void testGetVideoLoadingProgressView() throws Throwable { > On 2016/06/29 01:11:12, Tima Vaisburd wrote: > > On 2016/06/27 19:57:58, boliu wrote: > > > On 2016/06/27 19:38:38, Tima Vaisburd wrote: > > > > On 2016/06/27 18:59:52, boliu wrote: > > > > > why is this one failing? it doesn't call setForceVideoOverlayForTests > > > afaict? > > > > > > > > I can only find the call to getVideoLoadingProgressView() in > > ContentVideoView: > > > > > > > > > > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... > > > > > > > > I think without ContentVideoView getVideoLoadingProgressView() is not > > called, > > > > state change listener is not attached etc. > > > > > > Hmm.. then this also breaks WebChromeClient.getVideoLoadingProgressView. > > > > > > Can we get some data on how often this is used? Should also to make sure to > > > depcreate this method on the next version of android. > > > > With the recent change https://codereview.chromium.org/2083123003/ > > ContentVideoView (CVV) is always required for full screen mode and this tests > > passes. The CVV has been reintroduced to improve the transition to full > screen, > > it is not essential for video to be running. > > If that's behavior is going to stay long term, then don't need to disable this > test? My current understanding is that this CVV is the best idea for embedded/full screen transition at this time. I believe that we should not disable this test, whenever a future media change happens it will trigger attention to getVideoLoadingProgressView()
On 2016/06/29 18:38:52, Tima Vaisburd wrote: > On 2016/06/29 02:39:16, boliu wrote: > > > https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... > > File > > > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java > > (right): > > > > > https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... > > > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:55: > > public void testGetVideoLoadingProgressView() throws Throwable { > > On 2016/06/29 01:11:12, Tima Vaisburd wrote: > > > On 2016/06/27 19:57:58, boliu wrote: > > > > On 2016/06/27 19:38:38, Tima Vaisburd wrote: > > > > > On 2016/06/27 18:59:52, boliu wrote: > > > > > > why is this one failing? it doesn't call setForceVideoOverlayForTests > > > > afaict? > > > > > > > > > > I can only find the call to getVideoLoadingProgressView() in > > > ContentVideoView: > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... > > > > > > > > > > I think without ContentVideoView getVideoLoadingProgressView() is not > > > called, > > > > > state change listener is not attached etc. > > > > > > > > Hmm.. then this also breaks WebChromeClient.getVideoLoadingProgressView. > > > > > > > > Can we get some data on how often this is used? Should also to make sure > to > > > > depcreate this method on the next version of android. > > > > > > With the recent change https://codereview.chromium.org/2083123003/ > > > ContentVideoView (CVV) is always required for full screen mode and this > tests > > > passes. The CVV has been reintroduced to improve the transition to full > > screen, > > > it is not essential for video to be running. > > > > If that's behavior is going to stay long term, then don't need to disable this > > test? > > My current understanding is that this CVV is the best idea for embedded/full > screen > transition at this time. I believe that we should not disable this test, > whenever a future > media change happens it will trigger attention to getVideoLoadingProgressView() Not disabling SGTM.
On 2016/06/29 19:01:29, watk wrote: > On 2016/06/29 18:38:52, Tima Vaisburd wrote: > > On 2016/06/29 02:39:16, boliu wrote: > > > > > > https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... > > > File > > > > > > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... > > > > > > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:55: > > > public void testGetVideoLoadingProgressView() throws Throwable { > > > On 2016/06/29 01:11:12, Tima Vaisburd wrote: > > > > On 2016/06/27 19:57:58, boliu wrote: > > > > > On 2016/06/27 19:38:38, Tima Vaisburd wrote: > > > > > > On 2016/06/27 18:59:52, boliu wrote: > > > > > > > why is this one failing? it doesn't call > setForceVideoOverlayForTests > > > > > afaict? > > > > > > > > > > > > I can only find the call to getVideoLoadingProgressView() in > > > > ContentVideoView: > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... > > > > > > > > > > > > I think without ContentVideoView getVideoLoadingProgressView() is not > > > > called, > > > > > > state change listener is not attached etc. > > > > > > > > > > Hmm.. then this also breaks WebChromeClient.getVideoLoadingProgressView. > > > > > > > > > > Can we get some data on how often this is used? Should also to make sure > > to > > > > > depcreate this method on the next version of android. > > > > > > > > With the recent change https://codereview.chromium.org/2083123003/ > > > > ContentVideoView (CVV) is always required for full screen mode and this > > tests > > > > passes. The CVV has been reintroduced to improve the transition to full > > > screen, > > > > it is not essential for video to be running. > > > > > > If that's behavior is going to stay long term, then don't need to disable > this > > > test? > > > > My current understanding is that this CVV is the best idea for embedded/full > > screen > > transition at this time. I believe that we should not disable this test, > > whenever a future > > media change happens it will trigger attention to > getVideoLoadingProgressView() > > Not disabling SGTM. Done.
lgtm
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2029053003/#ps140001 (title: "Reenabled AwContentsClientGetVideoLoadingProgressViewTest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Fix AW tests for Spitzer
In constast to the MediaPlayerBridge/MediaSourcePlayer
based pipeline Spitzer never creates an underlay SurfaceView
(ContentVideoView) when it works for WebView.
This ContentVideoView was required for some tests. this CL removes
this requirements for all full screen tests and disables
hole punching tests since we consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
BUG=597495
==========
to
==========
Fix AW tests for Spitzer
In constast to the MediaPlayerBridge/MediaSourcePlayer
based pipeline Spitzer never creates an underlay SurfaceView
(ContentVideoView) when it works for WebView.
This ContentVideoView was required for some tests. this CL removes
this requirements for all full screen tests and disables
hole punching tests since we consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
BUG=597495
==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from
==========
Fix AW tests for Spitzer
In constast to the MediaPlayerBridge/MediaSourcePlayer
based pipeline Spitzer never creates an underlay SurfaceView
(ContentVideoView) when it works for WebView.
This ContentVideoView was required for some tests. this CL removes
this requirements for all full screen tests and disables
hole punching tests since we consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
BUG=597495
==========
to
==========
Fix AW tests for Spitzer
In constast to the MediaPlayerBridge/MediaSourcePlayer
based pipeline Spitzer never creates an underlay SurfaceView
(ContentVideoView) when it works for WebView.
This ContentVideoView was required for some tests. this CL removes
this requirements for all full screen tests and disables
hole punching tests since we consider them obsolete.
This is a prerequisite for enabling Spitzer in Android Webview
(https://codereview.chromium.org/2057743002).
BUG=597495
Committed: https://crrev.com/a08264d208ffa47ebadbdde57c8226f1ecee5114
Cr-Commit-Position: refs/heads/master@{#402950}
==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/a08264d208ffa47ebadbdde57c8226f1ecee5114 Cr-Commit-Position: refs/heads/master@{#402950}
Message was sent while issue was closed.
On 2016/06/27 20:42:04, boliu wrote: > On 2016/06/27 19:57:58, boliu wrote: > > > https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... > > File > > > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java > > (right): > > > > > https://codereview.chromium.org/2029053003/diff/120001/android_webview/javate... > > > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientGetVideoLoadingProgressViewTest.java:55: > > public void testGetVideoLoadingProgressView() throws Throwable { > > On 2016/06/27 19:38:38, Tima Vaisburd wrote: > > > On 2016/06/27 18:59:52, boliu wrote: > > > > why is this one failing? it doesn't call setForceVideoOverlayForTests > > afaict? > > > > > > I can only find the call to getVideoLoadingProgressView() in > ContentVideoView: > > > > > > https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro... > > > > > > I think without ContentVideoView getVideoLoadingProgressView() is not > called, > > > state change listener is not attached etc. > > > > Hmm.. then this also breaks WebChromeClient.getVideoLoadingProgressView. > > > > Can we get some data on how often this is used? > > I "volunteered" sgurun to find out :p unfortunately, we cannot find if a method is overwritten. > > > Should also to make sure to > > depcreate this method on the next version of android. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
