Use persistent ID/names for displays.

Use persistent ID/names for displays.

Adds to x11_util the API to get device manufacturer's ID, serial#, and
human readable name.  Use the manufacturer ID and serial# for gfx::Display::id().

BUG=139103
TEST=manually injected debug printing and made sure how the device ID/names would be.  The device name would be visible once crbug.com/130385 is fixed.


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

[Jun Mukai, 2012-08-09 09:02:06]

oshima, can you take another look?  I modified the code a bit to modify the
gfx::Display::id() itself, as we talked offline.

[oshima, 2012-08-10 03:09:51]

http://codereview.chromium.org/10826198/diff/7021/ash/display/display_control...
File ash/display/display_controller.h (right):

http://codereview.chromium.org/10826198/diff/7021/ash/display/display_control...
ash/display/display_controller.h:94: // map.
// TODO(oshima): remove |is_primary| when non extended desktop mode is removed.

http://codereview.chromium.org/10826198/diff/7021/ui/aura/display_change_obse...
File ui/aura/display_change_observer_x11.cc (right):

http://codereview.chromium.org/10826198/diff/7021/ui/aura/display_change_obse...
ui/aura/display_change_observer_x11.cc:162: }
Please add comment that we fallback the to use index when it fails to retrieve
these info.

http://codereview.chromium.org/10826198/diff/7021/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/10826198/diff/7021/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:999: return false;
Since you get XDisplay below, 

if (!display)
  return false; 

would be better.

http://codereview.chromium.org/10826198/diff/7021/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1004: XRRGetScreenResources(display, root_window);
can this handle when the X doesn't support Xrandr?

http://codereview.chromium.org/10826198/diff/7021/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1007: }
nuke {}

http://codereview.chromium.org/10826198/diff/7021/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1021: return false;
ditto.

http://codereview.chromium.org/10826198/diff/7021/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1061: *manufacturer_id = *reinterpret_cast<uint16*>(prop +
kManufacturerOffset);
I assume the format guarantees the correct alignment. Just want to confirm
because we just had issue on Arm due to mis-alignment.

[Jun Mukai, 2012-08-10 09:39:52]

http://codereview.chromium.org/10826198/diff/7021/ash/display/display_control...
File ash/display/display_controller.h (right):

http://codereview.chromium.org/10826198/diff/7021/ash/display/display_control...
ash/display/display_controller.h:94: // map.
On 2012/08/10 03:09:51, oshima wrote:
> // TODO(oshima): remove |is_primary| when non extended desktop mode is
removed.

Done.

http://codereview.chromium.org/10826198/diff/7021/ui/aura/display_change_obse...
File ui/aura/display_change_observer_x11.cc (right):

http://codereview.chromium.org/10826198/diff/7021/ui/aura/display_change_obse...
ui/aura/display_change_observer_x11.cc:162: }
On 2012/08/10 03:09:51, oshima wrote:
> Please add comment that we fallback the to use index when it fails to retrieve
> these info.

Done.

http://codereview.chromium.org/10826198/diff/7021/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/10826198/diff/7021/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:999: return false;
On 2012/08/10 03:09:51, oshima wrote:
> Since you get XDisplay below, 
> 
> if (!display)
>   return false; 
> 
> would be better.

Done.

http://codereview.chromium.org/10826198/diff/7021/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1004: XRRGetScreenResources(display, root_window);
On 2012/08/10 03:09:51, oshima wrote:
> can this handle when the X doesn't support Xrandr?

Done.

http://codereview.chromium.org/10826198/diff/7021/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1007: }
On 2012/08/10 03:09:51, oshima wrote:
> nuke {}

Done.

http://codereview.chromium.org/10826198/diff/7021/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1021: return false;
On 2012/08/10 03:09:51, oshima wrote:
> ditto.

Done.

http://codereview.chromium.org/10826198/diff/7021/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1061: *manufacturer_id = *reinterpret_cast<uint16*>(prop +
kManufacturerOffset);
On 2012/08/10 03:09:51, oshima wrote:
> I assume the format guarantees the correct alignment. Just want to confirm
> because we just had issue on Arm due to mis-alignment.

We do not have arm devices here... :(
As we discussed offline, it would not cause problems.  Let's move with this code
and see how it goes.

[Jun Mukai, 2012-08-14 11:45:14]

Hello Rahul and Stephane,

oshima is on vacation right now.  Could you review this CL?

[rkc, 2012-08-14 20:34:03]

On 2012/08/14 11:45:14, Jun Mukai wrote:
> Hello Rahul and Stephane,
> 
> oshima is on vacation right now.  Could you review this CL?

Added Scott, he'll be a much more appropriate reviewer for the ui/aura and ash/
parts.

[marcheu, 2012-08-14 21:29:53]

http://codereview.chromium.org/10826198/diff/7022/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/10826198/diff/7022/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1093: if (desc_buf[3] == 0xfc) {
maybe replace 0xfc with kMonitorNameDescriptor or something similar

http://codereview.chromium.org/10826198/diff/7022/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1098: } else if (desc_buf[3] == 0xfe &&
name_candidate.empty()) {
Here maybe replace 0xfe with kUnspecifiedTextDescriptor or something similar

http://codereview.chromium.org/10826198/diff/7022/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1109: }
At this point I'd probably sanity check the string. At least something like a
simple check for isascii() + return false will filter the worst ones.

[sky, 2012-08-14 22:32:05]

LGTM - I'm not a good reviewer for the X side of these changes. Ask Dan and/or
Oshima for that.

http://codereview.chromium.org/10826198/diff/7022/ui/gfx/display.h
File ui/gfx/display.h (right):

http://codereview.chromium.org/10826198/diff/7022/ui/gfx/display.h#newcode25
ui/gfx/display.h:25: // Returns 64-bit persistent ID for the specified
manufacturer's ID and
constructor/destructor should be before any other methods, including statics.

[Jun Mukai, 2012-08-15 05:14:08]

http://codereview.chromium.org/10826198/diff/7022/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/10826198/diff/7022/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1093: if (desc_buf[3] == 0xfc) {
On 2012/08/14 21:29:53, marcheu wrote:
> maybe replace 0xfc with kMonitorNameDescriptor or something similar
> 

Done.

http://codereview.chromium.org/10826198/diff/7022/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1098: } else if (desc_buf[3] == 0xfe &&
name_candidate.empty()) {
On 2012/08/14 21:29:53, marcheu wrote:
> Here maybe replace 0xfe with kUnspecifiedTextDescriptor or something similar

Done.

http://codereview.chromium.org/10826198/diff/7022/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1109: }
On 2012/08/14 21:29:53, marcheu wrote:
> At this point I'd probably sanity check the string. At least something like a
> simple check for isascii() + return false will filter the worst ones.

Done.  Using isprint() too.

http://codereview.chromium.org/10826198/diff/7022/ui/gfx/display.h
File ui/gfx/display.h (right):

http://codereview.chromium.org/10826198/diff/7022/ui/gfx/display.h#newcode25
ui/gfx/display.h:25: // Returns 64-bit persistent ID for the specified
manufacturer's ID and
On 2012/08/14 22:32:05, sky wrote:
> constructor/destructor should be before any other methods, including statics.

Done.

[Jun Mukai, 2012-08-15 07:53:42]

Dan, can you review the X11 code in this CL?

[Daniel Erat, 2012-08-15 14:13:04]

*x11* mostly looks fine to me; just a few nits.

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:314: display, &randr_version_major, &randr_version_minor);
nit: this shouldn't change while we're running.  cache the result in a static
variable?

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1007: if (!display)
is this needed?  is a display handle ever not available?

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1031: if (!display)
ditto here

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1056: XRRGetOutputProperty(display, output, edid_property,
0, 128, false, false,
nit: add comments documenting what the constants are so people don't need to go
digging for the header?

XRRGetOutputProperty(
    display,
    output,
    edid_property,
    0,  // offset
    128,  // length
    False,  // _delete
    False,  // pending
    etc.

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1059: DCHECK(actual_type == XA_INTEGER);
nit: DCHECK_EQ()?

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1061: 
check the length of the returned data here and bail out if it's shorter than
expected

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1106: // "FE" ("Unspecified text"). Thus here uses this
value as the fallback
nit: s/uses/use/

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1107: // of the lack of "FC". Note that multiple
descriptors may have "FE",
nit: s/of the lack of "FC"/if "FC" is missing/

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1118: bool succeed = true;
nit: s/succeed/success/

just do:

bool success = !human_readable_name->empty();

?  or, you could call XFree(prop) earlier, after the loop, and just return false
here as soon as you find any issues

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1119: // Verify the |human_readable_name| in case.
nit: this sentence ends a bit prematurely -- in case what?

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.h#newco...
ui/base/x/x11_util.h:246: // fails to get those data and doesn't touch
manufacturer ID/serial#/name.
nit: make it so NULL can be passed for any fields that the caller doesn't care
about, and document that behavior in this comment

[Jun Mukai, 2012-08-16 07:52:18]

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:314: display, &randr_version_major, &randr_version_minor);
On 2012/08/15 14:13:04, Daniel Erat wrote:
> nit: this shouldn't change while we're running.  cache the result in a static
> variable?

Done.

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1007: if (!display)
On 2012/08/15 14:13:04, Daniel Erat wrote:
> is this needed?  is a display handle ever not available?

oh, I looked over the files and nobody in this file cares that and I believe
it's always available.  removed the check.  Thanks.

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1031: if (!display)
On 2012/08/15 14:13:04, Daniel Erat wrote:
> ditto here

Done.

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1056: XRRGetOutputProperty(display, output, edid_property,
0, 128, false, false,
On 2012/08/15 14:13:04, Daniel Erat wrote:
> nit: add comments documenting what the constants are so people don't need to
go
> digging for the header?
> 
> XRRGetOutputProperty(
>     display,
>     output,
>     edid_property,
>     0,  // offset
>     128,  // length
>     False,  // _delete
>     False,  // pending
>     etc.

Done.

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1059: DCHECK(actual_type == XA_INTEGER);
On 2012/08/15 14:13:04, Daniel Erat wrote:
> nit: DCHECK_EQ()?

Done.

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.h#newco...
ui/base/x/x11_util.h:246: // fails to get those data and doesn't touch
manufacturer ID/serial#/name.
On 2012/08/15 14:13:04, Daniel Erat wrote:
> nit: make it so NULL can be passed for any fields that the caller doesn't care
> about, and document that behavior in this comment

Done.

[Daniel Erat, 2012-08-16 15:28:49]

http://codereview.chromium.org/10826198/diff/11002/ui/aura/display_change_obs...
File ui/aura/display_change_observer_x11.cc (right):

http://codereview.chromium.org/10826198/diff/11002/ui/aura/display_change_obs...
ui/aura/display_change_observer_x11.cc:158: &serial_number, NULL)) {
maybe also check that manufacturer_id is greater than 0 in this condition? 
otherwise, if the monitor is misbehaving, it seems like we could end up with two
displays with the same ID, which i assume will cause trouble later

http://codereview.chromium.org/10826198/diff/11002/ui/aura/display_change_obs...
ui/aura/display_change_observer_x11.cc:159: // Fallback to use index in case
that it fails to retrieve these data.
nit: change to something like "An ID based on the display's index will be
assigned later if this call fails."

http://codereview.chromium.org/10826198/diff/11002/ui/aura/display_change_obs...
ui/aura/display_change_observer_x11.cc:183: if (iter->id() == -1) {
nit: add a public constant static gfx::Display::kInvalidId and use it here and
in the c'tor

http://codereview.chromium.org/10826198/diff/11002/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/10826198/diff/11002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1036: Atom edid_property =
GetAtom(RR_PROPERTY_RANDR_EDID);
nit: cache this too?

http://codereview.chromium.org/10826198/diff/11002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1136: bool succeed = true;
i had some comments here in the previous round that didn't get responded to, i
think

http://codereview.chromium.org/10826198/diff/11002/ui/gfx/display.h
File ui/gfx/display.h (right):

http://codereview.chromium.org/10826198/diff/11002/ui/gfx/display.h#newcode21
ui/gfx/display.h:21: // Creates a display with invalid id(-1) as default.
nit: space between "id" and "(-1)"

[Jun Mukai, 2012-08-17 05:50:09]

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1061: 
On 2012/08/15 14:13:04, Daniel Erat wrote:
> check the length of the returned data here and bail out if it's shorter than
> expected

Done.

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1106: // "FE" ("Unspecified text"). Thus here uses this
value as the fallback
On 2012/08/15 14:13:04, Daniel Erat wrote:
> nit: s/uses/use/

Done.

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1107: // of the lack of "FC". Note that multiple
descriptors may have "FE",
On 2012/08/15 14:13:04, Daniel Erat wrote:
> nit: s/of the lack of "FC"/if "FC" is missing/

Done.

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1118: bool succeed = true;
On 2012/08/15 14:13:04, Daniel Erat wrote:
> nit: s/succeed/success/
> 
> just do:
> 
> bool success = !human_readable_name->empty();
> 
> ?  or, you could call XFree(prop) earlier, after the loop, and just return
false
> here as soon as you find any issues

Done.

http://codereview.chromium.org/10826198/diff/12002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1119: // Verify the |human_readable_name| in case.
On 2012/08/15 14:13:04, Daniel Erat wrote:
> nit: this sentence ends a bit prematurely -- in case what?

Done.

http://codereview.chromium.org/10826198/diff/11002/ui/aura/display_change_obs...
File ui/aura/display_change_observer_x11.cc (right):

http://codereview.chromium.org/10826198/diff/11002/ui/aura/display_change_obs...
ui/aura/display_change_observer_x11.cc:158: &serial_number, NULL)) {
On 2012/08/16 15:28:49, Daniel Erat wrote:
> maybe also check that manufacturer_id is greater than 0 in this condition? 
> otherwise, if the monitor is misbehaving, it seems like we could end up with
two
> displays with the same ID, which i assume will cause trouble later

Done.

http://codereview.chromium.org/10826198/diff/11002/ui/aura/display_change_obs...
ui/aura/display_change_observer_x11.cc:159: // Fallback to use index in case
that it fails to retrieve these data.
On 2012/08/16 15:28:49, Daniel Erat wrote:
> nit: change to something like "An ID based on the display's index will be
> assigned later if this call fails."

Done.

http://codereview.chromium.org/10826198/diff/11002/ui/aura/display_change_obs...
ui/aura/display_change_observer_x11.cc:183: if (iter->id() == -1) {
On 2012/08/16 15:28:49, Daniel Erat wrote:
> nit: add a public constant static gfx::Display::kInvalidId and use it here and
> in the c'tor

Done.

http://codereview.chromium.org/10826198/diff/11002/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/10826198/diff/11002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1036: Atom edid_property =
GetAtom(RR_PROPERTY_RANDR_EDID);
On 2012/08/16 15:28:49, Daniel Erat wrote:
> nit: cache this too?

Done.

http://codereview.chromium.org/10826198/diff/11002/ui/base/x/x11_util.cc#newc...
ui/base/x/x11_util.cc:1136: bool succeed = true;
On 2012/08/16 15:28:49, Daniel Erat wrote:
> i had some comments here in the previous round that didn't get responded to, i
> think

Sorry I've missed your comments.  Replied.

http://codereview.chromium.org/10826198/diff/11002/ui/gfx/display.h
File ui/gfx/display.h (right):

http://codereview.chromium.org/10826198/diff/11002/ui/gfx/display.h#newcode21
ui/gfx/display.h:21: // Creates a display with invalid id(-1) as default.
On 2012/08/16 15:28:49, Daniel Erat wrote:
> nit: space between "id" and "(-1)"
replaced by kInvalidID

[Daniel Erat, 2012-08-17 14:33:46]

http://codereview.chromium.org/10826198/diff/8002/ui/aura/display_change_obse...
File ui/aura/display_change_observer_x11.cc (right):

http://codereview.chromium.org/10826198/diff/8002/ui/aura/display_change_obse...
ui/aura/display_change_observer_x11.cc:162: gfx::Display::GetID(manufacturer_id,
serial_number));
Maybe I'm being overly paranoid here, but I wonder if it's worthwhile to also
keep a set to make sure that we don't try to use the same ID twice.  It's not
impossible for me to imagine a manufacturer being lazy and shipping monitors
that all report the same serial number...

http://codereview.chromium.org/10826198/diff/8002/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/10826198/diff/8002/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1071: DCHECK_LE(128u, nitems);
in my earlier comment, i meant that you should check that the length of the
property isn't too short, rather than whether it's longer than expected.  it'd
be good to do this inside of an if statement and return false on failure instead
of just doing it in a DCHECK (so we don't crash in production)

http://codereview.chromium.org/10826198/diff/8002/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

http://codereview.chromium.org/10826198/diff/8002/ui/base/x/x11_util.h#newcod...
ui/base/x/x11_util.h:247: // You can pass NULL to |manufacturer_id|,
|serial_number|, and
nit: change to "NULL can be passed for unwanted output parameters."

[Jun Mukai, 2012-08-20 05:56:38]

http://codereview.chromium.org/10826198/diff/8002/ui/aura/display_change_obse...
File ui/aura/display_change_observer_x11.cc (right):

http://codereview.chromium.org/10826198/diff/8002/ui/aura/display_change_obse...
ui/aura/display_change_observer_x11.cc:162: gfx::Display::GetID(manufacturer_id,
serial_number));
On 2012/08/17 14:33:46, Daniel Erat wrote:
> Maybe I'm being overly paranoid here, but I wonder if it's worthwhile to also
> keep a set to make sure that we don't try to use the same ID twice.  It's not
> impossible for me to imagine a manufacturer being lazy and shipping monitors
> that all report the same serial number...

Hmm, I see.  Added a check code so that index-based ID will be used in that
case.

http://codereview.chromium.org/10826198/diff/8002/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

http://codereview.chromium.org/10826198/diff/8002/ui/base/x/x11_util.cc#newco...
ui/base/x/x11_util.cc:1071: DCHECK_LE(128u, nitems);
On 2012/08/17 14:33:46, Daniel Erat wrote:
> in my earlier comment, i meant that you should check that the length of the
> property isn't too short, rather than whether it's longer than expected.  it'd
> be good to do this inside of an if statement and return false on failure
instead
> of just doing it in a DCHECK (so we don't crash in production)

Done.

http://codereview.chromium.org/10826198/diff/8002/ui/base/x/x11_util.h
File ui/base/x/x11_util.h (right):

http://codereview.chromium.org/10826198/diff/8002/ui/base/x/x11_util.h#newcod...
ui/base/x/x11_util.h:247: // You can pass NULL to |manufacturer_id|,
|serial_number|, and
On 2012/08/17 14:33:46, Daniel Erat wrote:
> nit: change to "NULL can be passed for unwanted output parameters."

Done.

[Daniel Erat, 2012-08-20 15:12:07]

lgtm

http://codereview.chromium.org/10826198/diff/7025/ash/display/display_control...
File ash/display/display_controller.h (right):

http://codereview.chromium.org/10826198/diff/7025/ash/display/display_control...
ash/display/display_controller.h:110: std::map<int64, aura::RootWindow*>
root_windows_;
nit: add a comment describing what the key value represents

http://codereview.chromium.org/10826198/diff/7025/ui/gfx/display.h
File ui/gfx/display.h (right):

http://codereview.chromium.org/10826198/diff/7025/ui/gfx/display.h#newcode90
ui/gfx/display.h:90: static const int64 kInvalidID;
nit: add blank line after this line

[Jun Mukai, 2012-08-21 02:13:11]

http://codereview.chromium.org/10826198/diff/7025/ash/display/display_control...
File ash/display/display_controller.h (right):

http://codereview.chromium.org/10826198/diff/7025/ash/display/display_control...
ash/display/display_controller.h:110: std::map<int64, aura::RootWindow*>
root_windows_;
On 2012/08/20 15:12:07, Daniel Erat wrote:
> nit: add a comment describing what the key value represents

Done.

http://codereview.chromium.org/10826198/diff/7025/ui/gfx/display.h
File ui/gfx/display.h (right):

http://codereview.chromium.org/10826198/diff/7025/ui/gfx/display.h#newcode90
ui/gfx/display.h:90: static const int64 kInvalidID;
On 2012/08/20 15:12:07, Daniel Erat wrote:
> nit: add blank line after this line

Done.

[commit-bot: I haz the power, 2012-08-21 04:03:13]

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

[commit-bot: I haz the power, 2012-08-21 04:19:19]

Try job failure for 10826198-3039 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-21 04:31:50]

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

[commit-bot: I haz the power, 2012-08-21 06:46:24]

Change committed as 152519

[yurys, 2012-08-21 11:57:17]

After this change I'm getting the following linker errror:

../../third_party/gold/gold64: obj/ui/libui.a(obj/ui/base/x/ui.x11_util.o): in
function ui::(anonymous
namespace)::IsRandRAvailable():../../ui/base/x/x11_util.cc:319: error: undefined
reference to 'XRRQueryVersion'
../../third_party/gold/gold64: obj/ui/libui.a(obj/ui/base/x/ui.x11_util.o): in
function ui::GetOutputDeviceHandles(std::vector<unsigned long,
std::allocator<unsigned long> >*):../../ui/base/x/x11_util.cc:1020: error:
undefined reference to 'XRRGetScreenResources'
../../third_party/gold/gold64: obj/ui/libui.a(obj/ui/base/x/ui.x11_util.o): in
function ui::GetOutputDeviceHandles(std::vector<unsigned long,
std::allocator<unsigned long> >*):../../ui/base/x/x11_util.cc:1023: error:
undefined reference to 'XRRFreeScreenResources'
../../third_party/gold/gold64: obj/ui/libui.a(obj/ui/base/x/ui.x11_util.o): in
function ui::GetOutputDeviceData(unsigned long, unsigned short*, unsigned int*,
std::basic_string<char, std::char_traits<char>, std::allocator<char>
>*):../../ui/base/x/x11_util.cc:1040: error: undefined reference to
'XRRListOutputProperties'
../../third_party/gold/gold64: obj/ui/libui.a(obj/ui/base/x/ui.x11_util.o): in
function ui::GetOutputDeviceData(unsigned long, unsigned short*, unsigned int*,
std::basic_string<char, std::char_traits<char>, std::allocator<char>
>*):../../ui/base/x/x11_util.cc:1068: error: undefined reference to
'XRRGetOutputProperty'
collect2: ld returned 1 exit status
ninja: build stopped: subcommand failed.


Any ideas how to fix this?

yurys@yurys-lin:/sources/chrome-git-new/src (master)$ apt-cache policy
libxrandr-dev 
libxrandr-dev:
  Installed: 2:1.3.0-3
  Candidate: 2:1.3.0-3

[mukai1, 2012-08-21 12:00:15]

sorry for that.  could you patch the change of
http://codereview.chromium.org/10836356/ ?
On Aug 21, 2012 8:57 PM, <yurys@chromium.org> wrote:

> After this change I'm getting the following linker errror:
>
> ../../third_party/gold/gold64: obj/ui/libui.a(obj/ui/base/x/**ui.x11_util.o):
> in
> function ui::(anonymous
> namespace)::IsRandRAvailable()**:../../ui/base/x/x11_util.cc:**319:
> error: undefined
> reference to 'XRRQueryVersion'
> ../../third_party/gold/gold64: obj/ui/libui.a(obj/ui/base/x/**ui.x11_util.o):
> in
> function ui::GetOutputDeviceHandles(**std::vector<unsigned long,
> std::allocator<unsigned long> >*):../../ui/base/x/x11_util.**cc:1020:
> error:
> undefined reference to 'XRRGetScreenResources'
> ../../third_party/gold/gold64: obj/ui/libui.a(obj/ui/base/x/**ui.x11_util.o):
> in
> function ui::GetOutputDeviceHandles(**std::vector<unsigned long,
> std::allocator<unsigned long> >*):../../ui/base/x/x11_util.**cc:1023:
> error:
> undefined reference to 'XRRFreeScreenResources'
> ../../third_party/gold/gold64: obj/ui/libui.a(obj/ui/base/x/**ui.x11_util.o):
> in
> function ui::GetOutputDeviceData(**unsigned long, unsigned short*,
> unsigned int*,
> std::basic_string<char, std::char_traits<char>, std::allocator<char>
>
>> *):../../ui/base/x/x11_util.**cc:1040: error: undefined reference to
>>
> 'XRRListOutputProperties'
> ../../third_party/gold/gold64: obj/ui/libui.a(obj/ui/base/x/**ui.x11_util.o):
> in
> function ui::GetOutputDeviceData(**unsigned long, unsigned short*,
> unsigned int*,
> std::basic_string<char, std::char_traits<char>, std::allocator<char>
>
>> *):../../ui/base/x/x11_util.**cc:1068: error: undefined reference to
>>
> 'XRRGetOutputProperty'
> collect2: ld returned 1 exit status
> ninja: build stopped: subcommand failed.
>
>
> Any ideas how to fix this?
>
> yurys@yurys-lin:/sources/**chrome-git-new/src (master)$ apt-cache policy
> libxrandr-dev
> libxrandr-dev:
>   Installed: 2:1.3.0-3
>   Candidate: 2:1.3.0-3
>
>
>
https://chromiumcodereview.**appspot.com/10826198/<https://chromiumcodereview...
>

[yurys, 2012-08-21 12:07:36]

On 2012/08/21 12:00:15, mukai1 wrote:
> sorry for that.  could you patch the change of
> http://codereview.chromium.org/10836356/ ?

That helps, thanks for quick reply! Should we land that change?

[mukai1, 2012-08-21 12:58:17]

Oops, I do not yet get the LGTM of the owner of UI -- sky will respond in
MTV time.
How urgent is it?  I would push it as TBR if it breaks some builders or
something.

-- Jun Mukai


2012/8/21 <yurys@chromium.org>

>
> On 2012/08/21 12:00:15, mukai1 wrote:
>
>> sorry for that.  could you patch the change of
>>
http://codereview.chromium.**org/10836356/<http://codereview.chromium.org/108...
>>
>
> That helps, thanks for quick reply! Should we land that change?
>
>
https://chromiumcodereview.**appspot.com/10826198/<https://chromiumcodereview...
>

[yurys, 2012-08-21 13:06:42]

On 2012/08/21 12:58:17, mukai1 wrote:
> Oops, I do not yet get the LGTM of the owner of UI -- sky will respond in
> MTV time.
> How urgent is it?  I would push it as TBR if it breaks some builders or
> something.
> 
Although the linker fails on my local machine I don't see any failing bots:( I
can live with the fix manually applied locally. Interesting thing is that my
colleague on a similar setup couldn't reproduce the problem.


> -- Jun Mukai
> 
> 
> 2012/8/21 <mailto:yurys@chromium.org>
> 
> >
> > On 2012/08/21 12:00:15, mukai1 wrote:
> >
> >> sorry for that.  could you patch the change of
> >>
>
http://codereview.chromium.**org/10836356/%3Chttp://codereview.chromium.org/1...
> >>
> >
> > That helps, thanks for quick reply! Should we land that change?
> >
> >
>
https://chromiumcodereview.**appspot.com/10826198/%3Chttps://chromiumcoderevi...>
> >

[mukai1, 2012-08-21 13:17:00]

I guess that's why your colleagues synced the tree slightly earlier than my
CL?  It hits on tasak@ (an webkit guy in Tokyo office) too.  I do not know
why builders don't report failures, but IIRC DumpRenderTree won't be built
in chromium builders.  I don't know webkit-side.

Sorry, I couldn't evaluate the severity of my failure.  If you feel the
patch should be landed ASAP, I would go with TBR.  The change is quite
small so it would be fine.  Please tell me your idea.

-- Jun Mukai


2012/8/21 <yurys@chromium.org>

> On 2012/08/21 12:58:17, mukai1 wrote:
>
>> Oops, I do not yet get the LGTM of the owner of UI -- sky will respond in
>> MTV time.
>> How urgent is it?  I would push it as TBR if it breaks some builders or
>> something.
>>
>
> Although the linker fails on my local machine I don't see any failing
> bots:( I
> can live with the fix manually applied locally. Interesting thing is that
> my
> colleague on a similar setup couldn't reproduce the problem.
>
>
>  -- Jun Mukai
>>
>
>
>  2012/8/21 <mailto:yurys@chromium.org>
>>
>
>  >
>> > On 2012/08/21 12:00:15, mukai1 wrote:
>> >
>> >> sorry for that.  could you patch the change of
>> >>
>>
>
> http://codereview.chromium.****org/10836356/%3Chttp://coderev**
> iew.chromium.org/10836356/%3E<http://codereview.chromium.org/10836356/%3E>
> ?
>
>  >>
>> >
>> > That helps, thanks for quick reply! Should we land that change?
>> >
>> >
>>
>
> https://chromiumcodereview.**a**ppspot.com/10826198/%3Chttps:/**
>
/chromiumcodereview.appspot.**com/10826198/<http://appspot.com/10826198/%3Chttps://chromiumcodereview.appspot.com/10826198/>
> >
>
>> >
>>
>
>
>
>
https://chromiumcodereview.**appspot.com/10826198/<https://chromiumcodereview...
>

[yurys, 2012-08-21 14:06:14]

On 2012/08/21 13:17:00, mukai1 wrote:
> I guess that's why your colleagues synced the tree slightly earlier than my
> CL?
He (loislo@) managed to build it from r152551 so your change is there.

>  It hits on tasak@ (an webkit guy in Tokyo office) too.  I do not know
> why builders don't report failures, but IIRC DumpRenderTree won't be built
> in chromium builders.  I don't know webkit-side.
> 
> Sorry, I couldn't evaluate the severity of my failure.  If you feel the
> patch should be landed ASAP, I would go with TBR.  The change is quite
> small so it would be fine.  Please tell me your idea.
> 
Since it breaks compilation and the fix is pretty straightforward I'd commit it
with tbr now and resolve possible issues later.



> -- Jun Mukai
> 
> 
> 2012/8/21 <mailto:yurys@chromium.org>
> 
> > On 2012/08/21 12:58:17, mukai1 wrote:
> >
> >> Oops, I do not yet get the LGTM of the owner of UI -- sky will respond in
> >> MTV time.
> >> How urgent is it?  I would push it as TBR if it breaks some builders or
> >> something.
> >>
> >
> > Although the linker fails on my local machine I don't see any failing
> > bots:( I
> > can live with the fix manually applied locally. Interesting thing is that
> > my
> > colleague on a similar setup couldn't reproduce the problem.
> >
> >
> >  -- Jun Mukai
> >>
> >
> >
> >  2012/8/21 <mailto:yurys@chromium.org>
> >>
> >
> >  >
> >> > On 2012/08/21 12:00:15, mukai1 wrote:
> >> >
> >> >> sorry for that.  could you patch the change of
> >> >>
> >>
> >
> > http://codereview.chromium.****org/10836356/%253Chttp://coderev**
> > iew.chromium.org/10836356/%3E<http://codereview.chromium.org/10836356/%3E>
> > ?
> >
> >  >>
> >> >
> >> > That helps, thanks for quick reply! Should we land that change?
> >> >
> >> >
> >>
> >
> > https://chromiumcodereview.**a**ppspot.com/10826198/%253Chttps:/**
> >
>
/chromiumcodereview.appspot.**com/10826198/<http://appspot.com/10826198/%3Chttps://chromiumcodereview.appspot.com/10826198/>
> > >
> >
> >> >
> >>
> >
> >
> >
> >
>
https://chromiumcodereview.**appspot.com/10826198/%3Chttps://chromiumcoderevi...>
> >

[mukai1, 2012-08-21 14:15:01]

Okay, then I will do the dcommit with tbr.  Please wait.


2012/8/21 <yurys@chromium.org>

> On 2012/08/21 13:17:00, mukai1 wrote:
>
>> I guess that's why your colleagues synced the tree slightly earlier than
>> my
>> CL?
>>
> He (loislo@) managed to build it from r152551 so your change is there.
>
>
>    It hits on tasak@ (an webkit guy in Tokyo office) too.  I do not know
>> why builders don't report failures, but IIRC DumpRenderTree won't be built
>> in chromium builders.  I don't know webkit-side.
>>
>
>  Sorry, I couldn't evaluate the severity of my failure.  If you feel the
>> patch should be landed ASAP, I would go with TBR.  The change is quite
>> small so it would be fine.  Please tell me your idea.
>>
>
> Since it breaks compilation and the fix is pretty straightforward I'd
> commit it
> with tbr now and resolve possible issues later.
>
>
>
>
>  -- Jun Mukai
>>
>
>
>  2012/8/21 <mailto:yurys@chromium.org>
>>
>
>  > On 2012/08/21 12:58:17, mukai1 wrote:
>> >
>> >> Oops, I do not yet get the LGTM of the owner of UI -- sky will respond
>> in
>> >> MTV time.
>> >> How urgent is it?  I would push it as TBR if it breaks some builders or
>> >> something.
>> >>
>> >
>> > Although the linker fails on my local machine I don't see any failing
>> > bots:( I
>> > can live with the fix manually applied locally. Interesting thing is
>> that
>> > my
>> > colleague on a similar setup couldn't reproduce the problem.
>> >
>> >
>> >  -- Jun Mukai
>> >>
>> >
>> >
>> >  2012/8/21 <mailto:yurys@chromium.org>
>> >>
>> >
>> >  >
>> >> > On 2012/08/21 12:00:15, mukai1 wrote:
>> >> >
>> >> >> sorry for that.  could you patch the change of
>> >> >>
>> >>
>> >
>> > http://codereview.chromium.******org/10836356/%253Chttp://**coderev**
>> > iew.chromium.org/10836356/%3E<**http://codereview.chromium.**
>> org/10836356/%3E <http://codereview.chromium.org/10836356/%3E>>
>>
>> > ?
>> >
>> >  >>
>> >> >
>> >> > That helps, thanks for quick reply! Should we land that change?
>> >> >
>> >> >
>> >>
>> >
>> >
https://chromiumcodereview.****a**ppspot.com/10826198/%**253Chttps:/**<http:/...
>> >
>>
>
> /chromiumcodereview.appspot.****com/10826198/<http://appspot.**
>
com/10826198/%3Chttps://**chromiumcodereview.appspot.**com/10826198/<http://appspot.com/10826198/%3Chttps://chromiumcodereview.appspot.com/10826198/>
> >
>
>  > >
>> >
>> >> >
>> >>
>> >
>> >
>> >
>> >
>>
>
> https://chromiumcodereview.**a**ppspot.com/10826198/%3Chttps:/**
>
/chromiumcodereview.appspot.**com/10826198/<http://appspot.com/10826198/%3Chttps://chromiumcodereview.appspot.com/10826198/>
> >
>
>> >
>>
>
>
>
>
https://chromiumcodereview.**appspot.com/10826198/<https://chromiumcodereview...
>

[mukai1, 2012-08-21 14:23:35]

Landed as crrev.com/152554



2012/8/21 Jun Mukai <mukai@google.com>

> Okay, then I will do the dcommit with tbr.  Please wait.
>
>
> 2012/8/21 <yurys@chromium.org>
>
> On 2012/08/21 13:17:00, mukai1 wrote:
>>
>>> I guess that's why your colleagues synced the tree slightly earlier than
>>> my
>>> CL?
>>>
>> He (loislo@) managed to build it from r152551 so your change is there.
>>
>>
>>    It hits on tasak@ (an webkit guy in Tokyo office) too.  I do not know
>>> why builders don't report failures, but IIRC DumpRenderTree won't be
>>> built
>>> in chromium builders.  I don't know webkit-side.
>>>
>>
>>  Sorry, I couldn't evaluate the severity of my failure.  If you feel the
>>> patch should be landed ASAP, I would go with TBR.  The change is quite
>>> small so it would be fine.  Please tell me your idea.
>>>
>>
>> Since it breaks compilation and the fix is pretty straightforward I'd
>> commit it
>> with tbr now and resolve possible issues later.
>>
>>
>>
>>
>>  -- Jun Mukai
>>>
>>
>>
>>  2012/8/21 <mailto:yurys@chromium.org>
>>>
>>
>>  > On 2012/08/21 12:58:17, mukai1 wrote:
>>> >
>>> >> Oops, I do not yet get the LGTM of the owner of UI -- sky will
>>> respond in
>>> >> MTV time.
>>> >> How urgent is it?  I would push it as TBR if it breaks some builders
>>> or
>>> >> something.
>>> >>
>>> >
>>> > Although the linker fails on my local machine I don't see any failing
>>> > bots:( I
>>> > can live with the fix manually applied locally. Interesting thing is
>>> that
>>> > my
>>> > colleague on a similar setup couldn't reproduce the problem.
>>> >
>>> >
>>> >  -- Jun Mukai
>>> >>
>>> >
>>> >
>>> >  2012/8/21 <mailto:yurys@chromium.org>
>>> >>
>>> >
>>> >  >
>>> >> > On 2012/08/21 12:00:15, mukai1 wrote:
>>> >> >
>>> >> >> sorry for that.  could you patch the change of
>>> >> >>
>>> >>
>>> >
>>> > http://codereview.chromium.******org/10836356/%253Chttp://**coderev**
>>> > iew.chromium.org/10836356/%3E<**http://codereview.chromium.**
>>> org/10836356/%3E <http://codereview.chromium.org/10836356/%3E>>
>>>
>>> > ?
>>> >
>>> >  >>
>>> >> >
>>> >> > That helps, thanks for quick reply! Should we land that change?
>>> >> >
>>> >> >
>>> >>
>>> >
>>> >
https://chromiumcodereview.****a**ppspot.com/10826198/%**253Chttps:/**<http:/...
>>> >
>>>
>>
>> /chromiumcodereview.appspot.****com/10826198/<http://appspot.**
>>
com/10826198/%3Chttps://**chromiumcodereview.appspot.**com/10826198/<http://appspot.com/10826198/%3Chttps://chromiumcodereview.appspot.com/10826198/>
>> >
>>
>>  > >
>>> >
>>> >> >
>>> >>
>>> >
>>> >
>>> >
>>> >
>>>
>>
>> https://chromiumcodereview.**a**ppspot.com/10826198/%3Chttps:/**
>>
/chromiumcodereview.appspot.**com/10826198/<http://appspot.com/10826198/%3Chttps://chromiumcodereview.appspot.com/10826198/>
>> >
>>
>>> >
>>>
>>
>>
>>
>>
https://chromiumcodereview.**appspot.com/10826198/<https://chromiumcodereview...
>>
>
>