ash: Get output info from chromeos instead of XRandR.

ash: Get output info from chromeos instead of XRandR.

This updates DisplayChangeObserverX11 to use the output info
passed by OutputConfigurator instead of querying XRandR for
it. Doing so avoids a 60 ms call to XRRGetScreenResources().

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

[Daniel Erat, 2013-08-16 22:17:15]

I'm planning to rename these files from _x11 to _chromeos and to move the
contents of the _util file into display_change_observer. Let me know if you'd
rather I did that in this change instead of in a following change.

[oshima, 2013-08-16 23:13:05]

On 2013/08/16 22:17:15, Daniel Erat wrote:
> I'm planning to rename these files from _x11 to _chromeos and to move the
> contents of the _util file into display_change_observer. Let me know if you'd
> rather I did that in this change instead of in a following change.

Which ever you like.

https://codereview.chromium.org/23286003/diff/1/ash/display/display_change_ob...
File ash/display/display_change_observer_x11.cc (right):

https://codereview.chromium.org/23286003/diff/1/ash/display/display_change_ob...
ash/display/display_change_observer_x11.cc:92: output.display_id);
output_index is different from the index of outputs. It has to be
the index in screen_resources->outputs because the index for the same output in
outputs will change.

https://codereview.chromium.org/23286003/diff/1/ash/display/display_change_ob...
ash/display/display_change_observer_x11.cc:124: id = output_index;
same here.

https://codereview.chromium.org/23286003/diff/1/ash/display/display_util_x11.cc
File ash/display/display_util_x11.cc (right):

https://codereview.chromium.org/23286003/diff/1/ash/display/display_util_x11....
ash/display/display_util_x11.cc:59: const OutputConfigurator::OutputSnapshot&
output) {
isn't it better to get this in output_configurator?

[Daniel Erat, 2013-08-16 23:35:36]

https://codereview.chromium.org/23286003/diff/1/ash/display/display_change_ob...
File ash/display/display_change_observer_x11.cc (right):

https://codereview.chromium.org/23286003/diff/1/ash/display/display_change_ob...
ash/display/display_change_observer_x11.cc:92: output.display_id);
On 2013/08/16 23:13:05, oshima wrote:
> output_index is different from the index of outputs. It has to be
> the index in screen_resources->outputs because the index for the same output
in
> outputs will change.

Thanks, I'll store the original index in the OutputSnapshot.

https://codereview.chromium.org/23286003/diff/1/ash/display/display_util_x11.cc
File ash/display/display_util_x11.cc (right):

https://codereview.chromium.org/23286003/diff/1/ash/display/display_util_x11....
ash/display/display_util_x11.cc:59: const OutputConfigurator::OutputSnapshot&
output) {
On 2013/08/16 23:13:05, oshima wrote:
> isn't it better to get this in output_configurator?

You mean, move this method to OutputConfigurator? That's fine by me.

[oshima, 2013-08-16 23:54:05]

https://codereview.chromium.org/23286003/diff/1/ash/display/display_util_x11.cc
File ash/display/display_util_x11.cc (right):

https://codereview.chromium.org/23286003/diff/1/ash/display/display_util_x11....
ash/display/display_util_x11.cc:59: const OutputConfigurator::OutputSnapshot&
output) {
On 2013/08/16 23:35:36, Daniel Erat wrote:
> On 2013/08/16 23:13:05, oshima wrote:
> > isn't it better to get this in output_configurator?
> 
> You mean, move this method to OutputConfigurator? That's fine by me.
On second thought, no, but we should remove this to display_util_chromeos too
because there is no x11 things any more.

[oshima, 2013-08-16 23:55:00]

On 2013/08/16 23:54:05, oshima wrote:
>
https://codereview.chromium.org/23286003/diff/1/ash/display/display_util_x11.cc
> File ash/display/display_util_x11.cc (right):
> 
>
https://codereview.chromium.org/23286003/diff/1/ash/display/display_util_x11....
> ash/display/display_util_x11.cc:59: const OutputConfigurator::OutputSnapshot&
> output) {
> On 2013/08/16 23:35:36, Daniel Erat wrote:
> > On 2013/08/16 23:13:05, oshima wrote:
> > > isn't it better to get this in output_configurator?
> > 
> > You mean, move this method to OutputConfigurator? That's fine by me.
> On second thought, no, but we should remove this to display_util_chromeos too
> because there is no x11 things any more.

Ah you meant both, not just display_change_observer, so I'm fine with what you
proposed.

[Daniel Erat, 2013-08-18 04:24:11]

I moved display_util into display_change_observer and renamed files from _x11 to
_chromeos (which seems more accurate, since they depend on files in the
/chromeos directory).

[oshima, 2013-08-19 20:00:24]

https://codereview.chromium.org/23286003/diff/1/ash/display/display_change_ob...
File ash/display/display_change_observer_x11.cc (right):

https://codereview.chromium.org/23286003/diff/1/ash/display/display_change_ob...
ash/display/display_change_observer_x11.cc:92: output.display_id);
On 2013/08/16 23:35:36, Daniel Erat (OOO 8-20 to 8-23) wrote:
> On 2013/08/16 23:13:05, oshima wrote:
> > output_index is different from the index of outputs. It has to be
> > the index in screen_resources->outputs because the index for the same output
> in
> > outputs will change.
> 
> Thanks, I'll store the original index in the OutputSnapshot.

This doesn't seem to be in the CL?

https://codereview.chromium.org/23286003/diff/12001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/23286003/diff/12001/ash/ash.gyp#newcode556
ash/ash.gyp:556: ['use_x11!=1', {
can you add rule in build/filename_rules.gypi instead?

Also shouldn't this be chromeos!=1?

https://codereview.chromium.org/23286003/diff/12001/ash/ash.gyp#newcode812
ash/ash.gyp:812: ['use_x11!=1', {
ditto

https://codereview.chromium.org/23286003/diff/12001/ash/display/display_chang...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/23286003/diff/12001/ash/display/display_chang...
ash/display/display_change_observer_chromeos.cc:3: // found in the LICENSE file.
can you adjust similarity so that we can keep history?

https://codereview.chromium.org/23286003/diff/12001/ash/display/display_chang...
File ash/display/display_change_observer_chromeos_unittest.cc (right):

https://codereview.chromium.org/23286003/diff/12001/ash/display/display_chang...
ash/display/display_change_observer_chromeos_unittest.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
same here.

[Daniel Erat, 2013-08-19 22:09:29]

https://codereview.chromium.org/23286003/diff/1/ash/display/display_change_ob...
File ash/display/display_change_observer_x11.cc (right):

https://codereview.chromium.org/23286003/diff/1/ash/display/display_change_ob...
ash/display/display_change_observer_x11.cc:92: output.display_id);
On 2013/08/19 20:00:24, oshima wrote:
> On 2013/08/16 23:35:36, Daniel Erat (OOO 8-20 to 8-23) wrote:
> > On 2013/08/16 23:13:05, oshima wrote:
> > > output_index is different from the index of outputs. It has to be
> > > the index in screen_resources->outputs because the index for the same
output
> > in
> > > outputs will change.
> > 
> > Thanks, I'll store the original index in the OutputSnapshot.
> 
> This doesn't seem to be in the CL?

Whoops, sorry. Adding.

https://codereview.chromium.org/23286003/diff/12001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/23286003/diff/12001/ash/ash.gyp#newcode556
ash/ash.gyp:556: ['use_x11!=1', {
On 2013/08/19 20:00:24, oshima wrote:
> can you add rule in build/filename_rules.gypi instead?
> 
> Also shouldn't this be chromeos!=1?

_chromeos.* should already be filtered out of non-Chrome-OS builds. I have
use_x11 so that these files will additionally be excluded from non-X11 Chrome OS
builds (if we ever do such a thing). It doesn't look like we support e.g.
_chomeos_x11.*.

https://codereview.chromium.org/23286003/diff/12001/ash/display/display_chang...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/23286003/diff/12001/ash/display/display_chang...
ash/display/display_change_observer_chromeos.cc:3: // found in the LICENSE file.
On 2013/08/19 20:00:24, oshima wrote:
> can you adjust similarity so that we can keep history?

Sure, I'll give this a try.

[oshima, 2013-08-19 22:27:11]

lgtm

[commit-bot: I haz the power, 2013-08-19 22:33:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/23286003/21001

[commit-bot: I haz the power, 2013-08-20 09:35:41]

Change committed as 218427