Overscan calibration UI.

Overscan calibration UI.

Puts arrows over the display rectangles and enables calibrating the display overscan by mouse clicks and touches.

BUG=139419
TEST=Login, connect secondary display, open chrome://settings/display, and play with its overscan calibrations.


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

[Jun Mukai, 2012-10-19 16:18:03]

oshima-san, can you review the C++ part of this?  Then I'll ask jhawkins for
webUI part.

[oshima, 2012-10-19 19:36:11]

http://codereview.chromium.org/11195036/diff/13/ash/display/multi_display_man...
File ash/display/multi_display_manager.cc (right):

http://codereview.chromium.org/11195036/diff/13/ash/display/multi_display_man...
ash/display/multi_display_manager.cc:167: return gfx::Insets();
return it != ... ? it->second() : gfx::Insetse();

http://codereview.chromium.org/11195036/diff/13/ash/display/multi_display_man...
File ash/display/multi_display_manager.h (right):

http://codereview.chromium.org/11195036/diff/13/ash/display/multi_display_man...
ash/display/multi_display_manager.h:82: // Returns an empty insets (0, 0, 0, 0)
if the display is not registered.
if no insets are specified for the display.

http://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/displ...
File chrome/browser/chromeos/display/overscan_calibrator.cc (right):

http://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/displ...
chrome/browser/chromeos/display/overscan_calibrator.cc:47:
inner_bounds_.Inset(insets);
Why this is necessary? Can you add comment?

http://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/displ...
chrome/browser/chromeos/display/overscan_calibrator.cc:54: const SkColor
transparent = SkColorSetARGB(0, 0, 0, 0);
static const
and move to the beginning
I actually wonder if there is a layer API that punch a hole on layer (android's
compositor had one). Can you ask piman?

http://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/displ...
chrome/browser/chromeos/display/overscan_calibrator.cc:59: float
device_scale_factor) {
We probably should cancel the operation when display
configuration has changed. Can you add TODO?

http://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/displ...
File chrome/browser/chromeos/display/overscan_calibrator.h (right):

http://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/displ...
chrome/browser/chromeos/display/overscan_calibrator.h:27: ~OverscanCalibrator();
virtual

http://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/displ...
chrome/browser/chromeos/display/overscan_calibrator.h:30: void Finish();
looks like this isn't used?

http://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/displ...
chrome/browser/chromeos/display/overscan_calibrator.h:44: gfx::Display display_;
const

http://codereview.chromium.org/11195036/diff/13/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

http://codereview.chromium.org/11195036/diff/13/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:137:
ash::internal::MultiDisplayManager* display_manager =
internal class shouldn't be used by non ash code.
Can you define GetOverscanInsets in DisplayController and use it from there?

http://codereview.chromium.org/11195036/diff/13/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:286: const
gfx::Display& display = display_manager->GetDisplayForId(display_id);
DCHECK(display.is_valid());

[Jun Mukai, 2012-10-19 21:39:34]

https://codereview.chromium.org/11195036/diff/13/ash/display/multi_display_ma...
File ash/display/multi_display_manager.cc (right):

https://codereview.chromium.org/11195036/diff/13/ash/display/multi_display_ma...
ash/display/multi_display_manager.cc:167: return gfx::Insets();
On 2012/10/19 19:36:11, oshima wrote:
> return it != ... ? it->second() : gfx::Insetse();

Done.

https://codereview.chromium.org/11195036/diff/13/ash/display/multi_display_ma...
File ash/display/multi_display_manager.h (right):

https://codereview.chromium.org/11195036/diff/13/ash/display/multi_display_ma...
ash/display/multi_display_manager.h:82: // Returns an empty insets (0, 0, 0, 0)
if the display is not registered.
On 2012/10/19 19:36:11, oshima wrote:
> if no insets are specified for the display.

Done.

https://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/disp...
File chrome/browser/chromeos/display/overscan_calibrator.cc (right):

https://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/disp...
chrome/browser/chromeos/display/overscan_calibrator.cc:47:
inner_bounds_.Inset(insets);
On 2012/10/19 19:36:11, oshima wrote:
> Why this is necessary? Can you add comment?

added.

https://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/disp...
chrome/browser/chromeos/display/overscan_calibrator.cc:54: const SkColor
transparent = SkColorSetARGB(0, 0, 0, 0);
On 2012/10/19 19:36:11, oshima wrote:
> static const
> and move to the beginning
> I actually wonder if there is a layer API that punch a hole on layer
(android's
> compositor had one). Can you ask piman?

done for static const.  I'll ask piman, thanks.

https://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/disp...
chrome/browser/chromeos/display/overscan_calibrator.cc:59: float
device_scale_factor) {
On 2012/10/19 19:36:11, oshima wrote:
> We probably should cancel the operation when display
> configuration has changed. Can you add TODO?
added TODO.

https://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/disp...
File chrome/browser/chromeos/display/overscan_calibrator.h (right):

https://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/disp...
chrome/browser/chromeos/display/overscan_calibrator.h:27: ~OverscanCalibrator();
On 2012/10/19 19:36:11, oshima wrote:
> virtual

Done.

https://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/disp...
chrome/browser/chromeos/display/overscan_calibrator.h:30: void Finish();
On 2012/10/19 19:36:11, oshima wrote:
> looks like this isn't used?

removed.

https://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/disp...
chrome/browser/chromeos/display/overscan_calibrator.h:44: gfx::Display display_;
On 2012/10/19 19:36:11, oshima wrote:
> const 

Done.

https://codereview.chromium.org/11195036/diff/13/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/11195036/diff/13/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:137:
ash::internal::MultiDisplayManager* display_manager =
On 2012/10/19 19:36:11, oshima wrote:
> internal class shouldn't be used by non ash code.
> Can you define GetOverscanInsets in DisplayController and use it from there?

Done.  Also I am using GetDisplayForId() below, so I defined this to ScreenAsh
too.

https://codereview.chromium.org/11195036/diff/13/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:286: const
gfx::Display& display = display_manager->GetDisplayForId(display_id);
On 2012/10/19 19:36:11, oshima wrote:
> DCHECK(display.is_valid());

Done.

[Jun Mukai, 2012-10-19 22:18:11]

https://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/disp...
File chrome/browser/chromeos/display/overscan_calibrator.cc (right):

https://codereview.chromium.org/11195036/diff/13/chrome/browser/chromeos/disp...
chrome/browser/chromeos/display/overscan_calibrator.cc:54: const SkColor
transparent = SkColorSetARGB(0, 0, 0, 0);
On 2012/10/19 21:39:34, Jun Mukai wrote:
> On 2012/10/19 19:36:11, oshima wrote:
> > static const
> > and move to the beginning
> > I actually wonder if there is a layer API that punch a hole on layer
> (android's
> > compositor had one). Can you ask piman?
> 
> done for static const.  I'll ask piman, thanks.

Just for recording: I asked piman for this, but no such mechanism in ui::Layer. 
So going with LayerDelegate would be the only way for now.

[oshima, 2012-10-19 23:05:33]

lgtm

https://codereview.chromium.org/11195036/diff/8002/ash/display/multi_display_...
File ash/display/multi_display_manager.h (right):

https://codereview.chromium.org/11195036/diff/8002/ash/display/multi_display_...
ash/display/multi_display_manager.h:84: gfx::Insets GetOverscanInsets(int64
display_id);
const; (same for displaycontroller)

[Jun Mukai, 2012-10-19 23:08:35]

https://codereview.chromium.org/11195036/diff/8002/ash/display/multi_display_...
File ash/display/multi_display_manager.h (right):

https://codereview.chromium.org/11195036/diff/8002/ash/display/multi_display_...
ash/display/multi_display_manager.h:84: gfx::Insets GetOverscanInsets(int64
display_id);
On 2012/10/19 23:05:33, oshima wrote:
> const; (same for displaycontroller)

Done.

[Jun Mukai, 2012-10-22 16:15:39]

jhawkins, could you take time to review the webui and its handler code?

The UI would look like this.
https://chromium.googlecode.com/issues/attachment?aid=1394190023000&name=Scre...

[James Hawkins, 2012-10-22 16:18:35]

On 2012/10/22 16:15:39, Jun Mukai wrote:
> jhawkins, could you take time to review the webui and its handler code?
> 

Shouldn't the OWNERs of options/chromeos review?

> The UI would look like this.
>
https://chromium.googlecode.com/issues/attachment?aid=1394190023000&name=Scre...

[Jun Mukai, 2012-10-22 17:51:18]

jhawkins, thanks for the suggestion.  xiyuan, could you review this?

[xiyuan, 2012-10-22 19:54:43]

Mostly nits.

http://codereview.chromium.org/11195036/diff/12002/chrome/browser/resources/o...
File chrome/browser/resources/options/chromeos/display_options.js (right):

http://codereview.chromium.org/11195036/diff/12002/chrome/browser/resources/o...
chrome/browser/resources/options/chromeos/display_options.js:63: var point =
getBoundsInPage(element.parentNode);
Should we use element.offsetParent instead?

Also think using a loop would be better than recursion.

http://codereview.chromium.org/11195036/diff/12002/chrome/browser/resources/o...
chrome/browser/resources/options/chromeos/display_options.js:68: return point;
nit: How about
  return {
    x: point.x + element.offsetLeft,
    y: point.y + element.offsetTop,
    width: element.offsetWidth,
    height: element.offsetHeight
  };

http://codereview.chromium.org/11195036/diff/12002/chrome/browser/resources/o...
chrome/browser/resources/options/chromeos/display_options.js:196: Finish:
function() {
nit: js method should start with lower case.
  Finish -> finish

http://codereview.chromium.org/11195036/diff/12002/chrome/browser/resources/o...
chrome/browser/resources/options/chromeos/display_options.js:204: Clear:
function() {
nit: Clear -> clear

http://codereview.chromium.org/11195036/diff/12002/chrome/browser/resources/o...
chrome/browser/resources/options/chromeos/display_options.js:302:
this.overscan_[this.location_] == 0) {
This does not match what the comment above says. Should this be "> 0" or should
the comment be updated?

[Jun Mukai, 2012-10-22 22:35:29]

https://codereview.chromium.org/11195036/diff/12002/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/display_options.js (right):

https://codereview.chromium.org/11195036/diff/12002/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.js:63: var point =
getBoundsInPage(element.parentNode);
On 2012/10/22 19:54:44, xiyuan wrote:
> Should we use element.offsetParent instead?
> 
> Also think using a loop would be better than recursion.

Done.

https://codereview.chromium.org/11195036/diff/12002/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.js:68: return point;
On 2012/10/22 19:54:44, xiyuan wrote:
> nit: How about
>   return {
>     x: point.x + element.offsetLeft,
>     y: point.y + element.offsetTop,
>     width: element.offsetWidth,
>     height: element.offsetHeight
>   };

Changed the code not to use recursion, so the code here also disappeared.

https://codereview.chromium.org/11195036/diff/12002/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.js:196: Finish:
function() {
On 2012/10/22 19:54:44, xiyuan wrote:
> nit: js method should start with lower case.
>   Finish -> finish

Done.

https://codereview.chromium.org/11195036/diff/12002/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.js:204: Clear:
function() {
On 2012/10/22 19:54:44, xiyuan wrote:
> nit: Clear -> clear

Done.

https://codereview.chromium.org/11195036/diff/12002/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.js:302:
this.overscan_[this.location_] == 0) {
On 2012/10/22 19:54:44, xiyuan wrote:
> This does not match what the comment above says. Should this be "> 0" or
should
> the comment be updated?

The code is correct, my intention is that this.overscan_[this.location_] must
not be negative as the result of this function.  Also it should just ignore the
event at all in such case.  Updated the comment.

[xiyuan, 2012-10-22 22:49:43]

LGTM

Thank you for making the changes.

[Jun Mukai, 2012-10-22 23:08:31]

sky, please review ash code too...

[sky, 2012-10-23 00:35:15]

LGTM

[commit-bot: I haz the power, 2012-10-23 00:40:34]

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

[commit-bot: I haz the power, 2012-10-23 04:27:22]

Change committed as 163498