Skip mulitple-dispay tests on win8

Skip mulitple-display tests on win8
Re-enable tests that should pass on win7.

BUG=247427
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205846

[scottmg, 2013-06-10 20:49:46]

Seems fine if a bit ugly, but I'm going to defer to +robertshield who did the
most recent digging in ash tests on win8.

[robertshield, 2013-06-10 21:13:58]

A nicer way to do this might be to add a static
DisplayManager::IsSingleDisplay() method or similar that returns true on Win8
and false everywhere else. What do you think?

https://codereview.chromium.org/16539005/diff/21001/ash/test/ash_test_base.h
File ash/test/ash_test_base.h (right):

https://codereview.chromium.org/16539005/diff/21001/ash/test/ash_test_base.h#...
ash/test/ash_test_base.h:125: // Utility macros to skip the all or part of a
test that fails on Win8 bots
ni: the all -> all

https://codereview.chromium.org/16539005/diff/21001/ash/test/ash_test_base.h#...
ash/test/ash_test_base.h:127: // Please refer to crbug.com/247427 for more
informatino about
nit: information

[oshima, 2013-06-10 21:29:43]

On 2013/06/10 21:13:58, robertshield wrote:
> A nicer way to do this might be to add a static
> DisplayManager::IsSingleDisplay() method or similar that returns true on Win8
> and false everywhere else. What do you think?

That's actually what it's doing, but with different name + macro.

Regarding method name, IsSingleDisplay is a bit misleading. How about

DisplayManager::SupportsMulti(ple)Displays()?

I used macro because it's simple enough and only in test code, but I can remove
macro if you feel strongly about it.

> 
> https://codereview.chromium.org/16539005/diff/21001/ash/test/ash_test_base.h
> File ash/test/ash_test_base.h (right):
> 
>
https://codereview.chromium.org/16539005/diff/21001/ash/test/ash_test_base.h#...
> ash/test/ash_test_base.h:125: // Utility macros to skip the all or part of a
> test that fails on Win8 bots
> ni: the all -> all
> 
>
https://codereview.chromium.org/16539005/diff/21001/ash/test/ash_test_base.h#...
> ash/test/ash_test_base.h:127: // Please refer to crbug.com/247427 for more
> informatino about
> nit: information

[James Cook, 2013-06-10 22:07:49]

I don't like the macro - I would much rather have lots of copies of:

if (!SupportsMultipleDisplays())
  return;

[robertshield, 2013-06-11 03:15:28]

On 2013/06/10 21:29:43, oshima wrote:
> On 2013/06/10 21:13:58, robertshield wrote:
> > A nicer way to do this might be to add a static
> > DisplayManager::IsSingleDisplay() method or similar that returns true on
Win8
> > and false everywhere else. What do you think?
> 
> That's actually what it's doing, but with different name + macro.
> 
> Regarding method name, IsSingleDisplay is a bit misleading. How about
> 
> DisplayManager::SupportsMulti(ple)Displays()?

That's a much better name than what I suggested. Mild preference for "Multiple"
rather than "Multi".

> 
> I used macro because it's simple enough and only in test code, but I can
remove
> macro if you feel strongly about it.
> 

Would prefer to not have the macro, it's not as clear as an explicit call for a
first time reader.

> > 
> > https://codereview.chromium.org/16539005/diff/21001/ash/test/ash_test_base.h
> > File ash/test/ash_test_base.h (right):
> > 
> >
>
https://codereview.chromium.org/16539005/diff/21001/ash/test/ash_test_base.h#...
> > ash/test/ash_test_base.h:125: // Utility macros to skip the all or part of a
> > test that fails on Win8 bots
> > ni: the all -> all
> > 
> >
>
https://codereview.chromium.org/16539005/diff/21001/ash/test/ash_test_base.h#...
> > ash/test/ash_test_base.h:127: // Please refer to crbug.com/247427 for more
> > informatino about
> > nit: information

[oshima, 2013-06-11 17:24:48]

ok done. PTAL.

[robertshield, 2013-06-11 17:28:30]

lgtm

https://codereview.chromium.org/16539005/diff/38001/ash/display/display_contr...
File ash/display/display_controller_unittest.cc (right):

https://codereview.chromium.org/16539005/diff/38001/ash/display/display_contr...
ash/display/display_controller_unittest.cc:95: return;
nit: indent

[oshima, 2013-06-11 17:34:23]

https://codereview.chromium.org/16539005/diff/38001/ash/display/display_contr...
File ash/display/display_controller_unittest.cc (right):

https://codereview.chromium.org/16539005/diff/38001/ash/display/display_contr...
ash/display/display_controller_unittest.cc:95: return;
On 2013/06/11 17:28:31, robertshield wrote:
> nit: indent

Done.

[James Cook, 2013-06-11 17:48:06]

LGTM after comments addressed.  Please change the title to "Skip
multi-display...."

https://codereview.chromium.org/16539005/diff/44001/ash/display/display_manag...
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/16539005/diff/44001/ash/display/display_manag...
ash/display/display_manager.cc:428: "skip (don't disable) the test using
|SupportMultiDisplay()|";
SupportsMultipleDisplays()

https://codereview.chromium.org/16539005/diff/44001/ash/test/ash_test_base.h
File ash/test/ash_test_base.h (right):

https://codereview.chromium.org/16539005/diff/44001/ash/test/ash_test_base.h#...
ash/test/ash_test_base.h:98: static bool SupportsMultipleDisplays();
Needs function comment.

[oshima, 2013-06-12 00:12:12]

https://codereview.chromium.org/16539005/diff/44001/ash/display/display_manag...
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/16539005/diff/44001/ash/display/display_manag...
ash/display/display_manager.cc:428: "skip (don't disable) the test using
|SupportMultiDisplay()|";
On 2013/06/11 17:48:07, James Cook (Chromium) wrote:
> SupportsMultipleDisplays()

Done.

https://codereview.chromium.org/16539005/diff/44001/ash/test/ash_test_base.h
File ash/test/ash_test_base.h (right):

https://codereview.chromium.org/16539005/diff/44001/ash/test/ash_test_base.h#...
ash/test/ash_test_base.h:98: static bool SupportsMultipleDisplays();
On 2013/06/11 17:48:07, James Cook (Chromium) wrote:
> Needs function comment.

Done.

[commit-bot: I haz the power, 2013-06-12 00:14:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/16539005/48001

[commit-bot: I haz the power, 2013-06-12 03:39:44]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-06-12 06:46:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/16539005/48001

[commit-bot: I haz the power, 2013-06-12 17:21:19]

Change committed as 205846