Add OptionsUI and its handler for multiple displays.

Add OptionsUI and its handler for multiple displays.


BUG=130385
TEST=manually checked with lumpy and an external display


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

[Jun Mukai, 2012-06-15 05:45:12]

yoshiki, can you check the JavaScript code?

[yoshiki, 2012-06-15 10:21:28]

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options2/chromeos/display_options.js (right):

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:10: var
kVisualScale = 1 / 10;
Use VISUAL_SCALE as name of constants and add @const keyword on the doc.

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:69: if
(!this.dragging) {
Remove braces when the statement is only one line

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:80: if (index < 0)
{
Remove braces.

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:88: y: e.pageY -
this.dragging.offset.y};
Add CRLF before the end brace because multi-line initializers should be like as
blocks.

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:91: y:
mouse_position.y - this.dragging.click_location.y};
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:139: case
SecondaryDisplayLayout.RIGHT:
Indent. "case" should be same indent as "switch".

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:144: case
SecondaryDisplayLayout.LEFT:
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:149: case
SecondaryDisplayLayout.TOP:
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:154: case
SecondaryDisplayLayout.BOTTOM:
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:170: if
(this.mirroring_) {
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:174: if (e.button
!= 0) {
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:178: if (e.target
== this.displays_view_) {
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:191: this.dragging
= {
DisplayOptions.dragging is a private property right? If so, add underscore at
the end of the name.

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:197: if
(display.div.className.indexOf('displays-focused') == -1) {
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:201:
this.displays_[i].div.className = 'displays-display';
'displays-primary' class would be removed, right?

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:252: if (i ==
this.displays_.length - 1) {
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:274: if
(this.layout_ == SecondaryDisplayLayout.LEFT) {
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:286: if (i == 0) {
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:296:
display.div.style.top = '0px';
Use '0' instead of '0px'.

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:301:
display.div.style.top = '0px';
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:306:
display.div.style.left = '0px';
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:311:
display.div.style.left = '0px';
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:337: if
(this.displays_.length <= 1) {
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:341: if
(this.focused_index_ == null) {
I think focused_index_ should be reset to 1 when displays_.length is changed.

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/chromeos/display_options.js:347: if
(this.mirroring_) {
ditto

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
File chrome/browser/resources/options2/options.js (right):

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/resourc...
chrome/browser/resources/options2/options.js:22: var DisplayOptions =
options.DisplayOptions;
Alphabetical order?

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right):

https://chromiumcodereview.appspot.com/10544171/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/options2/browser_options_handler2.cc:179: {
"displayOptionsButton", IDS_OPTIONS_SETTINGS_DISPLAY_OPTIONS_BUTTON },
Alphabetical order?

[Jun Mukai, 2012-06-18 01:15:10]

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
File chrome/browser/resources/options2/chromeos/display_options.js (right):

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:10: var
kVisualScale = 1 / 10;
On 2012/06/15 10:21:28, yoshiki wrote:
> Use VISUAL_SCALE as name of constants and add @const keyword on the doc.

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:69: if
(!this.dragging) {
On 2012/06/15 10:21:28, yoshiki wrote:
> Remove braces when the statement is only one line 

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:80: if (index < 0)
{
On 2012/06/15 10:21:28, yoshiki wrote:
> Remove braces.

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:88: y: e.pageY -
this.dragging.offset.y};
On 2012/06/15 10:21:28, yoshiki wrote:
> Add CRLF before the end brace because multi-line initializers should be like
as
> blocks.

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:91: y:
mouse_position.y - this.dragging.click_location.y};
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:139: case
SecondaryDisplayLayout.RIGHT:
On 2012/06/15 10:21:28, yoshiki wrote:
> Indent. "case" should be same indent as "switch".

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:144: case
SecondaryDisplayLayout.LEFT:
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:149: case
SecondaryDisplayLayout.TOP:
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:154: case
SecondaryDisplayLayout.BOTTOM:
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:170: if
(this.mirroring_) {
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:174: if (e.button
!= 0) {
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:178: if (e.target
== this.displays_view_) {
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:191: this.dragging
= {
On 2012/06/15 10:21:28, yoshiki wrote:
> DisplayOptions.dragging is a private property right? If so, add underscore at
> the end of the name.

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:197: if
(display.div.className.indexOf('displays-focused') == -1) {
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:201:
this.displays_[i].div.className = 'displays-display';
On 2012/06/15 10:21:28, yoshiki wrote:
> 'displays-primary' class would be removed, right?

displays-primary won't obtain the '-focused' class right now, so it's fine. 
Added a comment line.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:252: if (i ==
this.displays_.length - 1) {
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:274: if
(this.layout_ == SecondaryDisplayLayout.LEFT) {
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:286: if (i == 0) {
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:296:
display.div.style.top = '0px';
On 2012/06/15 10:21:28, yoshiki wrote:
> Use '0' instead of '0px'.

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:301:
display.div.style.top = '0px';
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:306:
display.div.style.left = '0px';
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:311:
display.div.style.left = '0px';
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:337: if
(this.displays_.length <= 1) {
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:341: if
(this.focused_index_ == null) {
On 2012/06/15 10:21:28, yoshiki wrote:
> I think focused_index_ should be reset to 1 when displays_.length is changed.

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:347: if
(this.mirroring_) {
On 2012/06/15 10:21:28, yoshiki wrote:
> ditto

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
File chrome/browser/resources/options2/options.js (right):

http://codereview.chromium.org/10544171/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/options.js:22: var DisplayOptions =
options.DisplayOptions;
On 2012/06/15 10:21:28, yoshiki wrote:
> Alphabetical order?

Done.

http://codereview.chromium.org/10544171/diff/1/chrome/browser/ui/webui/option...
File chrome/browser/ui/webui/options2/browser_options_handler2.cc (right):

http://codereview.chromium.org/10544171/diff/1/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options2/browser_options_handler2.cc:179: {
"displayOptionsButton", IDS_OPTIONS_SETTINGS_DISPLAY_OPTIONS_BUTTON },
On 2012/06/15 10:21:28, yoshiki wrote:
> Alphabetical order?

Done.

[yoshiki, 2012-06-18 03:17:32]

LGTM

[Jun Mukai, 2012-06-18 10:25:36]

jhawkins,

This CL is to add a new Options page for multi-displays for ChromeOS
(crbug.com/130385).  Could you review this?

oshima,
Can you review the usage of multi-displays internal code in
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc ?

[James Hawkins, 2012-06-18 15:13:49]

http://codereview.chromium.org/10544171/diff/5002/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10544171/diff/5002/chrome/app/generated_resour...
chrome/app/generated_resources.grd:13482: +	Manage displays
Why do you have the string 'Manage displays' twice?

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
File chrome/browser/resources/options2/browser_options.html (right):

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/browser_options.html:262: more than one
displays are found. -->
s/displays are/display is/

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/browser_options.html:266: <div>
Why are you using this extra div?

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/browser_options.html:268: <button
id="displayOptionsButton"
IDs use 'dash-form'.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
File chrome/browser/resources/options2/browser_options.js (right):

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/browser_options.js:1274:
$('display-options-section').hidden = false;
Please copy showTouchpadControls_ here instead of having two methods.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/display_options.css (right):

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:6: padding: 20px;
Please attach a screenshot.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/display_options.html (right):

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.html:5: <div
id="display-options-displays-view-host">
This div is empty.  What is put in here?

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.html:8: <button
id="display-options-toggle-mirroring" i18n-content="start-mirroring"></button>
80 cols.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.html:8: <button
id="display-options-toggle-mirroring" i18n-content="start-mirroring"></button>
i18n-content strings should be javaScriptCase.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/display_options.js (right):

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:6: 
Remove blank line.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:10: var
VISUAL_SCALE = 1 / 10;
/** const */

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:13: * Enumeration
of secondary display layout.  THe value has to be same as the
s/THe/ The/

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:29:
OptionsPage.call(this,
Collapse parameters to save lines.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:47: var self =
this;
Use bind instead of var self.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:101:
primary_div.offsetTop - primary_div.offsetLeft * diagonal_slope;
Indentation is off by two.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:103:
primary_div.offsetHeight + primary_div.offsetLeft * diagonal_slope;
Indentation is off by two.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:136: case
SecondaryDisplayLayout.RIGHT:
Indentation is off by two.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:213:
resetDisplaysView_: function() {
Document method.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:252: * Layouts the
display rectangles according to the current layout_.
s/Layouts/Lays out/

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
File chrome/browser/resources/options2/options.html (right):

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/options.html:42: <link rel="stylesheet"
href="chromeos/display_options.css">
Alphabetize.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/options.html:103: <include
src="chromeos/display_options.html">
Alphabetize.

[Jun Mukai, 2012-06-19 08:37:44]

http://codereview.chromium.org/10544171/diff/5002/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10544171/diff/5002/chrome/app/generated_resour...
chrome/app/generated_resources.grd:13482: +	Manage displays
On 2012/06/18 15:13:49, James Hawkins wrote:
> Why do you have the string 'Manage displays' twice?

removed

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
File chrome/browser/resources/options2/browser_options.html (right):

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/browser_options.html:262: more than one
displays are found. -->
On 2012/06/18 15:13:49, James Hawkins wrote:
> s/displays are/display is/

Done.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/browser_options.html:266: <div>
On 2012/06/18 15:13:49, James Hawkins wrote:
> Why are you using this extra div?

forgot the reason... removed.  thanks.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/browser_options.html:268: <button
id="displayOptionsButton"
On 2012/06/18 15:13:49, James Hawkins wrote:
> IDs use 'dash-form'.

Done.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
File chrome/browser/resources/options2/browser_options.js (right):

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/browser_options.js:1274:
$('display-options-section').hidden = false;
On 2012/06/18 15:13:49, James Hawkins wrote:
> Please copy showTouchpadControls_ here instead of having two methods.

Done.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/display_options.css (right):

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:6: padding: 20px;
On 2012/06/18 15:13:49, James Hawkins wrote:
> Please attach a screenshot.

Sorry, what do you mean?  I uploaded the latest screenshot on crbug.com.  Please
see
http://chromium.googlecode.com/issues/attachment?aid=1303850016000&name=scree...
or crbug.com/130385#16

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/display_options.html (right):

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.html:8: <button
id="display-options-toggle-mirroring" i18n-content="start-mirroring"></button>
On 2012/06/18 15:13:49, James Hawkins wrote:
> i18n-content strings should be javaScriptCase.

Done.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.html:8: <button
id="display-options-toggle-mirroring" i18n-content="start-mirroring"></button>
On 2012/06/18 15:13:49, James Hawkins wrote:
> 80 cols.

Done.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
File chrome/browser/resources/options2/chromeos/display_options.js (right):

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:6: 
On 2012/06/18 15:13:49, James Hawkins wrote:
> Remove blank line.

Done.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:10: var
VISUAL_SCALE = 1 / 10;
On 2012/06/18 15:13:49, James Hawkins wrote:
> /** const */

Done.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:13: * Enumeration
of secondary display layout.  THe value has to be same as the
On 2012/06/18 15:13:49, James Hawkins wrote:
> s/THe/ The/

Done.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:29:
OptionsPage.call(this,
On 2012/06/18 15:13:49, James Hawkins wrote:
> Collapse parameters to save lines.

Done.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:47: var self =
this;
On 2012/06/18 15:13:49, James Hawkins wrote:
> Use bind instead of var self.

Done.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:101:
primary_div.offsetTop - primary_div.offsetLeft * diagonal_slope;
On 2012/06/18 15:13:49, James Hawkins wrote:
> Indentation is off by two.

Done.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:103:
primary_div.offsetHeight + primary_div.offsetLeft * diagonal_slope;
On 2012/06/18 15:13:49, James Hawkins wrote:
> Indentation is off by two.

Done.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:136: case
SecondaryDisplayLayout.RIGHT:
On 2012/06/18 15:13:49, James Hawkins wrote:
> Indentation is off by two.

you mean the assignment below?  fixed those lines.  Thanks.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:213:
resetDisplaysView_: function() {
On 2012/06/18 15:13:49, James Hawkins wrote:
> Document method.

Done.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:252: * Layouts the
display rectangles according to the current layout_.
On 2012/06/18 15:13:49, James Hawkins wrote:
> s/Layouts/Lays out/

Done.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
File chrome/browser/resources/options2/options.html (right):

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/options.html:42: <link rel="stylesheet"
href="chromeos/display_options.css">
On 2012/06/18 15:13:49, James Hawkins wrote:
> Alphabetize.

Done.

http://codereview.chromium.org/10544171/diff/5002/chrome/browser/resources/op...
chrome/browser/resources/options2/options.html:103: <include
src="chromeos/display_options.html">
On 2012/06/18 15:13:49, James Hawkins wrote:
> Alphabetize.

Done.

[James Hawkins, 2012-06-20 16:21:03]

LGTM with nits.

Note that I highly suggest you get UX feedback ASAP.

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
File chrome/browser/resources/options2/browser_options.html (right):

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.html:267:
i18n-content="displayOptionsButton"></button>
nit: Indentation is off.  Should be 4 spaces from start of previous row.

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.html:267:
i18n-content="displayOptionsButton"></button>
nit: If an element wraps, the closing tag must be on a new line.

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.html:270: 
nit: Remove blank line.

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/display_options.css (right):

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/display_options.css:35: z-index: 1;
Why do you need to set the z-index?

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/display_options.html (right):

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/display_options.html:9:
i18n-content="startMirroring"></button>
nit: Indentation is off. Closing tag needs to be on new line.

[Jun Mukai, 2012-06-21 08:42:58]

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
File chrome/browser/resources/options2/browser_options.html (right):

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.html:267:
i18n-content="displayOptionsButton"></button>
On 2012/06/20 16:21:03, James Hawkins wrote:
> nit: Indentation is off.  Should be 4 spaces from start of previous row.

Done.

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.html:267:
i18n-content="displayOptionsButton"></button>
On 2012/06/20 16:21:03, James Hawkins wrote:
> nit: If an element wraps, the closing tag must be on a new line.

Done.

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
chrome/browser/resources/options2/browser_options.html:270: 
On 2012/06/20 16:21:03, James Hawkins wrote:
> nit: Remove blank line.

Done.

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/display_options.css (right):

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/display_options.css:35: z-index: 1;
On 2012/06/20 16:21:03, James Hawkins wrote:
> Why do you need to set the z-index?

Removed.  Thanks for pointing.  That was introduced once I allowed overlapping
rectangles.

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
File chrome/browser/resources/options2/chromeos/display_options.html (right):

http://codereview.chromium.org/10544171/diff/15001/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/display_options.html:9:
i18n-content="startMirroring"></button>
On 2012/06/20 16:21:03, James Hawkins wrote:
> nit: Indentation is off. Closing tag needs to be on new line.

Done.

[oshima, 2012-06-21 16:51:56]

Is this option visible only when the extended desktop is enbled?

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

http://codereview.chromium.org/10544171/diff/21001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:23: using
ash::internal::MonitorController;
nit: move this inside namespace

http://codereview.chromium.org/10544171/diff/21001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:84:
ash::Shell::GetInstance()->monitor_controller();
you don't need this (see below)

http://codereview.chromium.org/10544171/diff/21001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:90:
monitor_controller->IsExtendedDesktopEnabled() &&
MonitorController::IsExtendedDesktopEnalbed()
(it's static)

http://codereview.chromium.org/10544171/diff/21001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:153: 
does DCHECK_NE(-1, layout) make sense?

[Jun Mukai, 2012-06-22 01:36:03]

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

http://codereview.chromium.org/10544171/diff/21001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:23: using
ash::internal::MonitorController;
On 2012/06/21 16:51:56, oshima wrote:
> nit: move this inside namespace

Done.

http://codereview.chromium.org/10544171/diff/21001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:84:
ash::Shell::GetInstance()->monitor_controller();
On 2012/06/21 16:51:56, oshima wrote:
> you don't need this (see below)

Done.

http://codereview.chromium.org/10544171/diff/21001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:90:
monitor_controller->IsExtendedDesktopEnabled() &&
On 2012/06/21 16:51:56, oshima wrote:
> MonitorController::IsExtendedDesktopEnalbed()
> (it's static)

Done.

http://codereview.chromium.org/10544171/diff/21001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:153: 
On 2012/06/21 16:51:56, oshima wrote:
> does DCHECK_NE(-1, layout) make sense?

Then probably nicer to check if the |layout| is valid (one of
LEFT/RIGHT/TOP/BOTTOM).

[oshima, 2012-06-22 01:38:34]

lgtm

[commit-bot: I haz the power, 2012-06-22 01:42:35]

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

[commit-bot: I haz the power, 2012-06-22 02:02:54]

Try job failure for 10544171-26001 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-22 06:41:13]

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

[commit-bot: I haz the power, 2012-06-22 09:02:48]

Change committed as 143564