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

Issue 10928037: Revert 155224 - Add is_fullscreen to video updates (Closed)

Created:
8 years, 3 months ago by benjhayden
Modified:
8 years, 3 months ago
Reviewers:
rharrison
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

Revert 155224 - Add is_fullscreen to video updates http://build.chromium.org/p/chromium/builders/Win%20Aura/builds/21280/steps/ash_unittests/logs/stdio#failure1 [ RUN ] VideoDetectorTest.FullscreenWindow wm\video_detector_unittest.cc(250): error: Value of: observer_->num_invocations() Actual: 0 Expected: 1 wm\video_detector_unittest.cc(251): error: Value of: observer_->num_fullscreens() Actual: 0 Expected: 1 [ FAILED ] VideoDetectorTest.FullscreenWindow (138 ms) 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. Review URL: https://chromiumcodereview.appspot.com/10916123 TBR=rharrison@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=155232

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -95 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 +1 line, -5 lines 0 comments Download
M ash/wm/video_detector_unittest.cc View 7 chunks +4 lines, -54 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 +4 lines, -3 lines 0 comments Download
M chromeos/chromeos.gyp View 2 chunks +0 lines, -15 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 +2 lines, -4 lines 0 comments Download
M chromeos/dbus/power_manager_client.cc View 3 chunks +3 lines, -11 lines 0 comments Download

Messages

Total messages: 1 (0 generated)
benjhayden
8 years, 3 months ago (2012-09-06 19:56:02 UTC) #1

          

Powered by Google App Engine
This is Rietveld 408576698