Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8364039:
Initial views touchui GestureRecognizer support (Closed)
|
Created:
9 years, 2 months ago by Gajen Modified:
8 years, 11 months ago CC:
chromium-reviews, tfarina, dhollowa Visibility:
Public. |
DescriptionInitial views touchui GestureRecognizer support
Based on webkit's chromium gesture recognizer
BUG=none
TEST=manual
Patch Set 1 #
Total comments: 9
Patch Set 2 : Event injection as per Robert comments #
Total comments: 18
Patch Set 3 : Added ForwardGestureEvents()in RootView and incorporated comments #
Total comments: 8
Patch Set 4 : Wired gesture events into RootView and commandline switch enable-gesture #
Total comments: 13
Patch Set 5 : Added gesture tests in view_unittest #
Total comments: 33
Patch Set 6 : Implemenation of GR, independent of View/GM and more test in views_unittes #
Total comments: 2
Patch Set 7 : Reverted chrome_switches.cc changes and TOT sync up #
Total comments: 31
Patch Set 8 : Incorporated SKy's comments #Patch Set 9 : Added switch support in place of map::find() #
Total comments: 19
Patch Set 10 : Added GestureSignature enum type #
Messages
Total messages: 42 (0 generated)
Hi, Robert. This is 1st version of patch based on the approach suggested by you in mail discussion(21/09/11, with Subject "TouchUI Gesture Support"). This patch ports the GestureEvents and GestureRecognizer in views analogous to Webkit ones. Rest of the part 1)Adding OnGestureEvent in View,2)integration with GestureManager and RootView would be next, once I get Go Ahead signal from you. Please let me know you feedback, so that I can put subsequent patches to get the whole Gesture Integration in Views. Thx Gajen.
This is a good chromification of the code in WebKit that handles gestures but it has no wiring: events never enter this code and once produced, never leave. There are abundant other changes needed to complete use of the gestures. I'd be happy to discuss. It seems also that this code should replace gesture_manager.cc yes? http://codereview.chromium.org/8364039/diff/1/views/events/event.cc File views/events/event.cc (right): http://codereview.chromium.org/8364039/diff/1/views/events/event.cc#newcode304 views/events/event.cc:304: int x, these lines don't look indented right. I think they ought to align with the u in ui:: http://codereview.chromium.org/8364039/diff/1/views/touchui/gesture_recognize... File views/touchui/gesture_recognizer.cc (right): http://codereview.chromium.org/8364039/diff/1/views/touchui/gesture_recognize... views/touchui/gesture_recognizer.cc:13: double maximumTouchDownDurationInSecondsForClick = 0.8; A subsequent CL should connect these changes with http://code.google.com/p/chromium/issues/detail?id=100773 Might be nice to add a TODO http://codereview.chromium.org/8364039/diff/1/views/touchui/gesture_recognize... views/touchui/gesture_recognizer.cc:14: double minimumTouchDownDurationInSecondsForClick = 0.01; These ought to be const at the moment. They need leading k* in their names. http://codereview.chromium.org/8364039/diff/1/views/touchui/gesture_recognize... views/touchui/gesture_recognizer.cc:159: first_touch_position_.x(), Bad indent I think.
On 2011/10/21 20:02:10, rjkroege wrote: > This is a good chromification of the code in WebKit that handles gestures but it Thx > has no wiring: events never enter this code and once produced, never leave. > There are abundant other changes needed to complete use of the gestures. I'd be > happy to discuss. Please find second patch for this, based on my understanding, Would like further discussion and valuable comments. > > It seems also that this code should replace gesture_manager.cc yes? > > http://codereview.chromium.org/8364039/diff/1/views/events/event.cc > File views/events/event.cc (right): > > http://codereview.chromium.org/8364039/diff/1/views/events/event.cc#newcode304 > views/events/event.cc:304: int x, > these lines don't look indented right. I think they ought to align with the u in > ui:: > Done > http://codereview.chromium.org/8364039/diff/1/views/touchui/gesture_recognize... > File views/touchui/gesture_recognizer.cc (right): > Done > http://codereview.chromium.org/8364039/diff/1/views/touchui/gesture_recognize... > views/touchui/gesture_recognizer.cc:13: double > maximumTouchDownDurationInSecondsForClick = 0.8; > A subsequent CL should connect these changes with > http://code.google.com/p/chromium/issues/detail?id=100773 > > Might be nice to add a TODO > Forgot will add in new patch. > http://codereview.chromium.org/8364039/diff/1/views/touchui/gesture_recognize... > views/touchui/gesture_recognizer.cc:14: double > minimumTouchDownDurationInSecondsForClick = 0.01; > These ought to be const at the moment. They need leading k* in their names. > Forgot would incorporate in next patch. > http://codereview.chromium.org/8364039/diff/1/views/touchui/gesture_recognize... > views/touchui/gesture_recognizer.cc:159: first_touch_position_.x(), > Bad indent I think. Done
This seems to be progressing nicely. I'd like you to make wiring here more like the analogous code in WebKit. http://codereview.chromium.org/8364039/diff/6001/views/events/event.cc File views/events/event.cc (right): http://codereview.chromium.org/8364039/diff/6001/views/events/event.cc#newcod... views/events/event.cc:304: int x, I still think these ought to line up: int under ui? http://codereview.chromium.org/8364039/diff/6001/views/touchui/gesture_manage... File views/touchui/gesture_manager.cc (right): http://codereview.chromium.org/8364039/diff/6001/views/touchui/gesture_manage... views/touchui/gesture_manager.cc:35: false)); indent 1 more http://codereview.chromium.org/8364039/diff/6001/views/touchui/gesture_manage... views/touchui/gesture_manager.cc:63: source_widget->OnGestureEvent(converted); you can modify the code in RootView to contain the dispatch loop. RootView is analogous to WebViewImpl... http://codereview.chromium.org/8364039/diff/6001/views/touchui/gesture_manage... views/touchui/gesture_manager.cc:68: // TODO(Gajen): Do we need to support Gesture across different RootViews no. you'll see that the RootView owns a private GM instance. http://codereview.chromium.org/8364039/diff/6001/views/touchui/gesture_manager.h File views/touchui/gesture_manager.h (right): http://codereview.chromium.org/8364039/diff/6001/views/touchui/gesture_manage... views/touchui/gesture_manager.h:57: RootView* root_view_; Why do we need this pointer when the view of interest is passed in to PTEFG? This seems like a bad idea. http://codereview.chromium.org/8364039/diff/6001/views/touchui/gesture_manage... views/touchui/gesture_manager.h:60: GestureRecognizer gesture_recognizer; members need a trailing _ More general observation: why not make GestureRecognizer a subclass of GestureManager and override PTEFG when appropriate? (BTW: because GR makes different events than GM, you will need to make the flip-on of GR be optional as touchui currently depends on the synthetic mouse events. http://codereview.chromium.org/8364039/diff/6001/views/view.cc File views/view.cc (right): http://codereview.chromium.org/8364039/diff/6001/views/view.cc#newcode1944 views/view.cc:1944: // MousePressed. bad indent http://codereview.chromium.org/8364039/diff/6001/views/widget/root_view.cc File views/widget/root_view.cc (right): http://codereview.chromium.org/8364039/diff/6001/views/widget/root_view.cc#ne... views/widget/root_view.cc:388: if (gesture_manager_->ProcessTouchEventForGesture(e, this, status)) to bring this code more closely int how the WebKit code works, I would think that PTEFG should return an array of event instances that get dispatched here after executing it. http://codereview.chromium.org/8364039/diff/6001/views/widget/root_view.cc#ne... views/widget/root_view.cc:393: bool RootView::OnTouchEvent(const TouchEvent& event) { I am reasonably strongly convinced that this is wrong: there is a RootView::OnTouchEvent entry point above. It needs to be modified.
http://codereview.chromium.org/8364039/diff/1/views/events/event.cc File views/events/event.cc (right): http://codereview.chromium.org/8364039/diff/1/views/events/event.cc#newcode304 views/events/event.cc:304: int x, On 2011/10/21 20:02:10, rjkroege wrote: > these lines don't look indented right. I think they ought to align with the u in > ui:: Done. http://codereview.chromium.org/8364039/diff/1/views/touchui/gesture_recognize... File views/touchui/gesture_recognizer.cc (right): http://codereview.chromium.org/8364039/diff/1/views/touchui/gesture_recognize... views/touchui/gesture_recognizer.cc:13: double maximumTouchDownDurationInSecondsForClick = 0.8; On 2011/10/21 20:02:10, rjkroege wrote: > A subsequent CL should connect these changes with > http://code.google.com/p/chromium/issues/detail?id=100773 > > Might be nice to add a TODO Done. http://codereview.chromium.org/8364039/diff/1/views/touchui/gesture_recognize... views/touchui/gesture_recognizer.cc:14: double minimumTouchDownDurationInSecondsForClick = 0.01; On 2011/10/21 20:02:10, rjkroege wrote: > These ought to be const at the moment. They need leading k* in their names. Done. http://codereview.chromium.org/8364039/diff/1/views/touchui/gesture_recognize... views/touchui/gesture_recognizer.cc:159: first_touch_position_.x(), On 2011/10/21 20:02:10, rjkroege wrote: > Bad indent I think. Done. http://codereview.chromium.org/8364039/diff/1/views/touchui/gesture_recognize... views/touchui/gesture_recognizer.cc:159: first_touch_position_.x(), On 2011/10/21 20:02:10, rjkroege wrote: > Bad indent I think. Done. http://codereview.chromium.org/8364039/diff/6001/views/events/event.cc File views/events/event.cc (right): http://codereview.chromium.org/8364039/diff/6001/views/events/event.cc#newcod... views/events/event.cc:304: int x, On 2011/10/24 17:41:09, rjkroege wrote: > I still think these ought to line up: int under ui? Done. http://codereview.chromium.org/8364039/diff/6001/views/touchui/gesture_manage... File views/touchui/gesture_manager.cc (right): http://codereview.chromium.org/8364039/diff/6001/views/touchui/gesture_manage... views/touchui/gesture_manager.cc:35: false)); On 2011/10/24 17:41:09, rjkroege wrote: > indent 1 more Done. http://codereview.chromium.org/8364039/diff/6001/views/touchui/gesture_manage... views/touchui/gesture_manager.cc:63: source_widget->OnGestureEvent(converted); On 2011/10/24 17:41:09, rjkroege wrote: > you can modify the code in RootView to contain the dispatch loop. RootView is > analogous to WebViewImpl... Done temporarily, but would required changes when GR inherits from GM(posted few doubts in other comments for same), In inherited case GM would be same as old one. http://codereview.chromium.org/8364039/diff/6001/views/touchui/gesture_manage... views/touchui/gesture_manager.cc:68: // TODO(Gajen): Do we need to support Gesture across different RootViews On 2011/10/24 17:41:09, rjkroege wrote: > no. you'll see that the RootView owns a private GM instance. But, GM is singleton, so would be same across all RootViews. http://codereview.chromium.org/8364039/diff/6001/views/touchui/gesture_manager.h File views/touchui/gesture_manager.h (right): http://codereview.chromium.org/8364039/diff/6001/views/touchui/gesture_manage... views/touchui/gesture_manager.h:57: RootView* root_view_; On 2011/10/24 17:41:09, rjkroege wrote: > Why do we need this pointer when the view of interest is passed in to PTEFG? > This seems like a bad idea. GestureManager being singleton, can be in all possible RootViews, So, I guess, [source] view passed in PTEFG may be different in multitouch case. http://codereview.chromium.org/8364039/diff/6001/views/touchui/gesture_manage... views/touchui/gesture_manager.h:60: GestureRecognizer gesture_recognizer; On 2011/10/24 17:41:09, rjkroege wrote: > members need a trailing _ > Done > More general observation: why not make GestureRecognizer a subclass of > GestureManager and override PTEFG when appropriate? (BTW: because GR makes > different events than GM, you will need to make the flip-on of GR be optional as > touchui currently depends on the synthetic mouse events. Agreed, Do we need a GYP_DEFINE(say enable_gesture)??, this condition would instantiate GR in RootView's ctor. I have a doubt, as per Singleton template guidelines, class should be trivial and leaf only, so would it be right to inherit GR from GM. http://codereview.chromium.org/8364039/diff/6001/views/view.cc File views/view.cc (right): http://codereview.chromium.org/8364039/diff/6001/views/view.cc#newcode1944 views/view.cc:1944: // MousePressed. On 2011/10/24 17:41:09, rjkroege wrote: > bad indent Done. http://codereview.chromium.org/8364039/diff/6001/views/widget/root_view.cc File views/widget/root_view.cc (right): http://codereview.chromium.org/8364039/diff/6001/views/widget/root_view.cc#ne... views/widget/root_view.cc:388: if (gesture_manager_->ProcessTouchEventForGesture(e, this, status)) On 2011/10/24 17:41:09, rjkroege wrote: > to bring this code more closely int how the WebKit code works, I would think > that PTEFG should return an array of event instances that get dispatched here > after executing it. Then, do we need to change PTEFG signature in GM too..??? Currently added a separate method ForwardGestureEvents() in RootView, so that when GM inherits from GR this function wont change at all. http://codereview.chromium.org/8364039/diff/6001/views/widget/root_view.cc#ne... views/widget/root_view.cc:393: bool RootView::OnTouchEvent(const TouchEvent& event) { On 2011/10/24 17:41:09, rjkroege wrote: > I am reasonably strongly convinced that this is wrong: there is a > RootView::OnTouchEvent entry point above. It needs to be modified. My Bad..Now Removed this function as no longer required with addition of ForwardGestureEvents()
Hi, Robert. Waiting for your comments.
Progressing nicely. Am eager to see the rest. http://codereview.chromium.org/8364039/diff/14001/views/touchui/gesture_manag... File views/touchui/gesture_manager.cc (right): http://codereview.chromium.org/8364039/diff/14001/views/touchui/gesture_manag... views/touchui/gesture_manager.cc:31: if (root_view_ == NULL || root_view_ == source->GetWidget()->GetRootView()) { This code is not really right. It needs to look more like the original. This code also *must* be switchable. There is copious down-stream code that is relying on getting MouseEvent instances for user actions instead of gesture events. I would prefer seeing this enabled by a command line flag. And you'll need a unit test to verify that this all works as before until we flip the flag. http://codereview.chromium.org/8364039/diff/14001/views/touchui/gesture_manag... views/touchui/gesture_manager.cc:50: : root_view_(NULL) { Why do you have this code? I believe it is unnecessary. http://codereview.chromium.org/8364039/diff/14001/views/touchui/gesture_manag... File views/touchui/gesture_manager.h (right): http://codereview.chromium.org/8364039/diff/14001/views/touchui/gesture_manag... views/touchui/gesture_manager.h:54: // Note: Simultaneous Gestures happening across two different RootViews are in touchui, there is only one root view. This would need to be different for aura. The gesture manager might rightly belong in aura::Desktop http://codereview.chromium.org/8364039/diff/14001/views/widget/root_view.cc File views/widget/root_view.cc (right): http://codereview.chromium.org/8364039/diff/14001/views/widget/root_view.cc#n... views/widget/root_view.cc:447: // TODO(Gajen): Identify correct view and call respective view's Please don't leave this code out. :-)
Hi, Robert. How about testing do I need to add some test class similar to Webkit's InspectableGestureRecognizer. -Gajen http://codereview.chromium.org/8364039/diff/14001/views/touchui/gesture_manag... File views/touchui/gesture_manager.cc (right): http://codereview.chromium.org/8364039/diff/14001/views/touchui/gesture_manag... views/touchui/gesture_manager.cc:31: if (root_view_ == NULL || root_view_ == source->GetWidget()->GetRootView()) { On 2011/10/27 23:10:07, rjkroege wrote: > This code is not really right. It needs to look more like the original. Now GM is restored to original stage, and GR is derived from GM > > This code also *must* be switchable. There is copious down-stream code that is > relying on getting MouseEvent instances for user actions instead of gesture > events. I would prefer seeing this enabled by a command line flag. And you'll > need a unit test to verify that this all works as before until we flip the flag. Now, RootView's ctor instantiate GR if command line switch "enable-gesture" is set, else GM instance is used. So, I guess, now unit test is not required to verify the old behavior. http://codereview.chromium.org/8364039/diff/14001/views/touchui/gesture_manag... views/touchui/gesture_manager.cc:50: : root_view_(NULL) { On 2011/10/27 23:10:07, rjkroege wrote: > Why do you have this code? I believe it is unnecessary. Done. http://codereview.chromium.org/8364039/diff/14001/views/touchui/gesture_manag... File views/touchui/gesture_manager.h (right): http://codereview.chromium.org/8364039/diff/14001/views/touchui/gesture_manag... views/touchui/gesture_manager.h:54: // Note: Simultaneous Gestures happening across two different RootViews are On 2011/10/27 23:10:07, rjkroege wrote: > in touchui, there is only one root view. > > This would need to be different for aura. The gesture manager might rightly > belong in aura::Desktop Discarded completely as GR is now derived from GM http://codereview.chromium.org/8364039/diff/14001/views/widget/root_view.cc File views/widget/root_view.cc (right): http://codereview.chromium.org/8364039/diff/14001/views/widget/root_view.cc#n... views/widget/root_view.cc:447: // TODO(Gajen): Identify correct view and call respective view's On 2011/10/27 23:10:07, rjkroege wrote: > Please don't leave this code out. :-) Now made this public.
Slurping the tests out of webkit would be fine. Also, you need some tests to show that events are flowing correctly through the system. Please fix up root_view -- create a factory ideally to stay with your approach. However, I'd really like you to permit chaining the GRs together because the existing code in the touch build is usable via touch because of the delivery of synthetic mouse events. Flipping this flag as you've written it would make the UI unusable until every view that currently handles mouse events has an appropriate gesture event implement. I'd like it to be possible to add gesture event handling to views piecemeal which would require sending gesture events and then mouses on fail. This is very close to being done I think. Thanks very much. http://codereview.chromium.org/8364039/diff/21001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.h (right): http://codereview.chromium.org/8364039/diff/21001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:73: // Clears the GestureRecognizer to it's initial state. wrong its. though you probably copied it from me. :-) http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.cc File views/widget/root_view.cc (right): http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.cc#n... views/widget/root_view.cc:49: if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableGesture)) I don't think you want to do it this way. My preference would be to proceed as follows: interpose the gesture recognizer between the current gesture manager and the new gr so that touch events first enter the new gr, the gestures get dispatched, if no views want the new gestures, then let the existing gm create mouse events and dispatch that. This way, you don't need to have special command line switches. If you do want to proceed this way, I'd make an abstract factory that creates gesture_manager_ instances. http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.cc#n... views/widget/root_view.cc:433: status = gesture_handler_->ProcessGestureEvent(gesture_event); see comment about gesture_handler_ -- please rename. http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.h File views/widget/root_view.h (right): http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.h#ne... views/widget/root_view.h:188: View* gesture_handler_; I'd call this "gesture_handling_view_" to align the name with the comment. Or adjust the comment.
Slurping the tests out of webkit would be fine. Also, you need some tests to show that events are flowing correctly through the system. >>>>>> Ca have different CL for this..?? or needs to be done in this CL only. <<<<<< Please fix up root_view -- create a factory ideally to stay with your approach. However, I'd really like you to permit chaining the GRs together because the existing code in the touch build is usable via touch because of the delivery of synthetic mouse events. Flipping this flag as you've written it would make the UI unusable until every view that currently handles mouse events has an appropriate gesture event implement. >>>>>>>>> I, guess I made it possible by calling GestureManger::PTEFG() inside GestureRecognizer::PTEFG(), More details and approaches are covered in reply to your root_view.cc file comments. <<<<<<<<< I'd like it to be possible to add gesture event handling to views piecemeal which would require sending gesture events and then mouses on fail. >>>>> same as above.. <<<<<< This is very close to being done I think. Thanks very much. >> Most welcome. >> http://codereview.chromium.org/8364039/diff/21001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.h (right): http://codereview.chromium.org/8364039/diff/21001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:73: // Clears the GestureRecognizer to it's initial state. On 2011/11/01 18:42:22, rjkroege wrote: > wrong its. though you probably copied it from me. :-) Didn't Get, do u mean its Return type "virtual" should be omitted..?? http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.cc File views/widget/root_view.cc (right): http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.cc#n... views/widget/root_view.cc:49: if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableGesture)) On 2011/11/01 18:42:22, rjkroege wrote: > I don't think you want to do it this way. > > My preference would be to proceed as follows: interpose the gesture recognizer > between the current gesture manager and the new gr so that touch events first > enter the new gr, the gestures get dispatched, if no views want the new > gestures, then let the existing gm create mouse events and dispatch that. > Currently is it done same way in GR's PTEFG, where 1st gestures are recognized and passed to view's hierarchy by calling ForwardGestureEvents(), and if 1) gestures are not recognized, OR any of the recognized gesture is not handled by view' hierarchy, then mouse events are simulated by calling parent's PTEFG i.e GestureManger::PTEFG. > This way, you don't need to have special command line switches. > I guess switch would be better till we fully integrate all kinda Gestures (single/multi),at least provides us to switch back to old way. > If you do want to proceed this way, I'd make an abstract factory that creates > gesture_manager_ instances. I can go either of way. 1.Interposing GR: Below is my understanding of your suggestion: 1.1 GR won't be derived from GM, and GM would hold instance of GR. 1.2, inside GM's PTEFG() call GR's PTEFG()(or some renamed funct, as not being derived from GR), would recognize the Gestures and forwards them to RootView. 1.3 if no Gestures are recognized or handled by above process then simulate mouse events as per GM's PTEFG. 2.switch+ABS factory: Almost all is done except below: 2.1 Add a new file gesture_manager_factory.h declare interface for ABS factory. 2.2 Add new file gesture_manager_factory.cc, define ABS GestureManagerFactoy, DefaultGestureManagerFactory to cerate GM, and GestureRecognizerFactory to create GR Option 1 needs less code size than option 2, I'm open for any approach, please do let me know which one to choose and correction in above understanding. http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.h File views/widget/root_view.h (right): http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.h#ne... views/widget/root_view.h:188: View* gesture_handler_; On 2011/11/01 18:42:22, rjkroege wrote: > I'd call this "gesture_handling_view_" to align the name with the comment. Or > adjust the comment. Agreed, will incorporate in.
HI, Robert. Waiting for your comments..
On 2011/11/02 12:59:35, Gajen wrote: > Slurping the tests out of webkit would be fine. Also, you need some tests to > show that events are flowing correctly through the system. > >>>>>> > Ca have different CL for this..?? or needs to be done in this CL only. It can be a different CL.a > <<<<<< > > Please fix up root_view -- create a factory ideally to stay with your approach. > However, I'd really like you to permit chaining the GRs together because the > existing code in the touch build is usable via touch because of the delivery of > synthetic mouse events. Flipping this flag as you've written it would make the > UI unusable until every view that currently handles mouse events has an > appropriate gesture event implement. > > >>>>>>>>> > I, guess I made it possible by calling GestureManger::PTEFG() inside > GestureRecognizer::PTEFG(), > More details and approaches are covered in reply to your root_view.cc file > comments. OK. > <<<<<<<<< > > I'd like it to be possible to add gesture event handling to views piecemeal > which would require sending gesture events and then mouses on fail. > >>>>> > same as above.. > <<<<<< > > This is very close to being done I think. Thanks very much. > >> > Most welcome. > >> > > http://codereview.chromium.org/8364039/diff/21001/views/touchui/gesture_recog... > File views/touchui/gesture_recognizer.h (right): > > http://codereview.chromium.org/8364039/diff/21001/views/touchui/gesture_recog... > views/touchui/gesture_recognizer.h:73: // Clears the GestureRecognizer to it's > initial state. > On 2011/11/01 18:42:22, rjkroege wrote: > > wrong its. though you probably copied it from me. :-) > > Didn't Get, do u mean its Return type "virtual" should be omitted..?? > > http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.cc > File views/widget/root_view.cc (right): > > http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.cc#n... > views/widget/root_view.cc:49: if > (CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableGesture)) > On 2011/11/01 18:42:22, rjkroege wrote: > > I don't think you want to do it this way. > > > > My preference would be to proceed as follows: interpose the gesture recognizer > > between the current gesture manager and the new gr so that touch events first > > enter the new gr, the gestures get dispatched, if no views want the new > > gestures, then let the existing gm create mouse events and dispatch that. > > > Currently is it done same way in GR's PTEFG, where 1st gestures are recognized > and passed to view's hierarchy by calling ForwardGestureEvents(), and if 1) > gestures are not recognized, OR any of the recognized gesture is not handled by > view' hierarchy, then mouse events are simulated by calling parent's PTEFG i.e > GestureManger::PTEFG. > > > > This way, you don't need to have special command line switches. > > > I guess switch would be better till we fully integrate all kinda Gestures > (single/multi),at least provides us to switch back to old way. > > > If you do want to proceed this way, I'd make an abstract factory that creates > > gesture_manager_ instances. > > I can go either of way. > 1.Interposing GR: Below is my understanding of your suggestion: > 1.1 GR won't be derived from GM, and GM would hold instance of GR. > 1.2, inside GM's PTEFG() call GR's PTEFG()(or some renamed funct, as not > being derived from GR), would recognize the Gestures and forwards them to > RootView. > 1.3 if no Gestures are recognized or handled by above process then simulate > mouse events as per GM's PTEFG. > > > 2.switch+ABS factory: Almost all is done except below: > 2.1 Add a new file gesture_manager_factory.h declare interface for ABS > factory. > 2.2 Add new file gesture_manager_factory.cc, define > ABS GestureManagerFactoy, DefaultGestureManagerFactory to cerate GM, and > GestureRecognizerFactory to create GR > > Option 1 needs less code size than option 2, I'm open for any approach, please > do let me know which one to choose and correction in above understanding. > > http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.h > File views/widget/root_view.h (right): > > http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.h#ne... > views/widget/root_view.h:188: View* gesture_handler_; > On 2011/11/01 18:42:22, rjkroege wrote: > > I'd call this "gesture_handling_view_" to align the name with the comment. Or > > adjust the comment. > > Agreed, will incorporate in.
More comments... http://codereview.chromium.org/8364039/diff/21001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.h (right): http://codereview.chromium.org/8364039/diff/21001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:73: // Clears the GestureRecognizer to it's initial state. On 2011/11/02 12:59:35, Gajen wrote: > On 2011/11/01 18:42:22, rjkroege wrote: > > wrong its. though you probably copied it from me. :-) > > Didn't Get, do u mean its Return type "virtual" should be omitted..?? the word "it's" is incorrect grammatically. The word you want in the comment is "its". This is a nit. :-) http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.cc File views/widget/root_view.cc (right): http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.cc#n... views/widget/root_view.cc:49: if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableGesture)) On 2011/11/02 12:59:35, Gajen wrote: > On 2011/11/01 18:42:22, rjkroege wrote: > > I don't think you want to do it this way. > > > > My preference would be to proceed as follows: interpose the gesture recognizer > > between the current gesture manager and the new gr so that touch events first > > enter the new gr, the gestures get dispatched, if no views want the new > > gestures, then let the existing gm create mouse events and dispatch that. > > > Currently is it done same way in GR's PTEFG, where 1st gestures are recognized > and passed to view's hierarchy by calling ForwardGestureEvents(), and if 1) > gestures are not recognized, OR any of the recognized gesture is not handled by > view' hierarchy, then mouse events are simulated by calling parent's PTEFG i.e > GestureManger::PTEFG. > > > > This way, you don't need to have special command line switches. > > > I guess switch would be better till we fully integrate all kinda Gestures > (single/multi),at least provides us to switch back to old way. > > > If you do want to proceed this way, I'd make an abstract factory that creates > > gesture_manager_ instances. > > I can go either of way. > 1.Interposing GR: Below is my understanding of your suggestion: > 1.1 GR won't be derived from GM, and GM would hold instance of GR. > 1.2, inside GM's PTEFG() call GR's PTEFG()(or some renamed funct, as not > being derived from GR), would recognize the Gestures and forwards them to > RootView. > 1.3 if no Gestures are recognized or handled by above process then simulate > mouse events as per GM's PTEFG. > > > 2.switch+ABS factory: Almost all is done except below: > 2.1 Add a new file gesture_manager_factory.h declare interface for ABS > factory. > 2.2 Add new file gesture_manager_factory.cc, define > ABS GestureManagerFactoy, DefaultGestureManagerFactory to cerate GM, and > GestureRecognizerFactory to create GR > > Option 1 needs less code size than option 2, I'm open for any approach, please > do let me know which one to choose and correction in above understanding. Currently, we have the following: RootView::OnTouchEvent gets touch events. All of them are dropped into GM::PTEFG. It dispatches synthetic mouse events into the RootView directly. As evolved in the WebKit version of the GestureRecognizer, this is a bad design because it makes the GestureManager dependent on RootView. The GestureManager does not need to depend on RootView. It only needs to know about events. Events are separate from views thanks to the ongoing aura effort. Instead, as with the WebKit GestureRecognizer, the Chrome GestureManager could return an array of events to the RootView. The RootView could then dispatch these events. At present, we need to convert touch events into mouse events so that existing views code continues to work properly. I want gesture delivery and synthetic mouse event delivery to co-exist so that we can incrementally develop new views components that respond to gestures while retaining touch navigation of the chrome touchui build. This means that we need code that can deliver both gesture events *and* synthetic mouses at runtime. We want to be able to write views controls that respond to mouse or gesture events and treats them differently. This means that we need to code up gesture recognition so that we don't generate synthetic mouse events *if* gesture events have been handled. I think that we can accomplish as follows: 1. RootView::OnTouchEvent looks mostly like now. Except that it uses a GestureRecognizer instance. This returns an array of gestures. Which OTE dispatches to RootView::OnGestureEvent. 2. RootView::OnGestureEvent calls gesture_manager_ the way that OTE currently does *if* there is no consumer of the event. This will require assorted TODOs to indicate that it will be removed. 3. GestureRecognizer does not need to be subclass of GestureManager. Instead, we simply transition to the newer/better GR.
Hi, Robert. Added Gesture tests in view_unittest and found them working fine. http://codereview.chromium.org/8364039/diff/21001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.h (right): http://codereview.chromium.org/8364039/diff/21001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:73: // Clears the GestureRecognizer to it's initial state. On 2011/11/04 17:19:28, rjkroege wrote: > On 2011/11/02 12:59:35, Gajen wrote: > > On 2011/11/01 18:42:22, rjkroege wrote: > > > wrong its. though you probably copied it from me. :-) > > > > Didn't Get, do u mean its Return type "virtual" should be omitted..?? > > the word "it's" is incorrect grammatically. The word you want in the comment is > "its". This is a nit. :-) Done. http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.cc File views/widget/root_view.cc (right): http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.cc#n... views/widget/root_view.cc:49: if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableGesture)) On 2011/11/04 17:19:28, rjkroege wrote: > On 2011/11/02 12:59:35, Gajen wrote: > > On 2011/11/01 18:42:22, rjkroege wrote: > > > I don't think you want to do it this way. > > > > > > My preference would be to proceed as follows: interpose the gesture > recognizer > > > between the current gesture manager and the new gr so that touch events > first > > > enter the new gr, the gestures get dispatched, if no views want the new > > > gestures, then let the existing gm create mouse events and dispatch that. > > > > > Currently is it done same way in GR's PTEFG, where 1st gestures are > recognized > > and passed to view's hierarchy by calling ForwardGestureEvents(), and if 1) > > gestures are not recognized, OR any of the recognized gesture is not handled > by > > view' hierarchy, then mouse events are simulated by calling parent's PTEFG i.e > > GestureManger::PTEFG. > > > > > > > This way, you don't need to have special command line switches. > > > > > I guess switch would be better till we fully integrate all kinda Gestures > > (single/multi),at least provides us to switch back to old way. > > > > > If you do want to proceed this way, I'd make an abstract factory that > creates > > > gesture_manager_ instances. > > > > I can go either of way. > > 1.Interposing GR: Below is my understanding of your suggestion: > > 1.1 GR won't be derived from GM, and GM would hold instance of GR. > > 1.2, inside GM's PTEFG() call GR's PTEFG()(or some renamed funct, as not > > being derived from GR), would recognize the Gestures and forwards them to > > RootView. > > 1.3 if no Gestures are recognized or handled by above process then > simulate > > mouse events as per GM's PTEFG. > > > > > > 2.switch+ABS factory: Almost all is done except below: > > 2.1 Add a new file gesture_manager_factory.h declare interface for ABS > > factory. > > 2.2 Add new file gesture_manager_factory.cc, define > > ABS GestureManagerFactoy, DefaultGestureManagerFactory to cerate GM, > and > > GestureRecognizerFactory to create GR > > > > Option 1 needs less code size than option 2, I'm open for any approach, please > > do let me know which one to choose and correction in above understanding. > > Currently, we have the following: RootView::OnTouchEvent gets touch events. All > of them are dropped into GM::PTEFG. It dispatches synthetic mouse events into > the RootView directly. > > As evolved in the WebKit version of the GestureRecognizer, this is a bad design > because it makes the GestureManager dependent on RootView. The GestureManager > does not need to depend on RootView. It only needs to know about events. Events > are separate from views thanks to the ongoing aura effort. Instead, as with the > WebKit GestureRecognizer, the Chrome GestureManager could return an array of > events to the RootView. The RootView could then dispatch these events. > > At present, we need to convert touch events into mouse events so that existing > views code continues to work properly. I want gesture delivery and synthetic > mouse event delivery to co-exist so that we can incrementally develop new views > components that respond to gestures while retaining touch navigation of the > chrome touchui build. This means that we need code that can deliver both gesture > events *and* synthetic mouses at runtime. > > We want to be able to write views controls that respond to mouse or gesture > events and treats them differently. This means that we need to code up gesture > recognition so that we don't generate synthetic mouse events *if* gesture events > have been handled. > > I think that we can accomplish as follows: > > 1. RootView::OnTouchEvent looks mostly like now. Except that it uses a > GestureRecognizer instance. This returns an array of gestures. Which OTE > dispatches to RootView::OnGestureEvent. > > 2. RootView::OnGestureEvent calls gesture_manager_ the way that OTE currently > does *if* there is no consumer of the event. This will require assorted TODOs to > indicate that it will be removed. > > 3. GestureRecognizer does not need to be subclass of GestureManager. Instead, we > simply transition to the newer/better GR. > Look promising and Great. Can we push current implementation and then take up this Design/Refactoring activity in next immediate high priority CL, As I have Verified current approach and tested GR mouse conversion and Gesture events(Tap, Tap down) through view_unittest Added a TODO for same. > > > http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.cc#n... views/widget/root_view.cc:433: status = gesture_handler_->ProcessGestureEvent(gesture_event); On 2011/11/01 18:42:22, rjkroege wrote: > see comment about gesture_handler_ -- please rename. Done. http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.h File views/widget/root_view.h (right): http://codereview.chromium.org/8364039/diff/21001/views/widget/root_view.h#ne... views/widget/root_view.h:188: View* gesture_handler_; On 2011/11/01 18:42:22, rjkroege wrote: > I'd call this "gesture_handling_view_" to align the name with the comment. Or > adjust the comment. Done.
Hi Gajen, Sorry for the drive-by -- I've just got a few small comments. Ian http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.h (right): http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:29: // A GestureEvent wrapper for automatic memory management. You should use linked_ptr<GestureEvent> instead of PassGestureEvent. http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:132: bool IsClickOrScroll(const TouchEvent& event, Gestures* gestures); I think this should be InClickOrScroll to match InScroll. I find the current name very misleading since the function doesn't actually answer the question IsClickOrScroll (the boolean return value is completely unrelated). http://codereview.chromium.org/8364039/diff/30001/views/widget/root_view.cc File views/widget/root_view.cc (right): http://codereview.chromium.org/8364039/diff/30001/views/widget/root_view.cc#n... views/widget/root_view.cc:12: #include "chrome/common/chrome_switches.h" views/ is not allowed to depend on chrome/, so you can't use chrome_switches. See views/examples/examples_main.cc:181. You might want to declare kEnableGesture in the switches namespace here in this file (or possibly in an unnamed namespace).
http://codereview.chromium.org/8364039/diff/30001/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/8364039/diff/30001/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:1248: const char kEnableGesture[] = "enable-gesture"; these need to be in alphabetical order. like above 1242 http://codereview.chromium.org/8364039/diff/30001/chrome/common/chrome_switch... File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/8364039/diff/30001/chrome/common/chrome_switch... chrome/common/chrome_switches.h:362: extern const char kKeepMouseCursor[]; touchui always implies tookit views so replicating this is unnecessary. http://codereview.chromium.org/8364039/diff/30001/ui/base/events.h File ui/base/events.h (right): http://codereview.chromium.org/8364039/diff/30001/ui/base/events.h#newcode43 ui/base/events.h:43: ET_GESTURE_TAP, an alternative implementation of my desire for flag-free backwards compatible event distribution would be to have handlers for TAP & TAP_DOWN on RootView that by default dispatch mouse gestures. http://codereview.chromium.org/8364039/diff/30001/views/events/event.h File views/events/event.h (right): http://codereview.chromium.org/8364039/diff/30001/views/events/event.h#newcode69 views/events/event.h:69: void set_native_event(const NativeEvent& event) { native_event_ = event; } Per other discussion, am not convinced this is necessary. http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.cc (right): http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.cc:35: GestureRecognizer::PassGestureEvent::PassGestureEvent( This seems unnecessary. http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.cc:112: if (reinterpret_cast<views::internal::RootView* >( this code can be moved into the RootView. http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.h (right): http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:14: #include "views/view.h" I would really prefer you to remove this dependency before submitting this CL. http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:88: Gestures* StartGestureRecognitionProcess(const TouchEvent& event, you've even done most of the work. :-) http://codereview.chromium.org/8364039/diff/30001/views/view_unittest.cc File views/view_unittest.cc (right): http://codereview.chromium.org/8364039/diff/30001/views/view_unittest.cc#newc... views/view_unittest.cc:43: #include <X11/Xlib.h> I think that the need for this here is a sign of a problem. Can you explain why this is necessary? http://codereview.chromium.org/8364039/diff/30001/views/view_unittest.cc#newc... views/view_unittest.cc:666: // |v3| completely obscures |v2|, but all the touch events on |v3| should This is testing gesture events? Can you updated the comment. http://codereview.chromium.org/8364039/diff/30001/views/view_unittest.cc#newc... views/view_unittest.cc:684: memset(&native_event, 0, sizeof(native_event)); this should not be necessary. You can construct the event directly. The native event ought not to be necessary. http://codereview.chromium.org/8364039/diff/30001/views/view_unittest.cc#newc... views/view_unittest.cc:836: // Check mouse conversions shouldn't take place. mouse conversions are in-use in actual code. Please verify via tests that they continue to function properly.
Vollick Thanks for comments will incorporate your changes soon. Robert. Planning to implement view independent GR in this CL only. Need some discussion on native_event_ issue in views_unittest, Please checkout the comments for same and would appreciate your feedback. -Gajen http://codereview.chromium.org/8364039/diff/30001/chrome/common/chrome_switch... File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/8364039/diff/30001/chrome/common/chrome_switch... chrome/common/chrome_switches.h:362: extern const char kKeepMouseCursor[]; On 2011/11/09 02:43:30, rjkroege wrote: > touchui always implies tookit views so replicating this is unnecessary. Plan to remove this dependence by implementing View independent GR Design http://codereview.chromium.org/8364039/diff/30001/views/view_unittest.cc File views/view_unittest.cc (right): http://codereview.chromium.org/8364039/diff/30001/views/view_unittest.cc#newc... views/view_unittest.cc:43: #include <X11/Xlib.h> On 2011/11/09 02:43:30, rjkroege wrote: > I think that the need for this here is a sign of a problem. Can you explain why > this is necessary? This was required to instantiate base::NativeEvent (i.e XEvent*) for setting native_event_ of Event in views/event/event.h. If we get some way to set native_event_ this wont be required, need further discussion on this, captured some thoughts on below similar comment. http://codereview.chromium.org/8364039/diff/30001/views/view_unittest.cc#newc... views/view_unittest.cc:684: memset(&native_event, 0, sizeof(native_event)); On 2011/11/09 02:43:30, rjkroege wrote: > this should not be necessary. You can construct the event directly. The native > event ought not to be necessary. Since, TouchEvent generated this way would set to some undefined pointer value to native_event_(base::NativeEvent(i.e,XEvent*)) in base class Event. Which in-turn Segfaults, During Conversion from TouchEvent to MouseEvent in GestureManager::PTEFG(), during constructor calls MouseEvent::MouseEvent(const TouchEvent& touch) LocatedEvent::LocatedEvent(const NativeEvent& native_event) EventFlagsFromNative() So, to invoke Real GestureManager instead of Mock one we need some way to set the native_event_. A better solution to avoid inclusion of "X11" header and set_native_event() in event.h, would be to set native_event_ directly in ctor of TouchEvent, but this needs native_event_ be a protected member instead of private. Please let me know of any other solution.
Hi, Robert. Incorporated all the design changes as discussed earlier. Note: Applied patch from [1] http://codereview.chromium.org/8521001/ and found all views_unittest and Touch to Mouse conversion and propagation OK. This implies that every thing is fine and should work as before, so can be committed if you feel so. Validating Touch/Gesture to Mouse conversion and propagation in views_unittest would be dependent on [1] and can be incorporated once [1] is committed. http://codereview.chromium.org/8364039/diff/30001/chrome/common/chrome_switch... File chrome/common/chrome_switches.cc (right): http://codereview.chromium.org/8364039/diff/30001/chrome/common/chrome_switch... chrome/common/chrome_switches.cc:1248: const char kEnableGesture[] = "enable-gesture"; On 2011/11/09 02:43:30, rjkroege wrote: > these need to be in alphabetical order. like above 1242 Deprecated, as implemented new GR design http://codereview.chromium.org/8364039/diff/30001/chrome/common/chrome_switch... File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/8364039/diff/30001/chrome/common/chrome_switch... chrome/common/chrome_switches.h:362: extern const char kKeepMouseCursor[]; On 2011/11/09 18:14:43, Gajen wrote: > On 2011/11/09 02:43:30, rjkroege wrote: > > touchui always implies tookit views so replicating this is unnecessary. > > Plan to remove this dependence by implementing View independent GR Design Done. http://codereview.chromium.org/8364039/diff/30001/ui/base/events.h File ui/base/events.h (right): http://codereview.chromium.org/8364039/diff/30001/ui/base/events.h#newcode43 ui/base/events.h:43: ET_GESTURE_TAP, On 2011/11/09 02:43:30, rjkroege wrote: > an alternative implementation of my desire for flag-free backwards compatible > event distribution would be to have handlers for TAP & TAP_DOWN on RootView that > by default dispatch mouse gestures. I guess this is not needed now as New design implementation is done. http://codereview.chromium.org/8364039/diff/30001/views/events/event.h File views/events/event.h (right): http://codereview.chromium.org/8364039/diff/30001/views/events/event.h#newcode69 views/events/event.h:69: void set_native_event(const NativeEvent& event) { native_event_ = event; } On 2011/11/09 02:43:30, rjkroege wrote: > Per other discussion, am not convinced this is necessary. Deprecated in this patch, now issue is moved to http://codereview.chromium.org/8521001/. http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.cc (right): http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.cc:35: GestureRecognizer::PassGestureEvent::PassGestureEvent( On 2011/11/09 02:43:30, rjkroege wrote: > This seems unnecessary. Done. http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.cc:112: if (reinterpret_cast<views::internal::RootView* >( On 2011/11/09 02:43:30, rjkroege wrote: > this code can be moved into the RootView. Done. http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.h (right): http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:14: #include "views/view.h" On 2011/11/09 02:43:30, rjkroege wrote: > I would really prefer you to remove this dependency before submitting this CL. Done. New design implementation no longer required this. http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:29: // A GestureEvent wrapper for automatic memory management. On 2011/11/08 20:07:54, vollick wrote: > You should use linked_ptr<GestureEvent> instead of PassGestureEvent. Done. http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:88: Gestures* StartGestureRecognitionProcess(const TouchEvent& event, On 2011/11/09 02:43:30, rjkroege wrote: > you've even done most of the work. :-) Done. http://codereview.chromium.org/8364039/diff/30001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:132: bool IsClickOrScroll(const TouchEvent& event, Gestures* gestures); On 2011/11/08 20:07:54, vollick wrote: > I think this should be InClickOrScroll to match InScroll. I find the current > name very misleading since the function doesn't actually answer the question > IsClickOrScroll (the boolean return value is completely unrelated). Done. http://codereview.chromium.org/8364039/diff/30001/views/view_unittest.cc File views/view_unittest.cc (right): http://codereview.chromium.org/8364039/diff/30001/views/view_unittest.cc#newc... views/view_unittest.cc:43: #include <X11/Xlib.h> On 2011/11/09 02:43:30, rjkroege wrote: > I think that the need for this here is a sign of a problem. Can you explain why > this is necessary? Deprecated, currently MockGesture is used for GM. A real GM can only be used if we are able to properly convert Touch to Mouse event and propagation. A separate CL http://codereview.chromium.org/8521001/ is submitted for same. http://codereview.chromium.org/8364039/diff/30001/views/view_unittest.cc#newc... views/view_unittest.cc:666: // |v3| completely obscures |v2|, but all the touch events on |v3| should On 2011/11/09 02:43:30, rjkroege wrote: > This is testing gesture events? Can you updated the comment. Done. http://codereview.chromium.org/8364039/diff/30001/views/view_unittest.cc#newc... views/view_unittest.cc:684: memset(&native_event, 0, sizeof(native_event)); On 2011/11/09 02:43:30, rjkroege wrote: > this should not be necessary. You can construct the event directly. The native > event ought not to be necessary. Deprecated, as new CL http://codereview.chromium.org/8521001/ would decide this. http://codereview.chromium.org/8364039/diff/30001/views/view_unittest.cc#newc... views/view_unittest.cc:836: // Check mouse conversions shouldn't take place. On 2011/11/09 02:43:30, rjkroege wrote: > mouse conversions are in-use in actual code. Please verify via tests that they > continue to function properly. Verified touch to mouse conversion and propagation by applying new CL http://codereview.chromium.org/8521001/, once this CL is approved will change accordingly. http://codereview.chromium.org/8364039/diff/30001/views/widget/root_view.cc File views/widget/root_view.cc (right): http://codereview.chromium.org/8364039/diff/30001/views/widget/root_view.cc#n... views/widget/root_view.cc:12: #include "chrome/common/chrome_switches.h" On 2011/11/08 20:07:54, vollick wrote: > views/ is not allowed to depend on chrome/, so you can't use chrome_switches. > See views/examples/examples_main.cc:181. You might want to declare > kEnableGesture in the switches namespace here in this file (or possibly in an > unnamed namespace). Deprecated, as no longer required in new Design.
LGTM [with nits] for approach and event flow. http://codereview.chromium.org/8364039/diff/37001/chrome/common/chrome_switch... File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/8364039/diff/37001/chrome/common/chrome_switch... chrome/common/chrome_switches.h:365: #endif These are unnecessary. You need to remerge I think.
Incorporated Rob's changes. Sad and Sky, Can you both have a look into this please..??? as Vollick's patch http://codereview.chromium.org/8508024/ is dependent on this too. http://codereview.chromium.org/8364039/diff/37001/chrome/common/chrome_switch... File chrome/common/chrome_switches.h (right): http://codereview.chromium.org/8364039/diff/37001/chrome/common/chrome_switch... chrome/common/chrome_switches.h:365: #endif On 2011/11/14 20:51:36, rjkroege wrote: > These are unnecessary. You need to remerge I think. Done.
I don't know enough about underlying gesture processing to know if this is right. I'm hoping Rob or Sadrul does. It would be good if cpu could review this too as we'll likely want touch events for windows too. http://codereview.chromium.org/8364039/diff/44001/views/events/event.h File views/events/event.h (right): http://codereview.chromium.org/8364039/diff/44001/views/events/event.h#newcod... views/events/event.h:447: GestureEvent(ui::EventType type, int x, int y, int flags, When you wrap, each param goes on its own line. http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.cc (right): http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.cc:76: GestureTransitionFunctionMap::iterator it = edge_functions_.find( Is there a reason we need this level of complexity rather than a switch statement? http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.cc:97: GestureRecognizer::TouchState GestureRecognizer::TouchEventTypeToTouchState( // static http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.h (right): http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:23: // A GestureRecognizer recognizes gestures frome touch sequences. 'frome' -> from http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:24: // remove this line. http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:45: typedef std::vector< linked_ptr<GestureEvent> > Gestures; Do these need to be public? Also, we generally don't put a space between the first < and content. So, this should be: std::vector<linked_ptr<GestureEvent> > Gestures. http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:117: typedef std::map<int, GestureTransitionFunction> GestureTransitionFunctionMap; typedefs are generally first in a section. http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:120: gfx::Point first_touch_position_; Add descriptions for your fields. http://codereview.chromium.org/8364039/diff/44001/views/view.cc File views/view.cc (right): http://codereview.chromium.org/8364039/diff/44001/views/view.cc#newcode779 views/view.cc:779: DVLOG(1) << "visited the OnGestureEvent"; This is too spammy. Remove it. http://codereview.chromium.org/8364039/diff/44001/views/view_unittest.cc File views/view_unittest.cc (right): http://codereview.chromium.org/8364039/diff/44001/views/view_unittest.cc#newc... views/view_unittest.cc:281: class TestGestureRecognizer: public GestureRecognizer { 'r: ' -> 'r : ' http://codereview.chromium.org/8364039/diff/44001/views/view_unittest.cc#newc... views/view_unittest.cc:288: Gestures* ProcessTouchEventForGesture(const TouchEvent& event, virtual http://codereview.chromium.org/8364039/diff/44001/views/view_unittest.cc#newc... views/view_unittest.cc:290: TestGestureRecognizer(); constructor first http://codereview.chromium.org/8364039/diff/44001/views/view_unittest.cc#newc... views/view_unittest.cc:317: }; DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/8364039/diff/44001/views/view_unittest.cc#newc... views/view_unittest.cc:327: virtual ui::GestureStatus OnGestureEvent(const GestureEvent& event) OVERRIDE; DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/8364039/diff/44001/views/views.gyp File views/views.gyp (right): http://codereview.chromium.org/8364039/diff/44001/views/views.gyp#newcode43 views/views.gyp:43: '../chrome/chrome.gyp:common_constants', This isn't right. Why do we need this?
Thanks To sky for comments. Sad/Cpu, your comments Please..!!! http://codereview.chromium.org/8364039/diff/44001/views/events/event.h File views/events/event.h (right): http://codereview.chromium.org/8364039/diff/44001/views/events/event.h#newcod... views/events/event.h:447: GestureEvent(ui::EventType type, int x, int y, int flags, On 2011/11/15 16:54:44, sky wrote: > When you wrap, each param goes on its own line. Done. http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.cc (right): http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.cc:76: GestureTransitionFunctionMap::iterator it = edge_functions_.find( On 2011/11/15 16:54:44, sky wrote: > Is there a reason we need this level of complexity rather than a switch > statement? log n to O(1) can be achieved in switch case, but theoretically there could be 2^32 such switch states, So, I guess, its OK to have this. http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.cc:97: GestureRecognizer::TouchState GestureRecognizer::TouchEventTypeToTouchState( On 2011/11/15 16:54:44, sky wrote: > // static Done. http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.h (right): http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:23: // A GestureRecognizer recognizes gestures frome touch sequences. On 2011/11/15 16:54:44, sky wrote: > 'frome' -> from Done. http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:24: // On 2011/11/15 16:54:44, sky wrote: > remove this line. Done. http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:45: typedef std::vector< linked_ptr<GestureEvent> > Gestures; On 2011/11/15 16:54:44, sky wrote: > Do these need to be public? Also, we generally don't put a space between the > first < and content. So, this should be: > std::vector<linked_ptr<GestureEvent> > Gestures. Done. http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:117: typedef std::map<int, GestureTransitionFunction> GestureTransitionFunctionMap; On 2011/11/15 16:54:44, sky wrote: > typedefs are generally first in a section. Done. http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:120: gfx::Point first_touch_position_; On 2011/11/15 16:54:44, sky wrote: > Add descriptions for your fields. Done. http://codereview.chromium.org/8364039/diff/44001/views/view.cc File views/view.cc (right): http://codereview.chromium.org/8364039/diff/44001/views/view.cc#newcode779 views/view.cc:779: DVLOG(1) << "visited the OnGestureEvent"; On 2011/11/15 16:54:44, sky wrote: > This is too spammy. Remove it. Done. http://codereview.chromium.org/8364039/diff/44001/views/view_unittest.cc File views/view_unittest.cc (right): http://codereview.chromium.org/8364039/diff/44001/views/view_unittest.cc#newc... views/view_unittest.cc:281: class TestGestureRecognizer: public GestureRecognizer { On 2011/11/15 16:54:44, sky wrote: > 'r: ' -> 'r : ' Done. http://codereview.chromium.org/8364039/diff/44001/views/view_unittest.cc#newc... views/view_unittest.cc:288: Gestures* ProcessTouchEventForGesture(const TouchEvent& event, On 2011/11/15 16:54:44, sky wrote: > virtual Done. http://codereview.chromium.org/8364039/diff/44001/views/view_unittest.cc#newc... views/view_unittest.cc:290: TestGestureRecognizer(); On 2011/11/15 16:54:44, sky wrote: > constructor first Done. http://codereview.chromium.org/8364039/diff/44001/views/view_unittest.cc#newc... views/view_unittest.cc:317: }; On 2011/11/15 16:54:44, sky wrote: > DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/8364039/diff/44001/views/view_unittest.cc#newc... views/view_unittest.cc:327: virtual ui::GestureStatus OnGestureEvent(const GestureEvent& event) OVERRIDE; On 2011/11/15 16:54:44, sky wrote: > DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/8364039/diff/44001/views/views.gyp File views/views.gyp (right): http://codereview.chromium.org/8364039/diff/44001/views/views.gyp#newcode43 views/views.gyp:43: '../chrome/chrome.gyp:common_constants', On 2011/11/15 16:54:44, sky wrote: > This isn't right. Why do we need this? Done. Not needed after Removal of chrome_switches.
http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.cc (right): http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.cc:76: GestureTransitionFunctionMap::iterator it = edge_functions_.find( On 2011/11/16 15:08:41, Gajen wrote: > On 2011/11/15 16:54:44, sky wrote: > > Is there a reason we need this level of complexity rather than a switch > > statement? > > log n to O(1) can be achieved in switch case, but theoretically there could be > 2^32 such switch states, So, I guess, its OK to have this. Theoretically we could, but will we? The current code has 8, do you envision needing a lot more to warrant this level of complexity. I like the current approach if you need to dynamically support various cases, but AFAICT that isn't the case. As such, a switch statement is a lot simpler and easier to follow.
On 2011/11/16 16:27:33, sky wrote: > http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... > views/touchui/gesture_recognizer.cc:76: GestureTransitionFunctionMap::iterator > it = edge_functions_.find( > On 2011/11/16 15:08:41, Gajen wrote: > > On 2011/11/15 16:54:44, sky wrote: > > > Is there a reason we need this level of complexity rather than a switch > > > statement? > > > > log n to O(1) can be achieved in switch case, but theoretically there could be > > 2^32 such switch states, So, I guess, its OK to have this. > > Theoretically we could, but will we? The current code has 8, do you envision > needing a lot more to warrant this level of complexity. I like the current > approach if you need to dynamically support various cases, but AFAICT that isn't > the case. As such, a switch statement is a lot simpler and easier to follow. I guess to support all basic gestures(pan/flick/rotate/pinch/doubletab/triple/tap(single_touch+multi-touch)) we may reach easily >32 switch cases. Robert may provide more correct figure..?? Based on Assembly analysis(data below) for switch Vs std::Map::find(), switch case comparisons are more than find(), but find() has function call cost. In my opinion, the performance may be same but again it would vary gesture to gesture, which need more deeper analysis. Please let me know what Path to chose. ////////////////////////////////////////////////////////////////////// Compiler = gcc (Ubuntu 4.4.3-4ubuntu5) 4.4.3 cpu = Intel(R) Core(TM)2 Duo CPU E7200 @ 2.53GHz /////////////////////////////////////////////////////////////////////// Comparision Instructions Switch cost : O(n) JUMP map::find() : log n PUSHs+POPs+CALL + RET //////////////////////////////////////////////////////////////////////////// Sample program: int main() { int i; switch(i) { case 111: cout<<" case 111:"<<endl; break; case 5: cout<<" case 5:"<<endl; break; case 25678: cout<<" case 25678:"<<endl; break; case 39999: cout<<" case 39999:"<<endl; break; } } ////////////////////////////////////////////////////////////////////// Assembly out put: //////////////////////////////////////////////////////////////////// main: .LFB957: .cfi_startproc .cfi_personality 0x0,__gxx_personality_v0 pushl %ebp .cfi_def_cfa_offset 8 movl %esp, %ebp .cfi_offset 5, -8 .cfi_def_cfa_register 5 andl $-16, %esp subl $32, %esp movl 28(%esp), %eax cmpl $111, %eax je .L4 cmpl $111, %eax jg .L7 cmpl $5, %eax je .L3 jmp .L2 .L7: cmpl $25678, %eax je .L5 cmpl $39999, %eax je .L6 jmp .L2 .L3: movl $.LC0, 4(%esp) movl $_ZSt4cout, (%esp) call _ZStlsISt11char_traitsIcEERSt13basic_ostreamIcT_ES5_PKc movl $_ZSt4endlIcSt11char_traitsIcEERSt13basic_ostreamIT_T0_ES6_, 4(%esp) movl %eax, (%esp) call _ZNSolsEPFRSoS_E jmp .L2 .L4: movl $.LC1, 4(%esp) movl $_ZSt4cout, (%esp) call _ZStlsISt11char_traitsIcEERSt13basic_ostreamIcT_ES5_PKc movl $_ZSt4endlIcSt11char_traitsIcEERSt13basic_ostreamIT_T0_ES6_, 4(%esp) movl %eax, (%esp) call _ZNSolsEPFRSoS_E jmp .L2 .L5: movl $.LC2, 4(%esp) movl $_ZSt4cout, (%esp) call _ZStlsISt11char_traitsIcEERSt13basic_ostreamIcT_ES5_PKc movl $_ZSt4endlIcSt11char_traitsIcEERSt13basic_ostreamIT_T0_ES6_, 4(%esp) movl %eax, (%esp) call _ZNSolsEPFRSoS_E jmp .L2 .L6: movl $.LC3, 4(%esp) movl $_ZSt4cout, (%esp) call _ZStlsISt11char_traitsIcEERSt13basic_ostreamIcT_ES5_PKc movl $_ZSt4endlIcSt11char_traitsIcEERSt13basic_ostreamIT_T0_ES6_, 4(%esp) movl %eax, (%esp) call _ZNSolsEPFRSoS_E .L2: movl $0, %eax leave ret /////////////////////////////////////////
Hi, sky. Replaced switch in favor of map::find(). Sad/CPU, Please review this..
drive-by: I find the numeric constants in the switch statement a bit hard to follow. http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.cc (right): http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.cc:118: ((touch_id & 0xfff) << 5) | (gesture_state << 17)); If you're now using a switch statement, it might be easier to read if Signature was replaced with a function that used logic and returned enum constants with meaningful names rather than doing this bit twiddling. GestureRecognizer::State GetState(...), or something.
On 2011/11/16 16:27:33, sky wrote: > http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... > File views/touchui/gesture_recognizer.cc (right): > > http://codereview.chromium.org/8364039/diff/44001/views/touchui/gesture_recog... > views/touchui/gesture_recognizer.cc:76: GestureTransitionFunctionMap::iterator > it = edge_functions_.find( > On 2011/11/16 15:08:41, Gajen wrote: > > On 2011/11/15 16:54:44, sky wrote: > > > Is there a reason we need this level of complexity rather than a switch > > > statement? > > > > log n to O(1) can be achieved in switch case, but theoretically there could be > > 2^32 such switch states, So, I guess, its OK to have this. > > Theoretically we could, but will we? The current code has 8, do you envision > needing a lot more to warrant this level of complexity. I like the current > approach if you need to dynamically support various cases, but AFAICT that isn't > the case. As such, a switch statement is a lot simpler and easier to follow. Just a note on the use of map: Gajen imported most of this code from the existing webkit gesture recognizer. I chose the map in that context because the GR there needs to be adjusted per platform, by available input devices, by if it runs on the MT or WebKit thread and ultimately from JavaScript.
Just a few small comment/style nits. I think some of the code moved from views/ to ui/views. So you will need to merge with ToT. Once that is done and the comments are addressed, I will send it to the trybots. http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.cc (right): http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.cc:50: flags_ = event.flags(); It looks like flags_ isn't used anywhere else. Remove? http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.h (right): http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:34: enum TouchState { Let's move the comment from line 68 to here to explain that the ui::EventType is mapped to GestureRecognizer::TouchState so it can fit into 3 bits of Signature. http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:62: GestureState GetState() { return state_; } Simple accessors are usually like this: GestureState state() const { return state_; } http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:133: int flags_; Are these ui::EventFlags? http://codereview.chromium.org/8364039/diff/55001/views/view.h File views/view.h (right): http://codereview.chromium.org/8364039/diff/55001/views/view.h#newcode594 views/view.h:594: Remove extra new line. http://codereview.chromium.org/8364039/diff/55001/views/view_unittest.cc File views/view_unittest.cc (right): http://codereview.chromium.org/8364039/diff/55001/views/view_unittest.cc#newc... views/view_unittest.cc:48: #include "views/touchui/gesture_recognizer.h" You will have to merge with trunk. I think this is now ui/views/touchui/ http://codereview.chromium.org/8364039/diff/55001/views/widget/native_widget_... File views/widget/native_widget_delegate.h (right): http://codereview.chromium.org/8364039/diff/55001/views/widget/native_widget_... views/widget/native_widget_delegate.h:100: virtual bool OnGestureEvent(const GestureEvent& event) = 0; Shouldn't this return GestureStatus? http://codereview.chromium.org/8364039/diff/55001/views/widget/widget.cc File views/widget/widget.cc (right): http://codereview.chromium.org/8364039/diff/55001/views/widget/widget.cc#newc... views/widget/widget.cc:1057: return static_cast<internal::RootView*>(GetRootView())->OnGestureEvent(event); I think this can just be: return GetRootView()->OnGestureEvent(event); I see that the same fix also applies in line 1052 above.
some comments: 1- I wish you have done 3 or so smaller changes 2- we can unittest some syntetic event stream, can't we? 3- So for web content we will have a gesture recognizer that lives on the renderer, I wonder how that will work out in the sense of a webcontent area gesture being recognized differently there. As in a lot of our UI is domUI. I see danger here. 4- Please add Justin Schuh to the review. http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.cc (right): http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.cc:119: } I agree with vollick.
On 2011/11/21 00:58:08, cpu wrote: > some comments: > > 1- I wish you have done 3 or so smaller changes > 2- we can unittest some syntetic event stream, can't we? > 3- So for web content we will have a gesture recognizer that lives on the > renderer, I wonder how that will work out in the sense of a webcontent area > gesture being recognized differently there. As in a lot of our UI is domUI. I > see danger here. Background: this GR in this CL is a port of the GR in WebKit that I wrote. At the moment, it therefore provides the same gesture recognition functionality. This GR here is therefore good enough at this time. As for divergence of gestures: the gestures recognized in a views context are as yet simple and easily kept identical with web content. However, in the longer term (m19 or m20 timeframe probably) II would expect moving the GR out of WebKit and using the same GR from views and WebKit as part of integrating gesture recognition with the MT compositor. I think it's worth getting this in once the issues noted by Sadrul are fixed and we can polish up the rough parts as touch-in-aura evolves. Sky: what do you think? > > 4- Please add Justin Schuh to the review. > > http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... > File views/touchui/gesture_recognizer.cc (right): > > http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... > views/touchui/gesture_recognizer.cc:119: } > I agree with vollick.
Thanks to CPU for comments. 1- I wish you have done 3 or so smaller changes >>>>> #3, would be taken care by GestureRecognizer(GR) module defined in Renderer, and already implemented by Robert. <<<<<< 2- we can unittest some syntetic event stream, can't we? >>> Yes , we can and its already done in views_unittests. <<< 3- So for web content we will have a gesture recognizer that lives on the renderer, I wonder how that will work out in the sense of a webcontent area gesture being recognized differently there >>>>>>>>> Yes, Out of My little knowledge, each DOMUi's renderer's GR would be responsible for that and DOMUi's browser side(RWHV) would just forward Webkit based touch events to renderer. As there is difference in Touch Sequence specification of Webkit(w3c) and Platform so different GRs and for Performance it makes more sense to have GR closer to HTML DOM. <<<<< . As in a lot of our UI is domUI. I see danger here. >>> Not sure about danger cases, Robert Can you please throw more light on this. <<<<<<<<<<< 4- Please add Justin Schuh to the review. >>>>> Done. <<< http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.cc (right): http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.cc:50: flags_ = event.flags(); On 2011/11/18 19:01:50, sadrul wrote: > It looks like flags_ isn't used anywhere else. Remove? Done. http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.cc:118: ((touch_id & 0xfff) << 5) | (gesture_state << 17)); On 2011/11/18 15:25:34, vollick wrote: > If you're now using a switch statement, it might be easier to read if Signature > was replaced with a function that used logic and returned enum constants with > meaningful names rather than doing this bit twiddling. GestureRecognizer::State > GetState(...), or something. Added new enum GestureSignatureType and Changed GestureRecognizer::ProcessTouchEventForGesture(), which is more descriptive now, So, I guess this method can be left as it is. http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.cc:119: } On 2011/11/21 00:58:09, cpu wrote: > I agree with vollick. Ditto http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... File views/touchui/gesture_recognizer.h (right): http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:34: enum TouchState { On 2011/11/18 19:01:50, sadrul wrote: > Let's move the comment from line 68 to here to explain that the ui::EventType is > mapped to GestureRecognizer::TouchState so it can fit into 3 bits of Signature. Done. http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:62: GestureState GetState() { return state_; } On 2011/11/18 19:01:50, sadrul wrote: > Simple accessors are usually like this: > > GestureState state() const { return state_; } > Done. http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... views/touchui/gesture_recognizer.h:133: int flags_; On 2011/11/18 19:01:50, sadrul wrote: > Are these ui::EventFlags? Yes, and updated comments too. http://codereview.chromium.org/8364039/diff/55001/views/view.h File views/view.h (right): http://codereview.chromium.org/8364039/diff/55001/views/view.h#newcode594 views/view.h:594: On 2011/11/18 19:01:50, sadrul wrote: > Remove extra new line. Done. http://codereview.chromium.org/8364039/diff/55001/views/view_unittest.cc File views/view_unittest.cc (right): http://codereview.chromium.org/8364039/diff/55001/views/view_unittest.cc#newc... views/view_unittest.cc:48: #include "views/touchui/gesture_recognizer.h" On 2011/11/18 19:01:50, sadrul wrote: > You will have to merge with trunk. I think this is now ui/views/touchui/ Build issue in my machine,Will do in next patch. http://codereview.chromium.org/8364039/diff/55001/views/widget/widget.cc File views/widget/widget.cc (right): http://codereview.chromium.org/8364039/diff/55001/views/widget/widget.cc#newc... views/widget/widget.cc:1057: return static_cast<internal::RootView*>(GetRootView())->OnGestureEvent(event); On 2011/11/18 19:01:50, sadrul wrote: > I think this can just be: > > return GetRootView()->OnGestureEvent(event); > > I see that the same fix also applies in line 1052 above. Done.
On Mon, Nov 21, 2011 at 7:46 AM, <rjkroege@chromium.org> wrote: > On 2011/11/21 00:58:08, cpu wrote: >> >> some comments: > > >> 1- I wish you have done 3 or so smaller changes >> 2- we can unittest some syntetic event stream, can't we? >> 3- So for web content we will have a gesture recognizer that lives on the >> renderer, I wonder how that will work out in the sense of a webcontent >> area >> gesture being recognized differently there. As in a lot of our UI is >> domUI. I >> see danger here. > > > Background: this GR in this CL is a port of the GR in WebKit that I wrote. > At > the moment, it therefore provides the same gesture recognition > functionality. > This GR here is therefore good enough at this time. > > As for divergence of gestures: the gestures recognized in a views context > are as > yet simple and easily kept identical with web content. However, in the > longer > term (m19 or m20 timeframe probably) II would expect moving the GR out of > WebKit > and using the same GR from views and WebKit as part of integrating gesture > recognition with the MT compositor. How come it's going to take so long to move the GR out of WebKit? Should we do that now instead of porting? > I think it's worth getting this in once the issues noted by Sadrul are fixed > and > we can polish up the rough parts as touch-in-aura evolves. Sky: what do you > think? If we have an urgent need for this, then that sounds fine. OTOH if there isn't a pressing need I would rather see us do the right thing (that you mentioned above). -Scott > > >> 4- Please add Justin Schuh to the review. > > > > http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... >> >> File views/touchui/gesture_recognizer.cc (right): > > > > http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... >> >> views/touchui/gesture_recognizer.cc:119: } >> I agree with vollick. > > > > > http://codereview.chromium.org/8364039/
On 2011/11/21 15:57:55, sky wrote: > On Mon, Nov 21, 2011 at 7:46 AM, <mailto:rjkroege@chromium.org> wrote: > > On 2011/11/21 00:58:08, cpu wrote: > >> > >> some comments: > > > > > >> 1- I wish you have done 3 or so smaller changes > >> 2- we can unittest some syntetic event stream, can't we? > >> 3- So for web content we will have a gesture recognizer that lives on the > >> renderer, I wonder how that will work out in the sense of a webcontent > >> area > >> gesture being recognized differently there. As in a lot of our UI is > >> domUI. I > >> see danger here. > > > > > > Background: this GR in this CL is a port of the GR in WebKit that I wrote. > > At > > the moment, it therefore provides the same gesture recognition > > functionality. > > This GR here is therefore good enough at this time. > > > > As for divergence of gestures: the gestures recognized in a views context > > are as > > yet simple and easily kept identical with web content. However, in the > > longer > > term (m19 or m20 timeframe probably) II would expect moving the GR out of > > WebKit > > and using the same GR from views and WebKit as part of integrating gesture > > recognition with the MT compositor. > > How come it's going to take so long to move the GR out of WebKit? > Should we do that now instead of porting? > > > I think it's worth getting this in once the issues noted by Sadrul are fixed > > and > > we can polish up the rough parts as touch-in-aura evolves. Sky: what do you > > think? > > If we have an urgent need for this, then that sounds fine. OTOH if > there isn't a pressing need I would rather see us do the right thing > (that you mentioned above). Gajen did the porting for us. We're all occupied with Aura/touch/ubercompositor work until well into Q1. This approach is low-road but helps along integrating touch into aura in the short term -- something we'd want for assessing the UI for example. I'll see to making it get made right in the course of building out the ubercompositor. Rob. > > -Scott > > > > > > >> 4- Please add Justin Schuh to the review. > > > > > > > > > http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... > >> > >> File views/touchui/gesture_recognizer.cc (right): > > > > > > > > > http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... > >> > >> views/touchui/gesture_recognizer.cc:119: } > >> I agree with vollick. > > > > > > > > > > http://codereview.chromium.org/8364039/
On 2011/11/21 21:01:23, rjkroege wrote: > On 2011/11/21 15:57:55, sky wrote: > > On Mon, Nov 21, 2011 at 7:46 AM, <mailto:rjkroege@chromium.org> wrote: > > > On 2011/11/21 00:58:08, cpu wrote: > > >> > > >> some comments: > > > > > > > > >> 1- I wish you have done 3 or so smaller changes > > >> 2- we can unittest some syntetic event stream, can't we? > > >> 3- So for web content we will have a gesture recognizer that lives on the > > >> renderer, I wonder how that will work out in the sense of a webcontent > > >> area > > >> gesture being recognized differently there. As in a lot of our UI is > > >> domUI. I > > >> see danger here. > > > > > > > > > Background: this GR in this CL is a port of the GR in WebKit that I wrote. > > > At > > > the moment, it therefore provides the same gesture recognition > > > functionality. > > > This GR here is therefore good enough at this time. > > > > > > As for divergence of gestures: the gestures recognized in a views context > > > are as > > > yet simple and easily kept identical with web content. However, in the > > > longer > > > term (m19 or m20 timeframe probably) II would expect moving the GR out of > > > WebKit > > > and using the same GR from views and WebKit as part of integrating gesture > > > recognition with the MT compositor. > > > > How come it's going to take so long to move the GR out of WebKit? > > Should we do that now instead of porting? > > > > > > > I think it's worth getting this in once the issues noted by Sadrul are fixed > > > and > > > we can polish up the rough parts as touch-in-aura evolves. Sky: what do you > > > think? > > > > If we have an urgent need for this, then that sounds fine. OTOH if > > there isn't a pressing need I would rather see us do the right thing > > (that you mentioned above). > > > Gajen did the porting for us. We're all occupied with Aura/touch/ubercompositor > work until well into Q1. This approach is low-road but helps along integrating > touch into aura in the short term -- something we'd want for assessing the UI > for example. I'll see to making it get made right in the course of building out > the ubercompositor. What worries me is that this looks like work we'll end up having to undo in the near future. We already have at least two platforms where we need to forward the gesture recognition out to the browser for the OS to process it. Would it be better to focus on making that part work by giving us a single gesture recognizer interface in the Chrome layer, where we can hook up forwarding to the native OS recognizer if appropriate? > > Rob. > > > > > > -Scott > > > > > > > > > > >> 4- Please add Justin Schuh to the review. > > > > > > > > > > > > > > > http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... > > >> > > >> File views/touchui/gesture_recognizer.cc (right): > > > > > > > > > > > > > > > http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... > > >> > > >> views/touchui/gesture_recognizer.cc:119: } > > >> I agree with vollick. > > > > > > > > > > > > > > > http://codereview.chromium.org/8364039/
I voiced my concern and it seems is in the mind of the author and other reviewers so I'll remove myself from the rest of the review.
On 2011/11/22 18:12:51, Justin Schuh wrote: > On 2011/11/21 21:01:23, rjkroege wrote: > > On 2011/11/21 15:57:55, sky wrote: > > > On Mon, Nov 21, 2011 at 7:46 AM, <mailto:rjkroege@chromium.org> wrote: > > > > On 2011/11/21 00:58:08, cpu wrote: > > > >> > > > >> some comments: > > > > > > > > > > > >> 1- I wish you have done 3 or so smaller changes > > > >> 2- we can unittest some syntetic event stream, can't we? > > > >> 3- So for web content we will have a gesture recognizer that lives on the > > > >> renderer, I wonder how that will work out in the sense of a webcontent > > > >> area > > > >> gesture being recognized differently there. As in a lot of our UI is > > > >> domUI. I > > > >> see danger here. > > > > > > > > > > > > Background: this GR in this CL is a port of the GR in WebKit that I wrote. > > > > At > > > > the moment, it therefore provides the same gesture recognition > > > > functionality. > > > > This GR here is therefore good enough at this time. > > > > > > > > As for divergence of gestures: the gestures recognized in a views context > > > > are as > > > > yet simple and easily kept identical with web content. However, in the > > > > longer > > > > term (m19 or m20 timeframe probably) II would expect moving the GR out of > > > > WebKit > > > > and using the same GR from views and WebKit as part of integrating gesture > > > > recognition with the MT compositor. > > > > > > How come it's going to take so long to move the GR out of WebKit? > > > Should we do that now instead of porting? > > > > > > > > > > > I think it's worth getting this in once the issues noted by Sadrul are > fixed > > > > and > > > > we can polish up the rough parts as touch-in-aura evolves. Sky: what do > you > > > > think? > > > > > > If we have an urgent need for this, then that sounds fine. OTOH if > > > there isn't a pressing need I would rather see us do the right thing > > > (that you mentioned above). > > > > > > Gajen did the porting for us. We're all occupied with > Aura/touch/ubercompositor > > work until well into Q1. This approach is low-road but helps along integrating > > touch into aura in the short term -- something we'd want for assessing the UI > > for example. I'll see to making it get made right in the course of building > out > > the ubercompositor. > > What worries me is that this looks like work we'll end up having to undo in the > near future. We already have at least two platforms where we need to forward the > gesture recognition out to the browser for the OS to process it. Would it be > better to focus on making that part work by giving us a single gesture > recognizer interface in the Chrome layer, where we can hook up forwarding to the > native OS recognizer if appropriate? Aura needs gesture recognition in the short term, and it may take a long time for all interested parties to agree on the optimal solution and to get it implemented. Would you be willing to allow this stopgap if a bug were logged/assigned and a FIXME added to the code indicating that the recognition code needs to be consolidated? > > > > > > Rob. > > > > > > > > > > -Scott > > > > > > > > > > > > > > >> 4- Please add Justin Schuh to the review. > > > > > > > > > > > > > > > > > > > > > > http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... > > > >> > > > >> File views/touchui/gesture_recognizer.cc (right): > > > > > > > > > > > > > > > > > > > > > > http://codereview.chromium.org/8364039/diff/55001/views/touchui/gesture_recog... > > > >> > > > >> views/touchui/gesture_recognizer.cc:119: } > > > >> I agree with vollick. > > > > > > > > > > > > > > > > > > > > http://codereview.chromium.org/8364039/
LGTM with the understanding we're going to refactor this soon to avoid duplication.
On 2011/12/28 17:55:28, sky wrote: > LGTM with the understanding we're going to refactor this soon to avoid > duplication. Thankx, after TOT sync would commit soon.
On 2011/12/29 07:05:54, Gajen wrote: > On 2011/12/28 17:55:28, sky wrote: > > LGTM with the understanding we're going to refactor this soon to avoid > > duplication. > > Thankx, after TOT sync would commit soon. Tracking bug: http://crbug.com/108990 for eliminating code duplication.
On 2011/12/29 07:05:54, Gajen wrote: > On 2011/12/28 17:55:28, sky wrote: > > LGTM with the understanding we're going to refactor this soon to avoid > > duplication. > > Thankx, after TOT sync would commit soon. Merged with ToT here: http://codereview.chromium.org/9076002 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
