Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10828005:
Show "displays" section in the options page even for 1 display. It happens in case of mirroring. (Closed)
|
Created:
8 years, 5 months ago by Jun Mukai Modified:
8 years, 4 months ago CC:
chromium-reviews, arv (Not doing code reviews), stevenjb+watch_chromium.org, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionShow "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
Patch Set 1 #
Total comments: 7
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : rebase #
Messages
Total messages: 16 (0 generated)
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.
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.
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.
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?
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.
sure. LGTM
James, can you review this as the OWNER of options2?
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.
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.
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.
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.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/10828005/3002
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&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/10828005/8006
Change committed as 149113 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
