Pass media session id over IPC

Pass media session id over IPC

Add media_session_id to the MediaPlayerHostMsg struct in preparation
for letting the browser process create media players with user created
media sessions.

BUG=497735, 581728
Committed: https://crrev.com/8d0004f68f64284eeff2318fc39053f5b02145a3
Cr-Commit-Position: refs/heads/master@{#378983}

[davve, 2016-02-22 13:18:27]

Description was changed from

==========
Pass media session id over IPC
==========

to

==========
Pass media session id over IPC

Add media_session_id to the MediaPlayerHostMsg struct in preparation
for letting the browser process create media players with user created
media sessions.

BUG=497735, 581728
==========

[davve, 2016-02-22 13:32:24]

davve@opera.com changed reviewers:
+ avayvod@chromium.org, mkwst@chromium.org, mlamouri@chromium.org,
philipj@opera.com

[mlamouri (slow - plz ping), 2016-02-22 16:58:21]

https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:246: media_session_id_
= 0;
ditto

https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:247: if
(params.media_session())
style: use { } when there is line wrapping.

https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.h:433: // User created
media session id, if any. 0 means the non web
nit: use some const?

https://codereview.chromium.org/1641993003/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_cast_android.cc (right):

https://codereview.chromium.org/1641993003/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_cast_android.cc:177: frame->document().url(), true,
delegate_id, 0);
const?

[philipj_slow, 2016-02-23 07:06:56]

Non-owner LGTM % outstanding issues

https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.h:433: // User created
media session id, if any. 0 means the non web
On 2016/02/22 16:58:20, Mounir Lamouri (slow) wrote:
> nit: use some const?

For player_id_ as well, then?

https://codereview.chromium.org/1641993003/diff/40001/media/blink/webmediapla...
File media/blink/webmediaplayer_cast_android.cc (right):

https://codereview.chromium.org/1641993003/diff/40001/media/blink/webmediapla...
media/blink/webmediaplayer_cast_android.cc:177: frame->document().url(), true,
delegate_id, 0);
On 2016/02/22 16:58:20, Mounir Lamouri (slow) wrote:
> const?

What should be const here?

[davve, 2016-02-23 08:12:10]

https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:246: media_session_id_
= 0;
On 2016/02/22 16:58:20, Mounir Lamouri (slow) wrote:
> ditto

Done.

https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.cc:247: if
(params.media_session())
On 2016/02/22 16:58:20, Mounir Lamouri (slow) wrote:
> style: use { } when there is line wrapping.

Goes away when moved into the initializer list.

https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/...
File content/renderer/media/android/webmediaplayer_android.h (right):

https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/...
content/renderer/media/android/webmediaplayer_android.h:433: // User created
media session id, if any. 0 means the non web
On 2016/02/23 07:06:56, philipj_UTC7 wrote:
> On 2016/02/22 16:58:20, Mounir Lamouri (slow) wrote:
> > nit: use some const?

Done.

> For player_id_ as well, then?

Addressing player_id sounds like a follow-up regardless.

[mlamouri (slow - plz ping), 2016-02-23 10:52:52]

Sorry for not being clear with my previous review. By "const", I meant to use a
constant instead of plain `0` value in the code. I believe you had one defined
that you used in the renderer process. Can you re-use this?

[davve, 2016-02-23 11:23:16]

On 2016/02/23 10:52:52, Mounir Lamouri (slow) wrote:
> Sorry for not being clear with my previous review. By "const", I meant to use
a
> constant instead of plain `0` value in the code. I believe you had one defined
> that you used in the renderer process. Can you re-use this?

I remember I tried to put a header in content/common/ with the
constant, but never got past include header guards when trying to use
it from both the renderer and browser.

Currently there is a constant in
content/renderer/media/android/renderer_media_session_manager.cc to
documents that 0 means default.

If you have an idea how such cross-process constants should be
handled, I'm all for fixing this. Now, or in a follow-up CL.

[mlamouri (slow - plz ping), 2016-02-23 13:33:23]

Using content/common/ should work.

Though, in order to use the constant from media/blink/, you will need it to be
in Blink. Maybe the constant could be part of WebMediaSession.h which can be
accessed from content/?

[davve, 2016-02-25 09:28:22]

On 2016/02/23 13:33:23, Mounir Lamouri wrote:
> Using content/common/ should work.
> 
> Though, in order to use the constant from media/blink/, you will need it to be
> in Blink. Maybe the constant could be part of WebMediaSession.h which can be
> accessed from content/?

Moving to WebMediaSession.h works well at the moment. Let's go with that. PTAL.

[Mike West, 2016-02-25 11:11:53]

Adding the session ID to the IPC struct LGTM.

[mlamouri (slow - plz ping), 2016-02-25 14:00:38]

lgtm

[davve, 2016-02-26 13:47:29]

davve@opera.com changed reviewers:
+ dalecurtis@chromium.org, jochen@chromium.org

[davve, 2016-02-26 13:47:30]

+dalecurtis for content/renderer/media and media/ and +jochen for
content/common/

[DaleCurtis, 2016-02-26 19:14:30]

lgtm % fixing static const in header.

https://codereview.chromium.org/1641993003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h
(right):

https://codereview.chromium.org/1641993003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:13:
static const int kDefaultMediaSessionID = 0;
No static const in header files since each include gets its own copy. Instead
define it in the .cpp file and a prototype here or use an enum.

[DaleCurtis, 2016-02-26 19:16:44]

https://codereview.chromium.org/1641993003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h
(right):

https://codereview.chromium.org/1641993003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:13:
static const int kDefaultMediaSessionID = 0;
On 2016/02/26 at 19:14:30, DaleCurtis wrote:
> No static const in header files since each include gets its own copy. Instead
define it in the .cpp file and a prototype here or use an enum.

Discussion here
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/hDqJ4KBVqog

[davve, 2016-02-29 09:04:01]

https://codereview.chromium.org/1641993003/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h
(right):

https://codereview.chromium.org/1641993003/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:13:
static const int kDefaultMediaSessionID = 0;
On 2016/02/26 19:14:30, DaleCurtis wrote:
> No static const in header files since each include gets its own copy. Instead
> define it in the .cpp file and a prototype here or use an enum.

Thanks for looking! An enum looks like the appropriate choice here.

[jochen (gone - plz use gerrit), 2016-03-01 14:29:24]

lgtm

[davve, 2016-03-02 15:22:38]

The CQ bit was checked by davve@opera.com

[davve, 2016-03-02 15:22:39]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com,
mlamouri@chromium.org, mkwst@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1641993003/#ps120001
(title: "Enumify constant")

[commit-bot: I haz the power, 2016-03-02 15:22:59]

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

[commit-bot: I haz the power, 2016-03-02 17:21:21]

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

[commit-bot: I haz the power, 2016-03-02 17:21:22]

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

[davve, 2016-03-03 07:25:15]

The CQ bit was checked by davve@opera.com

[commit-bot: I haz the power, 2016-03-03 07:25:26]

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

[commit-bot: I haz the power, 2016-03-03 08:28:16]

Description was changed from

==========
Pass media session id over IPC

Add media_session_id to the MediaPlayerHostMsg struct in preparation
for letting the browser process create media players with user created
media sessions.

BUG=497735, 581728
==========

to

==========
Pass media session id over IPC

Add media_session_id to the MediaPlayerHostMsg struct in preparation
for letting the browser process create media players with user created
media sessions.

BUG=497735, 581728
==========

[commit-bot: I haz the power, 2016-03-03 08:28:18]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-03-03 08:29:28]

Description was changed from

==========
Pass media session id over IPC

Add media_session_id to the MediaPlayerHostMsg struct in preparation
for letting the browser process create media players with user created
media sessions.

BUG=497735, 581728
==========

to

==========
Pass media session id over IPC

Add media_session_id to the MediaPlayerHostMsg struct in preparation
for letting the browser process create media players with user created
media sessions.

BUG=497735, 581728

Committed: https://crrev.com/8d0004f68f64284eeff2318fc39053f5b02145a3
Cr-Commit-Position: refs/heads/master@{#378983}
==========

[commit-bot: I haz the power, 2016-03-03 08:29:30]

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