Tweak XInput2 event subscription

Tweak XInput2 event subscription

Combine MessagePumpGlibXs masters_ and floats_ fields into a single pointer_devices_ field.  This is to make it easier for us to select the pointer devices of interest in one place.

Also, don't attempt to call XFreeDeviceList if XListInputDevices returns null.  This should only happen when XInput2 isn't supported, which isn't a real touchui scenario, but is useful for development scenarios.

BUG=None
TEST=None


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

[Rick Byers, 2011-03-25 15:39:30]

Sadrul,
This is the tweak we talked about.  I've also included the fix for that crash
when XInput2 isn't available.

Thanks,
   Rick

[sadrul, 2011-03-25 15:49:40]

http://codereview.chromium.org/6736029/diff/1/base/message_pump_glib_x.cc
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/6736029/diff/1/base/message_pump_glib_x.cc#new...
base/message_pump_glib_x.cc:301: pointer_devices_.insert(devinfo->deviceid);
Braces aren't necessary here (there's nothing about this in the style guide, I
think, and there are examples of both in chromium code, but I have been
requested to not use braces in such cases a few times). Sorry I missed this in
the earlier review.

http://codereview.chromium.org/6736029/diff/1/base/message_pump_glib_x.cc#new...
base/message_pump_glib_x.cc:304: XIFreeDeviceInfo(devices);
Check for 'count' or 'devices' here too?

http://codereview.chromium.org/6736029/diff/1/views/touchui/touch_factory.cc
File views/touchui/touch_factory.cc (right):

http://codereview.chromium.org/6736029/diff/1/views/touchui/touch_factory.cc#...
views/touchui/touch_factory.cc:59: }
-braces

[Rick Byers, 2011-03-25 17:25:49]

Thanks for your feedback Sadrul!

http://codereview.chromium.org/6736029/diff/1/base/message_pump_glib_x.cc
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/6736029/diff/1/base/message_pump_glib_x.cc#new...
base/message_pump_glib_x.cc:301: pointer_devices_.insert(devinfo->deviceid);
On 2011/03/25 15:49:41, sadrul wrote:
> Braces aren't necessary here (there's nothing about this in the style guide, I
> think, and there are examples of both in chromium code, but I have been
> requested to not use braces in such cases a few times). Sorry I missed this in
> the earlier review.

Done.

http://codereview.chromium.org/6736029/diff/1/base/message_pump_glib_x.cc#new...
base/message_pump_glib_x.cc:304: XIFreeDeviceInfo(devices);
On 2011/03/25 15:49:41, sadrul wrote:
> Check for 'count' or 'devices' here too?

I don't think so - there should be no legitimate reason as far as I know for
XIQueryDevice to return null here.  We've already verified above that XInput2 is
supported.   

That said, if you think it's better to be defensive then I'd be ok with that
(though I generally feel it's better to let it crash when something you're not
prepared for occurs).

http://codereview.chromium.org/6736029/diff/1/views/touchui/touch_factory.cc
File views/touchui/touch_factory.cc (right):

http://codereview.chromium.org/6736029/diff/1/views/touchui/touch_factory.cc#...
views/touchui/touch_factory.cc:59: }
On 2011/03/25 15:49:41, sadrul wrote:
> -braces

Done.

[sadrul, 2011-03-25 18:04:56]

http://codereview.chromium.org/6736029/diff/1/base/message_pump_glib_x.cc
File base/message_pump_glib_x.cc (right):

http://codereview.chromium.org/6736029/diff/1/base/message_pump_glib_x.cc#new...
base/message_pump_glib_x.cc:304: XIFreeDeviceInfo(devices);
On 2011/03/25 17:25:49, rbyers wrote:
> On 2011/03/25 15:49:41, sadrul wrote:
> > Check for 'count' or 'devices' here too?
> 
> I don't think so - there should be no legitimate reason as far as I know for
> XIQueryDevice to return null here.  We've already verified above that XInput2
is
> supported.   
> 
> That said, if you think it's better to be defensive then I'd be ok with that
> (though I generally feel it's better to let it crash when something you're not
> prepared for occurs).

I thought this is where you were getting the crash with NX?

[Rick Byers, 2011-03-25 18:17:06]

On 2011/03/25 18:04:56, sadrul wrote:
> http://codereview.chromium.org/6736029/diff/1/base/message_pump_glib_x.cc
> File base/message_pump_glib_x.cc (right):
> 
>
http://codereview.chromium.org/6736029/diff/1/base/message_pump_glib_x.cc#new...
> base/message_pump_glib_x.cc:304: XIFreeDeviceInfo(devices);
> On 2011/03/25 17:25:49, rbyers wrote:
> > On 2011/03/25 15:49:41, sadrul wrote:
> > > Check for 'count' or 'devices' here too?
> > 
> > I don't think so - there should be no legitimate reason as far as I know for
> > XIQueryDevice to return null here.  We've already verified above that
XInput2
> is
> > supported.   
> > 
> > That said, if you think it's better to be defensive then I'd be ok with that
> > (though I generally feel it's better to let it crash when something you're
not
> > prepared for occurs).
> 
> I thought this is where you were getting the crash with NX?

No, it was only the code in touch_factory.cc (which you recently changed from
XIQueryDevice to XIListInputDevices)

[sadrul, 2011-03-25 18:27:45]

On 2011/03/25 18:17:06, rbyers wrote:
> On 2011/03/25 18:04:56, sadrul wrote:
> > http://codereview.chromium.org/6736029/diff/1/base/message_pump_glib_x.cc
> > File base/message_pump_glib_x.cc (right):
> > 
> >
>
http://codereview.chromium.org/6736029/diff/1/base/message_pump_glib_x.cc#new...
> > base/message_pump_glib_x.cc:304: XIFreeDeviceInfo(devices);
> > On 2011/03/25 17:25:49, rbyers wrote:
> > > On 2011/03/25 15:49:41, sadrul wrote:
> > > > Check for 'count' or 'devices' here too?
> > > 
> > > I don't think so - there should be no legitimate reason as far as I know
for
> > > XIQueryDevice to return null here.  We've already verified above that
> XInput2
> > is
> > > supported.   
> > > 
> > > That said, if you think it's better to be defensive then I'd be ok with
that
> > > (though I generally feel it's better to let it crash when something you're
> not
> > > prepared for occurs).
> > 
> > I thought this is where you were getting the crash with NX?
> 
> No, it was only the code in touch_factory.cc (which you recently changed from
> XIQueryDevice to XIListInputDevices)

Ah. I misunderstood. LGTM.

[commit-bot: I haz the power, 2011-03-25 18:31:56]

Running presubmit hooks...
** Presubmit ERRORS **
Run the command: svn pset svn:eol-style LF \
  base/message_pump_glib_x.cc \
  base/message_pump_glib_x.h

Presubmit checks took 1.4s to calculate.

Presubmit check for 6736029-5001 failed with code 1

[commit-bot: I haz the power, 2011-03-25 22:52:58]

Change committed as 79466