Let CastMediaBlocker observe MediaSession instead of WebContents

Let CastMediaBlocker observe MediaSession instead of WebContents

This CL is part of a larger effort of decoupling MediaSession
from WebContents.

BUG=658678

[Zhiqiang Zhang (Slow), 2016-10-24 14:43:36]

Description was changed from

==========
Let CastMediaBlocker observe MediaSession instead of WebContents

BUG=658678

temporary
==========

to

==========
Let CastMediaBlocker observe MediaSession instead of WebContents

This CL is part of a larger effort of decoupling MediaSession
from WebContents.

BUG=658678
==========

[Zhiqiang Zhang (Slow), 2016-10-24 18:29:31]

zqzhang@chromium.org changed reviewers:
+ derekjchow@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-24 18:29:32]

PTAL

https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.cc (left):

https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.cc:18:
base::Bind(&CastMediaBlocker::Resume, base::Unretained(this))) {}
Are these callbacks used for testing?
After we expose MediaSession in content/public, you can let MediaSessionMock
derive from content::MediaSession and use EXPECT_CALL on the mock. Then these
callbacks can be removed.

[derekjchow1, 2016-10-24 18:50:33]

https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.cc (left):

https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.cc:18:
base::Bind(&CastMediaBlocker::Resume, base::Unretained(this))) {}
On 2016/10/24 18:29:32, Zhiqiang Zhang wrote:
> Are these callbacks used for testing?
> After we expose MediaSession in content/public, you can let MediaSessionMock
> derive from content::MediaSession and use EXPECT_CALL on the mock. Then these
> callbacks can be removed.

Correct. Your approach SGTM. We can probably remove these callback if there's a
MockMediaSession we can rely on.

https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.cc (right):

https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.cc:17: media_session,
How is a content embedder supposed to obtain a reference to a MediaSession? For
Cast, we need to suspend media for a given WebContents. Is there a getter
function  in WebContents to obtain the MediaSession?

Similarily, is it expected there's a 1-1 mapping from MediaSession to
WebContents?

https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.h (right):

https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.h:8: #include "base/callback.h"
callback & callback_forward seems redundant.

https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.h:18: class CastMediaBlocker : public
content::MediaSessionObserver {
We actually inherit from this class internally, and override other
WebContentsObserver methods. For this CL, inherit from both observer interfaces,
and add a TODO (with my name stapled to it) to remove the WebContentsObserver
interface. I'll decouple the interfaces after your change lands.

[Zhiqiang Zhang (Slow), 2016-10-28 10:36:53]

PTAL

Got approval on the MediaSession-WebContents decoupling. We are good to go.

https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.cc (left):

https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.cc:18:
base::Bind(&CastMediaBlocker::Resume, base::Unretained(this))) {}
On 2016/10/24 18:50:33, derekjchow1 wrote:
> On 2016/10/24 18:29:32, Zhiqiang Zhang wrote:
> > Are these callbacks used for testing?
> > After we expose MediaSession in content/public, you can let MediaSessionMock
> > derive from content::MediaSession and use EXPECT_CALL on the mock. Then
these
> > callbacks can be removed.
> 
> Correct. Your approach SGTM. We can probably remove these callback if there's
a
> MockMediaSession we can rely on.

Done removing the callbacks. See the tests for details :)

https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.cc (right):

https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.cc:17: media_session,
On 2016/10/24 18:50:33, derekjchow1 wrote:
> How is a content embedder supposed to obtain a reference to a MediaSession?
For
> Cast, we need to suspend media for a given WebContents. Is there a getter
> function  in WebContents to obtain the MediaSession?
> 
> Similarily, is it expected there's a 1-1 mapping from MediaSession to
> WebContents?

Yes, it is 1-1 mapping.
You can use MediaSession::Get(WebContents*).
It's in another CL which is under review.

https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.h (right):

https://codereview.chromium.org/2450433002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.h:8: #include "base/callback.h"
On 2016/10/24 18:50:33, derekjchow1 wrote:
> callback & callback_forward seems redundant.

They are all removed since we don't use callback anymore.

[derekjchow1, 2016-10-28 16:46:16]

derekjchow@chromium.org changed reviewers:
+ halliwell@chromium.org

[derekjchow1, 2016-10-28 16:46:17]

lgtm, but you'll need owners approval in chromecast/ to submit this change.

Luke, could you take a look?

[halliwell, 2016-10-28 17:33:29]

lgtm % nit

https://codereview.chromium.org/2450433002/diff/80001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.h (right):

https://codereview.chromium.org/2450433002/diff/80001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.h:21: content::WebContentsObserver {
This inheritance seems pointless now.  Derek, are you planning to handle the
TODO here more or less immediately?

https://codereview.chromium.org/2450433002/diff/80001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.h:24: content::WebContents* web_contents);
nit: remove explicit

[derekjchow1, 2016-10-28 17:35:41]

https://codereview.chromium.org/2450433002/diff/80001/chromecast/browser/cast...
File chromecast/browser/cast_media_blocker.h (right):

https://codereview.chromium.org/2450433002/diff/80001/chromecast/browser/cast...
chromecast/browser/cast_media_blocker.h:21: content::WebContentsObserver {
On 2016/10/28 17:33:29, halliwell wrote:
> This inheritance seems pointless now.  Derek, are you planning to handle the
> TODO here more or less immediately?

Yes, I requested this so I could synchronize changing our internal builds with
the public one.