This patch implements Chromium's Aura gesture recognizer in terms of utouch-grail and utouch-frame …

This patch implements Chromium's Aura gesture recognizer in terms of utouch-grail and utouch-frame (see [1] and [2]), both part of Canonical's touch/gesture stack. Moreover, the patch extends Chromium's touch-handling capabilities to non-direct touch devices, e.g., Apple's Magic Trackpad. 

The following gestures are recognized:

 (1.) Two-finger drag
 (2.) Two-finger pinch
 (3.) Tap
 (4.) Double Tap

Gestures (1.), (3.) and (4.) are already wired up to actions on the UI (part of the chromium code base). Gesture (2.) is recognized by utouch-grail and all its properties including the corresponding affine 3x3 transformation matrix are reported. However, code paths for pixel-perfect and continuous zooming are currently missing and our demonstration is limited in this respect. However, once the necessary plumbing becomes available in chromium, pinch-to-zoom works out-of-the-box.

Finally, we added an abstraction of touch devices and their respective capabilities to provide a proof-of-concept for isolating away from platform-specific touch-stacks and thus, for the portability of utouch-frame and utouch-grail.

Please note that this patch requires an up-to-date xserver that supports XI 2.2.

Gyp flags:

 * build/gyp_chromium -Duse_xi2_mt=2 -Duse_aura=1

We developed and tested the patch under Ubuntu 12.04 (Precise Pangolin) Beta 1.
Please see https://wiki.ubuntu.com/Multitouch/CurrentWork/ChromiumTouch for a more detailed description of the patch.

[thomas.voss, 2012-03-28 15:57:51]

Please find a more detailed description in:
https://wiki.ubuntu.com/Multitouch/CurrentWork/ChromiumTouch

[tfarina, 2012-03-28 16:16:04]

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/gfx/size.cc
File ui/gfx/size.cc (right):

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/gfx/size.cc#newcode55
ui/gfx/size.cc:55: // NOTREACHED() << "negative width:" << width;
please, revert this change.

[tfarina, 2012-03-28 16:19:47]

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
File ui/base/touch/multi_touch_device.h (right):

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
ui/base/touch/multi_touch_device.h:4: #ifndef
UI_BASE_TOUCH_MULTI_TOUCH_DEVICE_H_
add a blank line above.

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
ui/base/touch/multi_touch_device.h:6: 
add #pragma once

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
ui/base/touch/multi_touch_device.h:11: #include <vector>
remove this unused include

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
ui/base/touch/multi_touch_device.h:28: : id_(-1),
initialize these members in source file instead.

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
ui/base/touch/multi_touch_device.h:68: // TODO(tvoss): Rework handling of
devices to rely on shared ptr's.
What is this TODO for?

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
ui/base/touch/multi_touch_device.h:69: //
DISALLOW_COPY_AND_ASSIGN(MultiTouchDevice);
add basictypes.h include for this. Also why is it commented?

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
ui/base/touch/multi_touch_device.h:71: }
add a blank line above.

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
ui/base/touch/multi_touch_device.h:71: }
}  // namespace ui

[Elliot Glaysher, 2012-03-28 17:48:59]

+jam (content), +sky (general aura master)

http://chromiumcodereview.appspot.com/9773024/diff/1/base/message_pump_x.cc
File base/message_pump_x.cc (right):

http://chromiumcodereview.appspot.com/9773024/diff/1/base/message_pump_x.cc#n...
base/message_pump_x.cc:12: #include <iostream>
This will add a static initializer to this file. Assuming this was intentional,
you probably want <ostream> instead.

http://chromiumcodereview.appspot.com/9773024/diff/1/content/plugin/plugin_th...
File content/plugin/plugin_thread.cc (right):

http://chromiumcodereview.appspot.com/9773024/diff/1/content/plugin/plugin_th...
content/plugin/plugin_thread.cc:85: //g_thread_init(NULL);
This is a longer term thing to fix. As is, please remove this because we do have
to support gtk 2.18 for a little longer.

http://chromiumcodereview.appspot.com/9773024/diff/1/content/utility/utility_...
File content/utility/utility_thread_impl.cc (right):

http://chromiumcodereview.appspot.com/9773024/diff/1/content/utility/utility_...
content/utility/utility_thread_impl.cc:142: g_thread_init(NULL);
Same thing here.

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/aura/gestures/gesture...
File ui/aura/gestures/gesture_recognizer_grail.cc (right):

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/aura/gestures/gesture...
ui/aura/gestures/gesture_recognizer_grail.cc:80: status =
grail_subscription_new(&subscription);
I know nothing about grail. I assume this all runs on the UI thread. Does
anything here block?

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/aura/gestures/gesture...
File ui/aura/gestures/gesture_recognizer_grail.h (right):

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/aura/gestures/gesture...
ui/aura/gestures/gesture_recognizer_grail.h:9: 
Add "base/compiler_specific.h" for the OVERRIDE macro

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/aura/gestures/gesture...
ui/aura/gestures/gesture_recognizer_grail.h:37: ui::TouchStatus status);
OVERRIDE on all virtual methods that you're overriding.

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/aura/gestures/gesture...
ui/aura/gestures/gesture_recognizer_grail.h:48: virtual Gestures*
AdvanceTouchQueue(Window* window,
virtual methods with nonempty bodies go in impl. we have a bot that checks for
this. Also OVERRIDE on the signature.

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
File ui/base/touch/multi_touch_device.h (right):

http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
ui/base/touch/multi_touch_device.h:35: 
Please define a dtor and put the implementation in the cc file. This class will
trip the clang bot otherwise.

[jam, 2012-03-28 19:42:55]

i defer my content lgtm to erg

[sky, 2012-03-28 20:30:36]

I only briefly looked at this. Sadrul and Robert know more about the
touch/gesture events than I.

http://codereview.chromium.org/9773024/diff/1/ui/aura/gestures/gesture_recogn...
File ui/aura/gestures/gesture_recognizer_aura.h (right):

http://codereview.chromium.org/9773024/diff/1/ui/aura/gestures/gesture_recogn...
ui/aura/gestures/gesture_recognizer_aura.h:9: #include
"ui/aura/gestures/gesture_recognizer.h"
Old include order is correct.

http://codereview.chromium.org/9773024/diff/1/ui/aura/gestures/gesture_recogn...
File ui/aura/gestures/gesture_recognizer_grail.h (right):

http://codereview.chromium.org/9773024/diff/1/ui/aura/gestures/gesture_recogn...
ui/aura/gestures/gesture_recognizer_grail.h:15: #include <map>
Include order is wrong here. For a header you want system includes, then
non-system includes.

http://codereview.chromium.org/9773024/diff/1/ui/aura/gestures/gesture_recogn...
ui/aura/gestures/gesture_recognizer_grail.h:26: class AURA_EXPORT
GestureRecognizerGrail : public GestureRecognizer {
Why do we need both a GestureRecognizer and GestureRecognizerGrail?

[thomas.voss, 2012-03-29 10:15:52]

On 2012/03/28 20:30:36, sky wrote:
> I only briefly looked at this. Sadrul and Robert know more about the
> touch/gesture events than I.
> 
>
http://codereview.chromium.org/9773024/diff/1/ui/aura/gestures/gesture_recogn...
> File ui/aura/gestures/gesture_recognizer_aura.h (right):
> 
>
http://codereview.chromium.org/9773024/diff/1/ui/aura/gestures/gesture_recogn...
> ui/aura/gestures/gesture_recognizer_aura.h:9: #include
> "ui/aura/gestures/gesture_recognizer.h"
> Old include order is correct.
> 
> 

Fixed.

http://codereview.chromium.org/9773024/diff/1/ui/aura/gestures/gesture_recogn...
> File ui/aura/gestures/gesture_recognizer_grail.h (right):
> 
>
http://codereview.chromium.org/9773024/diff/1/ui/aura/gestures/gesture_recogn...
> ui/aura/gestures/gesture_recognizer_grail.h:15: #include <map>
> Include order is wrong here. For a header you want system includes, then
> non-system includes.
>

Fixed.
 
>
http://codereview.chromium.org/9773024/diff/1/ui/aura/gestures/gesture_recogn...
> ui/aura/gestures/gesture_recognizer_grail.h:26: class AURA_EXPORT
> GestureRecognizerGrail : public GestureRecognizer {
> Why do we need both a GestureRecognizer and GestureRecognizerGrail?

If I understand the architecture correctly, the interface could be implemented
in a platform-specific manner or a default & generic implementation, e.g., in
terms of grail and frame.

[thomas.voss, 2012-03-29 10:20:49]

On 2012/03/28 17:48:59, Elliot Glaysher wrote:
> +jam (content), +sky (general aura master)
> 
> http://chromiumcodereview.appspot.com/9773024/diff/1/base/message_pump_x.cc
> File base/message_pump_x.cc (right):
> 
>
http://chromiumcodereview.appspot.com/9773024/diff/1/base/message_pump_x.cc#n...
> base/message_pump_x.cc:12: #include <iostream>
> This will add a static initializer to this file. Assuming this was
intentional,
> you probably want <ostream> instead.
> 

Done, the include was a leftover.

>
http://chromiumcodereview.appspot.com/9773024/diff/1/content/plugin/plugin_th...
> File content/plugin/plugin_thread.cc (right):
> 
>
http://chromiumcodereview.appspot.com/9773024/diff/1/content/plugin/plugin_th...
> content/plugin/plugin_thread.cc:85: //g_thread_init(NULL);
> This is a longer term thing to fix. As is, please remove this because we do
have
> to support gtk 2.18 for a little longer.
> 

Wrapped the respective calls in a macro that checks for GTK major & minor
revision. Otherwise, the compiler bails out complaining about calls to
deprecated functions. Makes sense?

>
http://chromiumcodereview.appspot.com/9773024/diff/1/content/utility/utility_...
> File content/utility/utility_thread_impl.cc (right):
> 
>
http://chromiumcodereview.appspot.com/9773024/diff/1/content/utility/utility_...
> content/utility/utility_thread_impl.cc:142: g_thread_init(NULL);
> Same thing here.
> 
>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/aura/gestures/gesture...
> File ui/aura/gestures/gesture_recognizer_grail.cc (right):
> 
>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/aura/gestures/gesture...
> ui/aura/gestures/gesture_recognizer_grail.cc:80: status =
> grail_subscription_new(&subscription);
> I know nothing about grail. I assume this all runs on the UI thread. Does
> anything here block?
> 

No, the subscription is (b)lock free. Added a comment in the source code stating
that clearly.

>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/aura/gestures/gesture...
> File ui/aura/gestures/gesture_recognizer_grail.h (right):
> 
>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/aura/gestures/gesture...
> ui/aura/gestures/gesture_recognizer_grail.h:9: 
> Add "base/compiler_specific.h" for the OVERRIDE macro
> 

Done.

>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/aura/gestures/gesture...
> ui/aura/gestures/gesture_recognizer_grail.h:37: ui::TouchStatus status);
> OVERRIDE on all virtual methods that you're overriding.
> 

Done. 

>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/aura/gestures/gesture...
> ui/aura/gestures/gesture_recognizer_grail.h:48: virtual Gestures*
> AdvanceTouchQueue(Window* window,
> virtual methods with nonempty bodies go in impl. we have a bot that checks for
> this. Also OVERRIDE on the signature.
>

Removed virtual from the signature, it was a leftover as well. Is this
sufficient or should I still move the function body to the implementation file?
 
>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
> File ui/base/touch/multi_touch_device.h (right):
> 
>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
> ui/base/touch/multi_touch_device.h:35: 
> Please define a dtor and put the implementation in the cc file. This class
will
> trip the clang bot otherwise.

Done.

[thomas.voss, 2012-03-29 10:23:43]

On 2012/03/28 16:19:47, tfarina wrote:
>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
> File ui/base/touch/multi_touch_device.h (right):
> 
>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
> ui/base/touch/multi_touch_device.h:4: #ifndef
> UI_BASE_TOUCH_MULTI_TOUCH_DEVICE_H_
> add a blank line above.
> 

Done.

>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
> ui/base/touch/multi_touch_device.h:6: 
> add #pragma once
>

Done. 
 
>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
> ui/base/touch/multi_touch_device.h:11: #include <vector>
> remove this unused include
>

Done. 
 
>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
> ui/base/touch/multi_touch_device.h:28: : id_(-1),
> initialize these members in source file instead.
>

Done.
 
>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
> ui/base/touch/multi_touch_device.h:68: // TODO(tvoss): Rework handling of
> devices to rely on shared ptr's.
> What is this TODO for?
>

It's a remark regarding device handling. While it is fine to copy the device
information at the present moment, future extensions might require unique
instances of a device. I changed TODO to REMARK and added a comment in the code.
Hope that makes sense.
 
>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
> ui/base/touch/multi_touch_device.h:69: //
> DISALLOW_COPY_AND_ASSIGN(MultiTouchDevice);
> add basictypes.h include for this. Also why is it commented?
> 

See before.

>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
> ui/base/touch/multi_touch_device.h:71: }
> add a blank line above.
> 

Done. 

>
http://chromiumcodereview.appspot.com/9773024/diff/1/ui/base/touch/multi_touc...
> ui/base/touch/multi_touch_device.h:71: }
> }  // namespace ui

Done.

[thomas.voss, 2012-03-29 10:39:56]

Updated the patch set according to comments.

[tdresser, 2012-03-29 17:44:38]

Unit tests?

http://codereview.chromium.org/9773024/diff/15008/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/9773024/diff/15008/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:4329: '../build/linux/system.gyp:frame',
Alphabetize.

http://codereview.chromium.org/9773024/diff/15008/ui/aura/aura.gyp
File ui/aura/aura.gyp (left):

http://codereview.chromium.org/9773024/diff/15008/ui/aura/aura.gyp#oldcode66
ui/aura/aura.gyp:66: 'gestures/gesture_recognizer_aura.cc',
Why are these being removed?

http://codereview.chromium.org/9773024/diff/15008/ui/aura/aura.gyp
File ui/aura/aura.gyp (right):

http://codereview.chromium.org/9773024/diff/15008/ui/aura/aura.gyp#newcode18
ui/aura/aura.gyp:18: '../../build/linux/system.gyp:frame',
Alphabetize

http://codereview.chromium.org/9773024/diff/15008/ui/aura/event.h
File ui/aura/event.h (right):

http://codereview.chromium.org/9773024/diff/15008/ui/aura/event.h#newcode311
ui/aura/event.h:311: struct Properties {
If this is being refactored, we should update the GestureRecognizerAura at the
same time.

Currently, GestureSequence::AppendLongPressGestureEvent uses delta_x to store a
point id.

This struct should be extended with an int point_id, and the  GestureEvent
constructor which takes two floats should be removed.

http://codereview.chromium.org/9773024/diff/15008/ui/aura/gestures/gesture_re...
File ui/aura/gestures/gesture_recognizer_grail.cc (right):

http://codereview.chromium.org/9773024/diff/15008/ui/aura/gestures/gesture_re...
ui/aura/gestures/gesture_recognizer_grail.cc:20: 
Once https://chromiumcodereview.appspot.com/9751011 lands, these values should
be read from GestureConfiguration.

http://codereview.chromium.org/9773024/diff/15008/ui/aura/gestures/gesture_re...
ui/aura/gestures/gesture_recognizer_grail.cc:219: assert(result != NULL);
I believe this should be a DCHECK, not an assert.

http://codereview.chromium.org/9773024/diff/15008/ui/ui.gyp
File ui/ui.gyp (right):

http://codereview.chromium.org/9773024/diff/15008/ui/ui.gyp#newcode25
ui/ui.gyp:25: '../build/linux/system.gyp:frame',
Alphabetize

http://codereview.chromium.org/9773024/diff/15008/ui/ui.gyp#newcode218
ui/ui.gyp:218: 'base/touch/axis.h',
Alphabetize

[thomas.voss, 2012-04-02 09:12:19]

On 2012/03/29 17:44:38, tdresser wrote:
> Unit tests?
> 

Working on that and trying to reuse as much of the existing tests in
gesture_recognizer_unittest.cc. 

> http://codereview.chromium.org/9773024/diff/15008/chrome/chrome_browser.gypi
> File chrome/chrome_browser.gypi (right):
> 
>
http://codereview.chromium.org/9773024/diff/15008/chrome/chrome_browser.gypi#...
> chrome/chrome_browser.gypi:4329: '../build/linux/system.gyp:frame',
> Alphabetize.
> 

Done.

> http://codereview.chromium.org/9773024/diff/15008/ui/aura/aura.gyp
> File ui/aura/aura.gyp (left):
> 
> http://codereview.chromium.org/9773024/diff/15008/ui/aura/aura.gyp#oldcode66
> ui/aura/aura.gyp:66: 'gestures/gesture_recognizer_aura.cc',
> Why are these being removed?
> 

Removed them unintentionally. Put the files in again and defined a gyp-flag
use_utouch (together with a pre-processor define USE_UTOUCH) to allow both
implementations to exist in parallel.

> http://codereview.chromium.org/9773024/diff/15008/ui/aura/aura.gyp
> File ui/aura/aura.gyp (right):
> 
> http://codereview.chromium.org/9773024/diff/15008/ui/aura/aura.gyp#newcode18
> ui/aura/aura.gyp:18: '../../build/linux/system.gyp:frame',
> Alphabetize
> 

Done. 

> http://codereview.chromium.org/9773024/diff/15008/ui/aura/event.h
> File ui/aura/event.h (right):
> 
> http://codereview.chromium.org/9773024/diff/15008/ui/aura/event.h#newcode311
> ui/aura/event.h:311: struct Properties {
> If this is being refactored, we should update the GestureRecognizerAura at the
> same time.
> 
> Currently, GestureSequence::AppendLongPressGestureEvent uses delta_x to store
a
> point id.
> 
> This struct should be extended with an int point_id, and the  GestureEvent
> constructor which takes two floats should be removed.
>

Done. Adjusted the unit-test as well to account for the change.
 
>
http://codereview.chromium.org/9773024/diff/15008/ui/aura/gestures/gesture_re...
> File ui/aura/gestures/gesture_recognizer_grail.cc (right):
> 
>
http://codereview.chromium.org/9773024/diff/15008/ui/aura/gestures/gesture_re...
> ui/aura/gestures/gesture_recognizer_grail.cc:20: 
> Once https://chromiumcodereview.appspot.com/9751011 lands, these values should
> be read from GestureConfiguration.
> 

ACK, going to rebase the patch on trunk once this lands.

>
http://codereview.chromium.org/9773024/diff/15008/ui/aura/gestures/gesture_re...
> ui/aura/gestures/gesture_recognizer_grail.cc:219: assert(result != NULL);
> I believe this should be a DCHECK, not an assert.
> 

ACK and done.

> http://codereview.chromium.org/9773024/diff/15008/ui/ui.gyp
> File ui/ui.gyp (right):
> 
> http://codereview.chromium.org/9773024/diff/15008/ui/ui.gyp#newcode25
> ui/ui.gyp:25: '../build/linux/system.gyp:frame',
> Alphabetize
> 
> http://codereview.chromium.org/9773024/diff/15008/ui/ui.gyp#newcode218
> ui/ui.gyp:218: 'base/touch/axis.h',
> Alphabetize

Both Done.

[thomas.voss, 2012-04-02 09:14:01]

Updated patch set according to comments. Introduced a gyp-flag use_utouch and
re-factored parts of the patch accordingly.

[rjkroege, 2012-04-02 17:05:15]

Some starting point questions inline.

My larger question: how do we manage the grail dependency? Would you plan on
adding it to third party? And we would need a policy to handle gardening it.

http://chromiumcodereview.appspot.com/9773024/diff/18001/chrome/chrome_browse...
File chrome/chrome_browser.gypi (right):

http://chromiumcodereview.appspot.com/9773024/diff/18001/chrome/chrome_browse...
chrome/chrome_browser.gypi:4327: '../build/linux/system.gyp:frame',
these should not be included unless the use_utouch define is enabled?

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/aura.gyp
File ui/aura/aura.gyp (right):

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/aura.gyp#new...
ui/aura/aura.gyp:68: 'gestures/gesture_point.cc',
preserve the alphabetic order?

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/aura.gyp#new...
ui/aura/aura.gyp:125: }, { #use_utouch!=1
shouldn't you be excluding the frame / grail dependency here?

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/event.h
File ui/aura/event.h (right):

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/event.h#newc...
ui/aura/event.h:312: Properties()
Why introduce the struct?

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/gestures/ges...
File ui/aura/gestures/gesture_recognizer_grail.cc (right):

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/gestures/ges...
ui/aura/gestures/gesture_recognizer_grail.cc:310: linked_ptr<GestureEvent>
ProcessDrag(UGSlice slice,
why not use a scoped_ptr?

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/gestures/ges...
ui/aura/gestures/gesture_recognizer_grail.cc:314: switch
(grail_slice_get_state(slice)) {
This code would have to be updated as we re-structure some of the gesture
protocol.

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/root_window.cc
File ui/aura/root_window.cc (right):

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/root_window....
ui/aura/root_window.cc:592: ui::GestureStatus RootWindow::ProcessGestureEvent(
this change seems unnecessary?

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/root_window_...
File ui/aura/root_window_host_linux.cc (right):

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/root_window_...
ui/aura/root_window_host_linux.cc:394: #else  // USE_UTOUCH
you should split out the change that is for the existing code paths.

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/ui.gyp
File ui/ui.gyp (right):

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/ui.gyp#newcode216
ui/ui.gyp:216: 'base/touch/axis.cc',
these need to get suppressed somewhere?

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/ui.gyp#newcode224
ui/ui.gyp:224: 'base/ui_base_exports.cc',
unnecessary

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/views/widget/nati...
File ui/views/widget/native_widget_aura.cc (right):

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/views/widget/nati...
ui/views/widget/native_widget_aura.cc:287: // NOTIMPLEMENTED();
leave it as it was?

[sadrul, 2012-04-03 19:52:01]

I left some comments. But I haven't really gone through all of the changes. This
changelist seems to include a number of changes that can probably be split up
into smaller pieces. That would certainly be easier to review :) For example:
 * Add support for building chrome on precise (with the necessary header-file
changes, and change to g_thread_init).
 * Introduction of axis in touch-factory
 * The actual GR changes.

Try to leave out white-space only changes for now, if possible.

http://codereview.chromium.org/9773024/diff/18001/build/linux/system.gyp
File build/linux/system.gyp (right):

http://codereview.chromium.org/9773024/diff/18001/build/linux/system.gyp#newc...
build/linux/system.gyp:94: 'target_name': 'frame',
Let's use 'utouch_frame' instead. 'frame' by itself is too ambiguous

http://codereview.chromium.org/9773024/diff/18001/ui/aura/gestures/gesture_re...
File ui/aura/gestures/gesture_recognizer_grail.cc (right):

http://codereview.chromium.org/9773024/diff/18001/ui/aura/gestures/gesture_re...
ui/aura/gestures/gesture_recognizer_grail.cc:21: static const base::TimeDelta
kLongPressTimeThreshold
Should you just use values from GestureConfiguration instead?

http://codereview.chromium.org/9773024/diff/18001/ui/aura/gestures/gesture_re...
ui/aura/gestures/gesture_recognizer_grail.cc:54: // for two-finger taps, drags
and pinches. Please
Does this mean there will be no single-finger gestures (e.g. tap, scroll)?

http://codereview.chromium.org/9773024/diff/18001/ui/aura/gestures/gesture_re...
ui/aura/gestures/gesture_recognizer_grail.cc:250:
ui::TouchFactory::GetInstance()->native_root_window_aura(),
This should use window_->native_window() or something of the sort.

http://codereview.chromium.org/9773024/diff/18001/ui/aura/gestures/gesture_re...
File ui/aura/gestures/gesture_recognizer_grail.h (right):

http://codereview.chromium.org/9773024/diff/18001/ui/aura/gestures/gesture_re...
ui/aura/gestures/gesture_recognizer_grail.h:29: explicit
GestureRecognizerGrail(RootWindow* window = NULL);
Avoid default args

http://codereview.chromium.org/9773024/diff/18001/ui/aura/root_window_host_li...
File ui/aura/root_window_host_linux.cc (right):

http://codereview.chromium.org/9773024/diff/18001/ui/aura/root_window_host_li...
ui/aura/root_window_host_linux.cc:400: }
This is already done in DispatcherLinux

http://codereview.chromium.org/9773024/diff/18001/ui/base/touch/touch_factory.cc
File ui/base/touch/touch_factory.cc (right):

http://codereview.chromium.org/9773024/diff/18001/ui/base/touch/touch_factory...
ui/base/touch/touch_factory.cc:27: ui::Axis::Type
TouchParamToAxisType(ui::TouchFactory::TouchParam tp) {
We have way too many types as it is. Either get rid of TouchFactory::TouchParam,
or don't add ui::Axis::Type

http://codereview.chromium.org/9773024/diff/18001/ui/base/touch/touch_factory...
ui/base/touch/touch_factory.cc:219: base::AutoLock al(touch_device_list_lock_);
Why?

http://codereview.chromium.org/9773024/diff/18001/ui/base/touch/touch_factory.h
File ui/base/touch/touch_factory.h (right):

http://codereview.chromium.org/9773024/diff/18001/ui/base/touch/touch_factory...
ui/base/touch/touch_factory.h:146: ::Window native_root_window_aura() const {
This belongs in ui/aura/, either in root_window or root_window_host (or perhaps
in both), especially since now there can be multiple root-windows.

http://codereview.chromium.org/9773024/diff/18001/ui/base/touch/touch_factory...
ui/base/touch/touch_factory.h:253: TouchDeviceList touch_device_list_;
Update the comment to explain the change for USE_XI2_MT

[thomas.voss, 2012-04-05 13:36:57]

On 2012/04/02 17:05:15, rjkroege wrote:
> Some starting point questions inline.
> 
> My larger question: how do we manage the grail dependency? Would you plan on
> adding it to third party? And we would need a policy to handle gardening it.
> 

We are fine with adding it to third_party as long as utouch-frame and -grail are
not forked but a kind of normal upstream workflow can be maintained. I adjusted
the patch for frame and grail residing in third_party and altered the DEPS file.
I will propose both frame and grail for inclusion in third_party separately. 

>
http://chromiumcodereview.appspot.com/9773024/diff/18001/chrome/chrome_browse...
> File chrome/chrome_browser.gypi (right):
> 
>
http://chromiumcodereview.appspot.com/9773024/diff/18001/chrome/chrome_browse...
> chrome/chrome_browser.gypi:4327: '../build/linux/system.gyp:frame',
> these should not be included unless the use_utouch define is enabled?
> 

Done.

> http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/aura.gyp
> File ui/aura/aura.gyp (right):
> 
>
http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/aura.gyp#new...
> ui/aura/aura.gyp:68: 'gestures/gesture_point.cc',
> preserve the alphabetic order?
> 

Done.

>
http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/aura.gyp#new...
> ui/aura/aura.gyp:125: }, { #use_utouch!=1
> shouldn't you be excluding the frame / grail dependency here?
> 

Done.

> http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/event.h
> File ui/aura/event.h (right):
> 
>
http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/event.h#newc...
> ui/aura/event.h:312: Properties()
> Why introduce the struct?
> 

I introduced the struct to summarize all of a gesture's properties in one place.
In addition, the need for encoding "alien" values in delta_x or delta_y vanishes
with such a struct while maintaining a "uniform" c'tor signature. I added a
comment in the source code to clarify my intention. Would it make sense to add a
full 3x3 transformation matrix in the struct? 

>
http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/gestures/ges...
> File ui/aura/gestures/gesture_recognizer_grail.cc (right):
> 
>
http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/gestures/ges...
> ui/aura/gestures/gesture_recognizer_grail.cc:310: linked_ptr<GestureEvent>
> ProcessDrag(UGSlice slice,
> why not use a scoped_ptr?
> 
> 

Refactored the methods such that the resulting vector of gestures is passed to
the methods. Handling of NULL return values is not required any more.

http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/gestures/ges...
> ui/aura/gestures/gesture_recognizer_grail.cc:314: switch
> (grail_slice_get_state(slice)) {
> This code would have to be updated as we re-structure some of the gesture
> protocol.
> 

No problem, happy to adjust/rebase.

>
http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/root_window.cc
> File ui/aura/root_window.cc (right):
> 
>
http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/root_window....
> ui/aura/root_window.cc:592: ui::GestureStatus RootWindow::ProcessGestureEvent(
> this change seems unnecessary?
> 

Reverted.

>
http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/root_window_...
> File ui/aura/root_window_host_linux.cc (right):
> 
>
http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/aura/root_window_...
> ui/aura/root_window_host_linux.cc:394: #else  // USE_UTOUCH
> you should split out the change that is for the existing code paths.
> 

Done. Re-enabled the original code flow.

> http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/ui.gyp
> File ui/ui.gyp (right):
> 
> http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/ui.gyp#newcode216
> ui/ui.gyp:216: 'base/touch/axis.cc',
> these need to get suppressed somewhere?
> 

Done.

> http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/ui.gyp#newcode224
> ui/ui.gyp:224: 'base/ui_base_exports.cc',
> unnecessary
> 

Under the assumption that you are referring to the spurious tab here: Done.

>
http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/views/widget/nati...
> File ui/views/widget/native_widget_aura.cc (right):
> 
>
http://chromiumcodereview.appspot.com/9773024/diff/18001/ui/views/widget/nati...
> ui/views/widget/native_widget_aura.cc:287: // NOTIMPLEMENTED();
> leave it as it was?

Reverted.

[thomas.voss, 2012-04-05 14:15:54]

On 2012/04/03 19:52:01, sadrul wrote:
> I left some comments. But I haven't really gone through all of the changes.
This
> changelist seems to include a number of changes that can probably be split up
> into smaller pieces. That would certainly be easier to review :) For example:
>  * Add support for building chrome on precise (with the necessary header-file
> changes, and change to g_thread_init).
>  * Introduction of axis in touch-factory
>  * The actual GR changes.
> 
> Try to leave out white-space only changes for now, if possible.
> 
> http://codereview.chromium.org/9773024/diff/18001/build/linux/system.gyp
> File build/linux/system.gyp (right):
> 
>
http://codereview.chromium.org/9773024/diff/18001/build/linux/system.gyp#newc...
> build/linux/system.gyp:94: 'target_name': 'frame',
> Let's use 'utouch_frame' instead. 'frame' by itself is too ambiguous
> 

Done, edited the patch to consider frame and grail living in third_party.

>
http://codereview.chromium.org/9773024/diff/18001/ui/aura/gestures/gesture_re...
> File ui/aura/gestures/gesture_recognizer_grail.cc (right):
> 
>
http://codereview.chromium.org/9773024/diff/18001/ui/aura/gestures/gesture_re...
> ui/aura/gestures/gesture_recognizer_grail.cc:21: static const base::TimeDelta
> kLongPressTimeThreshold
> Should you just use values from GestureConfiguration instead?
> 

Ack, adjusted for double-taps. Will adjust the long-press time threshold once I
have updated my working copy.

>
http://codereview.chromium.org/9773024/diff/18001/ui/aura/gestures/gesture_re...
> ui/aura/gestures/gesture_recognizer_grail.cc:54: // for two-finger taps, drags
> and pinches. Please
> Does this mean there will be no single-finger gestures (e.g. tap, scroll)?
> 

No, we are not limited to two-finger gestures here, grail is able to recognize
one-finger gestures as well. Still, the desired behavior might be different for
direct vs. indirect touch devices and we initially targeted the patch towards
indirect touch devices. I started altering our unit tests to consider one-touch
gestures as well but will take another iteration to complete.

>
http://codereview.chromium.org/9773024/diff/18001/ui/aura/gestures/gesture_re...
> ui/aura/gestures/gesture_recognizer_grail.cc:250:
> ui::TouchFactory::GetInstance()->native_root_window_aura(),
> This should use window_->native_window() or something of the sort.
> 

Ack.

>
http://codereview.chromium.org/9773024/diff/18001/ui/aura/gestures/gesture_re...
> File ui/aura/gestures/gesture_recognizer_grail.h (right):
> 
>
http://codereview.chromium.org/9773024/diff/18001/ui/aura/gestures/gesture_re...
> ui/aura/gestures/gesture_recognizer_grail.h:29: explicit
> GestureRecognizerGrail(RootWindow* window = NULL);
> Avoid default args
> 

Done, removed the default arg.

>
http://codereview.chromium.org/9773024/diff/18001/ui/aura/root_window_host_li...
> File ui/aura/root_window_host_linux.cc (right):
> 
>
http://codereview.chromium.org/9773024/diff/18001/ui/aura/root_window_host_li...
> ui/aura/root_window_host_linux.cc:400: }
> This is already done in DispatcherLinux
> 

Removed in root_window_host_linux.

>
http://codereview.chromium.org/9773024/diff/18001/ui/base/touch/touch_factory.cc
> File ui/base/touch/touch_factory.cc (right):
> 
>
http://codereview.chromium.org/9773024/diff/18001/ui/base/touch/touch_factory...
> ui/base/touch/touch_factory.cc:27: ui::Axis::Type
> TouchParamToAxisType(ui::TouchFactory::TouchParam tp) {
> We have way too many types as it is. Either get rid of
TouchFactory::TouchParam,
> or don't add ui::Axis::Type
> 

Ack, will remove TouchFactory::TouchParam in the next iteration.

>
http://codereview.chromium.org/9773024/diff/18001/ui/base/touch/touch_factory...
> ui/base/touch/touch_factory.cc:219: base::AutoLock
al(touch_device_list_lock_);
> Why?
> 
>
http://codereview.chromium.org/9773024/diff/18001/ui/base/touch/touch_factory.h
> File ui/base/touch/touch_factory.h (right):
> 
>
http://codereview.chromium.org/9773024/diff/18001/ui/base/touch/touch_factory...
> ui/base/touch/touch_factory.h:146: ::Window native_root_window_aura() const {
> This belongs in ui/aura/, either in root_window or root_window_host (or
perhaps
> in both), especially since now there can be multiple root-windows.
> 

Ack, will move the method in the next iteration.

>
http://codereview.chromium.org/9773024/diff/18001/ui/base/touch/touch_factory...
> ui/base/touch/touch_factory.h:253: TouchDeviceList touch_device_list_;
> Update the comment to explain the change for USE_XI2_MT

Done.

[thomas.voss, 2012-04-05 15:41:36]

Updated the patch set merging in comments from rjkroege and sadrul. Some of
sadrul's comments are not applied yet, but will be part of the next iteration.

Summary of the important changes:
  * Added unit tests for two-finger gestures on trackpads
    * Unit tests rely on xorg-gtest (see [1]), a dep that can live in
third_party as well and might prove useful for other tests as well.
  * Altered the setup such that frame and grail live in third_party.

[Elliot Glaysher, 2012-04-19 19:47:37]

ping sadrul

(Sorry that this fell on the floor, Thomas)

[sadrul, 2012-04-19 19:53:58]

On 2012/04/19 19:47:37, Elliot Glaysher wrote:
> ping sadrul
> 
> (Sorry that this fell on the floor, Thomas)

Ah, my bad. I was under the impression that this will need to be updated first
(the GR code has moved into ui/base/ from ui/aura/, and #18 gave me the
impression that this isn't ready for review yet).

[Elliot Glaysher, 2012-04-19 20:14:48]

On 2012/04/19 19:53:58, sadrul wrote:
> On 2012/04/19 19:47:37, Elliot Glaysher wrote:
> > ping sadrul
> > 
> > (Sorry that this fell on the floor, Thomas)
> 
> Ah, my bad. I was under the impression that this will need to be updated first
> (the GR code has moved into ui/base/ from ui/aura/, and #18 gave me the
> impression that this isn't ready for review yet).

Actually, now that I look at it, yeah, it'll also need the grail headers added
to third-party/

Doing things that way assumes that we can rely on grail as a build dependency;
does grail ship as part of the default Precise install? Would it be better to
dlopen grail.so like we already do with libunity?
(chrome/browser/ui/gtk/unity_service.cc)

[thomas.voss, 2012-04-20 13:33:05]

On 2012/04/19 19:53:58, sadrul wrote:
> On 2012/04/19 19:47:37, Elliot Glaysher wrote:
> > ping sadrul
> > 
> > (Sorry that this fell on the floor, Thomas)
> 
> Ah, my bad. I was under the impression that this will need to be updated first
> (the GR code has moved into ui/base/ from ui/aura/, and #18 gave me the
> impression that this isn't ready for review yet).

Sorry, my bad. The only outstanding change is "get rid of all of the enumerated
types for modelling axes". This is still on my plate but ready to go either
today or monday.

[thomas.voss, 2012-04-20 13:35:04]

On 2012/04/19 20:14:48, Elliot Glaysher wrote:
> On 2012/04/19 19:53:58, sadrul wrote:
> > On 2012/04/19 19:47:37, Elliot Glaysher wrote:
> > > ping sadrul
> > > 
> > > (Sorry that this fell on the floor, Thomas)
> > 
> > Ah, my bad. I was under the impression that this will need to be updated
first
> > (the GR code has moved into ui/base/ from ui/aura/, and #18 gave me the
> > impression that this isn't ready for review yet).
> 
> Actually, now that I look at it, yeah, it'll also need the grail headers added
> to third-party/
> 
> Doing things that way assumes that we can rely on grail as a build dependency;
> does grail ship as part of the default Precise install? Would it be better to
> dlopen grail.so like we already do with libunity?
> (chrome/browser/ui/gtk/unity_service.cc)
Both grail and frame ship as part of the default precise install. Still, I
altered the patch to pull grail/frame from third_party. 

Does it make sense to issue a request for inclusion of grail/frame with
third_party, yet?

[sadrul, 2012-04-20 13:53:28]

On 2012/04/20 13:33:05, thomas.voss wrote:
> On 2012/04/19 19:53:58, sadrul wrote:
> > On 2012/04/19 19:47:37, Elliot Glaysher wrote:
> > > ping sadrul
> > > 
> > > (Sorry that this fell on the floor, Thomas)
> > 
> > Ah, my bad. I was under the impression that this will need to be updated
first
> > (the GR code has moved into ui/base/ from ui/aura/, and #18 gave me the
> > impression that this isn't ready for review yet).
> 
> Sorry, my bad. The only outstanding change is "get rid of all of the
enumerated
> types for modelling axes". This is still on my plate but ready to go either
> today or monday.

Could you please merge with ToT? (Much of the gesture-recognizer code has moved
into ui/base/ from ui/aura, as per my comment above, and the email I sent you
when I was doing the refactoring)