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

Issue 1441883003: Hook up RendererMediaSessionManager with browser side (Closed)

Created:
5 years, 1 month ago by davve
Modified:
5 years ago
CC:
avayvod+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, 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, nasko+codewatch_chromium.org, posciak+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Hook up RendererMediaSessionManager with browser side Implements the basic IPC messages for activation and deactivation back and forth. The browser side is still unimplemented. BUG=497735 Committed: https://crrev.com/2536c36bebcb87fa8b2f18bd4da9b9951eb63607 Cr-Commit-Position: refs/heads/master@{#364673}

Patch Set 1 #

Patch Set 2 : Rebased and fixed session creation #

Patch Set 3 : Rebase and add tests #

Patch Set 4 : Clang compile fix #

Total comments: 10

Patch Set 5 : Address review comments #

Total comments: 40

Patch Set 6 : Address review comments #

Total comments: 4

Patch Set 7 : Remove now unnecessary line #

Total comments: 2

Patch Set 8 : Address review comment from philipj: ActiveDOMObject #

Total comments: 19

Patch Set 9 : Address review comment pre-rebase #

Patch Set 10 : Rebase #

Patch Set 11 : Simplify message return path by going straight to promise #

Total comments: 6

Patch Set 12 : Add documentation, style fix and scoped_ptr #

Total comments: 4

Patch Set 13 : Clarify comments in _messages file #

Patch Set 14 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+388 lines, -10 lines) Patch
A content/browser/media/android/browser_media_session_manager.h View 1 2 3 4 5 6 7 8 1 chunk +38 lines, -0 lines 0 comments Download
A content/browser/media/android/browser_media_session_manager.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +34 lines, -0 lines 0 comments Download
M content/browser/media/media_web_contents_observer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +10 lines, -0 lines 0 comments Download
M content/browser/media/media_web_contents_observer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +21 lines, -0 lines 0 comments Download
M content/common/content_message_generator.h View 1 chunk +1 line, -0 lines 0 comments Download
A content/common/media/media_session_messages_android.h View 1 2 3 4 5 6 7 8 9 10 13 1 chunk +33 lines, -0 lines 0 comments Download
M content/content_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -0 lines 0 comments Download
M content/content_common.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/media/android/renderer_media_session_manager.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +22 lines, -0 lines 0 comments Download
M content/renderer/media/android/renderer_media_session_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +46 lines, -0 lines 0 comments Download
M content/renderer/media/android/webmediasession_android.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -10 lines 0 comments Download
M content/renderer/media/android/webmediasession_android_unittest.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +166 lines, -0 lines 0 comments Download
M ipc/ipc_message_start.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/public/platform/modules/mediasession/WebMediaSession.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +9 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 50 (17 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441883003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441883003/40001
5 years ago (2015-11-27 16:00:52 UTC) #3
davve
PTAL
5 years ago (2015-11-27 16:04:18 UTC) #7
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_dbg_recipe/builds/152083)
5 years ago (2015-11-27 17:01:32 UTC) #9
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441883003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441883003/60001
5 years ago (2015-11-30 12:05:38 UTC) #11
mlamouri (slow - plz ping)
I had a first pass (not the tests yet). Comments below. https://codereview.chromium.org/1441883003/diff/60001/content/browser/media/android/browser_media_session_manager.h File content/browser/media/android/browser_media_session_manager.h (right): ...
5 years ago (2015-11-30 12:38:20 UTC) #12
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years ago (2015-11-30 13:27:02 UTC) #14
davve
https://codereview.chromium.org/1441883003/diff/60001/content/browser/media/android/browser_media_session_manager.h File content/browser/media/android/browser_media_session_manager.h (right): https://codereview.chromium.org/1441883003/diff/60001/content/browser/media/android/browser_media_session_manager.h#newcode9 content/browser/media/android/browser_media_session_manager.h:9: #include "content/browser/media/android/media_session.h" On 2015/11/30 12:38:20, Mounir Lamouri wrote: > ...
5 years ago (2015-11-30 14:19:48 UTC) #15
mlamouri (slow - plz ping)
Looks very good. If it were not for the IDMap, it would be l-g-t-m. https://codereview.chromium.org/1441883003/diff/80001/content/browser/media/android/browser_media_session_manager.cc ...
5 years ago (2015-12-02 15:53:31 UTC) #16
davve
Thanks for your thorough review, Mounir! https://codereview.chromium.org/1441883003/diff/80001/content/browser/media/android/browser_media_session_manager.cc File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/80001/content/browser/media/android/browser_media_session_manager.cc#newcode16 content/browser/media/android/browser_media_session_manager.cc:16: void BrowserMediaSessionManager::OnActivate(int session_id, ...
5 years ago (2015-12-04 12:43:09 UTC) #17
philipj_slow
https://codereview.chromium.org/1441883003/diff/100001/content/renderer/media/android/renderer_media_session_manager.cc File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/100001/content/renderer/media/android/renderer_media_session_manager.cc#newcode63 content/renderer/media/android/renderer_media_session_manager.cc:63: return; This is probably where we will end up ...
5 years ago (2015-12-04 14:01:04 UTC) #18
davve
https://codereview.chromium.org/1441883003/diff/100001/content/renderer/media/android/renderer_media_session_manager.cc File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/100001/content/renderer/media/android/renderer_media_session_manager.cc#newcode63 content/renderer/media/android/renderer_media_session_manager.cc:63: return; On 2015/12/04 14:01:04, philipj wrote: > This is ...
5 years ago (2015-12-04 14:52:55 UTC) #19
mlamouri (slow - plz ping)
https://codereview.chromium.org/1441883003/diff/140001/content/browser/media/android/browser_media_session_manager.h File content/browser/media/android/browser_media_session_manager.h (right): https://codereview.chromium.org/1441883003/diff/140001/content/browser/media/android/browser_media_session_manager.h#newcode9 content/browser/media/android/browser_media_session_manager.h:9: #include "ipc/ipc_message.h" nit: you can probably fwd declare IPC::Message. ...
5 years ago (2015-12-07 18:14:53 UTC) #20
davve
https://codereview.chromium.org/1441883003/diff/140001/content/browser/media/android/browser_media_session_manager.h File content/browser/media/android/browser_media_session_manager.h (right): https://codereview.chromium.org/1441883003/diff/140001/content/browser/media/android/browser_media_session_manager.h#newcode9 content/browser/media/android/browser_media_session_manager.h:9: #include "ipc/ipc_message.h" On 2015/12/07 18:14:52, Mounir Lamouri wrote: > ...
5 years ago (2015-12-08 10:21:23 UTC) #21
philipj_slow
https://codereview.chromium.org/1441883003/diff/140001/third_party/WebKit/Source/modules/mediasession/MediaSession.h File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right): https://codereview.chromium.org/1441883003/diff/140001/third_party/WebKit/Source/modules/mediasession/MediaSession.h#newcode32 third_party/WebKit/Source/modules/mediasession/MediaSession.h:32: bool hasPendingActivity() const override; On 2015/12/08 10:21:23, David Vest ...
5 years ago (2015-12-08 10:38:45 UTC) #22
davve
I personally like the simplicity of the latest version. PTAL. https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media/android/renderer_media_session_manager.cc File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media/android/renderer_media_session_manager.cc#newcode63 ...
5 years ago (2015-12-09 11:20:18 UTC) #23
mlamouri (slow - plz ping)
lgtm https://codereview.chromium.org/1441883003/diff/200001/content/renderer/media/android/renderer_media_session_manager.cc File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/200001/content/renderer/media/android/renderer_media_session_manager.cc#newcode49 content/renderer/media/android/renderer_media_session_manager.cc:49: blink::WebMediaSessionActivateCallback* callback) { nit: maybe this should be ...
5 years ago (2015-12-09 12:12:19 UTC) #24
davve
https://codereview.chromium.org/1441883003/diff/200001/content/renderer/media/android/renderer_media_session_manager.cc File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/200001/content/renderer/media/android/renderer_media_session_manager.cc#newcode49 content/renderer/media/android/renderer_media_session_manager.cc:49: blink::WebMediaSessionActivateCallback* callback) { On 2015/12/09 12:12:19, Mounir Lamouri wrote: ...
5 years ago (2015-12-09 14:02:24 UTC) #25
mlamouri (slow - plz ping)
still lgtm
5 years ago (2015-12-09 14:36:46 UTC) #26
philipj_slow
third_party/WebKit/ LGTM, and non-owner LGTM for the rest
5 years ago (2015-12-09 14:46:14 UTC) #27
davve
Jochen, can you take a look? Anything special review-wise since this involves IPC?
5 years ago (2015-12-09 14:59:52 UTC) #29
jochen (gone - plz use gerrit)
the messages file needs to be restricted to the security team like here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/OWNERS&l=14
5 years ago (2015-12-09 15:47:45 UTC) #30
mlamouri (slow - plz ping)
On 2015/12/09 at 15:47:45, jochen wrote: > the messages file needs to be restricted to ...
5 years ago (2015-12-09 16:52:07 UTC) #31
mlamouri (slow - plz ping)
+mkwst@ for IPC
5 years ago (2015-12-09 16:52:35 UTC) #33
Mike West
https://codereview.chromium.org/1441883003/diff/220001/content/browser/media/android/browser_media_session_manager.cc File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/220001/content/browser/media/android/browser_media_session_manager.cc#newcode17 content/browser/media/android/browser_media_session_manager.cc:17: NOTIMPLEMENTED(); What are these eventually going to do? It's ...
5 years ago (2015-12-09 17:39:59 UTC) #34
davve
https://codereview.chromium.org/1441883003/diff/220001/content/browser/media/android/browser_media_session_manager.cc File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/220001/content/browser/media/android/browser_media_session_manager.cc#newcode17 content/browser/media/android/browser_media_session_manager.cc:17: NOTIMPLEMENTED(); On 2015/12/09 17:39:59, Mike West wrote: > What ...
5 years ago (2015-12-10 08:28:38 UTC) #35
Mike West
On 2015/12/10 at 08:28:38, davve wrote: > On 2015/12/09 17:39:59, Mike West wrote: > > ...
5 years ago (2015-12-10 09:36:07 UTC) #36
davve
On 2015/12/09 15:47:45, jochen wrote: > the messages file needs to be restricted to the ...
5 years ago (2015-12-10 13:30:30 UTC) #37
jochen (gone - plz use gerrit)
lgtm
5 years ago (2015-12-10 15:36:06 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441883003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441883003/240001
5 years ago (2015-12-11 07:49:27 UTC) #41
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/135252) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, ...
5 years ago (2015-12-11 07:52:19 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1441883003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1441883003/260001
5 years ago (2015-12-11 08:55:04 UTC) #46
commit-bot: I haz the power
Committed patchset #14 (id:260001)
5 years ago (2015-12-11 12:05:31 UTC) #48
commit-bot: I haz the power
5 years ago (2015-12-11 12:07:01 UTC) #50
Message was sent while issue was closed.
Patchset 14 (id:??) landed as
https://crrev.com/2536c36bebcb87fa8b2f18bd4da9b9951eb63607
Cr-Commit-Position: refs/heads/master@{#364673}

Powered by Google App Engine
This is Rietveld 408576698