Refactor media out of WebContentsImpl to MediaWebContentsObserver.

Refactor media out of WebContentsImpl to MediaWebContentsObserver.

Pulls out the power save blocker management into the existing media
WebContentsObserver in preparation for sharing the desktop and
Android code paths for media playback.

MediaWebContentsObserver will become a MediaObserver in the future
and pass back playback control messages for both the android and
desktop implementations.

This is mostly just a cleanup without any functionality change,
except for:
- A fix to SiteEngagementHelper which was assuming there was
only one player sending notifications per WebContents.
- A change to the WebContentsObserver API to make the above
clearer to future implementors.
- A change to when the WCO API is called to make sure pause only
comes after play for implementor sanity. Previously a pause may
come during player destruction as well, even if a play was never
seen.

It gets a little weird extracting this functionality from the
WebContentsImpl implementation since it is still responsible for
vending play/pause observer notifications. Suggestions welcome!

BUG=529887
TEST=existing unit tests still pass.

Committed: https://crrev.com/88c24007f6dc13d7e9e6b249e734ba5b668e8023
Cr-Commit-Position: refs/heads/master@{#363906}

[DaleCurtis, 2015-12-02 01:37:18]

Description was changed from

==========
Refactor media out of WebContentsImpl to MediaWebContentsObserver.

Pulls out the power save blocker management into the existing media
WebContentsObserver in preparation for sharing the desktop and
Android code paths.

BUG=
==========

to

==========
Refactor media out of WebContentsImpl to MediaWebContentsObserver.

Pulls out the power save blocker management into the existing media
WebContentsObserver in preparation for sharing the desktop and
Android code paths for media playback.

MediaWebContentsObserver will become a MediaObserver in the future
and pass back playback control messages for both the android and
desktop implementations.

This is mostly just a cleanup without any functionality change,
except for:
- A fix to SiteEngagementHelper which was assuming there was
only one player sending notifications per WebContents.
- A change to the WebContentsObserver API to make the above
clearer to future implementors.
- A change to when the WCO API is called to make sure pause only
comes after play for implementor sanity. Previously a pause may
come during player destruction as well, even if a play was never
seen.

It gets a little weird extracting this functionality from the
WebContentsImpl implementation since it is still responsible for
vending play/pause observer notifications. Suggestions welcome!

BUG=529887
TEST=existing unit tests still pass.
==========

[DaleCurtis, 2015-12-02 01:37:19]

dalecurtis@chromium.org changed reviewers:
+ halliwell@chromium.org, nick@chromium.org, raymes@chromium.org

[DaleCurtis, 2015-12-02 01:38:13]

raymes: chrome/browser/engagement
halliwell: chromecast
ncarter: content/

cc:mlamouri, qinmin as an FYI in case they want to weigh in.

[halliwell, 2015-12-02 01:41:40]

On 2015/12/02 01:38:13, DaleCurtis wrote:
> raymes: chrome/browser/engagement
> halliwell: chromecast
> ncarter: content/
> 
> cc:mlamouri, qinmin as an FYI in case they want to weigh in.

chromecast/ lgtm

[raymes, 2015-12-02 02:01:18]

raymes@chromium.org changed reviewers:
+ calamity@chromium.org

[raymes, 2015-12-02 02:01:18]

I defer to calamity for site engagement stuff (I should probably be removed from
the OWNERS there)

[calamity, 2015-12-02 03:09:45]

site_engagement lgtm.

Thanks for fixing the issue for us!

+cc dominickn FYI.

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

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:68: render_frame_host);
If you use IPC_BEGIN_MESSAGE_MAP_WITH_PARAM, you can cause the RFH* to be passed
into the message handler methods, and get rid of render_frame_message_source_.
There's already an example in this file. 

IPC_BEGIN_MESSAGE_MAP_WITH_PARAM was added specifically for this use case.

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
content/browser/media/media_web_contents_observer.h:116: WebContentsImpl*
web_contents_;
WCO implementations typically don't need to cache a pointer to the WebContents;
they can use the web_contents() accessor in WebContentsObserver.

https://codereview.chromium.org/1478643002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1478643002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4660:
FOR_EACH_OBSERVER(WebContentsObserver, observers_, MediaStartedPlaying(id));
This FOR_EACH_OBSERVER will happen in the context of the OnMessageReceived
observerlist walk? So we have two observer-list walks on the stack -- which I
think is actually okay, I guess, since message filtering is kind of a special
case; we stop after handled becomes true.

https://codereview.chromium.org/1478643002/diff/40001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1478643002/diff/40001/content/public/browser/...
content/public/browser/web_contents_observer.h:432: virtual void
MediaStartedPlaying(const MediaPlayerId& id) {}
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]

(Just comments)

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
content/browser/media/media_web_contents_observer.h:116: WebContentsImpl*
web_contents_;
On 2015/12/02 17:49:31, ncarter wrote:
> WCO implementations typically don't need to cache a pointer to the
WebContents;
> they can use the web_contents() accessor in WebContentsObserver.

This is necessary because MediaStartedPlaying/MediaPaused are only on the WCI
and not the WC interface.

https://codereview.chromium.org/1478643002/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1478643002/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:4660:
FOR_EACH_OBSERVER(WebContentsObserver, observers_, MediaStartedPlaying(id));
On 2015/12/02 17:49:31, ncarter wrote:
> This FOR_EACH_OBSERVER will happen in the context of the OnMessageReceived
> observerlist walk? So we have two observer-list walks on the stack -- which I
> think is actually okay, I guess, since message filtering is kind of a special
> case; we stop after handled becomes true.

Correct, which is also the existing behavior.

https://codereview.chromium.org/1478643002/diff/40001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1478643002/diff/40001/content/public/browser/...
content/public/browser/web_contents_observer.h:432: virtual void
MediaStartedPlaying(const MediaPlayerId& id) {}
On 2015/12/02 17:49:31, ncarter wrote:
> 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.

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?

[qinmin, 2015-12-02 21:54:57]

qinmin@chromium.org changed reviewers:
+ qinmin@chromium.org

[qinmin, 2015-12-02 21:54:58]

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
content/browser/media/media_web_contents_observer.h:116: WebContentsImpl*
web_contents_;
On 2015/12/02 19:55:46, DaleCurtis wrote:
> On 2015/12/02 17:49:31, ncarter wrote:
> > WCO implementations typically don't need to cache a pointer to the
> WebContents;
> > they can use the web_contents() accessor in WebContentsObserver.
> 
> This is necessary because MediaStartedPlaying/MediaPaused are only on the WCI
> and not the WC interface.

we can do a cast to WCI when MediaStartedPlaying/MediaPaused is called, right?

[DaleCurtis, 2015-12-03 01:35:08]

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
content/browser/media/media_web_contents_observer.h:116: WebContentsImpl*
web_contents_;
On 2015/12/02 21:54:58, qinmin wrote:
> On 2015/12/02 19:55:46, DaleCurtis wrote:
> > On 2015/12/02 17:49:31, ncarter wrote:
> > > WCO implementations typically don't need to cache a pointer to the
> > WebContents;
> > > they can use the web_contents() accessor in WebContentsObserver.
> > 
> > This is necessary because MediaStartedPlaying/MediaPaused are only on the
WCI
> > and not the WC interface.
> 
> we can do a cast to WCI when MediaStartedPlaying/MediaPaused is called, right?

I forget if that's allowed here or if there are fakes/mocks/other injectables
which might be present.

[qinmin, 2015-12-03 15:46:31]

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
content/browser/media/media_web_contents_observer.h:116: WebContentsImpl*
web_contents_;
On 2015/12/03 01:35:08, DaleCurtis wrote:
> On 2015/12/02 21:54:58, qinmin wrote:
> > On 2015/12/02 19:55:46, DaleCurtis wrote:
> > > On 2015/12/02 17:49:31, ncarter wrote:
> > > > WCO implementations typically don't need to cache a pointer to the
> > > WebContents;
> > > > they can use the web_contents() accessor in WebContentsObserver.
> > > 
> > > This is necessary because MediaStartedPlaying/MediaPaused are only on the
> WCI
> > > and not the WC interface.
> > 
> > we can do a cast to WCI when MediaStartedPlaying/MediaPaused is called,
right?
> 
> I forget if that's allowed here or if there are fakes/mocks/other injectables
> which might be present. 

see example here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/de...

[DaleCurtis, 2015-12-04 01:35:12]

Patchset #4 (id:60001) has been deleted

[DaleCurtis, 2015-12-04 01:36:28]

Nick, thanks for the sanity test check, it found a bug with duplicate
notification delivery -- fixed! I believe this covers all the changes we
discussed, let me know if I've missed anything!

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.cc (right):

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
content/browser/media/media_web_contents_observer.cc:68: render_frame_host);
On 2015/12/02 17:49:31, ncarter wrote:
> If you use IPC_BEGIN_MESSAGE_MAP_WITH_PARAM, you can cause the RFH* to be
passed
> into the message handler methods, and get rid of render_frame_message_source_.
> There's already an example in this file. 
> 
> IPC_BEGIN_MESSAGE_MAP_WITH_PARAM was added specifically for this use case.

Done.

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
content/browser/media/media_web_contents_observer.h:116: WebContentsImpl*
web_contents_;
On 2015/12/03 15:46:31, qinmin wrote:
> On 2015/12/03 01:35:08, DaleCurtis wrote:
> > On 2015/12/02 21:54:58, qinmin wrote:
> > > On 2015/12/02 19:55:46, DaleCurtis wrote:
> > > > On 2015/12/02 17:49:31, ncarter wrote:
> > > > > WCO implementations typically don't need to cache a pointer to the
> > > > WebContents;
> > > > > they can use the web_contents() accessor in WebContentsObserver.
> > > > 
> > > > This is necessary because MediaStartedPlaying/MediaPaused are only on
the
> > WCI
> > > > and not the WC interface.
> > > 
> > > we can do a cast to WCI when MediaStartedPlaying/MediaPaused is called,
> right?
> > 
> > I forget if that's allowed here or if there are fakes/mocks/other
injectables
> > which might be present. 
> 
> see example here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/de...

Thanks! Done.

[DaleCurtis, 2015-12-04 01:36:36]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-04 01:41:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1478643002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1478643002/80001

[qinmin, 2015-12-04 19:12:03]

lgtm

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

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
File content/browser/media/media_web_contents_observer.h (right):

https://codereview.chromium.org/1478643002/diff/40001/content/browser/media/m...
content/browser/media/media_web_contents_observer.h:116: WebContentsImpl*
web_contents_;
On 2015/12/04 01:36:27, DaleCurtis wrote:
> On 2015/12/03 15:46:31, qinmin wrote:
> > On 2015/12/03 01:35:08, DaleCurtis wrote:
> > > On 2015/12/02 21:54:58, qinmin wrote:
> > > > On 2015/12/02 19:55:46, DaleCurtis wrote:
> > > > > On 2015/12/02 17:49:31, ncarter wrote:
> > > > > > WCO implementations typically don't need to cache a pointer to the
> > > > > WebContents;
> > > > > > they can use the web_contents() accessor in WebContentsObserver.
> > > > > 
> > > > > This is necessary because MediaStartedPlaying/MediaPaused are only on
> the
> > > WCI
> > > > > and not the WC interface.
> > > > 
> > > > we can do a cast to WCI when MediaStartedPlaying/MediaPaused is called,
> > right?
> > > 
> > > I forget if that's allowed here or if there are fakes/mocks/other
> injectables
> > > which might be present. 
> > 
> > see example here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/de...
> 
> Thanks! Done.

To confirm: inside of the content/ implementation, it's always okay to cast from
an interface class to an Impl class, except for RenderProcessHostImpl.
WebContents to WebContentsImpl is especially okay.

https://www.chromium.org/developers/content-module/content-api mentions this
briefly.

https://codereview.chromium.org/1478643002/diff/40001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1478643002/diff/40001/content/public/browser/...
content/public/browser/web_contents_observer.h:432: virtual void
MediaStartedPlaying(const MediaPlayerId& id) {}
On 2015/12/02 19:55:46, DaleCurtis wrote:
> On 2015/12/02 17:49:31, ncarter wrote:
> > 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.
> 
> 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?

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?

https://codereview.chromium.org/1478643002/diff/100001/content/test/web_conte...
File content/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/1478643002/diff/100001/content/test/web_conte...
content/test/web_contents_observer_sanity_checker.cc:83:
CHECK(active_media_players_.empty());
This CHECK needs to depend on |render_frame_host|, since there's nothing
stopping other existing frames from playing content.

You can repro this easily: have a page with an iframe. Have the main document
play media, and meanwhile remove the iframe from the document.

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

https://codereview.chromium.org/1478643002/diff/40001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1478643002/diff/40001/content/public/browser/...
content/public/browser/web_contents_observer.h:432: virtual void
MediaStartedPlaying(const MediaPlayerId& id) {}
On 2015/12/05 00:43:33, ncarter wrote:
> On 2015/12/02 19:55:46, DaleCurtis wrote:
> > On 2015/12/02 17:49:31, ncarter wrote:
> > > 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.
> > 
> > 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?
> 
> 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?

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.

https://codereview.chromium.org/1478643002/diff/100001/content/test/web_conte...
File content/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/1478643002/diff/100001/content/test/web_conte...
content/test/web_contents_observer_sanity_checker.cc:83:
CHECK(active_media_players_.empty());
On 2015/12/05 00:43:33, ncarter wrote:
> This CHECK needs to depend on |render_frame_host|, since there's nothing
> stopping other existing frames from playing content.
> 
> You can repro this easily: have a page with an iframe. Have the main document
> play media, and meanwhile remove the iframe from the document.

Whoops, yeah, done.

[DaleCurtis, 2015-12-05 01:26:31]

Patchset #6 (id:120001) has been deleted

[ncarter (slow), 2015-12-07 18:33:42]

lgtm, thanks for all the fixes!

[DaleCurtis, 2015-12-08 21:23:22]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-12-08 21:23:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
halliwell@chromium.org, calamity@chromium.org, qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/1478643002/#ps140001
(title: "Rename. Rebase. Fix.")

[commit-bot: I haz the power, 2015-12-08 21:25:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1478643002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1478643002/140001

[commit-bot: I haz the power, 2015-12-08 21:49:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 21:49:16]

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

[DaleCurtis, 2015-12-08 22:01:07]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-12-08 22:01:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
halliwell@chromium.org, nick@chromium.org, calamity@chromium.org,
qinmin@chromium.org
Link to the patchset: https://codereview.chromium.org/1478643002/#ps160001
(title: "Fix namespace for cast.")

[commit-bot: I haz the power, 2015-12-08 22:06:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1478643002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1478643002/160001

[commit-bot: I haz the power, 2015-12-09 02:11:26]

Description was changed from

==========
Refactor media out of WebContentsImpl to MediaWebContentsObserver.

Pulls out the power save blocker management into the existing media
WebContentsObserver in preparation for sharing the desktop and
Android code paths for media playback.

MediaWebContentsObserver will become a MediaObserver in the future
and pass back playback control messages for both the android and
desktop implementations.

This is mostly just a cleanup without any functionality change,
except for:
- A fix to SiteEngagementHelper which was assuming there was
only one player sending notifications per WebContents.
- A change to the WebContentsObserver API to make the above
clearer to future implementors.
- A change to when the WCO API is called to make sure pause only
comes after play for implementor sanity. Previously a pause may
come during player destruction as well, even if a play was never
seen.

It gets a little weird extracting this functionality from the
WebContentsImpl implementation since it is still responsible for
vending play/pause observer notifications. Suggestions welcome!

BUG=529887
TEST=existing unit tests still pass.
==========

to

==========
Refactor media out of WebContentsImpl to MediaWebContentsObserver.

Pulls out the power save blocker management into the existing media
WebContentsObserver in preparation for sharing the desktop and
Android code paths for media playback.

MediaWebContentsObserver will become a MediaObserver in the future
and pass back playback control messages for both the android and
desktop implementations.

This is mostly just a cleanup without any functionality change,
except for:
- A fix to SiteEngagementHelper which was assuming there was
only one player sending notifications per WebContents.
- A change to the WebContentsObserver API to make the above
clearer to future implementors.
- A change to when the WCO API is called to make sure pause only
comes after play for implementor sanity. Previously a pause may
come during player destruction as well, even if a play was never
seen.

It gets a little weird extracting this functionality from the
WebContentsImpl implementation since it is still responsible for
vending play/pause observer notifications. Suggestions welcome!

BUG=529887
TEST=existing unit tests still pass.
==========

[commit-bot: I haz the power, 2015-12-09 02:11:28]

Committed patchset #7 (id:160001)

[commit-bot: I haz the power, 2015-12-09 02:12:18]

Description was changed from

==========
Refactor media out of WebContentsImpl to MediaWebContentsObserver.

Pulls out the power save blocker management into the existing media
WebContentsObserver in preparation for sharing the desktop and
Android code paths for media playback.

MediaWebContentsObserver will become a MediaObserver in the future
and pass back playback control messages for both the android and
desktop implementations.

This is mostly just a cleanup without any functionality change,
except for:
- A fix to SiteEngagementHelper which was assuming there was
only one player sending notifications per WebContents.
- A change to the WebContentsObserver API to make the above
clearer to future implementors.
- A change to when the WCO API is called to make sure pause only
comes after play for implementor sanity. Previously a pause may
come during player destruction as well, even if a play was never
seen.

It gets a little weird extracting this functionality from the
WebContentsImpl implementation since it is still responsible for
vending play/pause observer notifications. Suggestions welcome!

BUG=529887
TEST=existing unit tests still pass.
==========

to

==========
Refactor media out of WebContentsImpl to MediaWebContentsObserver.

Pulls out the power save blocker management into the existing media
WebContentsObserver in preparation for sharing the desktop and
Android code paths for media playback.

MediaWebContentsObserver will become a MediaObserver in the future
and pass back playback control messages for both the android and
desktop implementations.

This is mostly just a cleanup without any functionality change,
except for:
- A fix to SiteEngagementHelper which was assuming there was
only one player sending notifications per WebContents.
- A change to the WebContentsObserver API to make the above
clearer to future implementors.
- A change to when the WCO API is called to make sure pause only
comes after play for implementor sanity. Previously a pause may
come during player destruction as well, even if a play was never
seen.

It gets a little weird extracting this functionality from the
WebContentsImpl implementation since it is still responsible for
vending play/pause observer notifications. Suggestions welcome!

BUG=529887
TEST=existing unit tests still pass.

Committed: https://crrev.com/88c24007f6dc13d7e9e6b249e734ba5b668e8023
Cr-Commit-Position: refs/heads/master@{#363906}
==========

[commit-bot: I haz the power, 2015-12-09 02:12:19]

Patchset 7 (id:??) landed as
https://crrev.com/88c24007f6dc13d7e9e6b249e734ba5b668e8023
Cr-Commit-Position: refs/heads/master@{#363906}