Implement MediaSession (metadata) per frame [NOT READY, HAS DEPENDENCY]

Implement MediaSession (metadata) per frame

This CL makes MediaSession as an extension to
window.navigator. There is now a MediaSession instance associated
with each frame. This is part of a greater effort to keep
blink::MediaSession in sync with spec.

Existing MediaSession constructors and activate/deactivate is
preserved. They will be further removed when we have made final
decisions on the spec.

The LayoutTests are out-dated but currently it is not possible to
update them since the implementation is Android-only. See
https://crbug.com/646490

Spec explainer:
https://github.com/WICG/mediasession/blob/master/explainer.md
Feature status:
https://www.chromestatus.com/features/5639924124483584

BUG=643190

[Zhiqiang Zhang (Slow), 2016-09-13 15:39:22]

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

[Zhiqiang Zhang (Slow), 2016-09-13 15:39:22]

This is an implementation for MediaSession per frame.
* Adding it as a partial interface for Navigator
* Having a MediaSession for each LocalFrame
* Only top-level frame metadata is propagated to the browser process.

I'm still keeping some old code
* It's still user-constructible
* activate()/deactivate() is still there
Should I remove them? or in later patches?

[Zhiqiang Zhang (Slow), 2016-09-13 15:40:06]

Description was changed from

==========
Implement MediaSession (metadata) per frame

WIP, not ready

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

to

==========
Implement MediaSession (metadata) per frame

Spec explainer:
https://github.com/WICG/mediasession/blob/master/explainer.md

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

[whywhat, 2016-09-13 16:02:24]

On 2016/09/13 at 15:39:22, zqzhang wrote:
> This is an implementation for MediaSession per frame.
> * Adding it as a partial interface for Navigator
> * Having a MediaSession for each LocalFrame
> * Only top-level frame metadata is propagated to the browser process.
> 
> I'm still keeping some old code
> * It's still user-constructible
> * activate()/deactivate() is still there
> Should I remove them? or in later patches?

I think this is fine if you do in the follow up patches (just don't forget to do
so before shipping the API).
It is behind the flag right now so should be a big deal.

Do you have a tracking bug for this CL?

[Zhiqiang Zhang (Slow), 2016-09-13 16:05:12]

Description was changed from

==========
Implement MediaSession (metadata) per frame

Spec explainer:
https://github.com/WICG/mediasession/blob/master/explainer.md

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

to

==========
Implement MediaSession (metadata) per frame

Spec explainer:
https://github.com/WICG/mediasession/blob/master/explainer.md

BUG=643190
==========

[Zhiqiang Zhang (Slow), 2016-09-13 16:05:33]

On 2016/09/13 16:02:24, whywhat wrote:
> On 2016/09/13 at 15:39:22, zqzhang wrote:
> > This is an implementation for MediaSession per frame.
> > * Adding it as a partial interface for Navigator
> > * Having a MediaSession for each LocalFrame
> > * Only top-level frame metadata is propagated to the browser process.
> > 
> > I'm still keeping some old code
> > * It's still user-constructible
> > * activate()/deactivate() is still there
> > Should I remove them? or in later patches?
> 
> I think this is fine if you do in the follow up patches (just don't forget to
do
> so before shipping the API).
> It is behind the flag right now so should be a big deal.
> 
> Do you have a tracking bug for this CL?

Yes, updated in the CL description.

[whywhat, 2016-09-13 16:23:26]

lgtm with nits

https://codereview.chromium.org/2252783004/diff/120001/content/browser/androi...
File content/browser/android/web_contents_observer_proxy.h (right):

https://codereview.chromium.org/2252783004/diff/120001/content/browser/androi...
content/browser/android/web_contents_observer_proxy.h:12: #include
"base/optional.h"
nit: looks like an accidental change; should be in the file below?

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
File content/browser/media/android/browser_media_session_manager.cc (right):

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
content/browser/media/android/browser_media_session_manager.cc:26: return
media_metadata_;
nit: this is allowed to be inlined in the header IIRC

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
content/browser/media/android/browser_media_session_manager.cc:41: const
base::Optional<MediaMetadata>& insecure_metadata) {
nit: the declaration of the method has this named as simply |metadata|
why is it |insecure|? update the header file and leave a comment explaining the
insecurity of the metadata.

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
content/browser/media/android/browser_media_session_manager.cc:44:
render_frame_host_->GetProcess()->ShutdownForBadMessage();
This is a preventative measure against p0wned renderers, right? Maybe leave a
comment.

In general case, we should prevent this from happening in the renderer process,
not kill the renderer whenever an iframe sets metadata.

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
content/browser/media/android/browser_media_session_manager.cc:51:
!MediaMetadataSanitizer::CheckSanity(insecure_metadata.value())) {
nit: This Sanitizer looks a bit odd: it checks the strings are not bigger than
the max IPC string length - I'd say IPC probably checks that already?

It has Sanitize methods which are never used. Any plans for those?

P.S. I understand you added the Sanitizer after palmer/dcheng reviews so there
must be sense behind it :) I'm just trying to remember.

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
File content/browser/media/android/browser_media_session_manager.h (right):

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
content/browser/media/android/browser_media_session_manager.h:8: #include
<memory>
nit: seems like this is unused

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
content/browser/media/android/browser_media_session_manager.h:13: #include
"content/public/common/media_metadata.h"
nit: is this needed for Optional? I thought as long as the
BrowserMediaSessionManager dtor is not inlined, the compiler will only need the
dtor of Optional<MediaMetadata> in the .cc file.

https://codereview.chromium.org/2252783004/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2252783004/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3819: #if defined(OS_ANDROID)
does it have to be android only? I thought we have MediaSession on desktop
behind the flag.

https://codereview.chromium.org/2252783004/diff/120001/content/renderer/media...
File content/renderer/media/android/renderer_media_session_manager.cc (left):

https://codereview.chromium.org/2252783004/diff/120001/content/renderer/media...
content/renderer/media/android/renderer_media_session_manager.cc:23:
DCHECK(sessions_.empty())
why removing this? no longer true?

https://codereview.chromium.org/2252783004/diff/120001/content/renderer/media...
File content/renderer/media/android/renderer_media_session_manager.cc (right):

https://codereview.chromium.org/2252783004/diff/120001/content/renderer/media...
content/renderer/media/android/renderer_media_session_manager.cc:70: return;
print a message here too?
are we 100% sure GetWebFrame() never returns null?

https://codereview.chromium.org/2252783004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.cpp
(right):

https://codereview.chromium.org/2252783004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.cpp:1:
#include "modules/mediasession/NavigatorMediaSession.h"
nit: copyright chromium authors yadayada

https://codereview.chromium.org/2252783004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h
(right):

https://codereview.chromium.org/2252783004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h:1:
#ifndef NavigatorMediaSession_h
nit copyright owners yadayada

https://codereview.chromium.org/2252783004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl
(right):

https://codereview.chromium.org/2252783004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl:1: [
Add comments (like copyright and a link to the spec, see other idl files for
reference).

[Zhiqiang Zhang (Slow), 2016-09-13 17:15:56]

Some quick replies.

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
File content/browser/media/android/browser_media_session_manager.cc (right):

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
content/browser/media/android/browser_media_session_manager.cc:44:
render_frame_host_->GetProcess()->ShutdownForBadMessage();
On 2016/09/13 16:23:26, whywhat wrote:
> This is a preventative measure against p0wned renderers, right? Maybe leave a
> comment.
> 
> In general case, we should prevent this from happening in the renderer
process,
> not kill the renderer whenever an iframe sets metadata.

There are sanity check and sanitizing when the renderer sends the message, see
RendererMediaSessionManager::SetMetadata().

I think that is fine. Since we have sanitized the message in the renderer, so if
the browser receives a dirty metadata, it should come from an attacker. It
should be fine to shutdown renderer in this case.

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
content/browser/media/android/browser_media_session_manager.cc:51:
!MediaMetadataSanitizer::CheckSanity(insecure_metadata.value())) {
On 2016/09/13 16:23:26, whywhat wrote:
> nit: This Sanitizer looks a bit odd: it checks the strings are not bigger than
> the max IPC string length - I'd say IPC probably checks that already?
> 
> It has Sanitize methods which are never used. Any plans for those?
> 
> P.S. I understand you added the Sanitizer after palmer/dcheng reviews so there
> must be sense behind it :) I'm just trying to remember.

I'm also trying to remember but I forgot most of it. I think their major concern
was artwork, since we shouldn't allow file urls be used as artwork.

https://codereview.chromium.org/2252783004/diff/120001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2252783004/diff/120001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:3819: #if defined(OS_ANDROID)
On 2016/09/13 16:23:26, whywhat wrote:
> does it have to be android only? I thought we have MediaSession on desktop
> behind the flag.

Yes, BrowserMediaSessionManager is in content/browser/media/android/

[Zhiqiang Zhang (Slow), 2016-09-13 17:27:29]

https://codereview.chromium.org/2252783004/diff/120001/content/renderer/media...
File content/renderer/media/android/renderer_media_session_manager.cc (left):

https://codereview.chromium.org/2252783004/diff/120001/content/renderer/media...
content/renderer/media/android/renderer_media_session_manager.cc:23:
DCHECK(sessions_.empty())
On 2016/09/13 16:23:26, whywhat wrote:
> why removing this? no longer true?

It was even not true before this patch.
blink::MediaSession has a unique_ptr<> pointing at WebMediaSession.
WebMediaSession unregisters itself in the dtor.
Since blink::MediaSession is gc'ed, it is quite likely that
RenderMediaSessionManager is destroyed and WebMediaSession is still alive,
hitting this DCHECK.

To trigger it, just go to the MediaSession test page, play the audio, and
navigate to another URL.

[Zhiqiang Zhang (Slow), 2016-09-13 18:51:52]

https://codereview.chromium.org/2252783004/diff/120001/content/browser/androi...
File content/browser/android/web_contents_observer_proxy.h (right):

https://codereview.chromium.org/2252783004/diff/120001/content/browser/androi...
content/browser/android/web_contents_observer_proxy.h:12: #include
"base/optional.h"
On 2016/09/13 16:23:26, whywhat wrote:
> nit: looks like an accidental change; should be in the file below?

Done.

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
File content/browser/media/android/browser_media_session_manager.cc (right):

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
content/browser/media/android/browser_media_session_manager.cc:26: return
media_metadata_;
On 2016/09/13 16:23:26, whywhat wrote:
> nit: this is allowed to be inlined in the header IIRC

Done.

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
content/browser/media/android/browser_media_session_manager.cc:41: const
base::Optional<MediaMetadata>& insecure_metadata) {
On 2016/09/13 16:23:26, whywhat wrote:
> nit: the declaration of the method has this named as simply |metadata|
> why is it |insecure|? update the header file and leave a comment explaining
the
> insecurity of the metadata.

Done. Updated the header file.

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
File content/browser/media/android/browser_media_session_manager.h (right):

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
content/browser/media/android/browser_media_session_manager.h:8: #include
<memory>
On 2016/09/13 16:23:26, whywhat wrote:
> nit: seems like this is unused

Done.

https://codereview.chromium.org/2252783004/diff/120001/content/browser/media/...
content/browser/media/android/browser_media_session_manager.h:13: #include
"content/public/common/media_metadata.h"
On 2016/09/13 16:23:26, whywhat wrote:
> nit: is this needed for Optional? I thought as long as the
> BrowserMediaSessionManager dtor is not inlined, the compiler will only need
the
> dtor of Optional<MediaMetadata> in the .cc file.

Probably not. I got a compiler error when moving the include to .cc file.

https://codereview.chromium.org/2252783004/diff/120001/content/renderer/media...
File content/renderer/media/android/renderer_media_session_manager.cc (right):

https://codereview.chromium.org/2252783004/diff/120001/content/renderer/media...
content/renderer/media/android/renderer_media_session_manager.cc:70: return;
On 2016/09/13 16:23:26, whywhat wrote:
> print a message here too?
> are we 100% sure GetWebFrame() never returns null?

Seems not 100%. Fixed.

https://codereview.chromium.org/2252783004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.cpp
(right):

https://codereview.chromium.org/2252783004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.cpp:1:
#include "modules/mediasession/NavigatorMediaSession.h"
On 2016/09/13 16:23:26, whywhat wrote:
> nit: copyright chromium authors yadayada

Done.

https://codereview.chromium.org/2252783004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h
(right):

https://codereview.chromium.org/2252783004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h:1:
#ifndef NavigatorMediaSession_h
On 2016/09/13 16:23:26, whywhat wrote:
> nit copyright owners yadayada

Done. Thanks ;)

https://codereview.chromium.org/2252783004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl
(right):

https://codereview.chromium.org/2252783004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl:1: [
On 2016/09/13 16:23:26, whywhat wrote:
> Add comments (like copyright and a link to the spec, see other idl files for
> reference).

Done.

[Zhiqiang Zhang (Slow), 2016-09-13 19:07:00]

Description was changed from

==========
Implement MediaSession (metadata) per frame

Spec explainer:
https://github.com/WICG/mediasession/blob/master/explainer.md

BUG=643190
==========

to

==========
Implement MediaSession (metadata) per frame

This CL makes MediaSession as an extension to
window.navigator. There is now a MediaSession instance associated
with each frame.

Existing MediaSession constructors and activate/deactivate is
preserved. They will be further removed when we have made final
decisions on the spec.

Also, the tests could be outdated but since the spec and
implementation is unstable for the moment, we should update the
tests eventually when the spec and implementation are
stable (https://crbug.com/646490).

Spec explainer:
https://github.com/WICG/mediasession/blob/master/explainer.md

BUG=643190
==========

[Zhiqiang Zhang (Slow), 2016-09-13 19:19:50]

Patchset #8 (id:140001) has been deleted

[Zhiqiang Zhang (Slow), 2016-09-13 19:26:53]

Patchset #8 (id:160001) has been deleted

[Zhiqiang Zhang (Slow), 2016-09-13 19:27:29]

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

[Zhiqiang Zhang (Slow), 2016-09-13 19:27:30]

+jochen to review contents/*

[haraken, 2016-09-14 00:10:45]

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

[haraken, 2016-09-14 00:10:46]

https://codereview.chromium.org/2252783004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h
(right):

https://codereview.chromium.org/2252783004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h:17: class
NavigatorMediaSession final : public GarbageCollected<NavigatorMediaSession>,
public Supplement<Navigator>, public DOMWindowProperty {

Do you really need to make NavigatorMediaSession inherit from DOMWindowProperty?

[jochen (gone - plz use gerrit), 2016-09-14 08:45:04]

can we have tests please?

can you add a link to the intent to implement to the CL description?

[mlamouri (slow - plz ping), 2016-09-14 10:08:45]

On the content/ side, the current code is android/ specific but also strongly
linked with WMPA. I think we should make this generic given that the entire
media/session/ code isn't Android specific. We have a WebContents callback when
media session is updated so we should just trigger this using this API. Also,
ideally, the iframe/top frame decision should be left at the last minute so our
architecture doesn't depend to much on this decision.

https://codereview.chromium.org/2252783004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl
(right):

https://codereview.chromium.org/2252783004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.idl:5: //
https://github.com/WICG/mediasession/blob/master/explainer.md
Link to the spec instead. If the first draft is out on time, which should happen
I believe.

[mlamouri (slow - plz ping), 2016-09-14 10:10:33]

On 2016/09/14 at 08:45:04, jochen wrote:
> can we have tests please?
> 
> can you add a link to the intent to implement to the CL description?

This is a reboot of https://www.chromestatus.com/features/5639924124483584.
Basically, the API has a reduced focus. Do you think we should send another
intent to implemenent?

[Zhiqiang Zhang (Slow), 2016-09-14 13:59:06]

Description was changed from

==========
Implement MediaSession (metadata) per frame

This CL makes MediaSession as an extension to
window.navigator. There is now a MediaSession instance associated
with each frame.

Existing MediaSession constructors and activate/deactivate is
preserved. They will be further removed when we have made final
decisions on the spec.

Also, the tests could be outdated but since the spec and
implementation is unstable for the moment, we should update the
tests eventually when the spec and implementation are
stable (https://crbug.com/646490).

Spec explainer:
https://github.com/WICG/mediasession/blob/master/explainer.md

BUG=643190
==========

to

==========
Implement MediaSession (metadata) per frame

This CL makes MediaSession as an extension to
window.navigator. There is now a MediaSession instance associated
with each frame.

Existing MediaSession constructors and activate/deactivate is
preserved. They will be further removed when we have made final
decisions on the spec.

The LayoutTests are out-dated but currently it is not possible to update them
since the implementation is Android-only. See https://crbug.com/646490

Spec explainer:
https://github.com/WICG/mediasession/blob/master/explainer.md
Feature status:
https://www.chromestatus.com/features/5639924124483584

BUG=643190
==========

[Zhiqiang Zhang (Slow), 2016-09-14 14:07:49]

On 2016/09/14 10:10:33, mlamouri wrote:
> On 2016/09/14 at 08:45:04, jochen wrote:
> > can we have tests please?
> > 

Currently the LayoutTests cannot be updated, since the implementation is
Android-only.

> > can you add a link to the intent to implement to the CL description?
> 
> This is a reboot of https://www.chromestatus.com/features/5639924124483584.
> Basically, the API has a reduced focus. Do you think we should send another
> intent to implemenent?

Added the chromestatus link.

Also, I wrote a doc for the ongoing greater effort (this CL is the first part),
added to the CL description.

[Zhiqiang Zhang (Slow), 2016-09-14 14:10:02]

Description was changed from

==========
Implement MediaSession (metadata) per frame

This CL makes MediaSession as an extension to
window.navigator. There is now a MediaSession instance associated
with each frame.

Existing MediaSession constructors and activate/deactivate is
preserved. They will be further removed when we have made final
decisions on the spec.

The LayoutTests are out-dated but currently it is not possible to update them
since the implementation is Android-only. See https://crbug.com/646490

Spec explainer:
https://github.com/WICG/mediasession/blob/master/explainer.md
Feature status:
https://www.chromestatus.com/features/5639924124483584

BUG=643190
==========

to

==========
Implement MediaSession (metadata) per frame

This CL makes MediaSession as an extension to
window.navigator. There is now a MediaSession instance associated
with each frame. This is part of a greater effort to keep
blink::MediaSession in sync with spec.

Existing MediaSession constructors and activate/deactivate is
preserved. They will be further removed when we have made final
decisions on the spec.

The LayoutTests are out-dated but currently it is not possible to
update them since the implementation is Android-only. See
https://crbug.com/646490

Spec explainer:
https://github.com/WICG/mediasession/blob/master/explainer.md
Feature status:
https://www.chromestatus.com/features/5639924124483584

BUG=643190
==========

[Zhiqiang Zhang (Slow), 2016-09-14 14:11:31]

PTAL. Addressed reviewer's comments.

Bisides, I updated the browser tests. Changes to LayoutTests is not possible at
the moment since the implementation is Android-only.

https://codereview.chromium.org/2252783004/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h
(right):

https://codereview.chromium.org/2252783004/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/NavigatorMediaSession.h:17: class
NavigatorMediaSession final : public GarbageCollected<NavigatorMediaSession>,
public Supplement<Navigator>, public DOMWindowProperty {
On 2016/09/14 00:10:46, haraken wrote:
> 
> Do you really need to make NavigatorMediaSession inherit from
DOMWindowProperty?

Ahh, it's not needed. Thanks!

[jochen (gone - plz use gerrit), 2016-09-15 10:21:05]

does the fact that it's android only mean that we still have the old api on
desktop? I'm not sure I understand why it's not possible to update the layout
tests... we could also just skip them on platforms where they don't work?

[Zhiqiang Zhang (Slow), 2016-09-15 10:31:28]

On 2016/09/15 10:21:05, jochen wrote:
> does the fact that it's android only mean that we still have the old api on
> desktop? I'm not sure I understand why it's not possible to update the layout
> tests... we could also just skip them on platforms where they don't work?

It's in `RenderFrameImpl::createMediaSession()`, it returns a null MediaSession
for desktop.
We are going to make this available for Desktop in follow-up CL, we will update
the layout tests in that CL.
Due to other dependencies, it cannot be landed before this one.

BTW, the current layout tests for MediaSession does not make much sense, so
updating them in this CL does not have benefit. See:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/virtual/m...
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/virtual/m...
They all get "Failed to construct 'MediaSession': Missing platform
implementation."

[Zhiqiang Zhang (Slow), 2016-09-15 10:45:38]

On 2016/09/15 10:31:28, Zhiqiang Zhang wrote:
> On 2016/09/15 10:21:05, jochen wrote:
> > does the fact that it's android only mean that we still have the old api on
> > desktop? I'm not sure I understand why it's not possible to update the
layout
> > tests... we could also just skip them on platforms where they don't work?
> 
> It's in `RenderFrameImpl::createMediaSession()`, it returns a null
MediaSession
> for desktop.
> We are going to make this available for Desktop in follow-up CL, we will
update
> the layout tests in that CL.
> Due to other dependencies, it cannot be landed before this one.
> 
> BTW, the current layout tests for MediaSession does not make much sense, so
> updating them in this CL does not have benefit. See:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/virtual/m...
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/virtual/m...
> They all get "Failed to construct 'MediaSession': Missing platform
> implementation."

I discussed with Mounir, and we decided to write another CL first, which moves
all Android-only code available to desktop, and use mojo service to replace the
IPC.
So we'll come back for this CL after that.