Added entry points to view/View to dispatch and process TouchEvents.

Added entry points to view/View to dispatch and process TouchEvents.

This change is part of a series of changes to bring TouchEvent
processing to views.

BUG=na
TESTED=compiled with and without touchui

[sky, 2010-08-18 00:06:35]

http://codereview.chromium.org/3192002/diff/2001/3002
File views/touchui/gesture_manager.h (right):

http://codereview.chromium.org/3192002/diff/2001/3002#newcode24
views/touchui/gesture_manager.h:24: 
nit: no newline here.

http://codereview.chromium.org/3192002/diff/2001/3004
File views/view.h (right):

http://codereview.chromium.org/3192002/diff/2001/3004#newcode1358
views/view.h:1358: // A reference to the gesture_engine_ for this.
Why is there a GestureManager per View? Shouldn't there be only one?

[DaveMoore, 2010-08-18 14:36:49]

http://codereview.chromium.org/3192002/diff/2001/3004
File views/view.h (right):

http://codereview.chromium.org/3192002/diff/2001/3004#newcode1147
views/view.h:1147: friend class GestureManager;
Why is there an assumption that the GestureManager will need access to private
View api? Shouldn't the interface between them be explicitly defined?

http://codereview.chromium.org/3192002/diff/2001/3004#newcode1184
views/view.h:1184: // RootView will invoke this with incoming TouchEvents.
What does the return value imply?

http://codereview.chromium.org/3192002/diff/2001/3005
File views/views.gyp (right):

http://codereview.chromium.org/3192002/diff/2001/3005#newcode362
views/views.gyp:362: ['touchui==0', {
The touchui gyp variable should also set the TOUCH_UI define.

[rjkroege, 2010-08-24 20:25:12]

http://codereview.chromium.org/3192002/diff/2001/3002
File views/touchui/gesture_manager.h (right):

http://codereview.chromium.org/3192002/diff/2001/3002#newcode24
views/touchui/gesture_manager.h:24: 
On 2010/08/18 00:06:35, sky wrote:
> nit: no newline here.

Done.

http://codereview.chromium.org/3192002/diff/2001/3004
File views/view.h (right):

http://codereview.chromium.org/3192002/diff/2001/3004#newcode1147
views/view.h:1147: friend class GestureManager;
On 2010/08/18 14:36:49, davemoore wrote:
> Why is there an assumption that the GestureManager will need access to private
> View api? Shouldn't the interface between them be explicitly defined?

It was a bad assumption. The GestureManager does not need to be a friend. Fixed.

http://codereview.chromium.org/3192002/diff/2001/3004#newcode1184
views/view.h:1184: // RootView will invoke this with incoming TouchEvents.
On 2010/08/18 14:36:49, davemoore wrote:
> What does the return value imply?

I expanded the comment.

http://codereview.chromium.org/3192002/diff/2001/3004#newcode1358
views/view.h:1358: // A reference to the gesture_engine_ for this.
On 2010/08/18 00:06:35, sky wrote:
> Why is there a GestureManager per View? Shouldn't there be only one?

When I wrote this, I had the idea that each view should be able to have its own
GestureManager but that typically only the RootView should actually use it.
However, I think you're right -- there should be only one GestureManager because
its purpose is to swallow all of the touch events that don't have specific
handlers.

I've modified this appropriately by making the GestureManager a singleton.

http://codereview.chromium.org/3192002/diff/2001/3005
File views/views.gyp (right):

http://codereview.chromium.org/3192002/diff/2001/3005#newcode362
views/views.gyp:362: ['touchui==0', {
On 2010/08/18 14:36:49, davemoore wrote:
> The touchui gyp variable should also set the TOUCH_UI define.

Done.

[sky, 2010-08-24 21:28:33]

http://codereview.chromium.org/3192002/diff/13001/14002
File views/touchui/gesture_manager.cc (right):

http://codereview.chromium.org/3192002/diff/13001/14002#newcode30
views/touchui/gesture_manager.cc:30: LOG(INFO) <<
"GestureManager::ProcessTouchEventForGesture: " <<
DLOG for this and all.

http://codereview.chromium.org/3192002/diff/13001/14003
File views/touchui/gesture_manager.h (right):

http://codereview.chromium.org/3192002/diff/13001/14003#newcode9
views/touchui/gesture_manager.h:9: #include "build/build_config.h"
Do you need this include?

http://codereview.chromium.org/3192002/diff/13001/14003#newcode11
views/touchui/gesture_manager.h:11: #include "views/event.h"
Forward declare TouchEvent and View.

http://codereview.chromium.org/3192002/diff/13001/14003#newcode17
views/touchui/gesture_manager.h:17: // incoming feed of touch events. A view
wishing to recognize gestures
This makes it sound like each view that cares about touch events needs to call
into the GestureManager. Is that really the case?

http://codereview.chromium.org/3192002/diff/13001/14003#newcode26
views/touchui/gesture_manager.h:26: virtual ~GestureManager();
Is this class really intended to be subclassed?

http://codereview.chromium.org/3192002/diff/13001/14004
File views/view.cc (right):

http://codereview.chromium.org/3192002/diff/13001/14004#newcode1333
views/view.cc:1333: #if defined(TOUCH_UI)
Match position in header.

http://codereview.chromium.org/3192002/diff/13001/14004#newcode1335
views/view.cc:1335: LOG(INFO) << "visited the OnTouchEvent";
DLOG

http://codereview.chromium.org/3192002/diff/13001/14006
File views/views.gyp (right):

http://codereview.chromium.org/3192002/diff/13001/14006#newcode366
views/views.gyp:366: 'touchui/gesture_manager.cc',
Create a rule for target_defaults that makes everything in touchui compile only
when touchui=0. This way you don't have to add every file here.

http://codereview.chromium.org/3192002/diff/13001/14007
File views/widget/root_view.cc (right):

http://codereview.chromium.org/3192002/diff/13001/14007#newcode85
views/widget/root_view.cc:85:
gesture_manager_(Singleton<GestureManager>::get()),
Singleton should be an implementation detail of GestureManager. If you want GM
to be a singleton provide a static method on it to get the instance and make the
constructor/destructor private.

http://codereview.chromium.org/3192002/diff/13001/14007#newcode310
views/widget/root_view.cc:310: gesture_manager_->ProcessTouchEventForGesture(e,
this, true);
I haven't played with touch event much, so I don't know how they normally work.
It seems odd to me that you get both a touch event and a mouse event. Is that
typical?

http://codereview.chromium.org/3192002/diff/13001/14008
File views/widget/root_view.h (right):

http://codereview.chromium.org/3192002/diff/13001/14008#newcode338
views/widget/root_view.h:338: // The the gesture_manager_ for this.
the the

[DaveMoore, 2010-08-24 21:32:28]

http://codereview.chromium.org/3192002/diff/13001/14007
File views/widget/root_view.cc (right):

http://codereview.chromium.org/3192002/diff/13001/14007#newcode87
views/widget/root_view.cc:87: #endif
If it's a singleton anyway why do we set it in each RootView?

http://codereview.chromium.org/3192002/diff/13001/14007#newcode303
views/widget/root_view.cc:303: // If touch_pressed_handler_ is non null, we are
currently processing
There should be test coverage for logic this complicated.

[rjkroege, 2010-08-30 17:39:11]

http://codereview.chromium.org/3192002/diff/13001/14002
File views/touchui/gesture_manager.cc (right):

http://codereview.chromium.org/3192002/diff/13001/14002#newcode30
views/touchui/gesture_manager.cc:30: LOG(INFO) <<
"GestureManager::ProcessTouchEventForGesture: " <<
On 2010/08/24 21:28:33, sky wrote:
> DLOG for this and all.

Done.

http://codereview.chromium.org/3192002/diff/13001/14003
File views/touchui/gesture_manager.h (right):

http://codereview.chromium.org/3192002/diff/13001/14003#newcode9
views/touchui/gesture_manager.h:9: #include "build/build_config.h"
On 2010/08/24 21:28:33, sky wrote:
> Do you need this include?

Done.

http://codereview.chromium.org/3192002/diff/13001/14003#newcode11
views/touchui/gesture_manager.h:11: #include "views/event.h"
On 2010/08/24 21:28:33, sky wrote:
> Forward declare TouchEvent and View.

Done.

http://codereview.chromium.org/3192002/diff/13001/14003#newcode17
views/touchui/gesture_manager.h:17: // incoming feed of touch events. A view
wishing to recognize gestures
On 2010/08/24 21:28:33, sky wrote:
> This makes it sound like each view that cares about touch events needs to call
> into the GestureManager. Is that really the case?

No. Clarified.

http://codereview.chromium.org/3192002/diff/13001/14003#newcode26
views/touchui/gesture_manager.h:26: virtual ~GestureManager();
On 2010/08/24 21:28:33, sky wrote:
> Is this class really intended to be subclassed?

Yes. Two reasons: it lets me mock it out in the unit tests and it lets me create
(in a future CL) the Renderer-specific version as a subclass.

http://codereview.chromium.org/3192002/diff/13001/14004
File views/view.cc (right):

http://codereview.chromium.org/3192002/diff/13001/14004#newcode1333
views/view.cc:1333: #if defined(TOUCH_UI)
On 2010/08/24 21:28:33, sky wrote:
> Match position in header.

Done.

http://codereview.chromium.org/3192002/diff/13001/14004#newcode1335
views/view.cc:1335: LOG(INFO) << "visited the OnTouchEvent";
On 2010/08/24 21:28:33, sky wrote:
> DLOG

Done.

http://codereview.chromium.org/3192002/diff/13001/14006
File views/views.gyp (right):

http://codereview.chromium.org/3192002/diff/13001/14006#newcode366
views/views.gyp:366: 'touchui/gesture_manager.cc',
On 2010/08/24 21:28:33, sky wrote:
> Create a rule for target_defaults that makes everything in touchui compile
only
> when touchui=0. This way you don't have to add every file here.

My gypi-fu is weak. I think I've done this.

http://codereview.chromium.org/3192002/diff/13001/14007
File views/widget/root_view.cc (right):

http://codereview.chromium.org/3192002/diff/13001/14007#newcode87
views/widget/root_view.cc:87: #endif
On 2010/08/24 21:32:28, davemoore wrote:
> If it's a singleton anyway why do we set it in each RootView?

As I understand the comment in the singleton class, there is a performance cost
to get the singleton pointer. Given that we could be looking up this on every
single event, I thought that the performance advantage was worth the space
overhead of a pointer/RootView.

It also makes it much easier to mock.

http://codereview.chromium.org/3192002/diff/13001/14007#newcode303
views/widget/root_view.cc:303: // If touch_pressed_handler_ is non null, we are
currently processing
On 2010/08/24 21:32:28, davemoore wrote:
> There should be test coverage for logic this complicated.

Done.

http://codereview.chromium.org/3192002/diff/13001/14007#newcode310
views/widget/root_view.cc:310: gesture_manager_->ProcessTouchEventForGesture(e,
this, true);
On 2010/08/24 21:28:33, sky wrote:
> I haven't played with touch event much, so I don't know how they normally
work.
> It seems odd to me that you get both a touch event and a mouse event. Is that
> typical?

The underlying infrastructure will eventually deliver mouse events from mice and
touch events from touch input devices as separate and distinct events. (As a use
case, imagine a tablet with an attached mouse -- there are two distinct
"pointers" and to make such a situation work, they need to be separate.)  In the
short term, to test the code in this CL, I made all mouse events into touch
events, injected them into the RootView as touch events, and then the interim
GestureManager turned them back into mouse events.

http://codereview.chromium.org/3192002/diff/13001/14008
File views/widget/root_view.h (right):

http://codereview.chromium.org/3192002/diff/13001/14008#newcode338
views/widget/root_view.h:338: // The the gesture_manager_ for this.
On 2010/08/24 21:28:33, sky wrote:
> the the

Done.

[sky, 2010-08-30 20:07:29]

http://codereview.chromium.org/3192002/diff/23001/24002
File views/touchui/gesture_manager.cc (right):

http://codereview.chromium.org/3192002/diff/23001/24002#newcode16
views/touchui/gesture_manager.cc:16: GestureManager::GestureManager() {
Remember to have order match header.

http://codereview.chromium.org/3192002/diff/23001/24003
File views/touchui/gesture_manager.h (right):

http://codereview.chromium.org/3192002/diff/23001/24003#newcode24
views/touchui/gesture_manager.h:24: static GestureManager* Get();
style guide says this should be after destructor.

http://codereview.chromium.org/3192002/diff/23001/24006
File views/view_unittest.cc (right):

http://codereview.chromium.org/3192002/diff/23001/24006#newcode26
views/view_unittest.cc:26: #if defined(TOUCH_UI)
Create a section after the current set of includes that has all the conditionals
includes and group this among them, eg:

#include non-conditional-includes

#define FOO
#include conditional-includes
#endif

http://codereview.chromium.org/3192002/diff/23001/24006#newcode213
views/view_unittest.cc:213: 
No newline after access keyword.

http://codereview.chromium.org/3192002/diff/23001/24006#newcode221
views/view_unittest.cc:221: int last_touch_event_;
fields after methods.

http://codereview.chromium.org/3192002/diff/23001/24006#newcode229
views/view_unittest.cc:229: };
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/3192002/diff/23001/24006#newcode463
views/view_unittest.cc:463: window_win->set_delete_on_destroy(false);
Don't you need set_delete_on_destroy for non-windows?

http://codereview.chromium.org/3192002/diff/23001/24009
File views/widget/root_view.h (right):

http://codereview.chromium.org/3192002/diff/23001/24009#newcode185
views/widget/root_view.h:185: #if defined(TOUCH_UI) && !defined(NDEBUG)
You sure you don't want UNIT_TEST as the define?

http://codereview.chromium.org/3192002/diff/23001/24009#newcode188
views/widget/root_view.h:188: virtual void SetGestureManager(GestureManager* g)
{ gesture_manager_ = g; }
set_gesture_manager. Does it need to be virtual?

[rjkroege, 2010-08-30 21:41:30]

http://codereview.chromium.org/3192002/diff/23001/24002
File views/touchui/gesture_manager.cc (right):

http://codereview.chromium.org/3192002/diff/23001/24002#newcode16
views/touchui/gesture_manager.cc:16: GestureManager::GestureManager() {
On 2010/08/30 20:07:29, sky wrote:
> Remember to have order match header.

Done.

http://codereview.chromium.org/3192002/diff/23001/24003
File views/touchui/gesture_manager.h (right):

http://codereview.chromium.org/3192002/diff/23001/24003#newcode24
views/touchui/gesture_manager.h:24: static GestureManager* Get();
On 2010/08/30 20:07:29, sky wrote:
> style guide says this should be after destructor.

Done.

http://codereview.chromium.org/3192002/diff/23001/24006
File views/view_unittest.cc (right):

http://codereview.chromium.org/3192002/diff/23001/24006#newcode26
views/view_unittest.cc:26: #if defined(TOUCH_UI)
On 2010/08/30 20:07:29, sky wrote:
> Create a section after the current set of includes that has all the
conditionals
> includes and group this among them, eg:
> 
> #include non-conditional-includes
> 
> #define FOO
> #include conditional-includes
> #endif

I was going to use the style guide pattern but was following along the pattern
of the #if defined(OS_WIN)... above. 

Done.

http://codereview.chromium.org/3192002/diff/23001/24006#newcode213
views/view_unittest.cc:213: 
On 2010/08/30 20:07:29, sky wrote:
> No newline after access keyword.

Done.

http://codereview.chromium.org/3192002/diff/23001/24006#newcode221
views/view_unittest.cc:221: int last_touch_event_;
On 2010/08/30 20:07:29, sky wrote:
> fields after methods.

Done.

http://codereview.chromium.org/3192002/diff/23001/24006#newcode229
views/view_unittest.cc:229: };
On 2010/08/30 20:07:29, sky wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/3192002/diff/23001/24006#newcode463
views/view_unittest.cc:463: window_win->set_delete_on_destroy(false);
On 2010/08/30 20:07:29, sky wrote:
> Don't you need set_delete_on_destroy for non-windows?

AFAIK, no.

http://codereview.chromium.org/3192002/diff/23001/24009
File views/widget/root_view.h (right):

http://codereview.chromium.org/3192002/diff/23001/24009#newcode185
views/widget/root_view.h:185: #if defined(TOUCH_UI) && !defined(NDEBUG)
On 2010/08/30 20:07:29, sky wrote:
> You sure you don't want UNIT_TEST as the define?

Yes.

http://codereview.chromium.org/3192002/diff/23001/24009#newcode188
views/widget/root_view.h:188: virtual void SetGestureManager(GestureManager* g)
{ gesture_manager_ = g; }
On 2010/08/30 20:07:29, sky wrote:
> set_gesture_manager. Does it need to be virtual?

No. Done.

[sky, 2010-08-30 21:56:22]

LGTM

http://codereview.chromium.org/3192002/diff/35001/36006
File views/view_unittest.cc (right):

http://codereview.chromium.org/3192002/diff/35001/36006#newcode38
views/view_unittest.cc:38: #if defined(TOUCH_UI)
Cleanup all the other includes to match what you've done.