[Chromecast] Fix media session blocking tests.

content/public/browser/media_session.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_PUBLIC_BROWSER_MEDIA_SESSION_H_
 #define CONTENT_PUBLIC_BROWSER_MEDIA_SESSION_H_
 
 #include "base/macros.h"
 #include "content/common/content_export.h"
 
 namespace blink {
 namespace mojom {
 enum class MediaSessionAction;
 }  // namespace mojom
 }  // namespace blink
 
 namespace content {
 
+class MediaSessionObserver;
 class WebContents;
 
 // MediaSession manages the media session and audio focus for a given
 // WebContents. There is only one MediaSession per WebContents.
 //
 // MediaSession allows clients to observe its changes via MediaSessionObserver,
 // and allows clients to resume/suspend/stop the managed players.
 class MediaSession {
  public:
   enum class SuspendType {
@@ skipping 28 matching lines @@
 
   // Let the media session start ducking such that the volume multiplier is
   // reduced.
   virtual void StartDucking() = 0;
 
   // Let the media session stop ducking such that the volume multiplier is
   // recovered.
   virtual void StopDucking() = 0;
 
  protected:
+  virtual void AddObserver(MediaSessionObserver* observer) = 0;
+  virtual void RemoveObserver(MediaSessionObserver* observer) = 0;
+
   MediaSession() = default;
+
+ private:
+  friend class MediaSessionObserver;

[mlamouri (slow - plz ping), 2017/01/16 11:13:20]

This is a bit confusing. Why does an interface needs to be friend with the
observer class?

[derekjchow1, 2017/01/18 02:25:33]

https://cs.chromium.org/chromium/src/content/public/browser/media_session_obs...

MediaSessionObserver will call the protected AddObserver/RemoveObserver
functions.

[Zhiqiang Zhang (Slow), 2017/01/18 21:57:51]

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?

[derekjchow1, 2017/01/19 01:28:14]

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).

[Zhiqiang Zhang (Slow), 2017/01/19 11:12:45]

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/.

 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_MEDIA_SESSION_H_