Initialize XInput2 on demand

Initialize XInput2 on demand - fixes ash_shell mouse events

MessagePumpX::HasXInput2 currently assumes a message pump has already been created and so XInput2 has been initialized.  This may not always be the case (eg. when deciding early at startup whether we should load touch-specific resources).

Instead we now initialize XInput2 on demand - either when the message pump is created, or when HasInput2 is called earlier.

BUG=128367
TEST=

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

[Rick Byers, 2012-05-16 18:27:41]

Hey Sadrul,
How does this look for a simple fix for the ash_shell mouse events issue? 
There's a deeper underlying issue around initialization ordering I'm trying to
avoid here.  I don't see any downside to just initializing XInput2 as early as
necessary, do you?

[sadrul, 2012-05-16 21:47:12]

LGTM (you will need OWNERS)

http://codereview.chromium.org/10386175/diff/1/base/message_pump_x.cc
File base/message_pump_x.cc (right):

http://codereview.chromium.org/10386175/diff/1/base/message_pump_x.cc#newcode41
base/message_pump_x.cc:41: bool xInput2Initialized = false;
xinput2_initialized
xinput2_supported

camelCase is for webKit!

[sadrul, 2012-05-16 21:50:45]

On 2012/05/16 18:27:41, Rick Byers wrote:
> Hey Sadrul,
> How does this look for a simple fix for the ash_shell mouse events issue? 
> There's a deeper underlying issue around initialization ordering I'm trying to
> avoid here.  I don't see any downside to just initializing XInput2 as early as
> necessary, do you?

As long as we are able to open the X display that early, initializing XI2 should
not be an issue.

[Rick Byers, 2012-05-17 14:06:56]

+mark for OWNERS

http://codereview.chromium.org/10386175/diff/1/base/message_pump_x.cc
File base/message_pump_x.cc (right):

http://codereview.chromium.org/10386175/diff/1/base/message_pump_x.cc#newcode41
base/message_pump_x.cc:41: bool xInput2Initialized = false;
On 2012/05/16 21:47:12, sadrul wrote:
> xinput2_initialized
> xinput2_supported
> 
> camelCase is for webKit!

Done.

[Mark Mentovai, 2012-05-17 14:17:34]

The restructuring is optional and you don’t have to do it if you hate it. If you
don’t do it, you should still move as many statics as you can into functions.

http://codereview.chromium.org/10386175/diff/5001/base/message_pump_x.cc
File base/message_pump_x.cc (right):

http://codereview.chromium.org/10386175/diff/5001/base/message_pump_x.cc#newc...
base/message_pump_x.cc:41: bool xinput2_initialized = false;
This can just be a static bool inside InitializeXInput2IfNeeded…

http://codereview.chromium.org/10386175/diff/5001/base/message_pump_x.cc#newc...
base/message_pump_x.cc:52: if (xinput2_initialized)
…but this pattern is a little bit of a pet peeve. You’re effectively using one
static (xinput2_initialized) as a guard variable around another static
(xinput2_supported), but statics can already have their own guard variables that
the compiler spits out automatically, so why not just leverage that?

Here’s how I would usually write this pattern:

bool InitializeXInput2Internal() {
  Display* display = base::MessagePumpX::GetDefaultXDisplay();
  // blah blah blah, returning true if Xinput2 is supported, and false if not.
}

bool InitializeXInput2() {
  static bool xinput2_supported = InitializeXInput2Internal();
  return xinput2_supported;
}

Now you only have one static, it tracks whether Xinput2 is supported, it lives
right in InitializeXInput2, and the compiler gives you the guard variable for it
so that its initializer (InitializeXInput2Internal) is only called once.

[Rick Byers, 2012-05-17 15:13:15]

http://codereview.chromium.org/10386175/diff/5001/base/message_pump_x.cc
File base/message_pump_x.cc (right):

http://codereview.chromium.org/10386175/diff/5001/base/message_pump_x.cc#newc...
base/message_pump_x.cc:52: if (xinput2_initialized)
On 2012/05/17 14:17:34, Mark Mentovai wrote:
> …but this pattern is a little bit of a pet peeve. You’re effectively using one
> static (xinput2_initialized) as a guard variable around another static
> (xinput2_supported), but statics can already have their own guard variables
that
> the compiler spits out automatically, so why not just leverage that?
> 
> Here’s how I would usually write this pattern:
> 
> bool InitializeXInput2Internal() {
>   Display* display = base::MessagePumpX::GetDefaultXDisplay();
>   // blah blah blah, returning true if Xinput2 is supported, and false if not.
> }
> 
> bool InitializeXInput2() {
>   static bool xinput2_supported = InitializeXInput2Internal();
>   return xinput2_supported;
> }
> 
> Now you only have one static, it tracks whether Xinput2 is supported, it lives
> right in InitializeXInput2, and the compiler gives you the guard variable for
it
> so that its initializer (InitializeXInput2Internal) is only called once.

I agree this is better, thanks!  Done.

[Mark Mentovai, 2012-05-17 15:15:49]

LGTM

for both normal review purposes and base OWNERS review.

[commit-bot: I haz the power, 2012-05-17 16:06:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/10386175/3003

[commit-bot: I haz the power, 2012-05-17 16:34:03]

Try job failure for 10386175-3003 on win for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-17 17:03:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rbyers@chromium.org/10386175/6004

[commit-bot: I haz the power, 2012-05-17 18:31:05]

Change committed as 137694