Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2623953002:
[Chromecast] Fix media session blocking tests. (Closed)
|
Created:
3 years, 11 months ago by derekjchow1 Modified:
3 years, 10 months ago CC:
alokp+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, halliwell+watch_chromium.org, jam, lcwu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromecast] Fix media session blocking tests.
This CL:
1) Remove unneeded web_contents logic in CastMediaBlockerTest
2) Makes MockMediaSession a pure mock
3) Adds Add/RemoveObservers to MediaSession base class.
This removes a bad static_cast from MockMediaSession to
MediaSessionImpl.
BUG=665118
TEST=cast_shell_unittests
Review-Url: https://codereview.chromium.org/2623953002
Cr-Commit-Position: refs/heads/master@{#447801}
Committed: https://chromium.googlesource.com/chromium/src/+/b2738d5ab33c68b6249b64b06d650c71fd90af8b
Patch Set 1 #
Total comments: 13
Patch Set 2 : [Chromecast] Fix media session blocking tests. #Patch Set 3 : Removed stale TODO #
Total comments: 2
Patch Set 4 : Move MockMediaSession into chromecast/ only #
Total comments: 22
Patch Set 5 : [Chromecast] Fix media session blocking tests. #
Total comments: 1
Patch Set 6 : [Chromecast] Fix media session blocking tests. #
Messages
Total messages: 53 (32 generated)
The CQ bit was checked by derekjchow@chromium.org
The CQ bit was unchecked by derekjchow@chromium.org
The CQ bit was checked by derekjchow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by derekjchow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
derekjchow@chromium.org changed reviewers: + alokp@chromium.org, avayvod@chromium.org, zqzhang@google.com
This CL fixes a bad static_cast in CastMediaBlockerTest. This occurs when the MediaSessionObserver constructor tries to static_cast MockMediaSession into a MediaSessionImpl. Anton, Zhiqiang PTAL.
chromecast/ lgtm https://codereview.chromium.org/2623953002/diff/1/chromecast/browser/cast_med... File chromecast/browser/cast_media_blocker_unittest.cc (right): https://codereview.chromium.org/2623953002/diff/1/chromecast/browser/cast_med... chromecast/browser/cast_media_blocker_unittest.cc:23: class MockMediaSession : public content::MediaSession { nit: can we move this mock class into content/public/browser, adjacent to media_session.h?
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
https://codereview.chromium.org/2623953002/diff/1/content/public/browser/medi... File content/public/browser/media_session.h (right): https://codereview.chromium.org/2623953002/diff/1/content/public/browser/medi... content/public/browser/media_session.h:74: friend class MediaSessionObserver; This is a bit confusing. Why does an interface needs to be friend with the observer class?
Description was changed from
==========
[Chromecast] Fix media session blocking tests.
This CL:
1) Remove unneeded web_contents logic in CastMediaBlockerTest
2) Makes MockMediaSession a pure mock
3) Adds Add/RemoveObservers to protected MediaSession API
This removes a bad static_cast from MockMediaSession to
MediaSessionImpl.
BUG=665118
TEST=cast_shell_unittests
==========
to
==========
[Chromecast] Fix media session blocking tests.
This CL:
1) Remove unneeded web_contents logic in CastMediaBlockerTest
2) Makes MockMediaSession a pure mock
3) Adds Add/RemoveObservers to protected MediaSession API
This removes a bad static_cast from MockMediaSession to
MediaSessionImpl.
BUG=665118
TEST=cast_shell_unittests
==========
zqzhang@chromium.org changed reviewers: + zqzhang@chromium.org - zqzhang@google.com
https://codereview.chromium.org/2623953002/diff/1/chromecast/browser/cast_med... File chromecast/browser/cast_media_blocker_unittest.cc (right): https://codereview.chromium.org/2623953002/diff/1/chromecast/browser/cast_med... chromecast/browser/cast_media_blocker_unittest.cc:34: MOCK_METHOD1(AddObserver, void(content::MediaSessionObserver*)); protected? Is it Add/RemoveObserver not overriden which caused the compile failure?
https://codereview.chromium.org/2623953002/diff/1/chromecast/browser/cast_med... File chromecast/browser/cast_media_blocker_unittest.cc (right): https://codereview.chromium.org/2623953002/diff/1/chromecast/browser/cast_med... chromecast/browser/cast_media_blocker_unittest.cc:23: class MockMediaSession : public content::MediaSession { On 2017/01/14 00:23:28, alokp wrote: > nit: can we move this mock class into content/public/browser, adjacent to > media_session.h? Anton, Zhiqiang, Mounir, would this be ok? I'd be happy to make this change if you're ok with it. https://codereview.chromium.org/2623953002/diff/1/chromecast/browser/cast_med... chromecast/browser/cast_media_blocker_unittest.cc:34: MOCK_METHOD1(AddObserver, void(content::MediaSessionObserver*)); On 2017/01/16 11:23:13, Zhiqiang Zhang wrote: > protected? > > Is it Add/RemoveObserver not overriden which caused the compile failure? Should be public: https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#... "You must always put a mock method definition (MOCK_METHOD*) in a public: section of the mock class, regardless of the method being mocked being public, protected, or private in the base class." Also, correct. The MediaSessionObserver constructor is called in CastMediaBlocker, which will static_cast a MediaSession into a MediaSessionImpl and call AddObserver. Previously, MockMediaSession didn't have AddObserver (because it's not a MediaSessionImpl). This patch fixes the issue by making AddObserver a protected function. https://codereview.chromium.org/2623953002/diff/1/content/public/browser/medi... File content/public/browser/media_session.h (right): https://codereview.chromium.org/2623953002/diff/1/content/public/browser/medi... content/public/browser/media_session.h:74: friend class MediaSessionObserver; On 2017/01/16 11:13:20, mlamouri wrote: > This is a bit confusing. Why does an interface needs to be friend with the > observer class? https://cs.chromium.org/chromium/src/content/public/browser/media_session_obs... MediaSessionObserver will call the protected AddObserver/RemoveObserver functions.
Sorry for the late reply. Some more suggestions. https://codereview.chromium.org/2623953002/diff/1/chromecast/browser/cast_med... File chromecast/browser/cast_media_blocker_unittest.cc (right): https://codereview.chromium.org/2623953002/diff/1/chromecast/browser/cast_med... chromecast/browser/cast_media_blocker_unittest.cc:23: class MockMediaSession : public content::MediaSession { On 2017/01/18 02:25:33, derekjchow1 wrote: > On 2017/01/14 00:23:28, alokp wrote: > > nit: can we move this mock class into content/public/browser, adjacent to > > media_session.h? > > Anton, Zhiqiang, Mounir, would this be ok? I'd be happy to make this change if > you're ok with it. I'm not sure content owners is happy with this. Maybe you can put it into content/public/test. https://codereview.chromium.org/2623953002/diff/1/chromecast/browser/cast_med... chromecast/browser/cast_media_blocker_unittest.cc:34: MOCK_METHOD1(AddObserver, void(content::MediaSessionObserver*)); On 2017/01/18 02:25:33, derekjchow1 wrote: > On 2017/01/16 11:23:13, Zhiqiang Zhang wrote: > > protected? > > > > Is it Add/RemoveObserver not overriden which caused the compile failure? > > Should be public: > > https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#... > > "You must always put a mock method definition (MOCK_METHOD*) in a public: > section of the mock class, regardless of the method being mocked being public, > protected, or private in the base class." > > Also, correct. The MediaSessionObserver constructor is called in > CastMediaBlocker, which will static_cast a MediaSession into a MediaSessionImpl > and call AddObserver. Previously, MockMediaSession didn't have AddObserver > (because it's not a MediaSessionImpl). > > This patch fixes the issue by making AddObserver a protected function. Acknowledged. https://codereview.chromium.org/2623953002/diff/1/content/public/browser/medi... File content/public/browser/media_session.h (right): https://codereview.chromium.org/2623953002/diff/1/content/public/browser/medi... content/public/browser/media_session.h:74: friend class MediaSessionObserver; On 2017/01/18 02:25:33, derekjchow1 wrote: > On 2017/01/16 11:13:20, mlamouri wrote: > > This is a bit confusing. Why does an interface needs to be friend with the > > observer class? > > https://cs.chromium.org/chromium/src/content/public/browser/media_session_obs... > > MediaSessionObserver will call the protected AddObserver/RemoveObserver > functions. I think you can follow the pattern of TestWebContents: Have a TestMediaSession inheriting from MediaSessionImpl, then you don't need to change MediaSession/MediaSessionObserver in content/public. Then you can have MockMediaSession mocking TestMediaSession. In this way you also don't need to change AddObserver/RemoveObserver. WDYT?
https://codereview.chromium.org/2623953002/diff/1/chromecast/browser/cast_med... File chromecast/browser/cast_media_blocker_unittest.cc (right): https://codereview.chromium.org/2623953002/diff/1/chromecast/browser/cast_med... chromecast/browser/cast_media_blocker_unittest.cc:23: class MockMediaSession : public content::MediaSession { On 2017/01/18 21:57:51, Zhiqiang Zhang wrote: > On 2017/01/18 02:25:33, derekjchow1 wrote: > > On 2017/01/14 00:23:28, alokp wrote: > > > nit: can we move this mock class into content/public/browser, adjacent to > > > media_session.h? > > > > Anton, Zhiqiang, Mounir, would this be ok? I'd be happy to make this change if > > you're ok with it. > > I'm not sure content owners is happy with this. > Maybe you can put it into content/public/test. SGTM. To be clear, you'd be ok with that location? https://codereview.chromium.org/2623953002/diff/1/content/public/browser/medi... File content/public/browser/media_session.h (right): https://codereview.chromium.org/2623953002/diff/1/content/public/browser/medi... content/public/browser/media_session.h:74: friend class MediaSessionObserver; On 2017/01/18 21:57:51, Zhiqiang Zhang wrote: > On 2017/01/18 02:25:33, derekjchow1 wrote: > > On 2017/01/16 11:13:20, mlamouri wrote: > > > This is a bit confusing. Why does an interface needs to be friend with the > > > observer class? > > > > > https://cs.chromium.org/chromium/src/content/public/browser/media_session_obs... > > > > MediaSessionObserver will call the protected AddObserver/RemoveObserver > > functions. > > I think you can follow the pattern of TestWebContents: > > Have a TestMediaSession inheriting from MediaSessionImpl, then you don't need to > change MediaSession/MediaSessionObserver in content/public. > > Then you can have MockMediaSession mocking TestMediaSession. > > In this way you also don't need to change AddObserver/RemoveObserver. > > WDYT? I disagree. Inheriting from MediaSessionImpl means that changes in actual behavior of MediaSessionImpl will affect MockMediaSession. This can have side effects in test. The purpose of this test is to make sure that CastMediaBlocker makes the correct calls to a MediaSession. We should be using a pure mock that is strictly used for checking (hence why we should be using the MediaSession interface instead of MediaSessionImpl).
https://codereview.chromium.org/2623953002/diff/1/chromecast/browser/cast_med... File chromecast/browser/cast_media_blocker_unittest.cc (right): https://codereview.chromium.org/2623953002/diff/1/chromecast/browser/cast_med... chromecast/browser/cast_media_blocker_unittest.cc:23: class MockMediaSession : public content::MediaSession { On 2017/01/19 01:28:14, derekjchow1 wrote: > On 2017/01/18 21:57:51, Zhiqiang Zhang wrote: > > On 2017/01/18 02:25:33, derekjchow1 wrote: > > > On 2017/01/14 00:23:28, alokp wrote: > > > > nit: can we move this mock class into content/public/browser, adjacent to > > > > media_session.h? > > > > > > Anton, Zhiqiang, Mounir, would this be ok? I'd be happy to make this change > if > > > you're ok with it. > > > > I'm not sure content owners is happy with this. > > Maybe you can put it into content/public/test. > > SGTM. To be clear, you'd be ok with that location? Yes, I'm OK with it. https://codereview.chromium.org/2623953002/diff/1/content/public/browser/medi... File content/public/browser/media_session.h (right): https://codereview.chromium.org/2623953002/diff/1/content/public/browser/medi... content/public/browser/media_session.h:74: friend class MediaSessionObserver; On 2017/01/19 01:28:14, derekjchow1 wrote: > On 2017/01/18 21:57:51, Zhiqiang Zhang wrote: > > On 2017/01/18 02:25:33, derekjchow1 wrote: > > > On 2017/01/16 11:13:20, mlamouri wrote: > > > > This is a bit confusing. Why does an interface needs to be friend with the > > > > observer class? > > > > > > > > > https://cs.chromium.org/chromium/src/content/public/browser/media_session_obs... > > > > > > MediaSessionObserver will call the protected AddObserver/RemoveObserver > > > functions. > > > > I think you can follow the pattern of TestWebContents: > > > > Have a TestMediaSession inheriting from MediaSessionImpl, then you don't need > to > > change MediaSession/MediaSessionObserver in content/public. > > > > Then you can have MockMediaSession mocking TestMediaSession. > > > > In this way you also don't need to change AddObserver/RemoveObserver. > > > > WDYT? > > I disagree. Inheriting from MediaSessionImpl means that changes in actual > behavior of MediaSessionImpl will affect MockMediaSession. This can have side > effects in test. > > The purpose of this test is to make sure that CastMediaBlocker makes the correct > calls to a MediaSession. We should be using a pure mock that is strictly used > for checking (hence why we should be using the MediaSession interface instead of > MediaSessionImpl). I see, TestWebContents is in content/test instead of content/public/test. So if you want to use it outside of content/, you have to use WebContentsTester::CreateTestWebContents, which doesn't fit our situation. I think you can put AddObserver/RemoveObserver in MediaSession and make it non-virtual. Then MediaSessionObserver can hold a pointer to MediaSession* instead of MediaSessionImpl*. As I remember the reason why AddObserver/RemoveObserver were put in MediaSessionImpl is that they are only called by MediaSessionObserver and there's no need to expose them. I think the situation has changed as you are mocking it from outside of content/.
Zhiqiang, PTAL. Moved MockMediaSession into content/public/test, and made Add/RemoveObserver functions part of MediaSession instead of MediaSessionImpl. I did have to make MediaSession as CONTENT_EXPORT in the process, so the test library could find the constructor/destructor for MediaSession. I'll add content/public/test OWNERS when we're ok with the CL.
lgtm w/ comments Also please update the CL description and explain why the changes to content::MediaSession is necessary (i.e. to make it mockable). https://codereview.chromium.org/2623953002/diff/40001/content/public/test/moc... File content/public/test/mock_media_session.h (right): https://codereview.chromium.org/2623953002/diff/40001/content/public/test/moc... content/public/test/mock_media_session.h:13: class MockMediaSession : public MediaSession { I think the major change is to make content::MediaSession mockable. As it is done now, maybe there's no need to put MockMediaSession in content/public/test anymore? As it's only mocked from chromecast/, you can put it in an anonymous namespace in the test file.
The CQ bit was checked by derekjchow@chromium.org to run a CQ dry run
https://codereview.chromium.org/2623953002/diff/40001/content/public/test/moc... File content/public/test/mock_media_session.h (right): https://codereview.chromium.org/2623953002/diff/40001/content/public/test/moc... content/public/test/mock_media_session.h:13: class MockMediaSession : public MediaSession { On 2017/01/20 16:27:56, Zhiqiang Zhang wrote: > I think the major change is to make content::MediaSession mockable. As it is > done now, maybe there's no need to put MockMediaSession in content/public/test > anymore? As it's only mocked from chromecast/, you can put it in an anonymous > namespace in the test file. SGTM. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
[Chromecast] Fix media session blocking tests.
This CL:
1) Remove unneeded web_contents logic in CastMediaBlockerTest
2) Makes MockMediaSession a pure mock
3) Adds Add/RemoveObservers to protected MediaSession API
This removes a bad static_cast from MockMediaSession to
MediaSessionImpl.
BUG=665118
TEST=cast_shell_unittests
==========
to
==========
[Chromecast] Fix media session blocking tests.
This CL:
1) Remove unneeded web_contents logic in CastMediaBlockerTest
2) Makes MockMediaSession a pure mock
3) Adds Add/RemoveObservers to MediaSession base class.
This removes a bad static_cast from MockMediaSession to
MediaSessionImpl.
BUG=665118
TEST=cast_shell_unittests
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The code itself looks fine but I have two general questions: - why not have AddObserver as virtual methods and simply implement them in the _impl? it seems odd to have them implemented in the interface - why make the observer class a friend instead of have Add/Remove Observer methods public? https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... File content/public/browser/media_session.cc (right): https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.cc:9: MediaSession::MediaSession() {} = default; https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.cc:11: MediaSession::~MediaSession() {} = default; https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... File content/public/browser/media_session.h (right): https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:71: base::ObserverList<MediaSessionObserver>& observers() { return observers_; } Can you move the implementation to the .cc file?
https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... File content/public/browser/media_session.cc (right): https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. You can probably remove this file now. https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... File content/public/browser/media_session.h (right): https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:28: class CONTENT_EXPORT MediaSession { Revert this change as MediaSession is back to a pure interface. https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:43: virtual ~MediaSession(); Revert this change as MediaSession is back to a pure interface. https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:69: MediaSession(); Revert this change as MediaSession is back to a pure interface. https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:71: base::ObserverList<MediaSessionObserver>& observers() { return observers_; } I think you don't need this any more. Revert this change and keep it in media_session_impl.cc https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:76: void AddObserver(MediaSessionObserver* observer); Make it pure-virtual. Keep it as private should be OK. https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:77: void RemoveObserver(MediaSessionObserver* observer); ditto https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:79: base::ObserverList<MediaSessionObserver> observers_; ditto (same reason as `observers()`)
On 2017/01/28 02:01:41, mlamouri (slow) wrote: > The code itself looks fine but I have two general questions: > - why not have AddObserver as virtual methods and simply implement them in the > _impl? it seems odd to have them implemented in the interface > - why make the observer class a friend instead of have Add/Remove Observer > methods public? Sorry, I was a bit confused about the access levels and GMock so I suggested this change. After tweaking the patch locally, I found there's a better way. I think from my POV we don't want to expose anything new in content/public/browser/media_session.h. Please see my new comments. Sorry it is pretty much like patch #1, with "protected" changed to "private"
The CQ bit was checked by derekjchow@chromium.org to run a CQ dry run
Moved back to pure virtual Add/RemoveObserver https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... File content/public/browser/media_session.cc (right): https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/01/30 12:09:10, Zhiqiang Zhang wrote: > You can probably remove this file now. Done. https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.cc:9: MediaSession::MediaSession() {} On 2017/01/28 02:01:40, mlamouri (slow - BlinkOn) wrote: > = default; Done. https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.cc:11: MediaSession::~MediaSession() {} On 2017/01/28 02:01:40, mlamouri (slow - BlinkOn) wrote: > = default; Done. https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... File content/public/browser/media_session.h (right): https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:28: class CONTENT_EXPORT MediaSession { On 2017/01/30 12:09:11, Zhiqiang Zhang wrote: > Revert this change as MediaSession is back to a pure interface. Done. https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:43: virtual ~MediaSession(); On 2017/01/30 12:09:10, Zhiqiang Zhang wrote: > Revert this change as MediaSession is back to a pure interface. Done. https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:69: MediaSession(); On 2017/01/30 12:09:11, Zhiqiang Zhang wrote: > Revert this change as MediaSession is back to a pure interface. Done. https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:71: base::ObserverList<MediaSessionObserver>& observers() { return observers_; } On 2017/01/28 02:01:40, mlamouri (slow - BlinkOn) wrote: > Can you move the implementation to the .cc file? Removed (only in MediaSessionImpl now). https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:71: base::ObserverList<MediaSessionObserver>& observers() { return observers_; } On 2017/01/30 12:09:10, Zhiqiang Zhang wrote: > I think you don't need this any more. Revert this change and keep it in > media_session_impl.cc Done. https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:76: void AddObserver(MediaSessionObserver* observer); On 2017/01/30 12:09:10, Zhiqiang Zhang wrote: > Make it pure-virtual. Keep it as private should be OK. Done. https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:77: void RemoveObserver(MediaSessionObserver* observer); On 2017/01/30 12:09:10, Zhiqiang Zhang wrote: > ditto Done. https://codereview.chromium.org/2623953002/diff/60001/content/public/browser/... content/public/browser/media_session.h:79: base::ObserverList<MediaSessionObserver> observers_; On 2017/01/30 12:09:10, Zhiqiang Zhang wrote: > ditto (same reason as `observers()`) Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
lgtm Please ask a content/ owner for approval
derekjchow@chromium.org changed reviewers: + jam@chromium.org
+jam, could you PTAL?
lgtm
https://codereview.chromium.org/2623953002/diff/80001/content/browser/media/s... File content/browser/media/session/media_session_impl.h (right): https://codereview.chromium.org/2623953002/diff/80001/content/browser/media/s... content/browser/media/session/media_session_impl.h:88: void AddObserver(MediaSessionObserver* observer) override; nit: move by the other overridden methods
The CQ bit was checked by derekjchow@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by derekjchow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alokp@chromium.org, jam@chromium.org, zqzhang@chromium.org Link to the patchset: https://codereview.chromium.org/2623953002/#ps100001 (title: "[Chromecast] Fix media session blocking tests.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486060033962920,
"parent_rev": "2df23785add5036585c322687025c83e03cb5764", "commit_rev":
"b2738d5ab33c68b6249b64b06d650c71fd90af8b"}
Message was sent while issue was closed.
Description was changed from
==========
[Chromecast] Fix media session blocking tests.
This CL:
1) Remove unneeded web_contents logic in CastMediaBlockerTest
2) Makes MockMediaSession a pure mock
3) Adds Add/RemoveObservers to MediaSession base class.
This removes a bad static_cast from MockMediaSession to
MediaSessionImpl.
BUG=665118
TEST=cast_shell_unittests
==========
to
==========
[Chromecast] Fix media session blocking tests.
This CL:
1) Remove unneeded web_contents logic in CastMediaBlockerTest
2) Makes MockMediaSession a pure mock
3) Adds Add/RemoveObservers to MediaSession base class.
This removes a bad static_cast from MockMediaSession to
MediaSessionImpl.
BUG=665118
TEST=cast_shell_unittests
Review-Url: https://codereview.chromium.org/2623953002
Cr-Commit-Position: refs/heads/master@{#447801}
Committed:
https://chromium.googlesource.com/chromium/src/+/b2738d5ab33c68b6249b64b06d65...
==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/b2738d5ab33c68b6249b64b06d65... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
