Migrating MediaSession messages to mojo

This CL migrates MediaSession message from IPC to mojo.
Currently MediaSession message is still Android-only, but it will
be available to Desktop in a follow-up CL.

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

Committed: https://crrev.com/93b95f8bff18d5c4389abd88ca0a33426d738d10
Cr-Commit-Position: refs/heads/master@{#423554}

[Zhiqiang Zhang (Slow), 2016-09-26 13:45:36]

Description was changed from

==========
temporary


temporary


not sending message when render_frame is null


finished metadata conversion


initial mojo migration, need metadata serialization

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

to

==========
temporary


temporary


not sending message when render_frame is null


finished metadata conversion


initial mojo migration, need metadata serialization

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

[Zhiqiang Zhang (Slow), 2016-09-26 13:48:38]

Description was changed from

==========
temporary


temporary


not sending message when render_frame is null


finished metadata conversion


initial mojo migration, need metadata serialization

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

to

==========
^
==========

[mlamouri (slow - plz ping), 2016-09-26 14:02:44]

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

[mlamouri (slow - plz ping), 2016-09-26 14:02:44]

What about having the service in modules/ like permissions?

[Zhiqiang Zhang (Slow), 2016-09-26 14:45:01]

On 2016/09/26 at 14:02:44, mlamouri wrote:
> What about having the service in modules/ like permissions?

I'm actually doing this (not uploaded yet).

Eventually found the reason why I cannot bind the service. The service was
registered to RenderFrameHost, but I was trying to bind the remote service from
RenderProcess. It should be RenderFrame.

[Zhiqiang Zhang (Slow), 2016-09-26 16:34:06]

Description was changed from

==========
^
==========

to

==========
^
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Zhiqiang Zhang (Slow), 2016-09-26 16:35:42]

Description was changed from

==========
^
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
This CL migrates MediaSession message from IPC to mojo.
Currently MediaSession message is still Android-only, but it will
be available to Desktop in a follow-up CL.
==========

[Zhiqiang Zhang (Slow), 2016-09-26 16:57:43]

Description was changed from

==========
This CL migrates MediaSession message from IPC to mojo.
Currently MediaSession message is still Android-only, but it will
be available to Desktop in a follow-up CL.
==========

to

==========
This CL migrates MediaSession message from IPC to mojo.
Currently MediaSession message is still Android-only, but it will
be available to Desktop in a follow-up CL.

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

[Zhiqiang Zhang (Slow), 2016-09-26 16:58:26]

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

[Zhiqiang Zhang (Slow), 2016-09-26 16:58:26]

+avayvod/mlamouri to review media*
+tsepez to review IPC->mojo changes

[Zhiqiang Zhang (Slow), 2016-09-26 17:26:33]

Description was changed from

==========
This CL migrates MediaSession message from IPC to mojo.
Currently MediaSession message is still Android-only, but it will
be available to Desktop in a follow-up CL.

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

to

==========
This CL migrates MediaSession message from IPC to mojo.
Currently MediaSession message is still Android-only, but it will
be available to Desktop in a follow-up CL.

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

[Zhiqiang Zhang (Slow), 2016-09-26 17:26:44]

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

[commit-bot: I haz the power, 2016-09-26 17:27:24]

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-09-26 17:33:05]

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

[commit-bot: I haz the power, 2016-09-26 17:33:06]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Tom Sepez, 2016-09-26 18:40:02]

messages LGTM

[Zhiqiang Zhang (Slow), 2016-09-26 18:44:30]

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

[commit-bot: I haz the power, 2016-09-26 18:45:11]

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-09-26 22:00:32]

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

[commit-bot: I haz the power, 2016-09-26 22:00:33]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mlamouri (slow - plz ping), 2016-09-27 08:33:11]

https://codereview.chromium.org/2367393002/diff/80001/content/browser/media/a...
File content/browser/media/android/media_web_contents_observer_android.h
(right):

https://codereview.chromium.org/2367393002/diff/80001/content/browser/media/a...
content/browser/media/android/media_web_contents_observer_android.h:99: using
MediaSessionManagerMap =
How much of this can be removed?

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

https://codereview.chromium.org/2367393002/diff/80001/content/renderer/media/...
content/renderer/media/android/renderer_media_session_manager.cc:105:
session_id, ToMojoMediaMetadata(sanitized_metadata));
Could this call and the other mojo calls happen from Blink instead of having
them happen in the content/renderer/ layer?

https://codereview.chromium.org/2367393002/diff/80001/mojo/common/common_cust...
File mojo/common/common_custom_types.mojom (right):

https://codereview.chromium.org/2367393002/diff/80001/mojo/common/common_cust...
mojo/common/common_custom_types.mojom:30: struct NullableString16;
This might need to be reviewed by a mojo person. I wonder if it could be
expressed as `String16?`.

https://codereview.chromium.org/2367393002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/modules/mediasession/media_session.mojom
(right):

https://codereview.chromium.org/2367393002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:26:
Deactivate(uint64 session_id) => ();
Do we need Activate/Deactivate?

[Zhiqiang Zhang (Slow), 2016-09-29 09:32:18]

Description was changed from

==========
This CL migrates MediaSession message from IPC to mojo.
Currently MediaSession message is still Android-only, but it will
be available to Desktop in a follow-up CL.

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

to

==========
This CL migrates MediaSession message from IPC to mojo.
Currently MediaSession message is still Android-only, but it will
be available to Desktop in a follow-up CL.

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

[mlamouri (slow - plz ping), 2016-09-29 10:26:40]

You merged a lot of stuff that maybe could have been split in different CLs but
lgtm

https://codereview.chromium.org/2367393002/diff/100001/content/browser/media/...
File content/browser/media/android/media_session_service_impl.cc (right):

https://codereview.chromium.org/2367393002/diff/100001/content/browser/media/...
content/browser/media/android/media_session_service_impl.cc:23: icon.src =
GURL(mojo_icon->src);
Should you sanitize the |mojo_icon|?

https://codereview.chromium.org/2367393002/diff/100001/content/browser/media/...
File content/browser/media/android/media_web_contents_observer_android.h
(right):

https://codereview.chromium.org/2367393002/diff/100001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.h:100:
base::ScopedPtrHashMap<RenderFrameHost*,
Why are you removing the comment but not the rest?

https://codereview.chromium.org/2367393002/diff/100001/content/common/media/m...
File content/common/media/media_session_service_struct_traits.h (right):

https://codereview.chromium.org/2367393002/diff/100001/content/common/media/m...
content/common/media/media_session_service_struct_traits.h:29: 
style: one line too many?

https://codereview.chromium.org/2367393002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):

https://codereview.chromium.org/2367393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:48: //
TODO(zqzhang): do sanitization
It would be better if you could do this in this CL.

https://codereview.chromium.org/2367393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:85: // Can only
getService from a frame!!!
I think a DCHECK would be better.

[Zhiqiang Zhang (Slow), 2016-09-30 19:38:01]

https://codereview.chromium.org/2367393002/diff/100001/content/browser/media/...
File content/browser/media/android/media_session_service_impl.cc (right):

https://codereview.chromium.org/2367393002/diff/100001/content/browser/media/...
content/browser/media/android/media_session_service_impl.cc:23: icon.src =
GURL(mojo_icon->src);
On 2016/09/29 10:26:40, mlamouri wrote:
> Should you sanitize the |mojo_icon|?

Yes, I need to sanitize it. But we are now sending the struct from blink,
content::SanitizeMediaMetadata is not available there (that's why I left a TODO,
but I'll fix it in this CL).

Do you prefer to write another sanitizer on the blink, or do sanitization here?
If we choose the second one, there will no longer be ShutDownForBadMessage here.

[Zhiqiang Zhang (Slow), 2016-09-30 19:42:29]

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

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

+hubbe for a general media review

Sorry the CL is large, but we are getting rid of painful plumbing thanks to
mojo.

[Zhiqiang Zhang (Slow), 2016-09-30 19:56:16]

https://codereview.chromium.org/2367393002/diff/100001/content/browser/media/...
File content/browser/media/android/media_web_contents_observer_android.h
(right):

https://codereview.chromium.org/2367393002/diff/100001/content/browser/media/...
content/browser/media/android/media_web_contents_observer_android.h:100:
base::ScopedPtrHashMap<RenderFrameHost*,
On 2016/09/29 10:26:40, mlamouri wrote:
> Why are you removing the comment but not the rest?

Undone this change.

https://codereview.chromium.org/2367393002/diff/100001/content/common/media/m...
File content/common/media/media_session_service_struct_traits.h (right):

https://codereview.chromium.org/2367393002/diff/100001/content/common/media/m...
content/common/media/media_session_service_struct_traits.h:29: 
On 2016/09/29 10:26:40, mlamouri wrote:
> style: one line too many?

Done.

https://codereview.chromium.org/2367393002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):

https://codereview.chromium.org/2367393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:85: // Can only
getService from a frame!!!
On 2016/09/29 10:26:40, mlamouri wrote:
> I think a DCHECK would be better.

Done.

[hubbe, 2016-09-30 20:04:22]

hubbe@chromium.org changed reviewers:
+ xhwang@chromium.org

[hubbe, 2016-09-30 20:04:23]

Well, it looks ok to me, but I'm not very well versed in mojo, mediasessions or
webkit...

Xiaohan, maybe you could take a look?

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

https://codereview.chromium.org/2367393002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaArtwork.h:34: String m_src;
Please document variables.

[xhwang, 2016-09-30 23:24:58]

Do you need to update the title before committing?

I am not familiar with MediaSession. Just some general/mojo comments. Thanks!

https://codereview.chromium.org/2367393002/diff/140001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/2367393002/diff/140001/content/browser/DEPS#n...
content/browser/DEPS:74:
"+third_party/WebKit/public/platform/modules/mediasession/media_session.mojom.h",
nit: sorry for bikeshedding. I found it odd to have "mediasession" and
"media_session" at the same time :) Just FYI

https://codereview.chromium.org/2367393002/diff/140001/content/browser/media/...
File content/browser/media/android/media_session_service_impl.cc (right):

https://codereview.chromium.org/2367393002/diff/140001/content/browser/media/...
content/browser/media/android/media_session_service_impl.cc:60:
blink::mojom::MediaMetadataPtr metadata) {
Have you considered to use typemapping so that the conversion part will be
hidden from this file?

https://www.chromium.org/developers/design-documents/mojo/type-mapping

https://codereview.chromium.org/2367393002/diff/140001/content/browser/media/...
File content/browser/media/android/media_session_service_impl.h (right):

https://codereview.chromium.org/2367393002/diff/140001/content/browser/media/...
content/browser/media/android/media_session_service_impl.h:25:
MediaSessionServiceImpl(RenderFrameHost* render_frame_host);
explicit just in case?

https://codereview.chromium.org/2367393002/diff/140001/content/common/media/m...
File content/common/media/media_session_service_struct_traits.h (right):

https://codereview.chromium.org/2367393002/diff/140001/content/common/media/m...
content/common/media/media_session_service_struct_traits.h:24: static mojo::
hmm, is the complete? Is this file used anywhere?

https://codereview.chromium.org/2367393002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right):

https://codereview.chromium.org/2367393002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaSession.h:17: class
MediaMetadata;
why switch the order?

https://codereview.chromium.org/2367393002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/mediasession/media_session.mojom
(right):

https://codereview.chromium.org/2367393002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:11:
struct MediaMetadataIcon {
Please add comments to new interfaces. It's very hard for other developers not
familiar with this code (like me) to understand what is going on.

https://codereview.chromium.org/2367393002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:24:
interface MediaSessionService {
nit: Do you really need the "Service" part? We used to call mojo interface
implementations "service". Then "service" has a special meaning in the mojo
world [1]. We used to have some mojo interfaces called *Service but I don't find
that adds too much value.

[1]
https://cs.chromium.org/chromium/src/services/shell/public/cpp/service.h?rcl=...

[Zhiqiang Zhang (Slow), 2016-10-03 14:31:12]

zqzhang@chromium.org changed reviewers:
+ foolip@chromium.org
- hubbe@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-03 14:31:14]

PTAL, addressed xhwang's comments

+foolip for WebKit review.

https://codereview.chromium.org/2367393002/diff/140001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/2367393002/diff/140001/content/browser/DEPS#n...
content/browser/DEPS:74:
"+third_party/WebKit/public/platform/modules/mediasession/media_session.mojom.h",
On 2016/09/30 23:24:58, xhwang wrote:
> nit: sorry for bikeshedding. I found it odd to have "mediasession" and
> "media_session" at the same time :) Just FYI

That's a bit odd, but I'd prefer keeping it, since .mojom file names are
following chromium file name rules.

https://codereview.chromium.org/2367393002/diff/140001/content/browser/media/...
File content/browser/media/android/media_session_service_impl.cc (right):

https://codereview.chromium.org/2367393002/diff/140001/content/browser/media/...
content/browser/media/android/media_session_service_impl.cc:60:
blink::mojom::MediaMetadataPtr metadata) {
On 2016/09/30 23:24:58, xhwang wrote:
> Have you considered to use typemapping so that the conversion part will be
> hidden from this file?
> 
> https://www.chromium.org/developers/design-documents/mojo/type-mapping

Done on the chromium side. The blink-side typemaping needs some tricky changes
which will make the CL even more complex. Added a TODO.

https://codereview.chromium.org/2367393002/diff/140001/content/browser/media/...
File content/browser/media/android/media_session_service_impl.h (right):

https://codereview.chromium.org/2367393002/diff/140001/content/browser/media/...
content/browser/media/android/media_session_service_impl.h:25:
MediaSessionServiceImpl(RenderFrameHost* render_frame_host);
On 2016/09/30 23:24:58, xhwang wrote:
> explicit just in case?

Done.

https://codereview.chromium.org/2367393002/diff/140001/content/common/media/m...
File content/common/media/media_session_service_struct_traits.h (right):

https://codereview.chromium.org/2367393002/diff/140001/content/common/media/m...
content/common/media/media_session_service_struct_traits.h:24: static mojo::
On 2016/09/30 23:24:58, xhwang wrote:
> hmm, is the complete? Is this file used anywhere?

Fixed the typemap now.

https://codereview.chromium.org/2367393002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right):

https://codereview.chromium.org/2367393002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaSession.h:17: class
MediaMetadata;
On 2016/09/30 23:24:58, xhwang wrote:
> why switch the order?

Oops, accidental change. Fixed.

https://codereview.chromium.org/2367393002/diff/140001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/mediasession/media_session.mojom
(right):

https://codereview.chromium.org/2367393002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:11:
struct MediaMetadataIcon {
On 2016/09/30 23:24:58, xhwang wrote:
> Please add comments to new interfaces. It's very hard for other developers not
> familiar with this code (like me) to understand what is going on.

Done.

https://codereview.chromium.org/2367393002/diff/140001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/mediasession/media_session.mojom:24:
interface MediaSessionService {
On 2016/09/30 23:24:58, xhwang wrote:
> nit: Do you really need the "Service" part? We used to call mojo interface
> implementations "service". Then "service" has a special meaning in the mojo
> world [1]. We used to have some mojo interfaces called *Service but I don't
find
> that adds too much value.
> 
> [1]
>
https://cs.chromium.org/chromium/src/services/shell/public/cpp/service.h?rcl=...

Done. The sad thing is that we have too many classes named "MediaSession" which
I was trying to avoid.

[Zhiqiang Zhang (Slow), 2016-10-03 15:46:55]

Patchset #9 (id:160001) has been deleted

[foolip, 2016-10-03 16:26:35]

third_party/WebKit lgtm % nits

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

https://codereview.chromium.org/2367393002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaSession.h:17: 
Cat on keyboard?

https://codereview.chromium.org/2367393002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaSession.h:38:
mojom::blink::MediaSession* getService(ScriptState*);
Document when this can return null and not?

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

https://codereview.chromium.org/2367393002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaSession.idl:11:
[CallWith=ScriptState]attribute MediaMetadata? metadata;
Space between extended attributes and "attribute"

[xhwang, 2016-10-03 16:46:03]

media OWNER lgtm with nits

https://codereview.chromium.org/2367393002/diff/140001/content/browser/DEPS
File content/browser/DEPS (right):

https://codereview.chromium.org/2367393002/diff/140001/content/browser/DEPS#n...
content/browser/DEPS:74:
"+third_party/WebKit/public/platform/modules/mediasession/media_session.mojom.h",
On 2016/10/03 14:31:13, Zhiqiang Zhang wrote:
> On 2016/09/30 23:24:58, xhwang wrote:
> > nit: sorry for bikeshedding. I found it odd to have "mediasession" and
> > "media_session" at the same time :) Just FYI
> 
> That's a bit odd, but I'd prefer keeping it, since .mojom file names are
> following chromium file name rules.

Got it. It seems the "mediasession" folder should be "media_session" since it's
two words. But it's totally up to you.

https://codereview.chromium.org/2367393002/diff/180001/content/browser/media/...
File content/browser/media/android/media_session_service.h (right):

https://codereview.chromium.org/2367393002/diff/180001/content/browser/media/...
content/browser/media/android/media_session_service.h:15: class
MediaSessionService
nit: Now the recommendation is to call this MediaSessionImpl since it implements
the MediaSession interface.

[Zhiqiang Zhang (Slow), 2016-10-03 20:20:59]

zqzhang@chromium.org changed reviewers:
+ clamy@chromium.org, rockot@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-03 20:21:01]

PTAL:
tsepez@ please review changes in *StructTraits* in
WebKit/Source/modules/mediasession/

+clamy for review changes in content/
+rockot to review changes in *typemap.gni and blink_bindings_configuration.gni

I'm really sorry this CL is huge, there won't be further big changes I think.

Changes in #9..#10
* Addressed some more comments.
* Added typemap in blink
* Added metadata sanitizer and fixed tests.

https://codereview.chromium.org/2367393002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/MediaSession.cpp (right):

https://codereview.chromium.org/2367393002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaSession.cpp:48: //
TODO(zqzhang): do sanitization
On 2016/09/29 10:26:40, mlamouri wrote:
> It would be better if you could do this in this CL.

Done.

https://codereview.chromium.org/2367393002/diff/180001/content/browser/media/...
File content/browser/media/android/media_session_service.h (right):

https://codereview.chromium.org/2367393002/diff/180001/content/browser/media/...
content/browser/media/android/media_session_service.h:15: class
MediaSessionService
On 2016/10/03 16:46:03, xhwang wrote:
> nit: Now the recommendation is to call this MediaSessionImpl since it
implements
> the MediaSession interface.

I'll keep the naming here, since there is already content::MediaSession meaning
a totally different thing (history reasons).

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

https://codereview.chromium.org/2367393002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaSession.h:17: 
On 2016/10/03 16:26:35, foolip wrote:
> Cat on keyboard?

Done.

https://codereview.chromium.org/2367393002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MediaSession.h:38:
mojom::blink::MediaSession* getService(ScriptState*);
On 2016/10/03 16:26:35, foolip wrote:
> Document when this can return null and not?

Done.

[Ken Rockot(use gerrit already), 2016-10-03 20:39:34]

https://codereview.chromium.org/2367393002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/MojoMediaArtwork.h (right):

https://codereview.chromium.org/2367393002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MojoMediaArtwork.h:15: struct
MojoMediaArtwork {
You should either not use a typemap at all in Blink, or typemap to the existing
MediaMetadata and MediaArtwork types. I really don't think we want to introduce
new structure types just so we can typemap to them. The only reason to ever do
that would be if you needed some extra validation logic for the type, which it
seems you don't.

Typemapping to the existing types is a bit more involved, because the
StructTraits will have to define SetUpContext and TearDownContext in order to
build up some intermediate state for serialization (e.g. to create a KURL from
the src String), and you may need to provide some better way to construct the
MediaArtwork and MediaMetadata types for deserialization.

As already mentioned, it would also be reasonable to simply not typemap anything
here (yet) and work directly with the generated mojom types for now. It's fine
for content to typemap while Blink does not.

[Zhiqiang Zhang (Slow), 2016-10-03 21:00:20]

https://codereview.chromium.org/2367393002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediasession/MojoMediaArtwork.h (right):

https://codereview.chromium.org/2367393002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediasession/MojoMediaArtwork.h:15: struct
MojoMediaArtwork {
On 2016/10/03 20:39:33, Ken Rockot wrote:
> You should either not use a typemap at all in Blink, or typemap to the
existing
> MediaMetadata and MediaArtwork types. I really don't think we want to
introduce
> new structure types just so we can typemap to them. The only reason to ever do
> that would be if you needed some extra validation logic for the type, which it
> seems you don't.
> 
> Typemapping to the existing types is a bit more involved, because the
> StructTraits will have to define SetUpContext and TearDownContext in order to
> build up some intermediate state for serialization (e.g. to create a KURL from
> the src String), and you may need to provide some better way to construct the
> MediaArtwork and MediaMetadata types for deserialization.
> 
> As already mentioned, it would also be reasonable to simply not typemap
anything
> here (yet) and work directly with the generated mojom types for now. It's fine
> for content to typemap while Blink does not.

OK. Thanks for your explanation!
I was trying to mapping to blink::MediaMetadata and blink::MediaArtwork but
failed (because of v8 dependency) so I came up this solution. As you said it's
OK for content to typemap while Blink does not, I 'll revert this patch.

No need to PTAL for tsepez now.

[Ken Rockot(use gerrit already), 2016-10-03 21:05:40]

lgtm

[Zhiqiang Zhang (Slow), 2016-10-04 14:16:56]

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

[commit-bot: I haz the power, 2016-10-04 14:17:23]

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-04 14:20:59]

Reverted blink typemap as suggested by rockot@.

Only need content/ approval now (no need for content/*media*).

[commit-bot: I haz the power, 2016-10-04 14:21:15]

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

[commit-bot: I haz the power, 2016-10-04 14:21:17]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[clamy, 2016-10-04 14:58:23]

clamy@chromium.org changed reviewers:
+ jam@chromium.org

[clamy, 2016-10-04 14:59:00]

@jam: could you take a look? I'm not familiar enough with mojo to do the review.

[jam, 2016-10-04 16:03:46]

lgtm

https://codereview.chromium.org/2367393002/diff/240001/content/browser/media/...
File content/browser/media/android/media_session_service.h (right):

https://codereview.chromium.org/2367393002/diff/240001/content/browser/media/...
content/browser/media/android/media_session_service.h:15: class
MediaSessionService
nit: MediaSessionImpl is the convention, no need to add Service at end

https://codereview.chromium.org/2367393002/diff/240001/content/browser/media/...
content/browser/media/android/media_session_service.h:16: : public
NON_EXPORTED_BASE(blink::mojom::MediaSession) {
I don't think you need the NON_EXPORTED_BASE since this class isn't exported

[Zhiqiang Zhang (Slow), 2016-10-06 11:06:31]

https://codereview.chromium.org/2367393002/diff/240001/content/browser/media/...
File content/browser/media/android/media_session_service.h (right):

https://codereview.chromium.org/2367393002/diff/240001/content/browser/media/...
content/browser/media/android/media_session_service.h:15: class
MediaSessionService
On 2016/10/04 at 16:03:46, jam wrote:
> nit: MediaSessionImpl is the convention, no need to add Service at end

I'm using `MediaSessionService` in mojo and `MediaSessionServiceImpl` in
content. The reason is that there's already `content::MediaSession` which means
a different thing.

https://codereview.chromium.org/2367393002/diff/240001/content/browser/media/...
content/browser/media/android/media_session_service.h:16: : public
NON_EXPORTED_BASE(blink::mojom::MediaSession) {
On 2016/10/04 at 16:03:46, jam wrote:
> I don't think you need the NON_EXPORTED_BASE since this class isn't exported

Done.

[Zhiqiang Zhang (Slow), 2016-10-06 11:06:40]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-06 11:06:41]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
mlamouri@chromium.org, xhwang@chromium.org, foolip@chromium.org,
rockot@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2367393002/#ps260001
(title: "renamed back to MediaSessionService/MediaSessionServiceImpl to avoid
naming confusion")

[commit-bot: I haz the power, 2016-10-06 11:06:55]

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 11:11:28]

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

[commit-bot: I haz the power, 2016-10-06 11:11:29]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[Zhiqiang Zhang (Slow), 2016-10-06 11:36:45]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-06 11:36:45]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
mlamouri@chromium.org, jam@chromium.org, foolip@chromium.org,
tsepez@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2367393002/#ps280001
(title: "fixed BUILD.gn for tests")

[commit-bot: I haz the power, 2016-10-06 11:37:03]

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 11:56:59]

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

[commit-bot: I haz the power, 2016-10-06 11:57:01]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Zhiqiang Zhang (Slow), 2016-10-06 12:45:50]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-10-06 12:45:50]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
mlamouri@chromium.org, jam@chromium.org, foolip@chromium.org,
tsepez@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2367393002/#ps300001
(title: "fixed the build")

[commit-bot: I haz the power, 2016-10-06 12:46:05]

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 14:11:15]

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

[commit-bot: I haz the power, 2016-10-06 14:11:16]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

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

The CQ bit was checked by zqzhang@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
mlamouri@chromium.org, jam@chromium.org, foolip@chromium.org,
tsepez@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2367393002/#ps320001
(title: "fixed layout tests")

[commit-bot: I haz the power, 2016-10-06 14:18:32]

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 16:14:50]

Description was changed from

==========
This CL migrates MediaSession message from IPC to mojo.
Currently MediaSession message is still Android-only, but it will
be available to Desktop in a follow-up CL.

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

to

==========
This CL migrates MediaSession message from IPC to mojo.
Currently MediaSession message is still Android-only, but it will
be available to Desktop in a follow-up CL.

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

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

Committed patchset #16 (id:320001)

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

Description was changed from

==========
This CL migrates MediaSession message from IPC to mojo.
Currently MediaSession message is still Android-only, but it will
be available to Desktop in a follow-up CL.

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

to

==========
This CL migrates MediaSession message from IPC to mojo.
Currently MediaSession message is still Android-only, but it will
be available to Desktop in a follow-up CL.

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

Committed: https://crrev.com/93b95f8bff18d5c4389abd88ca0a33426d738d10
Cr-Commit-Position: refs/heads/master@{#423554}
==========

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

Patchset 16 (id:??) landed as
https://crrev.com/93b95f8bff18d5c4389abd88ca0a33426d738d10
Cr-Commit-Position: refs/heads/master@{#423554}