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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 946643002: Use PowerSaveBlocker for audio and video on Chrome OS. (Closed)

Created:
5 years, 10 months ago by Daniel Erat
Modified:
5 years, 10 months ago
CC:
aandrey+blink_chromium.org, asanka, avayvod+watch_chromium.org, benjhayden+dwatch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, devtools-reviews_chromium.org, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, hashimoto+watch_chromium.org, hclam+watch_chromium.org, hguihot+watch_chromium.org, hubbe+watch_chromium.org, imcheng+watch_chromium.org, jam, jasonroberts+watch_google.com, mcasas+watch_chromium.org, mikhal+watch_chromium.org, miu+watch_chromium.org, nasko+codewatch_chromium.org, oshima+watch_chromium.org, pfeldman, posciak+watch_chromium.org, pwestin+watch_google.com, stevenjb+watch_chromium.org, wjia+watch_chromium.org, yurys
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Use PowerSaveBlocker for audio and video on Chrome OS. PowerSaveBlocker objects are constructed in response to web audio and video to keep the system from suspending or turning its display off. Historically, this was not used on Chrome OS, as Chrome OS has alternate mechanisms for detecting audio and video activity and using PowerSaveBlocker would interfere with enterprise power-management-policy prefs to ignore audio and video activity. However, low-framerate video, such as that used by WebRTC for Hangouts screen-sharing, can fail to trigger Chrome OS's video detection code, resulting in the screen being turned off while in use. This change renames PowerSaveBlocker's human-readable "reason" field to "description" and adds a new Reason enum that Chrome OS uses to identify and ignore audio- or video-triggered PowerSaveBlockers if needed. BUG=354723, 176405 TEST=watch /var/log/power_manager/powerd.LATEST on chrome os while on the receiving end of a screen-sharing hangout and observe that a policy is sent forcing the screen on Committed: https://crrev.com/f3c302a352d284298d790823f276c3efe943a9b6 Cr-Commit-Position: refs/heads/master@{#317612}

Patch Set 1 #

Patch Set 2 : fix compilation and add tests #

Patch Set 3 : fix ephemeral_app_browsertest #

Patch Set 4 : update a comment #

Total comments: 14

Patch Set 5 : merge #

Patch Set 6 : apply nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+328 lines, -133 lines) Patch
M chrome/browser/apps/ephemeral_app_browsertest.cc View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/drive/drive_uploader.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/media/cast_transport_host_filter.cc View 1 chunk +5 lines, -3 lines 0 comments Download
M chromeos/dbus/power_policy_controller.h View 1 2 3 4 5 4 chunks +44 lines, -13 lines 0 comments Download
M chromeos/dbus/power_policy_controller.cc View 1 2 3 4 5 6 chunks +67 lines, -26 lines 0 comments Download
M chromeos/dbus/power_policy_controller_unittest.cc View 1 5 chunks +76 lines, -8 lines 0 comments Download
M content/browser/devtools/render_frame_devtools_agent_host.cc View 1 chunk +4 lines, -5 lines 0 comments Download
M content/browser/download/download_browsertest.cc View 1 2 3 4 2 chunks +6 lines, -8 lines 0 comments Download
M content/browser/download/download_resource_handler.cc View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/loader/power_save_block_resource_throttle.cc View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/media/capture/desktop_capture_device.cc View 1 chunk +5 lines, -3 lines 0 comments Download
M content/browser/media/capture/desktop_capture_device_aura.cc View 1 chunk +5 lines, -3 lines 0 comments Download
M content/browser/media/webrtc_internals.cc View 1 chunk +5 lines, -3 lines 0 comments Download
M content/browser/power_save_blocker_android.cc View 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/power_save_blocker_chromeos.cc View 4 chunks +32 lines, -9 lines 0 comments Download
M content/browser/power_save_blocker_impl.h View 2 chunks +5 lines, -1 line 0 comments Download
M content/browser/power_save_blocker_impl.cc View 1 chunk +4 lines, -2 lines 0 comments Download
M content/browser/power_save_blocker_mac.cc View 4 chunks +12 lines, -11 lines 0 comments Download
M content/browser/power_save_blocker_ozone.cc View 1 chunk +2 lines, -1 line 0 comments Download
M content/browser/power_save_blocker_win.cc View 6 chunks +12 lines, -9 lines 0 comments Download
M content/browser/power_save_blocker_x11.cc View 6 chunks +10 lines, -9 lines 0 comments Download
M content/browser/web_contents/web_contents_impl.cc View 1 2 3 4 1 chunk +4 lines, -8 lines 0 comments Download
M content/public/browser/power_save_blocker.h View 1 chunk +15 lines, -2 lines 0 comments Download
M extensions/browser/api/power/power_api_manager.h View 1 chunk +1 line, -0 lines 0 comments Download
M extensions/browser/api/power/power_api_manager.cc View 2 chunks +4 lines, -2 lines 0 comments Download
M extensions/browser/api/power/power_api_unittest.cc View 1 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 13 (3 generated)
Daniel Erat
jochen, can you review chrome/ and content/? the only substantial change is the addition of ...
5 years, 10 months ago (2015-02-20 22:52:56 UTC) #2
Ken Rockot(use gerrit already)
nit: not exciting enough extensions lgtm
5 years, 10 months ago (2015-02-20 23:04:12 UTC) #3
jochen (gone - plz use gerrit)
lgtm
5 years, 10 months ago (2015-02-23 12:13:44 UTC) #4
bartfab (slow)
chromeos/dbus LGTM https://codereview.chromium.org/946643002/diff/60001/chromeos/dbus/power_policy_controller.cc File chromeos/dbus/power_policy_controller.cc (right): https://codereview.chromium.org/946643002/diff/60001/chromeos/dbus/power_policy_controller.cc#newcode68 chromeos/dbus/power_policy_controller.cc:68: !use_audio_activity) Nit: Add {}. https://codereview.chromium.org/946643002/diff/60001/chromeos/dbus/power_policy_controller.cc#newcode71 chromeos/dbus/power_policy_controller.cc:71: !use_video_activity) ...
5 years, 10 months ago (2015-02-23 13:56:50 UTC) #5
Daniel Erat
https://codereview.chromium.org/946643002/diff/60001/chromeos/dbus/power_policy_controller.cc File chromeos/dbus/power_policy_controller.cc (right): https://codereview.chromium.org/946643002/diff/60001/chromeos/dbus/power_policy_controller.cc#newcode68 chromeos/dbus/power_policy_controller.cc:68: !use_audio_activity) On 2015/02/23 13:56:49, bartfab wrote: > Nit: Add ...
5 years, 10 months ago (2015-02-23 16:34:05 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/946643002/100001
5 years, 10 months ago (2015-02-23 16:36:09 UTC) #9
commit-bot: I haz the power
Committed patchset #6 (id:100001)
5 years, 10 months ago (2015-02-23 18:33:10 UTC) #10
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/f3c302a352d284298d790823f276c3efe943a9b6 Cr-Commit-Position: refs/heads/master@{#317612}
5 years, 10 months ago (2015-02-23 18:34:06 UTC) #11
DaleCurtis
You'll also want to remove this #define from the unittest: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/web_contents/web_contents_impl_unittest.cc&l=2882
5 years, 10 months ago (2015-02-23 18:40:15 UTC) #12
Daniel Erat
5 years, 10 months ago (2015-02-23 21:42:39 UTC) #13
Message was sent while issue was closed. 
On 2015/02/23 18:40:15, DaleCurtis wrote:
> You'll also want to remove this #define from the unittest:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...

thanks! i'll send you a change to do that soon.

Powered by Google App Engine
This is Rietveld 408576698