Automatically set the overscan insets if the output has the flag and no preference is set.

Automatically set the overscan insets if the output has the flag and no preference is set.


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

[oshima, 2013-01-12 02:06:38]

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.cc
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.c...
ash/display/display_manager.cc:79: return gfx::Insets(height, width, height,
width);
Is 5% good assumption?

Isn't it better to just let a user to set overscan if overscan info wasn't
found?

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.c...
ash/display/display_manager.cc:561: : overscan(false),
overscan_insets_specified(false) {
one argument per line (I think)

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.c...
ash/display/display_manager.cc:592: }
the code looks a bit odd. Won't 

ui::GetOutputOverscanFlag(outputs[i], &display_info_[id].overscan);

Work? (the comment of the function says it won't touch the flag when not found)

and nuke {}

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.h
File ash/display/display_manager.h (right):

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.h...
ash/display/display_manager.h:146: // overscan.
True if we detect that the display has overscan area. False if the display
doesn't have it, or failed to detect it.

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.h...
ash/display/display_manager.h:147: bool overscan;
has_overscan ?

(according to http://en.wikipedia.org/wiki/Overscan, "overscan" can mean the
area)

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.h...
ash/display/display_manager.h:151: bool overscan_insets_specified;
custom_overscan_insets ?

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.h...
ash/display/display_manager.h:153: DisplayInfo();
move this to top

[Jun Mukai, 2013-01-14 18:18:51]

also added test

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.cc
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.c...
ash/display/display_manager.cc:79: return gfx::Insets(height, width, height,
width);
On 2013/01/12 02:06:38, oshima wrote:
> Is 5% good assumption?
> 
> Isn't it better to just let a user to set overscan if overscan info wasn't
> found?

No info, there's only a bool flag and marcheu suggested 5% of overscan.  see
https://code.google.com/p/chromium/issues/detail?id=139419#c3

But I'm afraid I made mistake, 5% overscan means 2.5% for each edge.

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.c...
ash/display/display_manager.cc:561: : overscan(false),
overscan_insets_specified(false) {
On 2013/01/12 02:06:38, oshima wrote:
> one argument per line (I think)

Done.

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.c...
ash/display/display_manager.cc:592: }
On 2013/01/12 02:06:38, oshima wrote:
> the code looks a bit odd. Won't 
> 
> ui::GetOutputOverscanFlag(outputs[i], &display_info_[id].overscan);
> 
> Work? (the comment of the function says it won't touch the flag when not
found)
> 
> and nuke {}

Done.

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.h
File ash/display/display_manager.h (right):

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.h...
ash/display/display_manager.h:146: // overscan.
On 2013/01/12 02:06:38, oshima wrote:
> True if we detect that the display has overscan area. False if the display
> doesn't have it, or failed to detect it.

Done.

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.h...
ash/display/display_manager.h:147: bool overscan;
On 2013/01/12 02:06:38, oshima wrote:
> has_overscan ?
> 
> (according to http://en.wikipedia.org/wiki/Overscan, "overscan" can mean the
> area)

done.
# sometimes 'overscan' is used as verb, which is my intention. but
'has_overscan' would be better.

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.h...
ash/display/display_manager.h:151: bool overscan_insets_specified;
On 2013/01/12 02:06:38, oshima wrote:
> custom_overscan_insets ?

done.  has_custom_overscan_insets for consistency.

https://codereview.chromium.org/11818008/diff/1/ash/display/display_manager.h...
ash/display/display_manager.h:153: DisplayInfo();
On 2013/01/12 02:06:38, oshima wrote:
> move this to top

Done.

[Jun Mukai, 2013-01-15 23:21:14]

ping?

[oshima, 2013-01-15 23:26:51]

thank you for adding tests.
lgtm with nit

https://codereview.chromium.org/11818008/diff/2002/ash/display/display_manage...
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/11818008/diff/2002/ash/display/display_manage...
ash/display/display_manager.cc:76: // Currently we assume 5% overscan and hope
for the best.
add " if TV claims it overscan, but doesn't expose how much"

https://codereview.chromium.org/11818008/diff/2002/ash/display/display_manage...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/11818008/diff/2002/ash/display/display_manage...
ash/display/display_manager_unittest.cc:527: // Didn't specify OverscanInsets
but it has.
is this comment incomplete?

[Jun Mukai, 2013-01-15 23:43:57]

https://codereview.chromium.org/11818008/diff/2002/ash/display/display_manage...
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/11818008/diff/2002/ash/display/display_manage...
ash/display/display_manager.cc:76: // Currently we assume 5% overscan and hope
for the best.
On 2013/01/15 23:26:51, oshima wrote:
> add " if TV claims it overscan, but doesn't expose how much"

Done.

https://codereview.chromium.org/11818008/diff/2002/ash/display/display_manage...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/11818008/diff/2002/ash/display/display_manage...
ash/display/display_manager_unittest.cc:527: // Didn't specify OverscanInsets
but it has.
On 2013/01/15 23:26:51, oshima wrote:
> is this comment incomplete?

Done.

[Jun Mukai, 2013-01-15 23:44:38]

jamescook, can you review as OWNERS?

[James Cook, 2013-01-16 00:15:53]

LGTM with one optional suggestion for a comment. Thanks again for adding a test!

https://codereview.chromium.org/11818008/diff/13001/ash/display/display_manag...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/11818008/diff/13001/ash/display/display_manag...
ash/display/display_manager.h:153: // the user may specify an empty
intentionally.
"an empty" -> "an empty insets" ?

[Jun Mukai, 2013-01-16 01:13:44]

https://codereview.chromium.org/11818008/diff/13001/ash/display/display_manag...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/11818008/diff/13001/ash/display/display_manag...
ash/display/display_manager.h:153: // the user may specify an empty
intentionally.
On 2013/01/16 00:15:53, James Cook (Chromium) wrote:
> "an empty" -> "an empty insets" ?

Done.

[commit-bot: I haz the power, 2013-01-16 01:17:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/11818008/15002

[commit-bot: I haz the power, 2013-01-16 03:27:36]

Retried try job too often on win for step(s) compile

[commit-bot: I haz the power, 2013-01-16 21:39:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/11818008/15002

[commit-bot: I haz the power, 2013-01-16 22:39:43]

Change committed as 177236