Polish "Displays" Options UI for ChromeOS.

Polish "Displays" Options UI for ChromeOS.

This follows the mock image of crbug.com/130385 comment 26.  This CL contains following changes.
- background color / line color
- an arrow from below section
- display names
- display name / resolution view
- "Apply" buttons
- centering the display rectangles in the area

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

[James Hawkins, 2012-08-03 18:08:23]

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

http://codereview.chromium.org/10843066/diff/2001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:13719: +	Apply
I'm almost certain we have an apply string somewhere else; please use that
instead.

http://codereview.chromium.org/10843066/diff/2001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:13722: +	Resolution
nit: Indentation should be two spaces, not one.

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

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:26:
padding-bottom: 15px;
nit: Use shorthand notation.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:36: border-left:
1px solid lightgrey;
RTL

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:46: right: 15px;
RTL

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:86:
margin-bottom: 5px;
Use shorthand notation.

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

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.html:9: <span
id="selected-display-name"></span><br/>
Don't use <br> when you really want a block element.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.html:10: <span
i18n-content="resolution"></span>:
I'm not a fan of this colon here; I don't think it will work like you want it to
in RTL.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.html:17:
i18n-content="startMirroring">
Does this not fit on the line above?  It's hard to eyeball it.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.html:21: <button
id="display-options-apply-button"
Don't use the element name in the ID, e.g., button.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.html:28: <!-- the
arrow of display-configuration is achieved by a div
nit: Capitalize first word of sentence.

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

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:10: // The margin
pixels to calculate the arrow position from left.  This happens
You use the word 'left' here: does this work in RTL?  You should probably use
the word 'start' instead.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:178:
this.focused_index_ = i;
This is the same functionality as the block above: please move this logic into
the logic above.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:218: * Update the
description of the selected display section.
s/Update/Updates.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:230: var
name_element = $('selected-display-name');
s/name_element/nameElement/

javaScript case here and elsewhere for variables.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:245:
arrow.style.left = display.div.offsetLeft + display.div.offsetWidth / 2 +
RTL

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:332: // We assume
that first display is primary and put a grey rectangle to
nit: Don't use pronouns (we) in comments; pronouns are ambiguous.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc
(right):

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:122: // We
do not have good data for displays such like vendor/model names.
s/We/.../

[Jun Mukai, 2012-08-06 08:07:32]

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

http://codereview.chromium.org/10843066/diff/2001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:13719: +	Apply
On 2012/08/03 18:08:23, James Hawkins wrote:
> I'm almost certain we have an apply string somewhere else; please use that
> instead.

Sorry, I couldn't find that... Do you have ideas?
I just found "Apply recommendation" message for
IDS_OPTIONS_CONTROLLED_SETTING_APPLY_RECOMMENDATION and "Applies To" for
IDS_POLICY_APPLIES_TO.  But both doesn't seem to be used instead of this.

http://codereview.chromium.org/10843066/diff/2001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:13722: +	Resolution
On 2012/08/03 18:08:23, James Hawkins wrote:
> nit: Indentation should be two spaces, not one.

I used Tab character instead of spaces.  Replaced them.

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

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:26:
padding-bottom: 15px;
On 2012/08/03 18:08:23, James Hawkins wrote:
> nit: Use shorthand notation.

Done.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:36: border-left:
1px solid lightgrey;
On 2012/08/03 18:08:23, James Hawkins wrote:
> RTL

This isn't relevant to RTL, 45deg-rotated div's left/top is actually upper half.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:46: right: 15px;
On 2012/08/03 18:08:23, James Hawkins wrote:
> RTL

Done.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:86:
margin-bottom: 5px;
On 2012/08/03 18:08:23, James Hawkins wrote:
> Use shorthand notation.

Done.

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

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.html:9: <span
id="selected-display-name"></span><br/>
On 2012/08/03 18:08:23, James Hawkins wrote:
> Don't use <br> when you really want a block element.

Done.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.html:10: <span
i18n-content="resolution"></span>:
On 2012/08/03 18:08:23, James Hawkins wrote:
> I'm not a fan of this colon here; I don't think it will work like you want it
to
> in RTL.

Done.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.html:17:
i18n-content="startMirroring">
On 2012/08/03 18:08:23, James Hawkins wrote:
> Does this not fit on the line above?  It's hard to eyeball it.

Done.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.html:21: <button
id="display-options-apply-button"
On 2012/08/03 18:08:23, James Hawkins wrote:
> Don't use the element name in the ID, e.g., button.

Done.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.html:28: <!-- the
arrow of display-configuration is achieved by a div
On 2012/08/03 18:08:23, James Hawkins wrote:
> nit: Capitalize first word of sentence.

Done.

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

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:10: // The margin
pixels to calculate the arrow position from left.  This happens
On 2012/08/03 18:08:23, James Hawkins wrote:
> You use the word 'left' here: does this work in RTL?  You should probably use
> the word 'start' instead.

Removed this constant.  According to kuscher's comment in crbug.com/130385#c31,
I put the display rectangles at the center of the area.  Not need to have this
constant.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:178:
this.focused_index_ = i;
On 2012/08/03 18:08:23, James Hawkins wrote:
> This is the same functionality as the block above: please move this logic into
> the logic above.

Done.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:218: * Update the
description of the selected display section.
On 2012/08/03 18:08:23, James Hawkins wrote:
> s/Update/Updates.

Done.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:230: var
name_element = $('selected-display-name');
On 2012/08/03 18:08:23, James Hawkins wrote:
> s/name_element/nameElement/
> 
> javaScript case here and elsewhere for variables.

Sorry breaking such a basic rule.  I reviewed the whole file and fixed the
names.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:245:
arrow.style.left = display.div.offsetLeft + display.div.offsetWidth / 2 +
On 2012/08/03 18:08:23, James Hawkins wrote:
> RTL

Removed ARROW_POSITION_MARGIN and working fine now on RTL too.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:332: // We assume
that first display is primary and put a grey rectangle to
On 2012/08/03 18:08:23, James Hawkins wrote:
> nit: Don't use pronouns (we) in comments; pronouns are ambiguous.

Done.

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc
(right):

http://codereview.chromium.org/10843066/diff/2001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:122: // We
do not have good data for displays such like vendor/model names.
On 2012/08/03 18:08:23, James Hawkins wrote:
> s/We/.../

Done.

[James Hawkins, 2012-08-06 16:44:43]

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

http://codereview.chromium.org/10843066/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:28: /* The arrow
at the border #display-configurations to point the focused display.
/* First
 * last.
 */

http://codereview.chromium.org/10843066/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:32:
-webkit-transform: rotate(45deg);
Won't we need to rotate by -45deg in RTL then?

http://codereview.chromium.org/10843066/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:88: border: solid
2px rgb(0, 138, 255);
Be consistent and use hex (hex is used in the rest of the file.

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

http://codereview.chromium.org/10843066/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:184:
display.div.className += ' displays-focused';
classList.add

http://codereview.chromium.org/10843066/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:317:
this.displaysView_.style.width = '100%';
Use a new CSS class with these properties instead.

[Jun Mukai, 2012-08-07 00:31:31]

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

http://codereview.chromium.org/10843066/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:28: /* The arrow
at the border #display-configurations to point the focused display.
On 2012/08/06 16:44:43, James Hawkins wrote:
> /* First
>  * last.
>  */

Done.

http://codereview.chromium.org/10843066/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:32:
-webkit-transform: rotate(45deg);
On 2012/08/06 16:44:43, James Hawkins wrote:
> Won't we need to rotate by -45deg in RTL then?

I checked with arabic language and working fine.  border-left is left in RTL
too.

http://codereview.chromium.org/10843066/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.css:88: border: solid
2px rgb(0, 138, 255);
On 2012/08/06 16:44:43, James Hawkins wrote:
> Be consistent and use hex (hex is used in the rest of the file.

Rather changed to use rgb() in the rest of the file.  Our coding style prefers
rgb() and the linter puts warning for it (grey scale seems one exceptional of
rgb() usage).

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

http://codereview.chromium.org/10843066/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:184:
display.div.className += ' displays-focused';
On 2012/08/06 16:44:43, James Hawkins wrote:
> classList.add

Done.

http://codereview.chromium.org/10843066/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:317:
this.displaysView_.style.width = '100%';
On 2012/08/06 16:44:43, James Hawkins wrote:
> Use a new CSS class with these properties instead.

Done.

[Jun Mukai, 2012-08-08 11:38:50]

James, please take a look at this CL.

[James Hawkins, 2012-08-08 15:02:28]

lgtm

[commit-bot: I haz the power, 2012-08-09 03:05:31]

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

[commit-bot: I haz the power, 2012-08-09 04:11:05]

Change committed as 150727