Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionHook 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 #Dependent Patchsets: Messages
Total messages: 50 (17 generated)
Description was changed from ========== Hook up RendererMediaSessionManager with browser side BUG=497735 ========== to ========== [WIP] Hook up RendererMediaSessionManager with browser side BUG=497735 ==========
The CQ bit was checked by davve@opera.com to run a CQ dry run
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
Description was changed from ========== [WIP] Hook up RendererMediaSessionManager with browser side BUG=497735 ========== to ========== 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 ==========
Description was changed from ========== 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 ========== to ========== 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 ==========
davve@opera.com changed reviewers: + mlamouri@chromium.org, philipj@opera.com
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
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_d...)
The CQ bit was checked by davve@opera.com to run a CQ dry run
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
I had a first pass (not the tests yet). Comments below. https://codereview.chromium.org/1441883003/diff/60001/content/browser/media/a... File content/browser/media/android/browser_media_session_manager.h (right): https://codereview.chromium.org/1441883003/diff/60001/content/browser/media/a... content/browser/media/android/browser_media_session_manager.h:9: #include "content/browser/media/android/media_session.h" nit: do you need this include? https://codereview.chromium.org/1441883003/diff/60001/content/browser/media/a... content/browser/media/android/browser_media_session_manager.h:23: int RoutingID(); nit: GetRoutingID(), probably |const| too. https://codereview.chromium.org/1441883003/diff/60001/content/renderer/media/... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/60001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:66: void RendererMediaSessionManager::Activate(int session_id, int activation_id) { nit: keep order from header file. ::Activate() and ::Deactivate() are above ::OnActivate() and ::OnDeactivate() https://codereview.chromium.org/1441883003/diff/60001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:77: bool RendererMediaSessionManager::Send(IPC::Message* message) { Why don't you use the RenderFrameObserver implementation? https://codereview.chromium.org/1441883003/diff/60001/content/renderer/media/... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/1441883003/diff/60001/content/renderer/media/... content/renderer/media/android/webmediasession_android.cc:26: int action_id = next_action_id_++; What the |action_id| is? Should that be a request_id? passed trough the IPC messages?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1441883003/diff/60001/content/browser/media/a... File content/browser/media/android/browser_media_session_manager.h (right): https://codereview.chromium.org/1441883003/diff/60001/content/browser/media/a... 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: > nit: do you need this include? Not for more than pulling in ipc_message.h. Fixed. https://codereview.chromium.org/1441883003/diff/60001/content/browser/media/a... content/browser/media/android/browser_media_session_manager.h:23: int RoutingID(); On 2015/11/30 12:38:20, Mounir Lamouri wrote: > nit: GetRoutingID(), probably |const| too. Done. (Maybe something to fix in BrowserMediaPlayerManager on a slow day) https://codereview.chromium.org/1441883003/diff/60001/content/renderer/media/... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/60001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:66: void RendererMediaSessionManager::Activate(int session_id, int activation_id) { On 2015/11/30 12:38:20, Mounir Lamouri wrote: > nit: keep order from header file. ::Activate() and ::Deactivate() are above > ::OnActivate() and ::OnDeactivate() Done. https://codereview.chromium.org/1441883003/diff/60001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:77: bool RendererMediaSessionManager::Send(IPC::Message* message) { On 2015/11/30 12:38:20, Mounir Lamouri wrote: > Why don't you use the RenderFrameObserver implementation? That's a really good question. Done. https://codereview.chromium.org/1441883003/diff/60001/content/renderer/media/... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/1441883003/diff/60001/content/renderer/media/... content/renderer/media/android/webmediasession_android.cc:26: int action_id = next_action_id_++; On 2015/11/30 12:38:20, Mounir Lamouri wrote: > What the |action_id| is? Should that be a request_id? passed trough the IPC > messages? Yes. Passed through to map OnDidActivate to the corresponding activate. I like the name request_id.
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/a... File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/80001/content/browser/media/a... content/browser/media/android/browser_media_session_manager.cc:16: void BrowserMediaSessionManager::OnActivate(int session_id, int activation_id) { nit: ditto https://codereview.chromium.org/1441883003/diff/80001/content/browser/media/a... content/browser/media/android/browser_media_session_manager.cc:23: int deactivation_id) { nit: ditto https://codereview.chromium.org/1441883003/diff/80001/content/browser/media/a... File content/browser/media/android/browser_media_session_manager.h (right): https://codereview.chromium.org/1441883003/diff/80001/content/browser/media/a... content/browser/media/android/browser_media_session_manager.h:20: void OnActivate(int session_id, int activation_id); nit: s/activation_id/request_id/ https://codereview.chromium.org/1441883003/diff/80001/content/browser/media/a... content/browser/media/android/browser_media_session_manager.h:21: void OnDeactivate(int session_id, int deactivation_id); nit: s/deactivation_id/request_id/ https://codereview.chromium.org/1441883003/diff/80001/content/common/media/me... File content/common/media/media_session_messages_android.h (right): https://codereview.chromium.org/1441883003/diff/80001/content/common/media/me... content/common/media/media_session_messages_android.h:21: int /* activation_id */, nit: request_id https://codereview.chromium.org/1441883003/diff/80001/content/common/media/me... content/common/media/media_session_messages_android.h:26: int /* deactivation_id */) ditto https://codereview.chromium.org/1441883003/diff/80001/content/common/media/me... content/common/media/media_session_messages_android.h:32: int /* activation_id */) dis tôt https://codereview.chromium.org/1441883003/diff/80001/content/common/media/me... content/common/media/media_session_messages_android.h:36: int /* deactivation_id */) dit tôt https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:49: activation_id)); nit: request_id https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:55: deactivation_id)); again https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:59: int activation_id, a gain https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:64: if (iter != sessions_.end()) nit: I would do: ``` if (iter == sessions_.end()) return; ``` because early return is the best return :) https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:69: int deactivation_id) { same here https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:73: if (iter != sessions_.end()) as above https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... File content/renderer/media/android/renderer_media_session_manager.h (right): https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.h:33: void OnDidDeactivate(int session_id, int deactivation_id); nit: use request_id for the four new methods here. https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... File content/renderer/media/android/webmediasession_android.h (right): https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/webmediasession_android.h:27: void OnDidDeactivate(int deactivation_id); request_id^2 https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/webmediasession_android.h:42: std::map<int, SessionAction> actions_; You might want to use IDMap here instead. I think it matches better what you are trying to do. https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... File content/renderer/media/android/webmediasession_android_unittest.cc (right): https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/webmediasession_android_unittest.cc:41: int success_count_; I think you can do `int success_count_ = 0;` and remove the ctor. https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/webmediasession_android_unittest.cc:66: WebMediaSessionTest* test_; I think you are leaking `test_`. Should you use a scoped ptr? https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/webmediasession_android_unittest.cc:251: Can you add a test where you try to send a message back for an id that doesn't exist. Then send a message back twice for an id that exists. First scenario: 0 success. Second: 1 success.
Thanks for your thorough review, Mounir! https://codereview.chromium.org/1441883003/diff/80001/content/browser/media/a... File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/80001/content/browser/media/a... content/browser/media/android/browser_media_session_manager.cc:16: void BrowserMediaSessionManager::OnActivate(int session_id, int activation_id) { On 2015/12/02 15:53:30, Mounir Lamouri OOO till Monday wrote: > nit: ditto Done. https://codereview.chromium.org/1441883003/diff/80001/content/browser/media/a... content/browser/media/android/browser_media_session_manager.cc:23: int deactivation_id) { On 2015/12/02 15:53:30, Mounir Lamouri OOO till Monday wrote: > nit: ditto Done. https://codereview.chromium.org/1441883003/diff/80001/content/browser/media/a... File content/browser/media/android/browser_media_session_manager.h (right): https://codereview.chromium.org/1441883003/diff/80001/content/browser/media/a... content/browser/media/android/browser_media_session_manager.h:20: void OnActivate(int session_id, int activation_id); On 2015/12/02 15:53:30, Mounir Lamouri OOO till Monday wrote: > nit: s/activation_id/request_id/ Done. https://codereview.chromium.org/1441883003/diff/80001/content/browser/media/a... content/browser/media/android/browser_media_session_manager.h:21: void OnDeactivate(int session_id, int deactivation_id); On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > nit: s/deactivation_id/request_id/ Done. https://codereview.chromium.org/1441883003/diff/80001/content/common/media/me... File content/common/media/media_session_messages_android.h (right): https://codereview.chromium.org/1441883003/diff/80001/content/common/media/me... content/common/media/media_session_messages_android.h:21: int /* activation_id */, On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > nit: request_id Done. https://codereview.chromium.org/1441883003/diff/80001/content/common/media/me... content/common/media/media_session_messages_android.h:26: int /* deactivation_id */) On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > ditto Done. https://codereview.chromium.org/1441883003/diff/80001/content/common/media/me... content/common/media/media_session_messages_android.h:32: int /* activation_id */) On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > dis tôt Done. https://codereview.chromium.org/1441883003/diff/80001/content/common/media/me... content/common/media/media_session_messages_android.h:36: int /* deactivation_id */) On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > dit tôt Done. https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:49: activation_id)); On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > nit: request_id Done. https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:55: deactivation_id)); On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > again Done. https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:59: int activation_id, On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > a gain Done. https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:64: if (iter != sessions_.end()) On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > nit: I would do: > ``` > if (iter == sessions_.end()) > return; > ``` > > because early return is the best return :) Done. https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:69: int deactivation_id) { On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > same here Done. https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.cc:73: if (iter != sessions_.end()) On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > as above Done. https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... File content/renderer/media/android/renderer_media_session_manager.h (right): https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/renderer_media_session_manager.h:33: void OnDidDeactivate(int session_id, int deactivation_id); On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > nit: use request_id for the four new methods here. Done. https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... File content/renderer/media/android/webmediasession_android.h (right): https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/webmediasession_android.h:27: void OnDidDeactivate(int deactivation_id); On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > request_id^2 Done. https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/webmediasession_android.h:42: std::map<int, SessionAction> actions_; On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > You might want to use IDMap here instead. I think it matches better what you are > trying to do. Excellent suggestion! Done. https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... File content/renderer/media/android/webmediasession_android_unittest.cc (right): https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/webmediasession_android_unittest.cc:41: int success_count_; On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > I think you can do `int success_count_ = 0;` and remove the ctor. Done. https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/webmediasession_android_unittest.cc:66: WebMediaSessionTest* test_; On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > I think you are leaking `test_`. Should you use a scoped ptr? Not sure I follow. WebMediaSessionTest is the test instance itself. The callback does not take ownership and WebMediaSessionTest's life-span always encompasses the callback's. https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... content/renderer/media/android/webmediasession_android_unittest.cc:251: On 2015/12/02 15:53:31, Mounir Lamouri OOO till Monday wrote: > Can you add a test where you try to send a message back for an id that doesn't > exist. Then send a message back twice for an id that exists. First scenario: 0 > success. Second: 1 success. Done.
https://codereview.chromium.org/1441883003/diff/100001/content/renderer/media... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/100001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:63: return; This is probably where we will end up if the MediaSession is GC'd while the activate request is in flight on the browser side. That would leave the promise mysteriously unresolved, it seems. (If we later add a statechange event to MediaSession, that would make the problem slightly worse.) Probably need to DCHECK something here, and also make MediaSession an ActiveDOMObject that cannot be GC'd while WebMediaSession::hasPendingCallbacks() or similar returns true. https://codereview.chromium.org/1441883003/diff/100001/content/renderer/media... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/1441883003/diff/100001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:39: if (!callback) Will this be possible if the GC problem is fixed? If not, NOTREACHED() before return here? Unless IPC is special and having dead code (return after NOTREACHED()) is verboten. Dito below. https://codereview.chromium.org/1441883003/diff/120001/content/renderer/media... File content/renderer/media/android/webmediasession_android_unittest.cc (right): https://codereview.chromium.org/1441883003/diff/120001/content/renderer/media... content/renderer/media/android/webmediasession_android_unittest.cc:16: const int kRouteId = 0; kRoutingID to match GetRoutingID()?
https://codereview.chromium.org/1441883003/diff/100001/content/renderer/media... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/100001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:63: return; On 2015/12/04 14:01:04, philipj wrote: > This is probably where we will end up if the MediaSession is GC'd while the > activate request is in flight on the browser side. That would leave the promise > mysteriously unresolved, it seems. (If we later add a statechange event to > MediaSession, that would make the problem slightly worse.) > > Probably need to DCHECK something here, and also make MediaSession an > ActiveDOMObject that cannot be GC'd while WebMediaSession::hasPendingCallbacks() > or similar returns true. Adding WebMediaSession::hasPendingCallbacks() seems to be worthwhile. I also add a test for handling messages for unknown session, which also would trigger this code-path. Unless we get a different consensus on what type of IPC-simulations to test, I can't add a DCHECK(). https://codereview.chromium.org/1441883003/diff/100001/content/renderer/media... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/1441883003/diff/100001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:39: if (!callback) On 2015/12/04 14:01:04, philipj wrote: > Will this be possible if the GC problem is fixed? If not, NOTREACHED() before > return here? Unless IPC is special and having dead code (return after > NOTREACHED()) is verboten. Dito below. Same as other issue. As long as we have unittests for IPC communication we expect to never happen, we can't have assert-type of checks here (AFAIK). https://codereview.chromium.org/1441883003/diff/120001/content/renderer/media... File content/renderer/media/android/webmediasession_android_unittest.cc (right): https://codereview.chromium.org/1441883003/diff/120001/content/renderer/media... content/renderer/media/android/webmediasession_android_unittest.cc:16: const int kRouteId = 0; On 2015/12/04 14:01:04, philipj wrote: > kRoutingID to match GetRoutingID()? kRouteId seems to be the popular name, so keeping for now.
https://codereview.chromium.org/1441883003/diff/140001/content/browser/media/... File content/browser/media/android/browser_media_session_manager.h (right): https://codereview.chromium.org/1441883003/diff/140001/content/browser/media/... content/browser/media/android/browser_media_session_manager.h:9: #include "ipc/ipc_message.h" nit: you can probably fwd declare IPC::Message. https://codereview.chromium.org/1441883003/diff/140001/content/common/media/m... File content/common/media/media_session_messages_android.h (right): https://codereview.chromium.org/1441883003/diff/140001/content/common/media/m... content/common/media/media_session_messages_android.h:22: bool /* success */) Just note that we could make |request_id| map to the right |session_id|. I think what you have here is fine given that we don't reuse |session_id| in the same process. https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:63: return; Is that expected? Should we DCHECK? https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:74: return; ditto. https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:44: return; Should you DCHECK()? It seems that this should never happen. https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:58: return; ditto https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media... File content/renderer/media/android/webmediasession_android.h (right): https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media... content/renderer/media/android/webmediasession_android.h:35: int next_request_id_; I don't think you need next_request_id_. https://codereview.chromium.org/1441883003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right): https://codereview.chromium.org/1441883003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.h:23: , public ActiveDOMObject { If you make MediaSession an ActiveDOMObject, you must update the IDL as well. https://codereview.chromium.org/1441883003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.h:32: bool hasPendingActivity() const override; I'm not entirely convinced with this addition. CallbackPromiseAdapter exists to prevent that kind of situation: the Promise has its own lifetime. This is an alternative. Feel free to ignore: - you can create MediaSessionCallback<> that has a m_webMediaSession as Persistent<> - then, you make m_webMediaSession a Member<> of MediaSession Then, the WebMediaSession instance would be destroyed when all callbacks and the MediaSession objects are destroyed. WDYT?
https://codereview.chromium.org/1441883003/diff/140001/content/browser/media/... File content/browser/media/android/browser_media_session_manager.h (right): https://codereview.chromium.org/1441883003/diff/140001/content/browser/media/... 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: > nit: you can probably fwd declare IPC::Message. Done. https://codereview.chromium.org/1441883003/diff/140001/content/common/media/m... File content/common/media/media_session_messages_android.h (right): https://codereview.chromium.org/1441883003/diff/140001/content/common/media/m... content/common/media/media_session_messages_android.h:22: bool /* success */) On 2015/12/07 18:14:52, Mounir Lamouri wrote: > Just note that we could make |request_id| map to the right |session_id|. I think > what you have here is fine given that we don't reuse |session_id| in the same > process. Acknowledged. https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media... File content/renderer/media/android/webmediasession_android.cc (right): https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:44: return; On 2015/12/07 18:14:52, Mounir Lamouri wrote: > Should you DCHECK()? It seems that this should never happen. Depends on what we want to test, I guess. In https://codereview.chromium.org/1441883003/diff/80001/content/renderer/media/... we added tests that passes unknown and unexpected *request_ids*. For symmetry, I added similar testing for *session_id* in ps#8. If we want such tests, we need to handle it run-time somehow, and not DCHECK it. (Or make the testharness expect the DCHECK to trigger. Don't know anything about that feasibility). Happy to take advice on general stance here. https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media... content/renderer/media/android/webmediasession_android.cc:58: return; On 2015/12/07 18:14:52, Mounir Lamouri wrote: > ditto See other reply. https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media... File content/renderer/media/android/webmediasession_android.h (right): https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media... content/renderer/media/android/webmediasession_android.h:35: int next_request_id_; On 2015/12/07 18:14:53, Mounir Lamouri wrote: > I don't think you need next_request_id_. Done. https://codereview.chromium.org/1441883003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right): https://codereview.chromium.org/1441883003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.h:23: , public ActiveDOMObject { On 2015/12/07 18:14:53, Mounir Lamouri wrote: > If you make MediaSession an ActiveDOMObject, you must update the IDL as well. Acknowledged. https://codereview.chromium.org/1441883003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.h:32: bool hasPendingActivity() const override; On 2015/12/07 18:14:53, Mounir Lamouri wrote: > I'm not entirely convinced with this addition. CallbackPromiseAdapter exists to > prevent that kind of situation: the Promise has its own lifetime. > > This is an alternative. Feel free to ignore: > - you can create MediaSessionCallback<> that has a m_webMediaSession as > Persistent<> > - then, you make m_webMediaSession a Member<> of MediaSession > > Then, the WebMediaSession instance would be destroyed when all callbacks and the > MediaSession objects are destroyed. > > WDYT? I think it's an interesting idea and it matches better what should happen. It's the promise that should stay alive, not the media session itself. I'll rebase on top of your metadata patch, with ActiveDOMObject reverted, and investigate further.
https://codereview.chromium.org/1441883003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right): https://codereview.chromium.org/1441883003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.h:32: bool hasPendingActivity() const override; On 2015/12/08 10:21:23, David Vest wrote: > On 2015/12/07 18:14:53, Mounir Lamouri wrote: > > I'm not entirely convinced with this addition. CallbackPromiseAdapter exists > to > > prevent that kind of situation: the Promise has its own lifetime. > > > > This is an alternative. Feel free to ignore: > > - you can create MediaSessionCallback<> that has a m_webMediaSession as > > Persistent<> > > - then, you make m_webMediaSession a Member<> of MediaSession > > > > Then, the WebMediaSession instance would be destroyed when all callbacks and > the > > MediaSession objects are destroyed. > > > > WDYT? > > I think it's an interesting idea and it matches better what should happen. It's > the promise that should stay alive, not the media session itself. I'll rebase on > top of your metadata patch, with ActiveDOMObject reverted, and investigate > further. Something like this should work, but the lifetime of WebMediaSessio would have to be independent of the MediaSession. The callbacks would have to have a keep the WebMediaSession alive and vice-versa.
I personally like the simplicity of the latest version. PTAL. https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/140001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:63: return; On 2015/12/07 18:14:52, Mounir Lamouri wrote: > Is that expected? Should we DCHECK? The equivalent DCHECKS added in ps#11. I removed tests that returns unexpected messages from the browser process. https://codereview.chromium.org/1441883003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediasession/MediaSession.h (right): https://codereview.chromium.org/1441883003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediasession/MediaSession.h:32: bool hasPendingActivity() const override; On 2015/12/08 10:38:45, philipj wrote: > On 2015/12/08 10:21:23, David Vest wrote: > > On 2015/12/07 18:14:53, Mounir Lamouri wrote: > > > I'm not entirely convinced with this addition. CallbackPromiseAdapter exists > > to > > > prevent that kind of situation: the Promise has its own lifetime. > > > > > > This is an alternative. Feel free to ignore: > > > - you can create MediaSessionCallback<> that has a m_webMediaSession as > > > Persistent<> > > > - then, you make m_webMediaSession a Member<> of MediaSession > > > > > > Then, the WebMediaSession instance would be destroyed when all callbacks and > > the > > > MediaSession objects are destroyed. > > > > > > WDYT? > > > > I think it's an interesting idea and it matches better what should happen. > It's > > the promise that should stay alive, not the media session itself. I'll rebase > on > > top of your metadata patch, with ActiveDOMObject reverted, and investigate > > further. > > Something like this should work, but the lifetime of WebMediaSessio would have > to be independent of the MediaSession. The callbacks would have to have a keep > the WebMediaSession alive and vice-versa. I punted on this issue in ps#11 by not involving the session at all when resolving the promise. I guess we have to revisit when adding event or such so that the promise needs to keep the blink session alive.
lgtm https://codereview.chromium.org/1441883003/diff/200001/content/renderer/media... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/200001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:49: blink::WebMediaSessionActivateCallback* callback) { nit: maybe this should be a scoped_ptr<> to make the ownership explicit? https://codereview.chromium.org/1441883003/diff/200001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:56: blink::WebMediaSessionDeactivateCallback* callback) { ditto https://codereview.chromium.org/1441883003/diff/200001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:70: blink::WebMediaSessionError(blink::WebMediaSessionError::Activate)); style: you need { } if you wrap line.
https://codereview.chromium.org/1441883003/diff/200001/content/renderer/media... File content/renderer/media/android/renderer_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/200001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:49: blink::WebMediaSessionActivateCallback* callback) { On 2015/12/09 12:12:19, Mounir Lamouri wrote: > nit: maybe this should be a scoped_ptr<> to make the ownership explicit? Done. https://codereview.chromium.org/1441883003/diff/200001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:56: blink::WebMediaSessionDeactivateCallback* callback) { On 2015/12/09 12:12:19, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1441883003/diff/200001/content/renderer/media... content/renderer/media/android/renderer_media_session_manager.cc:70: blink::WebMediaSessionError(blink::WebMediaSessionError::Activate)); On 2015/12/09 12:12:19, Mounir Lamouri wrote: > style: you need { } if you wrap line. Done.
still lgtm
third_party/WebKit/ LGTM, and non-owner LGTM for the rest
davve@opera.com changed reviewers: + jochen@chromium.org
Jochen, can you take a look? Anything special review-wise since this involves IPC?
the messages file needs to be restricted to the security team like here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/OWNE...
On 2015/12/09 at 15:47:45, jochen wrote: > the messages file needs to be restricted to the security team like here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/OWNE... content/common/media/ has the same logic. The Mesage file will automatically require security team review.
mlamouri@chromium.org changed reviewers: + mkwst@chromium.org
+mkwst@ for IPC
https://codereview.chromium.org/1441883003/diff/220001/content/browser/media/... File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/220001/content/browser/media/... content/browser/media/android/browser_media_session_manager.cc:17: NOTIMPLEMENTED(); What are these eventually going to do? It's difficult to evaluate the privilege this grants to a renderer without code. ;) Basically, I want to know what a corrupted renderer could do if it could send this message to the browser process. Skimming the spec, discussion of "keys" worries me. What kinds of keys would a user/renderer process gain access to? https://codereview.chromium.org/1441883003/diff/220001/content/common/media/m... File content/common/media/media_session_messages_android.h (right): https://codereview.chromium.org/1441883003/diff/220001/content/common/media/m... content/common/media/media_session_messages_android.h:17: // Messages for notifying the render process of media session status ------- Tiny nit: We usually have some sort of "Messages from browser to renderer" and "Messages from renderer to browser" for overly explicit clarity. :)
https://codereview.chromium.org/1441883003/diff/220001/content/browser/media/... File content/browser/media/android/browser_media_session_manager.cc (right): https://codereview.chromium.org/1441883003/diff/220001/content/browser/media/... content/browser/media/android/browser_media_session_manager.cc:17: NOTIMPLEMENTED(); On 2015/12/09 17:39:59, Mike West wrote: > What are these eventually going to do? It's difficult to evaluate the privilege > this grants to a renderer without code. ;) > > Basically, I want to know what a corrupted renderer could do if it could send > this message to the browser process. Skimming the spec, discussion of "keys" > worries me. What kinds of keys would a user/renderer process gain access to? On 2015/12/09 17:39:59, Mike West wrote: > What are these eventually going to do? It's difficult to evaluate the privilege > this grants to a renderer without code. ;) I can't blame you. :) (To some extent considering security implications must be an ongoing process, not just a one-off when adding the IPC messages?) These two specific messages will activate and deactivate media sessions on android. This boils down to requesting audio focus as described in: http://developer.android.com/training/managing-audio/audio-focus.html This is already done implicitly when starting media playback. What these messages enable is a javascript API to explicitly request audio focus without actually playing. The API is promise based and the return messages are for knowing when activation and deactivation have completed. > Basically, I want to know what a corrupted renderer could do if it could send > this message to the browser process. Skimming the spec, discussion of "keys" > worries me. What kinds of keys would a user/renderer process gain access to? Multimedia keys. On Android, one example is the play/pause button on your headphones. The renderer process doesn't get access to the keys themselves, but might enable or disable them for certain ongoing media playback. https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me... will do the actual work, perhaps with a little more of its guts exposed to allow activating at will. I don't see a new attack surface exposed, except from annoying the user that their background Spotify went quiet for some to them unknown reason. https://codereview.chromium.org/1441883003/diff/220001/content/common/media/m... File content/common/media/media_session_messages_android.h (right): https://codereview.chromium.org/1441883003/diff/220001/content/common/media/m... content/common/media/media_session_messages_android.h:17: // Messages for notifying the render process of media session status ------- On 2015/12/09 17:39:59, Mike West wrote: > Tiny nit: We usually have some sort of "Messages from browser to renderer" and > "Messages from renderer to browser" for overly explicit clarity. :) Done.
On 2015/12/10 at 08:28:38, davve wrote: > On 2015/12/09 17:39:59, Mike West wrote: > > What are these eventually going to do? It's difficult to evaluate the privilege > > this grants to a renderer without code. ;) > > I can't blame you. :) (To some extent considering security implications > must be an ongoing process, not just a one-off when adding the IPC > messages?) Well, yes, but most of the time folks don't loop us in when they're making interesting changes after we've already signed off on the message itself. :) > > Basically, I want to know what a corrupted renderer could do if it could send > > this message to the browser process. Skimming the spec, discussion of "keys" > > worries me. What kinds of keys would a user/renderer process gain access to? > > Multimedia keys. On Android, one example is the play/pause button on > your headphones. The renderer process doesn't get access to the keys > themselves, but might enable or disable them for certain ongoing media > playback. Ah. "Keys" like the keys on my keyboard. Not "keys" like public/private encryption keys. Got it. > I don't see a new attack surface exposed, except from annoying the > user that their background Spotify went quiet for some to them unknown > reason. I think I agree, thanks. IPC LGTM.
On 2015/12/09 15:47:45, jochen wrote: > the messages file needs to be restricted to the security team like here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/OWNE... Looks like this was taken care of already, right? Anything else to do here?
lgtm
The CQ bit was checked by davve@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1441883003/#ps240001 (title: "Clarify comments in _messages file")
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
The CQ bit was unchecked by commit-bot@chromium.org
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_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by davve@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, philipj@opera.com, mkwst@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1441883003/#ps260001 (title: "Rebase")
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
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/2536c36bebcb87fa8b2f18bd4da9b9951eb63607 Cr-Commit-Position: refs/heads/master@{#364673} |
