linux_aura: Implement most of DesktopScreenX11.

linux_aura: Implement most of DesktopScreenX11.

The linux_aura port didn't deal with multiple monitors very well because
it was treating the X root window as one big display. When xrandr is
present, get the screen areas from it, and exposes this data back to
chrome.

This patch also factors out the EDID parser than chromeos was using into
a common directory. Like chromeos, we use it to assign stable display
IDs.

BUG=287972
R=brettw@chromium.org, derat@chromium.org, sky@chromium.org

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

[Elliot Glaysher, 2013-09-19 20:53:11]

I'd like to get your opinion on my xrandr code. I couldn't find any
documentation, so I'm unsure how correct my code is. (It appears to do the right
thing On My Machine.)

https://codereview.chromium.org/23536057/diff/71018/base/edid_parser_x11.h
File base/edid_parser_x11.h (right):

https://codereview.chromium.org/23536057/diff/71018/base/edid_parser_x11.h#ne...
base/edid_parser_x11.h:15: namespace base {
Why put this in base? Because it's the only location that both chromeos/ and
ui/views/ can access. chromeos/ does not depend on ui/ (and from as much as I
can gather, shouldn't).

[Daniel Erat, 2013-09-19 22:13:03]

I didn't look at the tests yet.

https://codereview.chromium.org/23536057/diff/47001/chromeos/display/real_out...
File chromeos/display/real_output_configurator_delegate.cc (right):

https://codereview.chromium.org/23536057/diff/47001/chromeos/display/real_out...
chromeos/display/real_output_configurator_delegate.cc:7: #include <X11/Xlib.h>
nit: these are out-of-order now. does it not work otherwise?

https://codereview.chromium.org/23536057/diff/47001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/23536057/diff/47001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:53: xdisplay_,
&randr_version_major_, &randr_version_minor_);
i don't see any reason why the major and minor versions need to be members
instead of just locals within the c'tor. why not just:

  has_randr_ = XRRQueryVersion(...) && major == 1 && minor >= 3;

?

https://codereview.chromium.org/23536057/diff/47001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:161: if (!has_xrandr_)
can you just insert the fallback display in the c'tor for the no-xrandr case so
you don't need to special-case things here?

https://codereview.chromium.org/23536057/diff/47001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:284: NOTIMPLEMENTED();
maybe log an error instead? notimplemented seems strange here.

https://codereview.chromium.org/23536057/diff/47001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:291: if
(ui::GetIntArrayProperty(x_root_window_, "_NET_WORKAREA", &value) &&
there should be a separate 4-tuple for each desktop, right? do those typically
correspond to virtual desktops, or do you ever see each monitor get its own
desktop? i'm also not sure of the value of always taking the first workarea -- i
suspect that the right thing to do here is to also query the current desktop,
but then you'd probably need to update the bounds whenever the user switches
desktops. :-/

[Elliot Glaysher, 2013-09-19 23:08:33]

Changed to passing in fallback displays in the ctors. This let me add even more
tests.

https://codereview.chromium.org/23536057/diff/47001/chromeos/display/real_out...
File chromeos/display/real_output_configurator_delegate.cc (right):

https://codereview.chromium.org/23536057/diff/47001/chromeos/display/real_out...
chromeos/display/real_output_configurator_delegate.cc:7: #include <X11/Xlib.h>
On 2013/09/19 22:13:04, Daniel Erat wrote:
> nit: these are out-of-order now. does it not work otherwise?

This was an attempt at getting things to compile. (I tried to naievly
alphabetizing these, and found that dpms.h doesn't include everything from
Xlib.h that it needs.) Fixed.

https://codereview.chromium.org/23536057/diff/47001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/23536057/diff/47001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:161: if (!has_xrandr_)
On 2013/09/19 22:13:04, Daniel Erat wrote:
> can you just insert the fallback display in the c'tor for the no-xrandr case
so
> you don't need to special-case things here?

That's a really good idea, and lets me unit test a bunch more of these methods.

https://codereview.chromium.org/23536057/diff/47001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:291: if
(ui::GetIntArrayProperty(x_root_window_, "_NET_WORKAREA", &value) &&
On 2013/09/19 22:13:04, Daniel Erat wrote:
> there should be a separate 4-tuple for each desktop, right? do those typically
> correspond to virtual desktops, or do you ever see each monitor get its own
> desktop? i'm also not sure of the value of always taking the first workarea --
i
> suspect that the right thing to do here is to also query the current desktop,
> but then you'd probably need to update the bounds whenever the user switches
> desktops. :-/

All the documentation that I can find says that _NET_WORKAREA is always a
4-tuple. Testing this on my multimonitor xrandr enabled setup seems so:

$ xprop -root _NET_WORKAREA
_NET_WORKAREA(CARDINAL) = 0, 24, 3760, 1896

Some random stack overflow posts suggested just applying _NET_WORKAREA
everywhere, even if it over trims the work area on other monitors. If you know
of a better way to get this information, I'd love to know.

[Daniel Erat, 2013-09-19 23:32:36]

pretty much LGTM now

https://codereview.chromium.org/23536057/diff/47001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/23536057/diff/47001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:291: if
(ui::GetIntArrayProperty(x_root_window_, "_NET_WORKAREA", &value) &&
On 2013/09/19 23:08:34, Elliot Glaysher wrote:
> On 2013/09/19 22:13:04, Daniel Erat wrote:
> > there should be a separate 4-tuple for each desktop, right? do those
typically
> > correspond to virtual desktops, or do you ever see each monitor get its own
> > desktop? i'm also not sure of the value of always taking the first workarea
--
> i
> > suspect that the right thing to do here is to also query the current
desktop,
> > but then you'd probably need to update the bounds whenever the user switches
> > desktops. :-/
> 
> All the documentation that I can find says that _NET_WORKAREA is always a
> 4-tuple. Testing this on my multimonitor xrandr enabled setup seems so:
> 
> $ xprop -root _NET_WORKAREA
> _NET_WORKAREA(CARDINAL) = 0, 24, 3760, 1896
> 
> Some random stack overflow posts suggested just applying _NET_WORKAREA
> everywhere, even if it over trims the work area on other monitors. If you know
> of a better way to get this information, I'd love to know.

the spec
(http://standards.freedesktop.org/wm-spec/wm-spec-latest.html#idp6261696) says
that there's a workarea for each desktop. punting on this seems fine if it
doesn't look like window managers are following the spec, though.

https://codereview.chromium.org/23536057/diff/120001/base/edid_parser_x11.h
File base/edid_parser_x11.h (right):

https://codereview.chromium.org/23536057/diff/120001/base/edid_parser_x11.h#n...
base/edid_parser_x11.h:5: #ifndef BASE_EDID_PARSER_X11_H_
there's a base/win directory. i wonder if these files belong in a base/x11 one
instead of at the top level.

https://codereview.chromium.org/23536057/diff/120001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/23536057/diff/120001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:321: LOG(ERROR) << "XRandR
claims there are no valid displays. Falling back.";
i wonder if this legitimately happens if the user e.g. unplugs the sole monitor
from their desktop machine...

[Elliot Glaysher, 2013-09-20 17:23:02]

sky: views code

brettw: base changes. We can't put this code in ui/ because chromeos/ doesn't
(and probably shouldn't) depend on ui/. The only place that both ui/views/ and
chromeos/ can access is base/ (and net/, but that's obviously wrong). I assume
there's no problem since theres a whole bunch of hwnd code in base/?

[sky, 2013-09-20 17:45:43]

Elliot, you're an owner of these files (except views.gyp) so as long as derat
reviewed the views side that's all you need.

I'll LGTM the .gyp change.

[brettw, 2013-09-21 05:19:26]

base addition lgtm, didn't look at the details.

https://codereview.chromium.org/23536057/diff/127001/base/x11/edid_parser_x11.h
File base/x11/edid_parser_x11.h (right):

https://codereview.chromium.org/23536057/diff/127001/base/x11/edid_parser_x11...
base/x11/edid_parser_x11.h:13: typedef unsigned long XID;
Can you add a comment near the top somewhere saying what an EDID is? I doubt
most people have heard of it.

[commit-bot: I haz the power, 2013-09-24 17:40:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/23536057/192001

[commit-bot: I haz the power, 2013-09-24 18:10:14]

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

[Elliot Glaysher, 2013-09-24 19:53:18]

Committed patchset #20 manually as r225054 (presubmit successful).