Redesign display options for ChromeOS.

Redesign display options for ChromeOS.

Several new features have been added recently, and this new UI
allows users to customize them through web UI.

Updates display options page.

BUG=190897
R=oshima@chromium.org, xiyuan@chromium.org, jhawkins@chromium.org

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

[xiyuan, 2013-05-10 22:29:05]

LGTM with nits

https://codereview.chromium.org/14710011/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/chromeos/display_options.css (right):

https://codereview.chromium.org/14710011/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/chromeos/display_options.css:47: html[dir=rtl]
#display-options-buttons-container {
Should we remove this as well since we removed
#display-options-buttons-container rules above?

https://codereview.chromium.org/14710011/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/chromeos/display_options.js (right):

https://codereview.chromium.org/14710011/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/chromeos/display_options.js:179: }).bind(this);
nit: wrapping () around function is not necessary. That's what I learned from
dbeam's review. :)

https://codereview.chromium.org/14710011/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/chromeos/display_options.js:536:
resolution.innerHTML = '';
nit: resolution.textContent = '';

Avoid using innerHTML in general.

https://codereview.chromium.org/14710011/diff/1/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/14710011/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:203:
js_display->Set("ui_scales", js_scales);
nit: ui_scales -> uiScales since it's used in js.

[Jun Mukai, 2013-05-10 23:00:06]

https://codereview.chromium.org/14710011/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/chromeos/display_options.css (right):

https://codereview.chromium.org/14710011/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/chromeos/display_options.css:47: html[dir=rtl]
#display-options-buttons-container {
On 2013/05/10 22:29:05, xiyuan wrote:
> Should we remove this as well since we removed
> #display-options-buttons-container rules above?

Done.

https://codereview.chromium.org/14710011/diff/1/chrome/browser/resources/opti...
File chrome/browser/resources/options/chromeos/display_options.js (right):

https://codereview.chromium.org/14710011/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/chromeos/display_options.js:179: }).bind(this);
On 2013/05/10 22:29:05, xiyuan wrote:
> nit: wrapping () around function is not necessary. That's what I learned from
> dbeam's review. :)

Done.

https://codereview.chromium.org/14710011/diff/1/chrome/browser/resources/opti...
chrome/browser/resources/options/chromeos/display_options.js:536:
resolution.innerHTML = '';
On 2013/05/10 22:29:05, xiyuan wrote:
> nit: resolution.textContent = '';
> 
> Avoid using innerHTML in general.

Done.

https://codereview.chromium.org/14710011/diff/1/chrome/browser/ui/webui/optio...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/14710011/diff/1/chrome/browser/ui/webui/optio...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:203:
js_display->Set("ui_scales", js_scales);
On 2013/05/10 22:29:05, xiyuan wrote:
> nit: ui_scales -> uiScales since it's used in js.

Done.

[oshima, 2013-05-13 01:00:25]

https://codereview.chromium.org/14710011/diff/3002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/14710011/diff/3002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:204: }
You need to use floored value. I may also need to take overscan insets into
account, although I'm not sure how otherplatforms handles this. Maybe this is
fine.
Please consult alex and UX team.

[Jun Mukai, 2013-05-13 16:45:31]

https://codereview.chromium.org/14710011/diff/3002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/14710011/diff/3002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:204: }
On 2013/05/13 01:00:25, oshima wrote:
> You need to use floored value. I may also need to take overscan insets into
> account, although I'm not sure how otherplatforms handles this. Maybe this is
> fine.
> Please consult alex and UX team.

For flooring, it's done by calling "SetInteger" (it will cast the 2nd argument
to int). changed to use static_cast explicitly.

[Jun Mukai, 2013-05-13 23:42:37]

https://codereview.chromium.org/14710011/diff/3002/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/14710011/diff/3002/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:204: }
On 2013/05/13 16:45:31, Jun Mukai wrote:
> On 2013/05/13 01:00:25, oshima wrote:
> > You need to use floored value. I may also need to take overscan insets into
> > account, although I'm not sure how otherplatforms handles this. Maybe this
is
> > fine.
> > Please consult alex and UX team.
> 
> For flooring, it's done by calling "SetInteger" (it will cast the 2nd argument
> to int). changed to use static_cast explicitly.

chatted with Alex, and he said showing the original size is least confusing. So
let me proceed with this code.
If people gets confused, let's change this again in another CL.

[oshima, 2013-05-14 00:01:26]

> chatted with Alex, and he said showing the original size is > least confusing.
So
> let me proceed with this code.
> If people gets confused, let's change this again in another CL.
> Reply

SG

https://codereview.chromium.org/14710011/diff/11001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/14710011/diff/11001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:202:
"height", static_cast<int>(base_size.height() * ui_scales[i]));
sorry I wasn't clear. Please use gfx::ToFlooredInt
in ui/gfx/safe_integer_conversions.h.

then lgtm

[Jun Mukai, 2013-05-14 00:14:03]

On 2013/05/14 00:01:26, oshima wrote:
> > chatted with Alex, and he said showing the original size is > least
confusing.
> So
> > let me proceed with this code.
> > If people gets confused, let's change this again in another CL.
> > Reply
> 
> SG
> 
>
https://codereview.chromium.org/14710011/diff/11001/chrome/browser/ui/webui/o...
> File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
> (right):
> 
>
https://codereview.chromium.org/14710011/diff/11001/chrome/browser/ui/webui/o...
> chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:202:
> "height", static_cast<int>(base_size.height() * ui_scales[i]));
> sorry I wasn't clear. Please use gfx::ToFlooredInt
> in ui/gfx/safe_integer_conversions.h.
> 
> then lgtm

oh, I didn't know that. changed to use ToFlooredInt. Thanks!

[commit-bot: I haz the power, 2013-05-14 00:14:36]

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

[Jun Mukai, 2013-05-14 00:22:39]

+jhawkins

Oops, my change needs the approval of the owner of
chrome/browser/resource/options. jhawkins, could you review this?

change summary:
This CL changes the default visibility of display options.
- current code: hidden by default and shown when there are multiple displays
- new: always visible

This change is necessary because the new design of display options also provides
settings of a single display.

[commit-bot: I haz the power, 2013-05-14 00:24:40]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[James Hawkins, 2013-05-15 21:55:33]

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.html (left):

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.html:121: if more than one
display is found. -->
Why is this no longer true?

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/display_options.css (right):

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.css:51:
#selected-display-data-container h2 {
Why does this need to be different from any other h2?

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/display_options.html (right):

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.html:10: <table>
This is not tabular data; please don't use table and friends.

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

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.js:539: for (var i =
0; i < orientationOptions.length; i++) {
nit: No braces for single-line blocks.

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.js:543: if
(this.mirroring_) {
Each of these blocks is quite long; please break this method up into smaller
pieces.

[Jun Mukai, 2013-05-16 00:04:15]

PTAL
also I noticed there's unused ones in browser_options.js, so the new patchset
removed them.

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/options/browser_options.html (left):

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
chrome/browser/resources/options/browser_options.html:121: if more than one
display is found. -->
On 2013/05/15 21:55:33, James Hawkins wrote:
> Why is this no longer true?

This was true because display-options is the settings page for multiple display
arrangements (top/left/bottom/right, mirroring/non-mirroring, or something like
that)

This is not true because the new display-options also includes single display
settings like screen resolution change and screen rotation.

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/display_options.html (right):

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.html:10: <table>
On 2013/05/15 21:55:33, James Hawkins wrote:
> This is not tabular data; please don't use table and friends.

Done.

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

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.js:539: for (var i =
0; i < orientationOptions.length; i++) {
On 2013/05/15 21:55:33, James Hawkins wrote:
> nit: No braces for single-line blocks.

Done.

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.js:543: if
(this.mirroring_) {
On 2013/05/15 21:55:33, James Hawkins wrote:
> Each of these blocks is quite long; please break this method up into smaller
> pieces.

Done.

[James Hawkins, 2013-05-16 17:37:11]

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/display_options.css (right):

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.css:51:
#selected-display-data-container h2 {
On 2013/05/15 21:55:33, James Hawkins wrote:
> Why does this need to be different from any other h2?

This comment is unresolved.

https://codereview.chromium.org/14710011/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/display_options.html (right):

https://codereview.chromium.org/14710011/diff/28001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.html:12:
i18n-content="selectedDisplayTitleOptions"></div>
nit: Close tags of wrapped elements must be on a new line.

Here and elsewhere.

[Jun Mukai, 2013-05-16 23:51:10]

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/display_options.css (right):

https://codereview.chromium.org/14710011/diff/18001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.css:51:
#selected-display-data-container h2 {
On 2013/05/16 17:37:11, James Hawkins wrote:
> On 2013/05/15 21:55:33, James Hawkins wrote:
> > Why does this need to be different from any other h2?
> 
> This comment is unresolved.

sorry for not replying.
The new patchset changed that element from h2 to div.

https://codereview.chromium.org/14710011/diff/28001/chrome/browser/resources/...
File chrome/browser/resources/options/chromeos/display_options.html (right):

https://codereview.chromium.org/14710011/diff/28001/chrome/browser/resources/...
chrome/browser/resources/options/chromeos/display_options.html:12:
i18n-content="selectedDisplayTitleOptions"></div>
On 2013/05/16 17:37:11, James Hawkins wrote:
> nit: Close tags of wrapped elements must be on a new line.
> 
> Here and elsewhere.

Done.

[Jun Mukai, 2013-05-20 22:24:52]

Hello James,

I think I've fixed upon comments. Could you review this again?
thanks!

[James Hawkins, 2013-05-20 22:28:10]

LGTM with nit.

https://codereview.chromium.org/14710011/diff/34001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/14710011/diff/34001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:40: //
Assumes the display id is specified as the first argument.
s/id/ID/

[Jun Mukai, 2013-05-20 22:37:42]

https://codereview.chromium.org/14710011/diff/34001/chrome/browser/ui/webui/o...
File chrome/browser/ui/webui/options/chromeos/display_options_handler.cc
(right):

https://codereview.chromium.org/14710011/diff/34001/chrome/browser/ui/webui/o...
chrome/browser/ui/webui/options/chromeos/display_options_handler.cc:40: //
Assumes the display id is specified as the first argument.
On 2013/05/20 22:28:10, James Hawkins wrote:
> s/id/ID/

Done.

[commit-bot: I haz the power, 2013-05-20 22:37:57]

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

[commit-bot: I haz the power, 2013-05-21 06:19:31]

Retried try job too often on win7_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-05-21 06:47:24]

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

[commit-bot: I haz the power, 2013-05-21 08:26:05]

Change committed as 201256