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

Issue 10916123: Add is_fullscreen to video updates (Closed)

Created:
8 years, 3 months ago by rharrison
Modified:
8 years, 3 months ago
CC:
chromium-reviews, sadrul, nkostylev+watch_chromium.org, ben+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org
Visibility:
Public.

Description

Add is_fullscreen to video updates This is a re-attempt at http://codereview.chromium.org/10905026/, which ended up being reverted. The crash that caused the revert was actually due to powerd not being able to parse the protobuf correctly. https://gerrit.chromium.org/gerrit/32243/ corrects this issue, so once that CL lands this CL is good to go. I confirmed the combination of CLs works correctly. This is the original CL description: This adds a boolean to the video update message sent to powerd that indicates whether or not the video playing window is fullscreen'd. This is information used in powerd for controlling when to enable/disable keyboard backlights. Specifically if video is playing and fullscreen we assume that the user is doing something like watching a movie so will disable backlight. An additional change that I have made is to convert the message being sent from an int64 to a protobuffer that contains a int64 and a boolean. Currently, since the receiver of this message does not live in the same repo it means that everytime one wants to change the message format there is a small window where bad builds can be generated or one has to land two changes to the other repo, one to add handling of the new message format and then a second one to remove the old format. Converting to a protobuffer means that for common cases this issues are removed. This CL depends on CLs for power_manager (https://gerrit.chromium.org/gerrit/32092/), one for system_api (https://gerrit.chromium.org/gerrit/31916), and a DEPS roll(http://codereview.chromium.org/10915032/). There will be future CLs for power_manager that will use this information and more thourghly tests this code. BUG=chrome-os-partner:9203 TEST=Run updated unittests. Confirm there is no messages in the cros logs about this method being mishandled. Make sure that powerd is not crashing due to protocol buffer processing failing. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155224

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+95 lines, -17 lines) Patch
M ash/wm/video_detector.h View 1 chunk +1 line, -1 line 0 comments Download
M ash/wm/video_detector.cc View 2 chunks +5 lines, -1 line 0 comments Download
M ash/wm/video_detector_unittest.cc View 7 chunks +54 lines, -4 lines 0 comments Download
M chrome/browser/chromeos/power/video_activity_notifier.h View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/chromeos/power/video_activity_notifier.cc View 1 chunk +3 lines, -4 lines 0 comments Download
M chromeos/chromeos.gyp View 2 chunks +15 lines, -0 lines 0 comments Download
M chromeos/dbus/mock_power_manager_client.h View 1 chunk +1 line, -1 line 0 comments Download
M chromeos/dbus/power_manager_client.h View 1 chunk +4 lines, -2 lines 0 comments Download
M chromeos/dbus/power_manager_client.cc View 3 chunks +11 lines, -3 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
rharrison
8 years, 3 months ago (2012-09-05 18:31:26 UTC) #1
flackr
SLGTM
8 years, 3 months ago (2012-09-05 18:34:13 UTC) #2
Daniel Erat
If this is the same change as before, LGTM too.
8 years, 3 months ago (2012-09-05 18:40:11 UTC) #3
Nikita (slow)
lgtm
8 years, 3 months ago (2012-09-06 12:03:17 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rharrison@chromium.org/10916123/1
8 years, 3 months ago (2012-09-06 13:19:54 UTC) #5
commit-bot: I haz the power
Try job failure for 10916123-1 (retry) on win_rel for step "compile" (clobber build). It's a ...
8 years, 3 months ago (2012-09-06 14:47:01 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rharrison@chromium.org/10916123/1
8 years, 3 months ago (2012-09-06 14:50:36 UTC) #7
commit-bot: I haz the power
8 years, 3 months ago (2012-09-06 19:28:43 UTC) #8
Change committed as 155224

Powered by Google App Engine
This is Rietveld 408576698