Make MediaSession available on Desktop

Make MediaSession available on Desktop

This CL moves BrowserMediaSessionManager, MediaSessionServiceImpl
and MediaMetadataSanitizer to content/browser/media/session thus
make the MediaSession API available on Desktop.

BUG=649630
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/d84e7287c869e3b40177a3c39aef6ad11b0092d6
Cr-Commit-Position: refs/heads/master@{#423927}

[Zhiqiang Zhang (Slow), 2016-10-06 14:42:15]

Description was changed from

==========
Make MediaSession available on Desktop

BUG=649630
==========

to

==========
Make MediaSession available on Desktop

BUG=649630
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Zhiqiang Zhang (Slow), 2016-10-06 14:55:21]

zqzhang@chromium.org changed reviewers:
+ avayvod@chromium.org, mlamouri@chromium.org, xhwang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-06 14:55:22]

+avayvod/mlamouri for MediaSession review
+xhwang for general media review

I wish I could move media_metadata.h and the mojo typemap/StructTraits from
content/public/common or content/common to content/browser/media/session, but
since the generated chromium mojo bindings is shared by content/renderer and
content/browser, it may break the dependency rules, so I didn't do it.

[Zhiqiang Zhang (Slow), 2016-10-06 14:57:10]

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

[Zhiqiang Zhang (Slow), 2016-10-06 14:57:10]

+creis to approve BUILD.gn changes

[Zhiqiang Zhang (Slow), 2016-10-06 16:51:51]

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

[commit-bot: I haz the power, 2016-10-06 16:52:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 18:57:48]

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

[commit-bot: I haz the power, 2016-10-06 18:57:49]

Dry run: This issue passed the CQ dry run.

[whywhat, 2016-10-06 20:42:44]

lgtm

Maybe make your CL description more descriptive :)
Like say that you're moving media session impl from media/android to
media/session and thus enable it on non-Android platforms.

[Charlie Reis, 2016-10-06 21:53:31]

https://codereview.chromium.org/2399023002/diff/20001/content/browser/media/s...
File content/browser/media/session/media_metadata_sanitizer.h (right):

https://codereview.chromium.org/2399023002/diff/20001/content/browser/media/s...
content/browser/media/session/media_metadata_sanitizer.h:15: static bool
CheckSanity(const MediaMetadata& metadata);
This looks like it's called from
content/renderer/media/android/renderer_media_session_manager.cc (which can't
depend on content/browser), but I don't see a change to that file in this CL. 
The try jobs surprisingly seem happy, though.  What am I missing?  :)

[Zhiqiang Zhang (Slow), 2016-10-06 22:33:52]

https://codereview.chromium.org/2399023002/diff/20001/content/browser/media/s...
File content/browser/media/session/media_metadata_sanitizer.h (right):

https://codereview.chromium.org/2399023002/diff/20001/content/browser/media/s...
content/browser/media/session/media_metadata_sanitizer.h:15: static bool
CheckSanity(const MediaMetadata& metadata);
On 2016/10/06 21:53:30, Charlie Reis wrote:
> This looks like it's called from
> content/renderer/media/android/renderer_media_session_manager.cc (which can't
> depend on content/browser), but I don't see a change to that file in this CL. 
> The try jobs surprisingly seem happy, though.  What am I missing?  :)

That file was already removed in a previous patch
https://codereview.chromium.org/2367393002

But codesearch haven't updated yet ;)

[Charlie Reis, 2016-10-06 22:37:50]

BUILD.gn files LGTM.  (Be sure to get an owner to approve the rest, since my
LGTM technically covers them but I haven't taken a look.)

https://codereview.chromium.org/2399023002/diff/20001/content/browser/media/s...
File content/browser/media/session/media_metadata_sanitizer.h (right):

https://codereview.chromium.org/2399023002/diff/20001/content/browser/media/s...
content/browser/media/session/media_metadata_sanitizer.h:15: static bool
CheckSanity(const MediaMetadata& metadata);
On 2016/10/06 22:33:52, Zhiqiang Zhang wrote:
> On 2016/10/06 21:53:30, Charlie Reis wrote:
> > This looks like it's called from
> > content/renderer/media/android/renderer_media_session_manager.cc (which
can't
> > depend on content/browser), but I don't see a change to that file in this
CL. 
> > The try jobs surprisingly seem happy, though.  What am I missing?  :)
> 
> That file was already removed in a previous patch
> https://codereview.chromium.org/2367393002
> 
> But codesearch haven't updated yet ;)

Cool, thanks!

[Zhiqiang Zhang (Slow), 2016-10-07 09:40:47]

Description was changed from

==========
Make MediaSession available on Desktop

BUG=649630
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make MediaSession available on Desktop

This CL moves BrowserMediaSessionManager, MediaSessionServiceImpl
and MediaMetadataSanitizer to content/browser/media/session thus
make the MediaSession API available on Desktop.

BUG=649630
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[mlamouri (slow - plz ping), 2016-10-07 12:58:09]

I think we should remove the browser_media_session_manager files and move its
logic inside the media session service instead. It should be cleaner :)

[Zhiqiang Zhang (Slow), 2016-10-07 15:00:47]

On 2016/10/07 12:58:09, mlamouri wrote:
> I think we should remove the browser_media_session_manager files and move its
> logic inside the media session service instead. It should be cleaner :)

Done.

[Zhiqiang Zhang (Slow), 2016-10-07 15:00:58]

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

[commit-bot: I haz the power, 2016-10-07 15:01:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhiqiang Zhang (Slow), 2016-10-07 15:06:18]

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

[Zhiqiang Zhang (Slow), 2016-10-07 15:06:18]

xhwang/sandersd, please review the media/ changes.

[commit-bot: I haz the power, 2016-10-07 15:50:03]

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

[commit-bot: I haz the power, 2016-10-07 15:50:04]

Dry run: This issue passed the CQ dry run.

[xhwang, 2016-10-07 19:05:06]

Thanks for moving media session files to */media/session folder.

media/ OWNER LGTM

[Zhiqiang Zhang (Slow), 2016-10-07 19:12:18]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-07 19:12:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2399023002/#ps40001
(title: "merge BrowserMediaSessionManager into MediaSessionServiceImpl,
temporarily removing BrowserTests")

[commit-bot: I haz the power, 2016-10-07 19:12:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhiqiang Zhang (Slow), 2016-10-07 19:14:06]

On 2016/10/07 19:05:06, xhwang wrote:
> Thanks for moving media session files to */media/session folder.
> 
> media/ OWNER LGTM

True, this will definitely speed up our review process and release media/
owners' burden.
Yay!

[commit-bot: I haz the power, 2016-10-07 19:22:30]

Description was changed from

==========
Make MediaSession available on Desktop

This CL moves BrowserMediaSessionManager, MediaSessionServiceImpl
and MediaMetadataSanitizer to content/browser/media/session thus
make the MediaSession API available on Desktop.

BUG=649630
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make MediaSession available on Desktop

This CL moves BrowserMediaSessionManager, MediaSessionServiceImpl
and MediaMetadataSanitizer to content/browser/media/session thus
make the MediaSession API available on Desktop.

BUG=649630
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-10-07 19:22:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-07 19:24:52]

Description was changed from

==========
Make MediaSession available on Desktop

This CL moves BrowserMediaSessionManager, MediaSessionServiceImpl
and MediaMetadataSanitizer to content/browser/media/session thus
make the MediaSession API available on Desktop.

BUG=649630
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make MediaSession available on Desktop

This CL moves BrowserMediaSessionManager, MediaSessionServiceImpl
and MediaMetadataSanitizer to content/browser/media/session thus
make the MediaSession API available on Desktop.

BUG=649630
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/d84e7287c869e3b40177a3c39aef6ad11b0092d6
Cr-Commit-Position: refs/heads/master@{#423927}
==========

[commit-bot: I haz the power, 2016-10-07 19:24:54]

Patchset 3 (id:??) landed as
https://crrev.com/d84e7287c869e3b40177a3c39aef6ad11b0092d6
Cr-Commit-Position: refs/heads/master@{#423927}

[sandersd (OOO until July 31), 2016-10-07 22:33:48]

https://codereview.chromium.org/2399023002/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session_service_impl.cc (right):

https://codereview.chromium.org/2399023002/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_service_impl.cc:42:
NOTIMPLEMENTED();
?

[Zhiqiang Zhang (Slow), 2016-10-07 22:37:03]

https://codereview.chromium.org/2399023002/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session_service_impl.cc (right):

https://codereview.chromium.org/2399023002/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_service_impl.cc:42:
NOTIMPLEMENTED();
On 2016/10/07 22:33:48, sandersd wrote:
> ?

Yes, this `NOTIMPLEMENTED` was from BrowserMediaSessionManager, which is now
merged into MediaSessionServiceImpl