Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(596)

Issue 1641993003: Pass media session id over IPC (Closed)

Created:
4 years, 10 months ago by davve
Modified:
4 years, 9 months ago
CC:
avayvod+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-media_chromium.org, posciak+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@possible-next-split
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : Rebase #

Patch Set 3 : Fix typo in comment #

Total comments: 9

Patch Set 4 : Add some const #

Patch Set 5 : Rebase and move constant so that it can be used in media/blink/ #

Patch Set 6 : Final explicit zero exorcism #

Total comments: 3

Patch Set 7 : Enumify constant #

Messages

Total messages: 29 (9 generated)
davve
4 years, 10 months ago (2016-02-22 13:32:25 UTC) #3
mlamouri (slow - plz ping)
https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/android/webmediaplayer_android.cc#newcode246 content/renderer/media/android/webmediaplayer_android.cc:246: media_session_id_ = 0; ditto https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/android/webmediaplayer_android.cc#newcode247 content/renderer/media/android/webmediaplayer_android.cc:247: if (params.media_session()) style: ...
4 years, 10 months ago (2016-02-22 16:58:21 UTC) #4
philipj_slow
Non-owner LGTM % outstanding issues https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/android/webmediaplayer_android.h File content/renderer/media/android/webmediaplayer_android.h (right): https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/android/webmediaplayer_android.h#newcode433 content/renderer/media/android/webmediaplayer_android.h:433: // User created media ...
4 years, 10 months ago (2016-02-23 07:06:56 UTC) #5
davve
https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/android/webmediaplayer_android.cc File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/1641993003/diff/40001/content/renderer/media/android/webmediaplayer_android.cc#newcode246 content/renderer/media/android/webmediaplayer_android.cc:246: media_session_id_ = 0; On 2016/02/22 16:58:20, Mounir Lamouri (slow) ...
4 years, 10 months ago (2016-02-23 08:12:10 UTC) #6
mlamouri (slow - plz ping)
Sorry for not being clear with my previous review. By "const", I meant to use ...
4 years, 10 months ago (2016-02-23 10:52:52 UTC) #7
davve
On 2016/02/23 10:52:52, Mounir Lamouri (slow) wrote: > Sorry for not being clear with my ...
4 years, 10 months ago (2016-02-23 11:23:16 UTC) #8
mlamouri (slow - plz ping)
Using content/common/ should work. Though, in order to use the constant from media/blink/, you will ...
4 years, 10 months ago (2016-02-23 13:33:23 UTC) #9
davve
On 2016/02/23 13:33:23, Mounir Lamouri wrote: > Using content/common/ should work. > > Though, in ...
4 years, 10 months ago (2016-02-25 09:28:22 UTC) #10
Mike West
Adding the session ID to the IPC struct LGTM.
4 years, 10 months ago (2016-02-25 11:11:53 UTC) #11
mlamouri (slow - plz ping)
lgtm
4 years, 10 months ago (2016-02-25 14:00:38 UTC) #12
davve
+dalecurtis for content/renderer/media and media/ and +jochen for content/common/
4 years, 10 months ago (2016-02-26 13:47:30 UTC) #14
DaleCurtis
lgtm % fixing static const in header. https://codereview.chromium.org/1641993003/diff/100001/third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h (right): https://codereview.chromium.org/1641993003/diff/100001/third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h#newcode13 third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:13: static const ...
4 years, 10 months ago (2016-02-26 19:14:30 UTC) #15
DaleCurtis
https://codereview.chromium.org/1641993003/diff/100001/third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h (right): https://codereview.chromium.org/1641993003/diff/100001/third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h#newcode13 third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:13: static const int kDefaultMediaSessionID = 0; On 2016/02/26 at ...
4 years, 10 months ago (2016-02-26 19:16:44 UTC) #16
davve
https://codereview.chromium.org/1641993003/diff/100001/third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h File third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h (right): https://codereview.chromium.org/1641993003/diff/100001/third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h#newcode13 third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h:13: static const int kDefaultMediaSessionID = 0; On 2016/02/26 19:14:30, ...
4 years, 9 months ago (2016-02-29 09:04:01 UTC) #17
jochen (gone - plz use gerrit)
lgtm
4 years, 9 months ago (2016-03-01 14:29:24 UTC) #18
commit-bot: I haz the power
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
4 years, 9 months ago (2016-03-02 15:22:59 UTC) #21
commit-bot: I haz the power
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_ng/builds/182789)
4 years, 9 months ago (2016-03-02 17:21:22 UTC) #23
commit-bot: I haz the power
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
4 years, 9 months ago (2016-03-03 07:25:26 UTC) #25
commit-bot: I haz the power
Committed patchset #7 (id:120001)
4 years, 9 months ago (2016-03-03 08:28:18 UTC) #27
commit-bot: I haz the power
4 years, 9 months ago (2016-03-03 08:29:30 UTC) #29
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/8d0004f68f64284eeff2318fc39053f5b02145a3
Cr-Commit-Position: refs/heads/master@{#378983}

Powered by Google App Engine
This is Rietveld 408576698