touch: Allow grabbing/ungrabbing touch devices for XInput2.

touch: Allow grabbing/ungrabbing touch devices for XInput2.

This allows touch devices to be grabbed when events from the mouse/keyboard are
grabbed. This also exposes TouchFactory, which will eventually be used in more
places.

BUG=none
TEST=none

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

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

[sadrul, 2011-01-19 18:40:31]

Please add additional reviewers as you see fit.

[rjkroege, 2011-01-19 19:26:01]

With the break-out of the touch code, you could augment/add some unit tests?
(You knew I'd say that right?)

[sky, 2011-01-19 19:34:54]

http://codereview.chromium.org/6300007/diff/1/views/controls/menu/menu_host_g...
File views/controls/menu/menu_host_gtk.cc (right):

http://codereview.chromium.org/6300007/diff/1/views/controls/menu/menu_host_g...
views/controls/menu/menu_host_gtk.cc:8: #if defined(HAVE_XINPUT2)
newline between 7 and 8

http://codereview.chromium.org/6300007/diff/1/views/controls/menu/menu_host_g...
views/controls/menu/menu_host_gtk.cc:17: #if defined(HAVE_XINPUT2)
newline between 16 and 17.

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

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.cc#...
views/touchui/touch_factory.cc:19: TouchFactory::TouchFactory() :
touch_device_lookup_(),
put these on the next line and indented 4 spaces.

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.cc#...
views/touchui/touch_factory.cc:25: for (std::vector<unsigned
int>::const_iterator iter = devices.begin();
Do you need to clear out touch_device_lookup_ and touch_device_list_ before
entering the loop?

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.cc#...
views/touchui/touch_factory.cc:26: iter != devices.end(); ++iter) {
indent one more space

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.cc#...
views/touchui/touch_factory.cc:39: if (touch_device_list_.size() == 0)
empty

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

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.h#n...
views/touchui/touch_factory.h:11: #include <X11/Xlib.h>
Is it possible to forward declare the types you need so that folks don't end up
including X11? app/x11_util.h forward declares some X types.

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.h#n...
views/touchui/touch_factory.h:21: static TouchFactory* GetInstance();
you've got an extra space here.

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.h#n...
views/touchui/touch_factory.h:39: TouchFactory();
I think you forget an implementation of this.

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.h#n...
views/touchui/touch_factory.h:45: std::bitset<128> touch_device_lookup_;
Are you sure you want to use both a bitset and vector here? Why not just a map?
I can't imagine this is called in performance intensive places where it really
matters.

[sadrul, 2011-01-19 20:10:17]

http://codereview.chromium.org/6300007/diff/1/views/controls/menu/menu_host_g...
File views/controls/menu/menu_host_gtk.cc (right):

http://codereview.chromium.org/6300007/diff/1/views/controls/menu/menu_host_g...
views/controls/menu/menu_host_gtk.cc:8: #if defined(HAVE_XINPUT2)
On 2011/01/19 19:34:54, sky wrote:
> newline between 7 and 8

Done.

http://codereview.chromium.org/6300007/diff/1/views/controls/menu/menu_host_g...
views/controls/menu/menu_host_gtk.cc:17: #if defined(HAVE_XINPUT2)
On 2011/01/19 19:34:54, sky wrote:
> newline between 16 and 17.

Done.

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

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.cc#...
views/touchui/touch_factory.cc:19: TouchFactory::TouchFactory() :
touch_device_lookup_(),
On 2011/01/19 19:34:54, sky wrote:
> put these on the next line and indented 4 spaces.

Done.

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.cc#...
views/touchui/touch_factory.cc:25: for (std::vector<unsigned
int>::const_iterator iter = devices.begin();
On 2011/01/19 19:34:54, sky wrote:
> Do you need to clear out touch_device_lookup_ and touch_device_list_ before
> entering the loop?

At this moment, this is called exactly once. But resetting the list here sounds
like a good idea. Done.

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.cc#...
views/touchui/touch_factory.cc:26: iter != devices.end(); ++iter) {
On 2011/01/19 19:34:54, sky wrote:
> indent one more space

Done.

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.cc#...
views/touchui/touch_factory.cc:39: if (touch_device_list_.size() == 0)
On 2011/01/19 19:34:54, sky wrote:
> empty

Done.

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

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.h#n...
views/touchui/touch_factory.h:11: #include <X11/Xlib.h>
On 2011/01/19 19:34:54, sky wrote:
> Is it possible to forward declare the types you need so that folks don't end
up
> including X11? app/x11_util.h forward declares some X types.

Indeed! I wasn't sure if we wanted to typedef Window to unsigned long, since
there are multiple variations in X11/X.h, but looks like in app/x11_util.h, XID
is typedef'ed to unsigned long. Did the same for Window here. Thanks for the
pointer!

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.h#n...
views/touchui/touch_factory.h:21: static TouchFactory* GetInstance();
On 2011/01/19 19:34:54, sky wrote:
> you've got an extra space here.

Done.

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.h#n...
views/touchui/touch_factory.h:39: TouchFactory();
On 2011/01/19 19:34:54, sky wrote:
> I think you forget an implementation of this.

It is there in touch_factory.cc

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.h#n...
views/touchui/touch_factory.h:45: std::bitset<128> touch_device_lookup_;
On 2011/01/19 19:34:54, sky wrote:
> Are you sure you want to use both a bitset and vector here? Why not just a
map?
> I can't imagine this is called in performance intensive places where it really
> matters.

The lookup table is used for each pointer event (i.e. move/press/release events
from mouse, touch devices). For a simple gesture, there can be hundreds of such
events, and a lookup table would be useful to speed things up.

We could get rid of touch_device_list_, since it's currently only used for
grabbing/ungrabbing devices (which happens for popup windows, e.g. menus). But
without it, we would have to go over the lookup table to decide which devices to
grab on, and that seems a bit awkward.

Ultimately, it is possible that we can do away with both the list and the lookup
table, and have a single device-id for the touch device. But that is TBD until
more experiments with devices. I can leave a note here accordingly if that
sounds good to you?

[sky, 2011-01-19 23:15:30]

LGTM

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

http://codereview.chromium.org/6300007/diff/1/views/touchui/touch_factory.h#n...
views/touchui/touch_factory.h:45: std::bitset<128> touch_device_lookup_;
On 2011/01/19 20:10:17, sadrul wrote:
> On 2011/01/19 19:34:54, sky wrote:
> > Are you sure you want to use both a bitset and vector here? Why not just a
> map?
> > I can't imagine this is called in performance intensive places where it
really
> > matters.
> 
> The lookup table is used for each pointer event (i.e. move/press/release
events
> from mouse, touch devices). For a simple gesture, there can be hundreds of
such
> events, and a lookup table would be useful to speed things up.
> 
> We could get rid of touch_device_list_, since it's currently only used for
> grabbing/ungrabbing devices (which happens for popup windows, e.g. menus). But
> without it, we would have to go over the lookup table to decide which devices
to
> grab on, and that seems a bit awkward.
> 
> Ultimately, it is possible that we can do away with both the list and the
lookup
> table, and have a single device-id for the touch device. But that is TBD until
> more experiments with devices. I can leave a note here accordingly if that
> sounds good to you?

Sounds good.

[rjkroege, 2011-01-19 23:20:17]

LGTM (but like I said: TODO for the unit tests.)

[sadrul, 2011-01-19 23:31:12]

On 2011/01/19 23:20:17, rjkroege wrote:
> LGTM (but like I said: TODO for the unit tests.)

Thanks. I have started working on unit tests (it looks non-trivial). I should be
submitting a CL for review this week for it.

[sadrul, 2011-01-20 02:51:22]

Please take a look at the fix for the build failure.

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

http://codereview.chromium.org/6300007/diff/18001/views/touchui/touch_factory...
views/touchui/touch_factory.cc:82: #endif  // defined(HAVE_XINPUT2)
I needed this '#if defined' to fix build failures on systems that don't have
XInput2. I tried to simply exclude this file from compiling if XInput2 is not
available, by excluding it in views/views.gyp, but there doesn't look to be a
[tolerable] way of doing this (conditionally including/excluding the file based
on a gyp-variable in build/linux/system.gyp:x11, where HAVE_XINPUT2 is defined,
didn't work).

[rjkroege, 2011-01-20 16:36:30]

per discussion... please do this nicer

[sadrul, 2011-01-20 17:22:59]

On 2011/01/20 16:36:30, rjkroege wrote:
> per discussion... please do this nicer

Added the check in gyp for excluding the file when xinput2 is not available.

[rjkroege, 2011-01-20 17:52:41]

LGTM.

[sadrul, 2011-01-20 19:05:43]

On 2011/01/20 17:52:41, rjkroege wrote:
> LGTM.

The trybots are also happy! sky?

[sky, 2011-01-20 20:54:36]

LGTM