Add ability to set resolution on external display

Add ability to set resolution on external display

 * OutputConfigurator gets selected resolution via StateController::GetResolutionForDisplay
 * It only shows one selection per one resolution.
   Non interlaced takes precedence and best refresh rate
   is chosen.
 * Resolution list is added to DisplayInfo. UI can use this
   to present options to users.
 * Created separate display_util_x11 for utility functions that
   depends on xrandr.

BUG=230733
TEST=covered by tests. more to come. manual test should be possible after UI is added. (crbug.com/266097)

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

[oshima, 2013-07-31 16:39:11]

I'm adding a few more tests, but the rest is ready for review.

derat -> chromeos/display, ash/ changes
mukai -> ash/, c/b/chromeos/display changes

https://codereview.chromium.org/21297003/diff/17001/ash/accelerators/accelera...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/21297003/diff/17001/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:74: #include
"ash/display/display_info.h"
Changes in this file is just for my testing and I'll revert
before landing, so please ignore them.

[Daniel Erat, 2013-07-31 17:34:04]

https://codereview.chromium.org/21297003/diff/17001/ash/accelerators/accelera...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/21297003/diff/17001/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:74: #include
"ash/display/display_info.h"
On 2013/07/31 16:39:11, oshima wrote:
> Changes in this file is just for my testing and I'll revert
> before landing, so please ignore them.

am i misunderstanding? the change in e.g. HandleScaleUI() doesn't look like it's
for testing

https://codereview.chromium.org/21297003/diff/17001/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:198: NOTREACHED() << "Unknown
rotatgion:" << current;
nit: s/rotatgion/rotation/

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_chang...
File ash/display/display_change_observer_x11.h (right):

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_chang...
ash/display/display_change_observer_x11.h:36: size_t* width,
why size_t? are you worried that these won't fit in ints? note that e.g.
gfx::Size uses ints.

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_info.cc
File ash/display/display_info.cc (right):

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_info....
ash/display/display_info.cc:26: : size(s), interlaced(i) {
nit: one member per line if they don't all fit on the same line as the signature

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_info....
ash/display/display_info.cc:226: resolutions_str += "," + iter->size.ToString();
shouldn't you test for !resolutions_str.empty() before adding the comma?

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_info.h
File ash/display/display_info.h (right):

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_info....
ash/display/display_info.h:23: Resolution(const gfx::Size& s, bool i);
nit: name parameters 'size' and 'interlaced' instead (you can do:

  Resolution::Resolution(...)
      : size(size),
        interlaced(interlaced) {
  }

)

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_manag...
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_manag...
ash/display/display_manager.cc:341: const gfx::Size& resolution_in_pixel) {
s/pixel/pixels/ (to match header)

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_manag...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_manag...
ash/display/display_manager.h:133: // Sets the displays's resolution.
nit: s/displays's/display's/

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_manag...
ash/display/display_manager.h:281: // Selected resolutions for displays.
nit: mention that keys are display IDs

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_util_...
File ash/display/display_util_x11.cc (right):

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_util_...
ash/display/display_util_x11.cc:19: // A list of bogus sizes in mm that X
detects and should be ignored.
nit: s/and should/and that should/

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_util_...
ash/display/display_util_x11.cc:21: const unsigned long
kInvalidDisplaySizeList[][2] = {
add a comment stating that this needs to be sorted by ascending size
(ShouldIgnoreSize will break otherwise)

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_util_...
ash/display/display_util_x11.cc:76:
resolution_map.insert(ResolutionMap::value_type(
why not just do:

  resolution_map[size] = Resolution(...)

?

https://codereview.chromium.org/21297003/diff/17001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://codereview.chromium.org/21297003/diff/17001/chrome/browser/chromeos/d...
chrome/browser/chromeos/display/display_preferences.cc:142: gfx::Size
resolution_in_pixel(width, height);
nit: s/pixel/pixels/

https://codereview.chromium.org/21297003/diff/17001/chromeos/display/output_u...
File chromeos/display/output_util.cc (right):

https://codereview.chromium.org/21297003/diff/17001/chromeos/display/output_u...
chromeos/display/output_util.cc:366: // found first time.
nit: s/found first/found the first/

https://codereview.chromium.org/21297003/diff/17001/chromeos/display/output_u...
chromeos/display/output_util.cc:403: }
nit: add // namespace test

https://codereview.chromium.org/21297003/diff/17001/chromeos/display/output_u...
File chromeos/display/output_util.h (right):

https://codereview.chromium.org/21297003/diff/17001/chromeos/display/output_u...
chromeos/display/output_util.h:72: // Find a mode that matches the given size
with highest reflesh
nit: s/reflesh/refresh/

https://codereview.chromium.org/21297003/diff/17001/chromeos/display/output_u...
chromeos/display/output_util.h:73: // rate. Non interlaced mode takes
precedence, so non interlaced mode
nit:

Non-interlaced modes take precedence, so a non-interlaced mode with a lower
refresh rate will be used even if there is an interlaced mode with a higher
refresh rate.

[oshima, 2013-07-31 21:59:48]

I'm still working on test for display_preference.

https://codereview.chromium.org/21297003/diff/17001/ash/accelerators/accelera...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/21297003/diff/17001/ash/accelerators/accelera...
ash/accelerators/accelerator_controller.cc:74: #include
"ash/display/display_info.h"
On 2013/07/31 17:34:05, Daniel Erat wrote:
> On 2013/07/31 16:39:11, oshima wrote:
> > Changes in this file is just for my testing and I'll revert
> > before landing, so please ignore them.
> 
> am i misunderstanding? the change in e.g. HandleScaleUI() doesn't look like
it's
> for testing

Sorry if it wasn't clear enough.
It was for me to test this feature without UI (UI will be added separately).
This file as been removed from CL in the latest patch.

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_chang...
File ash/display/display_change_observer_x11.h (right):

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_chang...
ash/display/display_change_observer_x11.h:36: size_t* width,
On 2013/07/31 17:34:05, Daniel Erat wrote:
> why size_t? are you worried that these won't fit in ints? note that e.g.
> gfx::Size uses ints.

sorry I forgot to fix this. I first thought this is what xrandr uses, which I
was wrong. Fixed.

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_info.cc
File ash/display/display_info.cc (right):

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_info....
ash/display/display_info.cc:26: : size(s), interlaced(i) {
On 2013/07/31 17:34:05, Daniel Erat wrote:
> nit: one member per line if they don't all fit on the same line as the
signature

Done.

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_info....
ash/display/display_info.cc:226: resolutions_str += "," + iter->size.ToString();
On 2013/07/31 17:34:05, Daniel Erat wrote:
> shouldn't you test for !resolutions_str.empty() before adding the comma?

Done.

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_info.h
File ash/display/display_info.h (right):

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_info....
ash/display/display_info.h:23: Resolution(const gfx::Size& s, bool i);
On 2013/07/31 17:34:05, Daniel Erat wrote:
> nit: name parameters 'size' and 'interlaced' instead (you can do:
> 
>   Resolution::Resolution(...)
>       : size(size),
>         interlaced(interlaced) {
>   }
> 
> )

Done. I'll keept the constructor in .cc as this has non trivial
member variable.

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_manag...
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_manag...
ash/display/display_manager.cc:341: const gfx::Size& resolution_in_pixel) {
On 2013/07/31 17:34:05, Daniel Erat wrote:
> s/pixel/pixels/ (to match header)

Done.

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_manag...
File ash/display/display_manager.h (right):

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_manag...
ash/display/display_manager.h:133: // Sets the displays's resolution.
On 2013/07/31 17:34:05, Daniel Erat wrote:
> nit: s/displays's/display's/

Done.

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_manag...
ash/display/display_manager.h:281: // Selected resolutions for displays.
On 2013/07/31 17:34:05, Daniel Erat wrote:
> nit: mention that keys are display IDs

Done.

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_util_...
File ash/display/display_util_x11.cc (right):

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_util_...
ash/display/display_util_x11.cc:19: // A list of bogus sizes in mm that X
detects and should be ignored.
On 2013/07/31 17:34:05, Daniel Erat wrote:
> nit: s/and should/and that should/

Done.

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_util_...
ash/display/display_util_x11.cc:21: const unsigned long
kInvalidDisplaySizeList[][2] = {
On 2013/07/31 17:34:05, Daniel Erat wrote:
> add a comment stating that this needs to be sorted by ascending size
> (ShouldIgnoreSize will break otherwise)

Only requirement is the 1st element is minimum size required. Updated the
comment.

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_util_...
ash/display/display_util_x11.cc:76:
resolution_map.insert(ResolutionMap::value_type(
On 2013/07/31 17:34:05, Daniel Erat wrote:
> why not just do:
> 
>   resolution_map[size] = Resolution(...)
> 
> ?

That didn't work because Resolution doesn't have default constructor
(Resultion()).
I'm not stl expert and I may be missing something though. Let me know if you
want to add it instead (or if there is better way).

https://codereview.chromium.org/21297003/diff/17001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://codereview.chromium.org/21297003/diff/17001/chrome/browser/chromeos/d...
chrome/browser/chromeos/display/display_preferences.cc:142: gfx::Size
resolution_in_pixel(width, height);
On 2013/07/31 17:34:05, Daniel Erat wrote:
> nit: s/pixel/pixels/

Done.

https://codereview.chromium.org/21297003/diff/17001/chromeos/display/output_u...
File chromeos/display/output_util.cc (right):

https://codereview.chromium.org/21297003/diff/17001/chromeos/display/output_u...
chromeos/display/output_util.cc:366: // found first time.
On 2013/07/31 17:34:05, Daniel Erat wrote:
> nit: s/found first/found the first/

Done.

https://codereview.chromium.org/21297003/diff/17001/chromeos/display/output_u...
chromeos/display/output_util.cc:403: }
On 2013/07/31 17:34:05, Daniel Erat wrote:
> nit: add // namespace test

Done.

https://codereview.chromium.org/21297003/diff/17001/chromeos/display/output_u...
File chromeos/display/output_util.h (right):

https://codereview.chromium.org/21297003/diff/17001/chromeos/display/output_u...
chromeos/display/output_util.h:72: // Find a mode that matches the given size
with highest reflesh
On 2013/07/31 17:34:05, Daniel Erat wrote:
> nit: s/reflesh/refresh/

Done.

https://codereview.chromium.org/21297003/diff/17001/chromeos/display/output_u...
chromeos/display/output_util.h:73: // rate. Non interlaced mode takes
precedence, so non interlaced mode
On 2013/07/31 17:34:05, Daniel Erat wrote:
> nit:
> 
> Non-interlaced modes take precedence, so a non-interlaced mode with a lower
> refresh rate will be used even if there is an interlaced mode with a higher
> refresh rate.

Done.

[Daniel Erat, 2013-07-31 22:10:17]

LGTM with a comment

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_util_...
File ash/display/display_util_x11.cc (right):

https://codereview.chromium.org/21297003/diff/17001/ash/display/display_util_...
ash/display/display_util_x11.cc:76:
resolution_map.insert(ResolutionMap::value_type(
On 2013/07/31 21:59:49, oshima wrote:
> On 2013/07/31 17:34:05, Daniel Erat wrote:
> > why not just do:
> > 
> >   resolution_map[size] = Resolution(...)
> > 
> > ?
> 
> That didn't work because Resolution doesn't have default constructor
> (Resultion()).
> I'm not stl expert and I may be missing something though. Let me know if you
> want to add it instead (or if there is better way).

ah, makes sense. this seems fine

https://codereview.chromium.org/21297003/diff/29001/ash/display/display_info.cc
File ash/display/display_info.cc (right):

https://codereview.chromium.org/21297003/diff/29001/ash/display/display_info....
ash/display/display_info.cc:227: if (resolutions_str.empty())
the other way, right? you should only append the comma if
!resolutions_str.empty()

[Jun Mukai, 2013-07-31 23:09:59]

https://codereview.chromium.org/21297003/diff/29001/ash/display/display_info.cc
File ash/display/display_info.cc (right):

https://codereview.chromium.org/21297003/diff/29001/ash/display/display_info....
ash/display/display_info.cc:106: display_info.SetBounds(bounds_in_pixel);
should we add the specified display's size to |resolutions_| ?

https://codereview.chromium.org/21297003/diff/29001/net/http/http_cache_unitt...
File net/http/http_cache_unittest.cc (left):

https://codereview.chromium.org/21297003/diff/29001/net/http/http_cache_unitt...
net/http/http_cache_unittest.cc:5319: // Verify that no-cache resources are
stored in cache, but are not fetched from
why these tests are removed?

[oshima, 2013-07-31 23:29:26]

https://codereview.chromium.org/21297003/diff/29001/ash/display/display_info.cc
File ash/display/display_info.cc (right):

https://codereview.chromium.org/21297003/diff/29001/ash/display/display_info....
ash/display/display_info.cc:106: display_info.SetBounds(bounds_in_pixel);
On 2013/07/31 23:10:00, Jun Mukai wrote:
> should we add the specified display's size to |resolutions_| ?
I didn't add it because it's not necessary, and in order to
use it for testing, it's better to add them manually.
(rather than adding another format)

https://codereview.chromium.org/21297003/diff/29001/ash/display/display_info....
ash/display/display_info.cc:227: if (resolutions_str.empty())
On 2013/07/31 22:10:17, Daniel Erat wrote:
> the other way, right? you should only append the comma if
> !resolutions_str.empty()

oops, you're right. done.

https://codereview.chromium.org/21297003/diff/29001/net/http/http_cache_unitt...
File net/http/http_cache_unittest.cc (left):

https://codereview.chromium.org/21297003/diff/29001/net/http/http_cache_unitt...
net/http/http_cache_unittest.cc:5319: // Verify that no-cache resources are
stored in cache, but are not fetched from
On 2013/07/31 23:10:00, Jun Mukai wrote:
> why these tests are removed?

This change has already been reverted and shouldn't be here.
I probably screwed my local branch somehow. This is removed from
latest patch.

[Jun Mukai, 2013-07-31 23:38:06]

lgtm

[oshima, 2013-08-01 00:05:31]

added test scenarios to DisplayPreferenceTest::BasicStores

mukai-san, can you take another look?

[Jun Mukai, 2013-08-01 02:41:00]

lgtm

please fix a few typo but code looks good to me.

https://codereview.chromium.org/21297003/diff/35002/chrome/browser/chromeos/d...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://codereview.chromium.org/21297003/diff/35002/chrome/browser/chromeos/d...
chrome/browser/chromeos/display/display_preferences.cc:202: LOG(ERROR) << "Store
Resolution for:" << display.ToString();
remove this log

https://codereview.chromium.org/21297003/diff/35002/chrome/browser/chromeos/d...
File chrome/browser/chromeos/display/display_preferences_unittest.cc (right):

https://codereview.chromium.org/21297003/diff/35002/chrome/browser/chromeos/d...
chrome/browser/chromeos/display/display_preferences_unittest.cc:260: //
Resolution is saved only when the resolutino is set
s/resolutino/resolution/

https://codereview.chromium.org/21297003/diff/35002/chrome/browser/chromeos/d...
chrome/browser/chromeos/display/display_preferences_unittest.cc:322: // External
dispaly's selected resolution must should not change
remove either "must" or "should"

[oshima, 2013-08-01 06:02:50]

https://codereview.chromium.org/21297003/diff/35002/chrome/browser/chromeos/d...
File chrome/browser/chromeos/display/display_preferences.cc (right):

https://codereview.chromium.org/21297003/diff/35002/chrome/browser/chromeos/d...
chrome/browser/chromeos/display/display_preferences.cc:202: LOG(ERROR) << "Store
Resolution for:" << display.ToString();
On 2013/08/01 02:41:00, Jun Mukai wrote:
> remove this log

Done.

https://codereview.chromium.org/21297003/diff/35002/chrome/browser/chromeos/d...
File chrome/browser/chromeos/display/display_preferences_unittest.cc (right):

https://codereview.chromium.org/21297003/diff/35002/chrome/browser/chromeos/d...
chrome/browser/chromeos/display/display_preferences_unittest.cc:260: //
Resolution is saved only when the resolutino is set
On 2013/08/01 02:41:00, Jun Mukai wrote:
> s/resolutino/resolution/

Done.

https://codereview.chromium.org/21297003/diff/35002/chrome/browser/chromeos/d...
chrome/browser/chromeos/display/display_preferences_unittest.cc:322: // External
dispaly's selected resolution must should not change
On 2013/08/01 02:41:00, Jun Mukai wrote:
> remove either "must" or "should"

Done.

[commit-bot: I haz the power, 2013-08-01 06:03:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/21297003/72001

[commit-bot: I haz the power, 2013-08-01 11:18:25]

Change committed as 214997