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

Issue 2767253002: Don't GONE the unsuspended video only players (Closed)

Created:
3 years, 9 months ago by whywhat
Modified:
3 years, 9 months ago
CC:
apacible+watch_chromium.org, chromium-reviews, erickung+watch_chromium.org, feature-media-reviews_chromium.org, miu+watch_chromium.org, xjz+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't GONE the unsuspended video only players Video only players are reported to RendererWebMediaPlayerDelegate as GONE even if they're playing. This CL would make sure we only report suspended players as GONE. This affects the logic in MediaWebContentsDelegate that creates the power blocker for videos. BUG=703105 TEST=manual Review-Url: https://codereview.chromium.org/2767253002 Cr-Commit-Position: refs/heads/master@{#458995} Committed: https://chromium.googlesource.com/chromium/src/+/5f34b644166c2dc02f7f937f69ce0fd08e89e626

Patch Set 1 #

Patch Set 2 : Updated logic, updated tests #

Patch Set 3 : Remove log statement #

Patch Set 4 : Fixed logic for audible videos... #

Total comments: 2

Patch Set 5 : Fixed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+38 lines, -13 lines) Patch
M media/blink/webmediaplayer_impl.cc View 1 2 3 4 2 chunks +17 lines, -10 lines 0 comments Download
M media/blink/webmediaplayer_impl_unittest.cc View 1 2 chunks +21 lines, -3 lines 0 comments Download

Messages

Total messages: 23 (11 generated)
whywhat
Updated logic, updated tests
3 years, 9 months ago (2017-03-22 22:33:21 UTC) #1
whywhat
Remove log statement
3 years, 9 months ago (2017-03-22 22:34:40 UTC) #2
whywhat
Fixed logic for audible videos...
3 years, 9 months ago (2017-03-22 22:47:45 UTC) #3
whywhat
PTaL I basically restored the old |has_session_playing| condition. I tried avoiding |!alive| by inversing the ...
3 years, 9 months ago (2017-03-22 22:49:40 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2767253002/60001
3 years, 9 months ago (2017-03-22 22:50:12 UTC) #7
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the ...
3 years, 9 months ago (2017-03-22 22:50:13 UTC) #9
sandersd (OOO until July 31)
Logic appears correct to me. I'm assuming that you've verified that this does not create ...
3 years, 9 months ago (2017-03-23 00:02:36 UTC) #12
whywhat
Fixed comments
3 years, 9 months ago (2017-03-23 01:15:51 UTC) #15
whywhat
On 2017/03/23 at 00:02:36, sandersd wrote: > Logic appears correct to me. I'm assuming that ...
3 years, 9 months ago (2017-03-23 01:16:39 UTC) #16
whywhat
https://codereview.chromium.org/2767253002/diff/60001/media/blink/webmediaplayer_impl.cc File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2767253002/diff/60001/media/blink/webmediaplayer_impl.cc#newcode2008 media/blink/webmediaplayer_impl.cc:2008: // |has_remote_controls| indicates if the player can be controlled ...
3 years, 9 months ago (2017-03-23 01:17:24 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2767253002/80001
3 years, 9 months ago (2017-03-23 01:18:00 UTC) #20
commit-bot: I haz the power
3 years, 9 months ago (2017-03-23 03:15:20 UTC) #23
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5f34b644166c2dc02f7f937f69ce...

Powered by Google App Engine
This is Rietveld 408576698