Add is_fullscreen to video updates

Add is_fullscreen to video updates
    
This is a re-attempt at http://codereview.chromium.org/10916123/, which also ended up being reverted.
    
This was reverted due to a failing unit test on Win_Aura. Through an unfortunate set of mistakes this builder was not in the default try set or the CQ set, but could close the tree leading to the revert. Through testing I have been able to reproduce the failure on linux_chromeos, so it might be a flakey failure, since it passed sometimes on that bot. This CL is basically the same as the CL list above, except that there is now a window->Focus() call in the unit test, since the reason the test was failing was when it was being made full screen it ceased to visible. This fixes the issue within this test. I will added a comment about this on the bug tracking the underlying issue.
    
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 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 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=156008

[rharrison, 2012-09-07 17:00:01]

Sorry for having to upload this CL yet again. It was reverted due to a failure
on Win_Aura. I have disabled the failing test on Win_Aura, because it is testing
functionality that isn't used on that platforms. The tests pass on the cros
platforms where the functionality is used. Once this is landed I will create a
bug to investigate why this test is broken in this case, but I don't think at
the moment it is a high priority item.

[flackr, 2012-09-07 17:06:17]

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

http://codereview.chromium.org/10913134/diff/1/ash/wm/video_detector_unittest...
ash/wm/video_detector_unittest.cc:241: TEST_F(VideoDetectorTest,
DISABLED_FullscreenWindow) {
Typically I've seen us use the following pattern for disabling failing tests on
a platform:
#if defined(OS_WIN)
#define MAYBE_FullscreenWindow DISABLED_FullscreenWindow
#else
#define MAYBE_FullscreenWindow FullscreenWindow
#endif

TEST_F(VideoDetectorTest, MAYBE_FullscreenWindow) {
...

I assume this is to keep brace matching and other code parsing correct.

[Daniel Erat, 2012-09-07 17:07:09]

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

http://codereview.chromium.org/10913134/diff/1/ash/wm/video_detector.cc#newco...
ash/wm/video_detector.cc:107: bool is_fullscreen =
wm::IsWindowFullscreen(window);
Does this method work differently on Windows than on Linux?  It looks like the
test is setting ui::SHOW_STATE_FULLSCREEN, so I'm unclear on why this would be.

Regardless of whether this code is needed on Windows, it's still surprising that
it doesn't work there.  Please file a bug to figure out why it's not working and
enable the test on Windows.

[rharrison, 2012-09-07 18:01:22]

On 2012/09/07 17:07:09, Daniel Erat wrote:
> http://codereview.chromium.org/10913134/diff/1/ash/wm/video_detector.cc
> File ash/wm/video_detector.cc (right):
> 
>
http://codereview.chromium.org/10913134/diff/1/ash/wm/video_detector.cc#newco...
> ash/wm/video_detector.cc:107: bool is_fullscreen =
> wm::IsWindowFullscreen(window);
> Does this method work differently on Windows than on Linux?  It looks like the
> test is setting ui::SHOW_STATE_FULLSCREEN, so I'm unclear on why this would
be.
> 
> Regardless of whether this code is needed on Windows, it's still surprising
that
> it doesn't work there.  Please file a bug to figure out why it's not working
and
> enable the test on Windows.

I really don't understand why there is a difference between the linux and window
builds. I have looked at the code a bit and there is no obvious defines changing
the build. Running the test in isolation still fails, so there shouldn't be
pollution from other tests. If you look I actually used the test above as a
template, so the only difference is the property set. If you look at the
failure, it is not merely failing to report the fullscreen state, it appears to
not detect video at all.

I in principle do not disagree with the enable/fail and file a bug idea, but
WinAura closes the tree currently, so if I land it like that I will break the
tree again and get reverted again. My understanding is that WinAura is not
supposed to be a tree closer and have filed a bug about this
https://code.google.com/p/chromium/issues/detail?id=147228.

[Daniel Erat, 2012-09-07 18:08:49]

On 2012/09/07 18:01:22, rharrison wrote:
> On 2012/09/07 17:07:09, Daniel Erat wrote:
> > http://codereview.chromium.org/10913134/diff/1/ash/wm/video_detector.cc
> > File ash/wm/video_detector.cc (right):
> > 
> >
>
http://codereview.chromium.org/10913134/diff/1/ash/wm/video_detector.cc#newco...
> > ash/wm/video_detector.cc:107: bool is_fullscreen =
> > wm::IsWindowFullscreen(window);
> > Does this method work differently on Windows than on Linux?  It looks like
the
> > test is setting ui::SHOW_STATE_FULLSCREEN, so I'm unclear on why this would
> be.
> > 
> > Regardless of whether this code is needed on Windows, it's still surprising
> that
> > it doesn't work there.  Please file a bug to figure out why it's not working
> and
> > enable the test on Windows.
> 
> I really don't understand why there is a difference between the linux and
window
> builds. I have looked at the code a bit and there is no obvious defines
changing
> the build. Running the test in isolation still fails, so there shouldn't be
> pollution from other tests. If you look I actually used the test above as a
> template, so the only difference is the property set. If you look at the
> failure, it is not merely failing to report the fullscreen state, it appears
to
> not detect video at all.
> 
> I in principle do not disagree with the enable/fail and file a bug idea, but
> WinAura closes the tree currently, so if I land it like that I will break the
> tree again and get reverted again. My understanding is that WinAura is not
> supposed to be a tree closer and have filed a bug about this
> https://code.google.com/p/chromium/issues/detail?id=147228.

Thanks, but please also file a bug about figuring out why the fullscreen state
isn't working on Windows and re-enabling the new test there.

[rharrison, 2012-09-07 18:18:31]

On 2012/09/07 18:08:49, Daniel Erat wrote:
> On 2012/09/07 18:01:22, rharrison wrote:
> > On 2012/09/07 17:07:09, Daniel Erat wrote:
> > > http://codereview.chromium.org/10913134/diff/1/ash/wm/video_detector.cc
> > > File ash/wm/video_detector.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10913134/diff/1/ash/wm/video_detector.cc#newco...
> > > ash/wm/video_detector.cc:107: bool is_fullscreen =
> > > wm::IsWindowFullscreen(window);
> > > Does this method work differently on Windows than on Linux?  It looks like
> the
> > > test is setting ui::SHOW_STATE_FULLSCREEN, so I'm unclear on why this
would
> > be.
> > > 
> > > Regardless of whether this code is needed on Windows, it's still
surprising
> > that
> > > it doesn't work there.  Please file a bug to figure out why it's not
working
> > and
> > > enable the test on Windows.
> > 
> > I really don't understand why there is a difference between the linux and
> window
> > builds. I have looked at the code a bit and there is no obvious defines
> changing
> > the build. Running the test in isolation still fails, so there shouldn't be
> > pollution from other tests. If you look I actually used the test above as a
> > template, so the only difference is the property set. If you look at the
> > failure, it is not merely failing to report the fullscreen state, it appears
> to
> > not detect video at all.
> > 
> > I in principle do not disagree with the enable/fail and file a bug idea, but
> > WinAura closes the tree currently, so if I land it like that I will break
the
> > tree again and get reverted again. My understanding is that WinAura is not
> > supposed to be a tree closer and have filed a bug about this
> > https://code.google.com/p/chromium/issues/detail?id=147228.
> 
> Thanks, but please also file a bug about figuring out why the fullscreen state
> isn't working on Windows and re-enabling the new test there.

Sorry, I misunderstood your previous comment. I thought you wanted the test
re-enabled in this CL. A bug to fix and re-enable the test work has been filed
as https://code.google.com/p/chromium/issues/detail?id=147310

[rharrison, 2012-09-07 18:20:55]

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

http://codereview.chromium.org/10913134/diff/1/ash/wm/video_detector_unittest...
ash/wm/video_detector_unittest.cc:241: TEST_F(VideoDetectorTest,
DISABLED_FullscreenWindow) {
On 2012/09/07 17:06:18, flackr wrote:
> Typically I've seen us use the following pattern for disabling failing tests
on
> a platform:
> #if defined(OS_WIN)
> #define MAYBE_FullscreenWindow DISABLED_FullscreenWindow
> #else
> #define MAYBE_FullscreenWindow FullscreenWindow
> #endif
> 
> TEST_F(VideoDetectorTest, MAYBE_FullscreenWindow) {
> ...
> 
> I assume this is to keep brace matching and other code parsing correct.

Done.

[Daniel Erat, 2012-09-07 18:28:24]

lgtm

[flackr, 2012-09-07 19:16:48]

LGTM

nit: Could reference the bug before the disable define block (similar to
http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/han...).

[rharrison, 2012-09-10 18:18:04]

On 2012/09/07 19:16:48, flackr wrote:
> LGTM
> 
> nit: Could reference the bug before the disable define block (similar to
>
http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/han...).

cros_daisy is sick, http://code.google.com/p/chromium-os/issues/detail?id=34275,
waiting on the fix to run the try and then hit the CQ

[commit-bot: I haz the power, 2012-09-11 13:22:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/[email protected]/10913134/2003

[commit-bot: I haz the power, 2012-09-11 13:23:32]

Change committed as 156008