Show "displays" section in the options page even for 1 display. It happens in case of mirroring.

Show "displays" section in the options page even for 1 display.  It happens in case of mirroring.


BUG=135580
TEST=manually checked on lumpy


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

[oshima, 2012-07-25 07:53:41]

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

http://codereview.chromium.org/10828005/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:234: // We show at
least 2 display rectangles to have the "mirroring" looking.
We only support up to 2 displays now, and there is only one display from
chrome's perspective, so comment should be something like

// Always show two displays because there must be two displays when
// the display_options is enabled.  Don't rely on displays_.length because
// it is the number of displays that chrome sees, not a user sees.

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

http://codereview.chromium.org/10828005/diff/1/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:90:
output_state != chromeos::STATE_SINGLE);
I think it's easier to read/follow if you list valid cases like

output_state == chromeos::STATE_DUAL_MIRROR || ..

rather than opposite.

[oshima, 2012-07-25 07:54:25]

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

http://codereview.chromium.org/10828005/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:234: // We show at
least 2 display rectangles to have the "mirroring" looking.
On 2012/07/25 07:53:41, oshima wrote:
> We only support up to 2 displays now, and there is only one display from
> chrome's perspective, so comment should be something like

should read:

"there is only one display from chrome's perspective in mirror mode"

> 
> // Always show two displays because there must be two displays when
> // the display_options is enabled.  Don't rely on displays_.length because
> // it is the number of displays that chrome sees, not a user sees.

[Jun Mukai, 2012-07-25 08:14:43]

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

http://codereview.chromium.org/10828005/diff/1/chrome/browser/resources/optio...
chrome/browser/resources/options2/chromeos/display_options.js:234: // We show at
least 2 display rectangles to have the "mirroring" looking.
On 2012/07/25 07:53:41, oshima wrote:
> We only support up to 2 displays now, and there is only one display from
> chrome's perspective, so comment should be something like
> 
> // Always show two displays because there must be two displays when
> // the display_options is enabled.  Don't rely on displays_.length because
> // it is the number of displays that chrome sees, not a user sees.

Done.

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

http://codereview.chromium.org/10828005/diff/1/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:90:
output_state != chromeos::STATE_SINGLE);
On 2012/07/25 07:53:41, oshima wrote:
> I think it's easier to read/follow if you list valid cases like
> 
> output_state == chromeos::STATE_DUAL_MIRROR || ..
> 
> rather than opposite.

Are there no chances to add a new state even when we want to support >2
displays?  I was just afraid to forget keeping the consistency in that case.

[oshima, 2012-07-25 13:03:37]

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

http://codereview.chromium.org/10828005/diff/1/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:90:
output_state != chromeos::STATE_SINGLE);
On 2012/07/25 08:14:43, Jun Mukai wrote:
> On 2012/07/25 07:53:41, oshima wrote:
> > I think it's easier to read/follow if you list valid cases like
> > 
> > output_state == chromeos::STATE_DUAL_MIRROR || ..
> > 
> > rather than opposite.
> 
> Are there no chances to add a new state even when we want to support >2
> displays?  I was just afraid to forget keeping the consistency in that case.

Existing code can break when state enum changes too. How about adding
int OutputConfigurator::GetNumPhisicalDisplays() and use it instead?

[Jun Mukai, 2012-07-25 14:44:42]

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

http://codereview.chromium.org/10828005/diff/1/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options2/chromeos/display_options_handler.cc:90:
output_state != chromeos::STATE_SINGLE);
On 2012/07/25 13:03:37, oshima wrote:
> On 2012/07/25 08:14:43, Jun Mukai wrote:
> > On 2012/07/25 07:53:41, oshima wrote:
> > > I think it's easier to read/follow if you list valid cases like
> > > 
> > > output_state == chromeos::STATE_DUAL_MIRROR || ..
> > > 
> > > rather than opposite.
> > 
> > Are there no chances to add a new state even when we want to support >2
> > displays?  I was just afraid to forget keeping the consistency in that case.
> 
> Existing code can break when state enum changes too. How about adding
> int OutputConfigurator::GetNumPhisicalDisplays() and use it instead?

Can we do that in another CL?  That doesn't sound so relevant to this CL
description.

[oshima, 2012-07-25 14:51:03]

sure. LGTM

[Jun Mukai, 2012-07-25 15:05:21]

James, can you review this as the OWNER of options2?

[James Hawkins, 2012-07-25 15:07:16]

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

http://codereview.chromium.org/10828005/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:239: height +
num_displays * 2 + 'px';
What is this 2 about?  In general I'd pull these magic numbers into well-named
constant variables.

[Jun Mukai, 2012-07-27 08:43:52]

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

http://codereview.chromium.org/10828005/diff/7001/chrome/browser/resources/op...
chrome/browser/resources/options2/chromeos/display_options.js:239: height +
num_displays * 2 + 'px';
On 2012/07/25 15:07:16, James Hawkins wrote:
> What is this 2 about?  In general I'd pull these magic numbers into well-named
> constant variables.

Introduced a const and replaced.

[James Hawkins, 2012-07-30 03:14:35]

LGTM with optional nit.

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

http://codereview.chromium.org/10828005/diff/11001/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/display_options.js:239: var
num_displays = Math.max(2, this.displays_.length);
Optional nit: Pull this two into a const, e.g., MIN_NUM_DISPLAYS or something.

[Jun Mukai, 2012-07-30 07:58:41]

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

http://codereview.chromium.org/10828005/diff/11001/chrome/browser/resources/o...
chrome/browser/resources/options2/chromeos/display_options.js:239: var
num_displays = Math.max(2, this.displays_.length);
On 2012/07/30 03:14:35, James Hawkins wrote:
> Optional nit: Pull this two into a const, e.g., MIN_NUM_DISPLAYS or something.

Done.

[commit-bot: I haz the power, 2012-07-30 07:59:05]

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

[commit-bot: I haz the power, 2012-07-30 12:49:49]

Try job failure for 10828005-3002 (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-07-31 00:28:51]

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

[commit-bot: I haz the power, 2012-07-31 01:56:57]

Change committed as 149113