Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1813373003:
Release media session and power save blockers for casting. (Closed)
|
Created:
4 years, 9 months ago by DaleCurtis Modified:
4 years, 9 months ago Reviewers:
hubbe CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRelease media session and power save blockers for casting.
BUG=595903
TEST=none
Committed: https://crrev.com/6b91661ebac34b1096186920caff52cf8b142c55
Cr-Commit-Position: refs/heads/master@{#382670}
Patch Set 1 #
Total comments: 4
Messages
Total messages: 14 (3 generated)
dalecurtis@chromium.org changed reviewers: + hubbe@chromium.org
https://codereview.chromium.org/1813373003/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1813373003/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:887: delegate_->PlayerGone(delegate_id_); This doesn't seem right. The browser side already treats playing/pausing differently when is_remote is set. Or am I missing something?
https://codereview.chromium.org/1813373003/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1813373003/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:887: delegate_->PlayerGone(delegate_id_); On 2016/03/18 at 21:40:24, hubbe wrote: > This doesn't seem right. > The browser side already treats playing/pausing differently when is_remote is set. Or am I missing something? AFAICT WMPI::pause() is never called when suspending for remote. So I turn it back to you, am I missing something? :)
https://codereview.chromium.org/1813373003/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1813373003/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:887: delegate_->PlayerGone(delegate_id_); On 2016/03/18 21:43:38, DaleCurtis wrote: > On 2016/03/18 at 21:40:24, hubbe wrote: > > This doesn't seem right. > > The browser side already treats playing/pausing differently when is_remote is > set. Or am I missing something? > > AFAICT WMPI::pause() is never called when suspending for remote. So I turn it > back to you, am I missing something? :) I could have sworn this was working correctly at some point. I don't suppose we have a test for this somewhere to make sure we don't screw it up again? Calling PlayerGone seems really weird to me, is that really the right function? Do we need to do anything when we come back from remoting?
https://codereview.chromium.org/1813373003/diff/1/media/blink/webmediaplayer_... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/1813373003/diff/1/media/blink/webmediaplayer_... media/blink/webmediaplayer_impl.cc:887: delegate_->PlayerGone(delegate_id_); On 2016/03/18 at 23:09:43, hubbe wrote: > On 2016/03/18 21:43:38, DaleCurtis wrote: > > On 2016/03/18 at 21:40:24, hubbe wrote: > > > This doesn't seem right. > > > The browser side already treats playing/pausing differently when is_remote is > > set. Or am I missing something? > > > > AFAICT WMPI::pause() is never called when suspending for remote. So I turn it > > back to you, am I missing something? :) > > I could have sworn this was working correctly at some point. > I don't suppose we have a test for this somewhere to make sure we don't screw it up again? > > Calling PlayerGone seems really weird to me, is that really the right function? Do we need to do anything when we come back from remoting? OnPipelineResumed() will restart playback if necessary and send the right signals. You do want PlayerGone() here since we want the media session to disappear. I haven't tested since I don't have a cast setup here, but if you want to verify that this is necessary that'd be really helpful.
On 2016/03/18 23:22:24, DaleCurtis wrote: > https://codereview.chromium.org/1813373003/diff/1/media/blink/webmediaplayer_... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/1813373003/diff/1/media/blink/webmediaplayer_... > media/blink/webmediaplayer_impl.cc:887: delegate_->PlayerGone(delegate_id_); > On 2016/03/18 at 23:09:43, hubbe wrote: > > On 2016/03/18 21:43:38, DaleCurtis wrote: > > > On 2016/03/18 at 21:40:24, hubbe wrote: > > > > This doesn't seem right. > > > > The browser side already treats playing/pausing differently when is_remote > is > > > set. Or am I missing something? > > > > > > AFAICT WMPI::pause() is never called when suspending for remote. So I turn > it > > > back to you, am I missing something? :) > > > > I could have sworn this was working correctly at some point. > > I don't suppose we have a test for this somewhere to make sure we don't screw > it up again? > > > > Calling PlayerGone seems really weird to me, is that really the right > function? Do we need to do anything when we come back from remoting? > > OnPipelineResumed() will restart playback if necessary and send the right > signals. You do want PlayerGone() here since we want the media session to > disappear. I haven't tested since I don't have a cast setup here, but if you > want to verify that this is necessary that'd be really helpful. I can verify on monday, here is an LGTM if you feel a need to check in before then. :)
No rush, will wait until you can test :)
The CQ bit was checked by hubbe@chromium.org
lgtm Verily, this fixes the notifications.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1813373003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1813373003/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Release media session and power save blockers for casting. BUG=595903 TEST=none ========== to ========== Release media session and power save blockers for casting. BUG=595903 TEST=none Committed: https://crrev.com/6b91661ebac34b1096186920caff52cf8b142c55 Cr-Commit-Position: refs/heads/master@{#382670} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/6b91661ebac34b1096186920caff52cf8b142c55 Cr-Commit-Position: refs/heads/master@{#382670} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
