Refactor media out of WebContentsImpl to MediaWebContentsObserver.

content/public/browser/web_contents_observer.h

 // Copyright (c) 2012 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_WEB_CONTENTS_OBSERVER_H_
 #define CONTENT_PUBLIC_BROWSER_WEB_CONTENTS_OBSERVER_H_
 
 #include "base/process/kill.h"
 #include "base/process/process_handle.h"
 #include "content/common/content_export.h"
@@ skipping 406 matching lines @@
   // Invoked when a user cancels a before unload dialog.
   virtual void BeforeUnloadDialogCancelled() {}
 
   // Invoked when an accessibility event is received from the renderer process.
   virtual void AccessibilityEventReceived(
       const std::vector<AXEventNotificationDetails>& details) {}
 
   // Invoked when theme color is changed to |theme_color|.
   virtual void DidChangeThemeColor(SkColor theme_color) {}
 
-  // Invoked when media is playing.
-  virtual void MediaStartedPlaying() {}
-
-  // Invoked when media is paused.
-  virtual void MediaPaused() {}
+  // Invoked when media is playing or paused.  |id| is unique per player and per
+  // RenderFrameHost.  There may be multiple players within a RenderFrameHost
+  // and subsequently within a WebContents.  MediaStartedPlaying() will always
+  // be followed by MediaPaused() after player teardown.
+  using MediaPlayerId = std::pair<RenderFrameHost*, int64_t>;
+  virtual void MediaStartedPlaying(const MediaPlayerId& id) {}

[ncarter (slow), 2015/12/02 17:49:31]

If a frame is detached while media is playing, what are the semantics around
these RenderFrameHost* pointers getting invalidated? We should not tolerate
these pointers outlasting the RFH, even if they're only used as IDs.

[DaleCurtis, 2015/12/02 19:55:46]

We could use uintptr_t if we want too; I don't think anyone needs it as a
RenderFrameHost, I just changed it to avoid extraneous casting. Also, a
MediaPaused() event will be delivered during OnRenderFrameDeleted, during which
I would expect callers to release any ids they are holding. Are you okay with
upping the documentation here or would you prefer a more technical solution?

[ncarter (slow), 2015/12/05 00:43:33]

The uintptr_t vs. RFH* distinction wasn't what I was getting at.

The problem with using pointers as ids is that memory addresses get reused in a
way something like a process_id/routing_id pair doesn't. So if you use a pointer
(even opaquely via something like uintptr_t) as an ID, you need to make sure
that all references to that ID are cleaned up before the free() happens.

Your existing implementations of these observer interfaces are fine, so that
might be good enough. When I first saw this I didn't realize that "Paused" meant
that observers should forget about the |id| -- StoppedPlaying might be a better
name for this method?

[DaleCurtis, 2015/12/05 01:18:02]

Sounds like a good idea. It doesn't technically mean that, but from the POV of a
WCO it should. I've also added a comment that observers must release all copies
of |id| when MediaStoppedPlaying is received.

+  virtual void MediaPaused(const MediaPlayerId& id) {}
 
   // Invoked when media session has changed its state.
   virtual void MediaSessionStateChanged(bool is_controllable,
                                         bool is_suspended) {}
 
   // Invoked when the renderer process changes the page scale factor.
   virtual void OnPageScaleFactorChanged(float page_scale_factor) {}
 
   // Invoked if an IPC message is coming from a specific RenderFrameHost.
   virtual bool OnMessageReceived(const IPC::Message& message,
@@ skipping 32 matching lines @@
   void ResetWebContents();
 
   WebContentsImpl* web_contents_;
 
   DISALLOW_COPY_AND_ASSIGN(WebContentsObserver);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_PUBLIC_BROWSER_WEB_CONTENTS_OBSERVER_H_