Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1819113002:
[MediaSession, Android] Adding visibility tests for MediaSession (Closed)
|
Created:
4 years, 9 months ago by Zhiqiang Zhang (Slow) Modified:
4 years, 8 months ago CC:
DaleCurtis, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[MediaSession, Android] Adding visibility tests for MediaSession
This CL adds integration tests for MediaSession to check
if the MediaSession is in desired state when the tab goes background.
BUG=596645
Committed: https://crrev.com/39f00fb6ba430547a155098e760b5452a4e7ae0e
Cr-Commit-Position: refs/heads/master@{#384579}
Patch Set 1 : initial working patch #Patch Set 2 : fixing non-Android builds #Patch Set 3 : remove redundant tests #Patch Set 4 : disable tests for desktop #
Total comments: 16
Patch Set 5 : addressed Mounir's comments #Patch Set 6 : fixing tests #
Total comments: 2
Patch Set 7 : use default timeouts #
Total comments: 2
Patch Set 8 : use message loop & callback instead of polling with timeout #
Total comments: 18
Patch Set 9 : addressed Dale's comments #Patch Set 10 : merge _browsertest.h into _browsertest.cc #Patch Set 11 : rebase since unified media pipeline is now default on Android #Patch Set 12 : fix command line flag #
Messages
Total messages: 44 (20 generated)
Patchset #1 (id:1) has been deleted
zqzhang@chromium.org changed reviewers: + mlamouri@chromium.org
Initial working patch. Just to show how the tests generally works.
Hi Dale! These new tests are failing on desktop, but not on Android, both using spitzer. Do you have any ideas? I did some logging, and it shows that WMPI tries to StartPipeline() several times but failed and got error "PIPELINE_ERROR demuxer: could not open".
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Probably this is being fixed as we speak with https://codereview.chromium.org/1810333004
On 2016/03/22 18:01:38, DaleCurtis wrote: > Probably this is being fixed as we speak with > https://codereview.chromium.org/1810333004 I tried your patch and it works. Thanks!
As discussed offline, I think it would be better if these were unit tests but given the complexity to test WMPA and WMPI in a consistent way, I understand that browser tests work better. https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... File content/browser/media/session/media_session.h (right): https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session.h:129: CONTENT_EXPORT State audio_focus_state_for_test() const; Could you make MediaSessionIntegrationBrowserTestBase a friend? So it can directly access the private member? https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... File content/browser/media/session/media_session_integration_browsertest_base.cc (right): https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_integration_browsertest_base.cc:40: shell()->LoadURL(GetTestUrl("android/media", "media-session.html")); Maybe you could move this file in a follow-up? Media session is no longer android-only. https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_integration_browsertest_base.cc:89: base::TimeDelta::FromSeconds(5), base::TimeDelta::FromMilliseconds(50), I'm not sure if there is a better way but you pointed it's coming from another media test file. I will leave it for dalecurtis@ to decide. https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_integration_browsertest_base.cc:104: base::Unretained(this)))); Could you improve the readability here? Maybe something like: { LoadTestPage(); RunScript(...) ASSERT_TRUE(...); RunScript(...); ASSERT_TRUE(...); RunScript(...); ASSERT_TRUE(..); } same below. https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_integration_browsertest_base.cc:122: } Could you have these tests with kEnableMediaSuspend and kDisableMediaSuspend? https://code.google.com/p/chromium/codesearch#chromium/src/media/base/media_s... https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... File content/browser/media/session/media_session_integration_browsertest_base.h (right): https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_integration_browsertest_base.h:9: namespace base { style: I believe you are missing a line between the #include and the namespace declaration. https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_integration_browsertest_base.h:19: class MediaSessionIntegrationBrowserTestBase nit: I would call this MediaSessionVisibilityBrowserTestBase https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... File content/browser/media/session/media_session_integration_default_pipeline_browsertest.cc (right): https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_integration_default_pipeline_browsertest.cc:9: #if defined(OS_ANDROID) Maybe you could remove this?
Description was changed from ========== [MediaSession, Android] Adding integration tests for MediaSession This CL adds integration tests for MediaSession. Especially for checking if the MediaSession is in desired state when the tab goes background. BUG=596645 ========== to ========== [MediaSession, Android] Adding visibility tests for MediaSession This CL adds integration tests for MediaSession to check if the MediaSession is in desired state when the tab goes background. BUG=596645 ==========
PTAL. * Addressed Mounir's comments * Added more configurations * ConditionalWait() is waiting for Dale's reply (see Mounir's comments in patch set 4) https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... File content/browser/media/session/media_session.h (right): https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session.h:129: CONTENT_EXPORT State audio_focus_state_for_test() const; On 2016/03/23 15:10:15, Mounir Lamouri wrote: > Could you make MediaSessionIntegrationBrowserTestBase a friend? So it can > directly access the private member? Done. https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... File content/browser/media/session/media_session_integration_browsertest_base.cc (right): https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_integration_browsertest_base.cc:40: shell()->LoadURL(GetTestUrl("android/media", "media-session.html")); On 2016/03/23 15:10:16, Mounir Lamouri wrote: > Maybe you could move this file in a follow-up? Media session is no longer > android-only. Sure, will do. https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_integration_browsertest_base.cc:104: base::Unretained(this)))); On 2016/03/23 15:10:15, Mounir Lamouri wrote: > Could you improve the readability here? Maybe something like: > { > LoadTestPage(); > > RunScript(...) > ASSERT_TRUE(...); > > RunScript(...); > ASSERT_TRUE(...); > > RunScript(...); > ASSERT_TRUE(..); > } > > same below. Done. https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_integration_browsertest_base.cc:122: } On 2016/03/23 15:10:16, Mounir Lamouri wrote: > Could you have these tests with kEnableMediaSuspend and kDisableMediaSuspend? > > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/media_s... Done. https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... File content/browser/media/session/media_session_integration_browsertest_base.h (right): https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_integration_browsertest_base.h:9: namespace base { On 2016/03/23 15:10:16, Mounir Lamouri wrote: > style: I believe you are missing a line between the #include and the namespace > declaration. Done. https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_integration_browsertest_base.h:19: class MediaSessionIntegrationBrowserTestBase On 2016/03/23 15:10:16, Mounir Lamouri wrote: > nit: I would call this MediaSessionVisibilityBrowserTestBase Done. https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... File content/browser/media/session/media_session_integration_default_pipeline_browsertest.cc (right): https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_integration_default_pipeline_browsertest.cc:9: #if defined(OS_ANDROID) On 2016/03/23 15:10:16, Mounir Lamouri wrote: > Maybe you could remove this? Done.
https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... File content/browser/media/session/media_session_integration_browsertest_base.cc (right): https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_integration_browsertest_base.cc:89: base::TimeDelta::FromSeconds(5), base::TimeDelta::FromMilliseconds(50), On 2016/03/23 at 15:10:16, Mounir Lamouri wrote: > I'm not sure if there is a better way but you pointed it's coming from another media test file. I will leave it for dalecurtis@ to decide. there are some testing::Timeout values that you should use instead: https://code.google.com/p/chromium/codesearch#chromium/src/base/test/test_tim... There might be some browser test helper for polling for a condition: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...
On 2016/03/24 18:22:41, DaleCurtis wrote: > https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... > File content/browser/media/session/media_session_integration_browsertest_base.cc > (right): > > https://codereview.chromium.org/1819113002/diff/80001/content/browser/media/s... > content/browser/media/session/media_session_integration_browsertest_base.cc:89: > base::TimeDelta::FromSeconds(5), base::TimeDelta::FromMilliseconds(50), > On 2016/03/23 at 15:10:16, Mounir Lamouri wrote: > > I'm not sure if there is a better way but you pointed it's coming from another > media test file. I will leave it for dalecurtis@ to decide. > > there are some testing::Timeout values that you should use instead: > > https://code.google.com/p/chromium/codesearch#chromium/src/base/test/test_tim... > > There might be some browser test helper for polling for a condition: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... Thanks, Dale. However I didn't found any polling utilities yet. Maybe I can move it to `browser_test_utils` in a follow-up?
https://codereview.chromium.org/1819113002/diff/120001/content/browser/media/... File content/browser/media/session/media_session_visibility_browsertest_base.cc (right): https://codereview.chromium.org/1819113002/diff/120001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.cc:59: bool MediaSessionVisibilityBrowserTestBase::ConditionalWait( You shouldn't need to manually handle timeouts, this should be handled by the browsertest harness itself. I also don't know if this is the right way to go about this. What about having a new method on MediaSession::SetOnAudioStateChangedCallbackForTesting(). This callback would be executed when audio_focus_state_ changed. This would let you get rid of all the waits in this file I think.
lgtm if Dale is happy :) https://codereview.chromium.org/1819113002/diff/140001/content/browser/media/... File content/browser/media/session/media_session_visibility_browsertest_instances.cc (right): https://codereview.chromium.org/1819113002/diff/140001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_instances.cc:53: public MediaSessionVisibilityBrowserTestBase { I think you might need to use a kDisableUnifiedMediaPipeline when unified media pipeline will be enabled by default, see https://crbug.com/597014. Maybe add a TODO?
PTAL, now I use callback and message loop to wait for media session state change. https://codereview.chromium.org/1819113002/diff/120001/content/browser/media/... File content/browser/media/session/media_session_visibility_browsertest_base.cc (right): https://codereview.chromium.org/1819113002/diff/120001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.cc:59: bool MediaSessionVisibilityBrowserTestBase::ConditionalWait( On 2016/03/24 20:21:25, DaleCurtis wrote: > You shouldn't need to manually handle timeouts, this should be handled by the > browsertest harness itself. > > I also don't know if this is the right way to go about this. What about having a > new method on MediaSession::SetOnAudioStateChangedCallbackForTesting(). This > callback would be executed when audio_focus_state_ changed. This would let you > get rid of all the waits in this file I think. Done. https://codereview.chromium.org/1819113002/diff/140001/content/browser/media/... File content/browser/media/session/media_session_visibility_browsertest_instances.cc (right): https://codereview.chromium.org/1819113002/diff/140001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_instances.cc:53: public MediaSessionVisibilityBrowserTestBase { On 2016/03/31 10:27:53, Mounir Lamouri wrote: > I think you might need to use a kDisableUnifiedMediaPipeline when unified media > pipeline will be enabled by default, see https://crbug.com/597014. Maybe add a > TODO? Done.
The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819113002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819113002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits. File layout is a bit inconsistent with the rest of content/ I think, but I'm not in love with monolithic test files, so I don't have a strong objection to the separate base test plus instances files. https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... File content/browser/media/session/media_session.h (right): https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session.h:162: typedef base::Callback<void(State)> StateChangedCallback; as of c++11 we use 'using' now, check out http://chromium-cpp.appspot.com https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... File content/browser/media/session/media_session_visibility_browsertest_base.cc (right): https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.cc:26: MediaSessionVisibilityBrowserTestBase() { Is this the style that git cl format applied? It seems a bit unusual to have a four space indent on the following line. https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.cc:39: new MessageLoopRunner; () after class name. ditto for all "new XXX" below. https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.cc:73: void MediaSessionVisibilityBrowserTestBase::RunScript(std::string script) { const& for strings. https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.cc:80: state_loop_runner.second = new MessageLoopRunner; () https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.cc:89: Extra line. https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.cc:90: void MediaSessionVisibilityBrowserTestBase::Wait(base::TimeDelta timeout) { You don't really need this, but up to you. THe browser tests have built in timeouts that will be applied. You could just use base::RunLoop().Run(), though this may result in cleaner error messages it may be flaky on the slower bots. https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... File content/browser/media/session/media_session_visibility_browsertest_base.h (right): https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.h:70: }; DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.h:74: #define INCLUDE_TEST_FROM_BASE_CLASS(test_fixture, test_name) \ We generally try to avoid #define's. Can you avoid this? Or keep it to a single .cc file instead of a header file?
Patchset #9 (id:180001) has been deleted
* Addressed Dale's comments with replies. * Merged browsertest_instances and browsertest_base to match the file layout. * Going to commit. https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... File content/browser/media/session/media_session.h (right): https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session.h:162: typedef base::Callback<void(State)> StateChangedCallback; On 2016/03/31 16:50:26, DaleCurtis wrote: > as of c++11 we use 'using' now, check out http://chromium-cpp.appspot.com Done. https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... File content/browser/media/session/media_session_visibility_browsertest_base.cc (right): https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.cc:26: MediaSessionVisibilityBrowserTestBase() { On 2016/03/31 16:50:26, DaleCurtis wrote: > Is this the style that git cl format applied? It seems a bit unusual to have a > four space indent on the following line. Done. https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.cc:39: new MessageLoopRunner; On 2016/03/31 16:50:26, DaleCurtis wrote: > () after class name. ditto for all "new XXX" below. Done. https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.cc:73: void MediaSessionVisibilityBrowserTestBase::RunScript(std::string script) { On 2016/03/31 16:50:26, DaleCurtis wrote: > const& for strings. Done. https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.cc:80: state_loop_runner.second = new MessageLoopRunner; On 2016/03/31 16:50:26, DaleCurtis wrote: > () Done. https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.cc:89: On 2016/03/31 16:50:26, DaleCurtis wrote: > Extra line. Done. https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.cc:90: void MediaSessionVisibilityBrowserTestBase::Wait(base::TimeDelta timeout) { On 2016/03/31 16:50:26, DaleCurtis wrote: > You don't really need this, but up to you. THe browser tests have built in > timeouts that will be applied. You could just use base::RunLoop().Run(), though > this may result in cleaner error messages it may be flaky on the slower bots. I think we still need this since this is for verifying something NOT happen. e.g. If we don't want MediaSession go suspended/inactive after some action, I have to wait for a short time and check the state. The browser test timeout might be longer than the media duration, and the MediaSession will go inactive. We have met the same issue in MediaSessionTest.java. The way we are doing now might be flaky, but we still don't have better solution yet. https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... File content/browser/media/session/media_session_visibility_browsertest_base.h (right): https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.h:70: }; On 2016/03/31 16:50:26, DaleCurtis wrote: > DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/1819113002/diff/160001/content/browser/media/... content/browser/media/session/media_session_visibility_browsertest_base.h:74: #define INCLUDE_TEST_FROM_BASE_CLASS(test_fixture, test_name) \ On 2016/03/31 16:50:26, DaleCurtis wrote: > We generally try to avoid #define's. Can you avoid this? Or keep it to a single > .cc file instead of a header file? Moved to .cc file
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1819113002/#ps200001 (title: "addressed Dale's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819113002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819113002/200001
We usually forgo having a .h file for browsertests since you merged them.
The CQ bit was unchecked by zqzhang@chromium.org
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1819113002/#ps220001 (title: "merge _browsertest.h into _browsertest.cc")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819113002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819113002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1819113002/#ps240001 (title: "rebase since unified media pipeline is now default on Android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819113002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819113002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by zqzhang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1819113002/#ps260001 (title: "fix command line flag")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1819113002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1819113002/260001
Message was sent while issue was closed.
Description was changed from ========== [MediaSession, Android] Adding visibility tests for MediaSession This CL adds integration tests for MediaSession to check if the MediaSession is in desired state when the tab goes background. BUG=596645 ========== to ========== [MediaSession, Android] Adding visibility tests for MediaSession This CL adds integration tests for MediaSession to check if the MediaSession is in desired state when the tab goes background. BUG=596645 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== [MediaSession, Android] Adding visibility tests for MediaSession This CL adds integration tests for MediaSession to check if the MediaSession is in desired state when the tab goes background. BUG=596645 ========== to ========== [MediaSession, Android] Adding visibility tests for MediaSession This CL adds integration tests for MediaSession to check if the MediaSession is in desired state when the tab goes background. BUG=596645 Committed: https://crrev.com/39f00fb6ba430547a155098e760b5452a4e7ae0e Cr-Commit-Position: refs/heads/master@{#384579} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/39f00fb6ba430547a155098e760b5452a4e7ae0e Cr-Commit-Position: refs/heads/master@{#384579} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
