Add is_fullscreen to video updates

Add is_fullscreen to video updates
    
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 every time 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 test 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.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154905

[rharrison, 2012-08-30 19:09:02]

This CL is probably not ready for review yet, until I have the DEPS roll done to
pick up the protobuf I am using it will not build. I will update when I have
successfully built it.

flackr, general review since I have been talking with you about the CL

sky, I added you since you suggested the change in design from my previous CL,
so I wanted to make sure this was an acceptable implementation

satorux, mostly D-Bus usage review

derat, I am making changes and doing in clean up in code you wrote

[rharrison, 2012-08-30 19:16:06]

http://codereview.chromium.org/10905026/diff/1/chromeos/dbus/power_manager_cl...
File chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/10905026/diff/1/chromeos/dbus/power_manager_cl...
chromeos/dbus/power_manager_client.cc:235: VideoDetectUpdate protobuf;
I am going to change the name of this protobuf to VideoActivityUpdate, since
that makes more sense given the context.

[Daniel Erat, 2012-08-30 19:27:10]

See http://crbug.com/114128, which concerns adding still more suspend-blocking
to Chrome.  Since we already have separate messages for video activity and user
activity, I think it may be better to unify all of this before adding more to
it.

Thoughts about a singleton named something like PowerManagementNotifier that
gets told about video activity, user activity, etc. and tells powerd about it
via a more-detailed protocol buffer that describes the exact type of activity
that has been seen?  Suggests about whether we should keep the current
notify-every-five-seconds-over-D-Bus-while-there's-activity approach or do
something different?

[rharrison, 2012-08-30 19:46:55]

On 2012/08/30 19:27:10, Daniel Erat wrote:
> See http://crbug.com/114128, which concerns adding still more suspend-blocking
> to Chrome.  Since we already have separate messages for video activity and
user
> activity, I think it may be better to unify all of this before adding more to
> it.
> 
> Thoughts about a singleton named something like PowerManagementNotifier that
> gets told about video activity, user activity, etc. and tells powerd about it
> via a more-detailed protocol buffer that describes the exact type of activity
> that has been seen?  Suggests about whether we should keep the current
> notify-every-five-seconds-over-D-Bus-while-there's-activity approach or do
> something different?

I agree that unifying all of these notifications into a single message would be
the right way forward. Given the heavy usage of observer patterns in Chrome I
think it make sense to have a single notifier class as you have suggested.

I am personally preferential to attempting some sort of event driven approach
with a rate limit by type of notification, i.e. only transition the video state
at 1Hz or something like that. The main reason I am interested in this is for
things like the keyboard backlight control that I am working on I want to keep
the latency relatively low, though a max 5 or N second delay might be
acceptable. 

There is a delay in time between this paragraph and the previous one. The more I
think on it, the more I suspect the notify periodically approach is workable
since it is not a delay on the start of a state, but more of a delay on the end.
We get relatively immediate information when video occurs, so we can make sure
to not suspend or what ever, but it doesn't matter if it takes a little more
time to suspend after the video is done. Of course I think we would need to go
through the notifications and the behaviours that they generate to make sure
this is generally true.

[Daniel Erat, 2012-08-30 20:02:32]

On 2012/08/30 19:46:55, rharrison wrote:
> On 2012/08/30 19:27:10, Daniel Erat wrote:
> > See http://crbug.com/114128, which concerns adding still more
suspend-blocking
> > to Chrome.  Since we already have separate messages for video activity and
> user
> > activity, I think it may be better to unify all of this before adding more
to
> > it.
> > 
> > Thoughts about a singleton named something like PowerManagementNotifier that
> > gets told about video activity, user activity, etc. and tells powerd about
it
> > via a more-detailed protocol buffer that describes the exact type of
activity
> > that has been seen?  Suggests about whether we should keep the current
> > notify-every-five-seconds-over-D-Bus-while-there's-activity approach or do
> > something different?
> 
> I agree that unifying all of these notifications into a single message would
be
> the right way forward. Given the heavy usage of observer patterns in Chrome I
> think it make sense to have a single notifier class as you have suggested.
> 
> I am personally preferential to attempting some sort of event driven approach
> with a rate limit by type of notification, i.e. only transition the video
state
> at 1Hz or something like that. The main reason I am interested in this is for
> things like the keyboard backlight control that I am working on I want to keep
> the latency relatively low, though a max 5 or N second delay might be
> acceptable. 
> 
> There is a delay in time between this paragraph and the previous one. The more
I
> think on it, the more I suspect the notify periodically approach is workable
> since it is not a delay on the start of a state, but more of a delay on the
end.
> We get relatively immediate information when video occurs, so we can make sure
> to not suspend or what ever, but it doesn't matter if it takes a little more
> time to suspend after the video is done. Of course I think we would need to go
> through the notifications and the behaviours that they generate to make sure
> this is generally true.

Yes, this was my reasoning when going with the notify-periodically approach. 
For both video and user activity, we want immediate notification when the
activity begins (so we can undim the screen) but don't care about inaccuracies
with regard to when it ends.

Additionally, in the case where activity stops and then restarts soon
afterwards, we can deal with a delay in the second activity notification ... or
so I thought before seeing http://crbug.com/143706 (see explanation in comment
#5). :-/  In that particular case, Chrome piggybacking on powerd's active/idle
notifications seems like a source of future bugs anyway, so I was okay with just
not supporting it.  Note that the dim-keyboard-for-fullscreen-video case may be
affected similarly, though -- if the user is watching a video, stops, and then
restarts it less than five seconds later, there will be a delay before the
second notification is sent.  We should be able to work around this in Chrome by
resetting the notification delay when we see the video activity stop, so we can
notify immediately when it starts again.

The other reason why I went with periodic notifications instead of notifying
just at active/idle transitions was to avoid powerd ending up indefinitely in
the wrong state if it crashes and gets restarted.  Chrome doesn't currently
expose any D-Bus method calls, as far as I'm aware, so there's no easy way for
powerd to query the current state at startup.  (It could emit a D-Bus signal
that tells Chrome to send the current state, though.)

[Daniel Erat, 2012-08-30 20:33:59]

http://codereview.chromium.org/10905026/diff/1/ash/wm/mock_video_detector_obs...
File ash/wm/mock_video_detector_observer.h (right):

http://codereview.chromium.org/10905026/diff/1/ash/wm/mock_video_detector_obs...
ash/wm/mock_video_detector_observer.h:15: class MockVideoDetectorObserver :
public VideoDetectorObserver {
i dislike gmock (i think that its code is hard to read, and that it spams logs,
and that it often encourages testing of minutia of behavior that we don't
actually care about).  that said, the use here seems legitimate (you're testing
whether the fullscreen parameter is true or not).

beng@ points out that there's no use of gmock in ash currently and suggests that
he'd like to keep things that way.  would you object to tracking the number of
fullscreen/non-fullscreen calls in the existing mock class instead of adding a
gmock implementation?

http://codereview.chromium.org/10905026/diff/1/ash/wm/video_detector.cc
File ash/wm/video_detector.cc (right):

http://codereview.chromium.org/10905026/diff/1/ash/wm/video_detector.cc#newco...
ash/wm/video_detector.cc:106: bool is_fullscreen = (ui::SHOW_STATE_FULLSCREEN ==
nit: include ash/wm/window_util.h and use IsWindowFullscreen()

http://codereview.chromium.org/10905026/diff/1/chrome/browser/chromeos/power/...
File chrome/browser/chromeos/power/video_activity_notifier.cc (right):

http://codereview.chromium.org/10905026/diff/1/chrome/browser/chromeos/power/...
chrome/browser/chromeos/power/video_activity_notifier.cc:14: const int
kNotifyIntervalSec = 5;
you should delete this if you're not using it, but see below.

http://codereview.chromium.org/10905026/diff/1/chrome/browser/chromeos/power/...
chrome/browser/chromeos/power/video_activity_notifier.cc:30:
DBusThreadManager::Get()->GetPowerManagerClient()->
the presence of two rate limits was intentional (well, maybe not intentional,
but known).  within chrome, i wanted to rate-limit how frequently observers get
called; here, i wanted to further rate-limit how frequently we send messages
over d-bus (which is presumably a more-costly action).

i think that the rate limit here should be kept, at the very least, since this
code is better-informed about how frequently we should send d-bus messages than
the upstream code in VideoDetector.

http://codereview.chromium.org/10905026/diff/1/chromeos/dbus/power_manager_cl...
File chromeos/dbus/power_manager_client.cc (right):

http://codereview.chromium.org/10905026/diff/1/chromeos/dbus/power_manager_cl...
chromeos/dbus/power_manager_client.cc:235: VideoDetectUpdate protobuf;
On 2012/08/30 19:16:07, rharrison wrote:
> I am going to change the name of this protobuf to VideoActivityUpdate, since
> that makes more sense given the context.

agreed

[rharrison, 2012-08-31 18:57:13]

http://codereview.chromium.org/10905026/diff/1/ash/wm/mock_video_detector_obs...
File ash/wm/mock_video_detector_observer.h (right):

http://codereview.chromium.org/10905026/diff/1/ash/wm/mock_video_detector_obs...
ash/wm/mock_video_detector_observer.h:15: class MockVideoDetectorObserver :
public VideoDetectorObserver {
On 2012/08/30 20:33:59, Daniel Erat wrote:
> i dislike gmock (i think that its code is hard to read, and that it spams
logs,
> and that it often encourages testing of minutia of behavior that we don't
> actually care about).  that said, the use here seems legitimate (you're
testing
> whether the fullscreen parameter is true or not).
> 
> beng@ points out that there's no use of gmock in ash currently and suggests
that
> he'd like to keep things that way.  would you object to tracking the number of
> fullscreen/non-fullscreen calls in the existing mock class instead of adding a
> gmock implementation?

Since the preference is to not use it I am fine with changing this.

http://codereview.chromium.org/10905026/diff/1/ash/wm/video_detector.cc
File ash/wm/video_detector.cc (right):

http://codereview.chromium.org/10905026/diff/1/ash/wm/video_detector.cc#newco...
ash/wm/video_detector.cc:106: bool is_fullscreen = (ui::SHOW_STATE_FULLSCREEN ==
On 2012/08/30 20:33:59, Daniel Erat wrote:
> nit: include ash/wm/window_util.h and use IsWindowFullscreen()

Done.

http://codereview.chromium.org/10905026/diff/1/chrome/browser/chromeos/power/...
File chrome/browser/chromeos/power/video_activity_notifier.cc (right):

http://codereview.chromium.org/10905026/diff/1/chrome/browser/chromeos/power/...
chrome/browser/chromeos/power/video_activity_notifier.cc:30:
DBusThreadManager::Get()->GetPowerManagerClient()->
On 2012/08/30 20:33:59, Daniel Erat wrote:
> the presence of two rate limits was intentional (well, maybe not intentional,
> but known).  within chrome, i wanted to rate-limit how frequently observers
get
> called; here, i wanted to further rate-limit how frequently we send messages
> over d-bus (which is presumably a more-costly action).
> 
> i think that the rate limit here should be kept, at the very least, since this
> code is better-informed about how frequently we should send d-bus messages
than
> the upstream code in VideoDetector.

I have reverted this removal.

[rharrison, 2012-08-31 18:57:48]

I have confirmed that this builds now. I am waiting on completing the CrOS side
to finish testing.

[Daniel Erat, 2012-08-31 19:15:57]

lgtm

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
File ash/wm/video_detector_unittest.cc (right):

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:111: // Send not-quite-enough adaquately-sized
updates.
nit: re-add blank line before this one and fix indenting

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:120: // We should get notified after the next
update, but not in response to
ditto

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:130: // Spread out the frames over two
seconds; we shouldn't detect video.
ditto

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:140: TEST_F(VideoDetectorTest,
WindowNotVisible) {
ditto

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:158: // Make the window visible and send more
updates.
ditto

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:167: // We also shouldn't report video in a
window that's fully offscreen.
ditto

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:179: TEST_F(VideoDetectorTest,
MultipleWindows) {
ditto

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:238: -               
VideoDetector::kMinUpdateHeight));
editor mishap?  it looks like you have a minus sign and a bunch of spaces at the
beginning of this line

[rharrison, 2012-09-04 20:12:56]

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
File ash/wm/video_detector_unittest.cc (right):

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:111: // Send not-quite-enough adaquately-sized
updates.
On 2012/08/31 19:15:58, Daniel Erat wrote:
> nit: re-add blank line before this one and fix indenting

Done.

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:120: // We should get notified after the next
update, but not in response to
On 2012/08/31 19:15:58, Daniel Erat wrote:
> ditto

Done.

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:130: // Spread out the frames over two
seconds; we shouldn't detect video.
On 2012/08/31 19:15:58, Daniel Erat wrote:
> ditto

Done.

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:140: TEST_F(VideoDetectorTest,
WindowNotVisible) {
On 2012/08/31 19:15:58, Daniel Erat wrote:
> ditto

Done.

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:158: // Make the window visible and send more
updates.
On 2012/08/31 19:15:58, Daniel Erat wrote:
> ditto

Done.

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:167: // We also shouldn't report video in a
window that's fully offscreen.
On 2012/08/31 19:15:58, Daniel Erat wrote:
> ditto

Done.

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:179: TEST_F(VideoDetectorTest,
MultipleWindows) {
On 2012/08/31 19:15:58, Daniel Erat wrote:
> ditto

Done.

http://codereview.chromium.org/10905026/diff/1010/ash/wm/video_detector_unitt...
ash/wm/video_detector_unittest.cc:238: -               
VideoDetector::kMinUpdateHeight));
On 2012/08/31 19:15:58, Daniel Erat wrote:
> editor mishap?  it looks like you have a minus sign and a bunch of spaces at
the
> beginning of this line

I suspect all of these issues come from me fouling up something when using diff
mode in emacs to remove my gmock changes. Thanks for catching all of this.

[flackr, 2012-09-05 00:27:33]

lgtm

[commit-bot: I haz the power, 2012-09-05 04:51:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rharrison@chromium.org/10905026/1012

[commit-bot: I haz the power, 2012-09-05 07:16:26]

Change committed as 154905