Allow offset for secondary display position in chrome://settings/display.

Allow offset for secondary display position in chrome://settings/display.

- Adding 'offset' value in the preference and set to display controller
- Allow offset in Options UI too
- Redesign the Options UI a bit: now it has "apply" button rather than just applying on mouse-up, and remove 'ok' button.  See crbug.com/130385#c26 for the mock.

R=ben@chromium.org
BUG=137516, 130385
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151444

[Ben Goodger (Google), 2012-08-01 15:42:57]

oshima for first review.

[Jun Mukai, 2012-08-02 01:52:04]

On 2012/08/01 15:42:57, Ben Goodger (Google) wrote:
> oshima for first review.

oshima is on vacation currently and will be back in the next week.  Should I
wait for his back or ask another person for the review?

[Jun Mukai, 2012-08-06 02:40:02]

oshima, can you take a look at this?

[oshima, 2012-08-06 04:15:55]

https://chromiumcodereview.appspot.com/10836046/diff/15/ash/display/display_c...
File ash/display/display_controller.cc (right):

https://chromiumcodereview.appspot.com/10836046/diff/15/ash/display/display_c...
ash/display/display_controller.cc:261: }
you need to check & adjust the offset if the secondary display is detached from
the primary.

https://chromiumcodereview.appspot.com/10836046/diff/15/ash/display/display_c...
File ash/display/display_controller.h (right):

https://chromiumcodereview.appspot.com/10836046/diff/15/ash/display/display_c...
ash/display/display_controller.h:104: int secondary_display_offset_;
document this (or in accessor method)

[Jun Mukai, 2012-08-06 05:27:27]

http://codereview.chromium.org/10836046/diff/15/ash/display/display_controlle...
File ash/display/display_controller.cc (right):

http://codereview.chromium.org/10836046/diff/15/ash/display/display_controlle...
ash/display/display_controller.cc:261: }
On 2012/08/06 04:15:55, oshima wrote:
> you need to check & adjust the offset if the secondary display is detached
from
> the primary.

Added the check for offset value above.

http://codereview.chromium.org/10836046/diff/15/ash/display/display_controller.h
File ash/display/display_controller.h (right):

http://codereview.chromium.org/10836046/diff/15/ash/display/display_controlle...
ash/display/display_controller.h:104: int secondary_display_offset_;
On 2012/08/06 04:15:55, oshima wrote:
> document this (or in accessor method)

Done.

[oshima_google, 2012-08-06 05:44:37]

http://codereview.chromium.org/10836046/diff/2003/ash/display/display_control...
File ash/display/display_controller.cc (right):

http://codereview.chromium.org/10836046/diff/2003/ash/display/display_control...
ash/display/display_controller.cc:250: offset = 0;
I think it's better to adjust the offset (just keep 10~50 pixels), instead of
ignoring them.

[Jun Mukai, 2012-08-06 06:58:35]

http://codereview.chromium.org/10836046/diff/2003/ash/display/display_control...
File ash/display/display_controller.cc (right):

http://codereview.chromium.org/10836046/diff/2003/ash/display/display_control...
ash/display/display_controller.cc:250: offset = 0;
On 2012/08/06 05:44:37, oshima1 wrote:
> I think it's better to adjust the offset (just keep 10~50 pixels), instead of
> ignoring them.

Done.

[Jun Mukai, 2012-08-06 07:30:54]

On 2012/08/06 06:58:35, Jun Mukai wrote:
>
http://codereview.chromium.org/10836046/diff/2003/ash/display/display_control...
> File ash/display/display_controller.cc (right):
> 
>
http://codereview.chromium.org/10836046/diff/2003/ash/display/display_control...
> ash/display/display_controller.cc:250: offset = 0;
> On 2012/08/06 05:44:37, oshima1 wrote:
> > I think it's better to adjust the offset (just keep 10~50 pixels), instead
of
> > ignoring them.
> 
> Done.

Fixed the code a bit according to an offline talk.  Now it uses std::min/max. 
oshima, could you take another look?

[oshima, 2012-08-06 07:38:49]

c++ lgtm

http://codereview.chromium.org/10836046/diff/10003/ash/display/display_contro...
File ash/display/display_controller.cc (right):

http://codereview.chromium.org/10836046/diff/10003/ash/display/display_contro...
ash/display/display_controller.cc:250: // TODO(oshima|mukai): Implement more
flexible layout.
remove this comment.

[Jun Mukai, 2012-08-07 08:51:01]

ben, can you take a look at the code?

[Ben Goodger (Google), 2012-08-07 18:16:51]

You should have someone who knows webui stuff look through the js/html changes.
C++ LGTM.

[Jun Mukai, 2012-08-10 02:12:59]

James, please also review this.

[James Hawkins, 2012-08-11 17:37:41]

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/display_options.js (right):

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/display_options.js:10: // The number
of pixels to share the edges between displays.
nit: Blank line above this.

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/display_options.js:66: var offset =
null;
nit: No need to set offset to null here.

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/display_options.js:230: var top =
Math.max(draggingDiv.offsetTop,
nit: Start of parameter rows must align on the same column.  Here and elsewhere.

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/display_options.js:368:
(boundingBox.left + boundingBox.right) / 2,
nit: Indentation is off here.

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc
(right):

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:182:
return;
When does this happen?

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/chromeos/display_options_handler.h
(right):

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.h:54: void
FadeOutForDisplayLayoutFinished(int layout, int offset);
nit: Document parameters.

[Jun Mukai, 2012-08-13 05:33:25]

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/display_options.js (right):

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/display_options.js:10: // The number
of pixels to share the edges between displays.
On 2012/08/11 17:37:42, James Hawkins wrote:
> nit: Blank line above this.

Done.

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/display_options.js:66: var offset =
null;
On 2012/08/11 17:37:42, James Hawkins wrote:
> nit: No need to set offset to null here.

Done.

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/display_options.js:230: var top =
Math.max(draggingDiv.offsetTop,
On 2012/08/11 17:37:42, James Hawkins wrote:
> nit: Start of parameter rows must align on the same column.  Here and
elsewhere.

Done.

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/display_options.js:368:
(boundingBox.left + boundingBox.right) / 2,
On 2012/08/11 17:37:42, James Hawkins wrote:
> nit: Indentation is off here.

Done.

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc
(right):

http://codereview.chromium.org/10836046/diff/12012/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:182:
return;
On 2012/08/11 17:37:42, James Hawkins wrote:
> When does this happen?

Good point.  Added "SendDisplayInfo" to refresh the JS page status.

[James Hawkins, 2012-08-13 15:45:12]

LGTM with nits.

http://codereview.chromium.org/10836046/diff/10012/ash/display/display_contro...
File ash/display/display_controller.cc (right):

http://codereview.chromium.org/10836046/diff/10012/ash/display/display_contro...
ash/display/display_controller.cc:27: namespace {
Optional nit: Blank lines after  the opening brace and before the closing brace.

http://codereview.chromium.org/10836046/diff/10012/ash/display/display_contro...
ash/display/display_controller.cc:260: // Ignore the offset in case that the
secondary display doesn't share edges
nit: s/ that//

http://codereview.chromium.org/10836046/diff/10012/chrome/browser/chromeos/pr...
File chrome/browser/chromeos/preferences.cc (right):

http://codereview.chromium.org/10836046/diff/10012/chrome/browser/chromeos/pr...
chrome/browser/chromeos/preferences.cc:257: 0,
Optional nit: Condense parameters to save a line.

[Jun Mukai, 2012-08-14 01:50:40]

http://codereview.chromium.org/10836046/diff/10012/ash/display/display_contro...
File ash/display/display_controller.cc (right):

http://codereview.chromium.org/10836046/diff/10012/ash/display/display_contro...
ash/display/display_controller.cc:27: namespace {
On 2012/08/13 15:45:12, James Hawkins wrote:
> Optional nit: Blank lines after  the opening brace and before the closing
brace.

Done.

http://codereview.chromium.org/10836046/diff/10012/ash/display/display_contro...
ash/display/display_controller.cc:260: // Ignore the offset in case that the
secondary display doesn't share edges
On 2012/08/13 15:45:12, James Hawkins wrote:
> nit: s/ that//

Done.

http://codereview.chromium.org/10836046/diff/10012/chrome/browser/chromeos/pr...
File chrome/browser/chromeos/preferences.cc (right):

http://codereview.chromium.org/10836046/diff/10012/chrome/browser/chromeos/pr...
chrome/browser/chromeos/preferences.cc:257: 0,
On 2012/08/13 15:45:12, James Hawkins wrote:
> Optional nit: Condense parameters to save a line.

I would follow the style in that file -- most of other RegisterXXXPref does this
style for some reasons.

[commit-bot: I haz the power, 2012-08-14 02:28:05]

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

[commit-bot: I haz the power, 2012-08-14 04:05:52]

Try job failure for 10836046-1015 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-14 07:11:42]

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

[commit-bot: I haz the power, 2012-08-14 09:46:49]

Change committed as 151444