Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10365009:
Adding Gesture Recognition to RenderWidgetHostViewWin (web client) (Closed)
|
Created:
8 years, 7 months ago by girard Modified:
8 years, 7 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, ben+watch_chromium.org, tfarina, jam, penghuang+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, James Su, gideonwald Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdding Gesture Recognition to RenderWidgetHostViewWin (web client)
Gesture and Touch events are now passed into the content window.
This is only enabled if --enable-touch-events is flagged on the command line.
BUG=124938
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137920
Patch Set 1 #
Total comments: 13
Patch Set 2 : Removed views dependency. #
Total comments: 30
Patch Set 3 : Addresses review from rjkroege #
Total comments: 11
Patch Set 4 : Added call to has_touch_handler() #Patch Set 5 : Refactored LocalTouchEvent and WebTouchState (combines their operations.) #
Total comments: 26
Patch Set 6 : Cleaned up code/format, plus removed memory leak. #
Total comments: 22
Patch Set 7 : More review cleanup. #Patch Set 8 : Merge cleanup. (Implementing four new virtuals added in the base.) #
Messages
Total messages: 21 (0 generated)
This also includes code from http://codereview.chromium.org/10134045/ GR now works in RenderWidgetHostWin.
https://chromiumcodereview.appspot.com/10365009/diff/1/content/browser/render... File content/browser/renderer_host/render_widget_host_view_win.cc (right): https://chromiumcodereview.appspot.com/10365009/diff/1/content/browser/render... content/browser/renderer_host/render_widget_host_view_win.cc:61: #include "ui/views/events/event.h" afaik: content is not allowed to depend on views. https://chromiumcodereview.appspot.com/10365009/diff/1/content/browser/render... content/browser/renderer_host/render_widget_host_view_win.cc:1004: ui::GestureEvent* RenderWidgetHostViewWin::CreateGestureEvent( my intuition is that this class is not the right place to put this code. https://chromiumcodereview.appspot.com/10365009/diff/1/content/browser/render... content/browser/renderer_host/render_widget_host_view_win.cc:1951: render_widget_host_->ForwardTouchEvent(touch_state_.touch_event()); are the touch-ids normalized here? https://chromiumcodereview.appspot.com/10365009/diff/1/content/browser/render... content/browser/renderer_host/render_widget_host_view_win.cc:1967: // Ignore this touch if there is a backlog. why? you drop the moves on the floor? wouldn't the right model be to coalesce the moves like mousemoves before sending the touches to webkit but sending every event sent to webkit to the GR. The GR ought to be very fast compared to a trip to WebKit and so should have no trouble keeping up? https://chromiumcodereview.appspot.com/10365009/diff/1/content/browser/render... content/browser/renderer_host/render_widget_host_view_win.cc:1998: for (ui::GestureRecognizer::Gestures::iterator g_it = gestures->begin(); we have code that knows how to make Web events out of aura events. Maybe it should make Web events out of ui events instead? then this code wouldn't need to be here too? https://chromiumcodereview.appspot.com/10365009/diff/1/content/browser/render... content/browser/renderer_host/render_widget_host_view_win.cc:2043: case ui::ET_GESTURE_TAP_DOWN: Something looks wrong here. Taps from the GR are handled in WebKit. I would expect this to do the wrong thing. https://chromiumcodereview.appspot.com/10365009/diff/1/content/browser/render... content/browser/renderer_host/render_widget_host_view_win.cc:2052: ForwardMouseEventToRenderer(WM_LBUTTONDBLCLK, 0, Same comment https://chromiumcodereview.appspot.com/10365009/diff/1/ui/aura/root_window.h File ui/aura/root_window.h (right): https://chromiumcodereview.appspot.com/10365009/diff/1/ui/aura/root_window.h#... ui/aura/root_window.h:306: const gfx::Point& location, i'd split this. To avoid confusing the reviewers. :) https://chromiumcodereview.appspot.com/10365009/diff/1/ui/base/gestures/gestu... File ui/base/gestures/gesture_recognizer.h (right): https://chromiumcodereview.appspot.com/10365009/diff/1/ui/base/gestures/gestu... ui/base/gestures/gesture_recognizer.h:41: virtual bool IsQueueEmpty(GestureConsumer* consumer) = 0; need a comment saying what does this does.
I got part way through and then saw comments from Rob. I'll let you resolve those before I continue. https://chromiumcodereview.appspot.com/10365009/diff/1/content/browser/render... File content/browser/renderer_host/render_widget_host_view_win.cc (right): https://chromiumcodereview.appspot.com/10365009/diff/1/content/browser/render... content/browser/renderer_host/render_widget_host_view_win.cc:338: ALLOW_THIS_IN_INITIALIZER_LIST( No scoped_ptr since you're creating all the time (I sent rationale in previous review). https://chromiumcodereview.appspot.com/10365009/diff/1/content/browser/render... File content/browser/renderer_host/render_widget_host_view_win.h (right): https://chromiumcodereview.appspot.com/10365009/diff/1/content/browser/render... content/browser/renderer_host/render_widget_host_view_win.h:264: const base::Time& time, Same comment from other review: no refs for times. Treat time/timedelta as you would an enum value. You wouldn't pass those by ref either. https://chromiumcodereview.appspot.com/10365009/diff/1/content/browser/render... content/browser/renderer_host/render_widget_host_view_win.h:363: WebKit::WebInputEvent::Type eventType, eventType -> event_type https://chromiumcodereview.appspot.com/10365009/diff/1/content/browser/render... content/browser/renderer_host/render_widget_host_view_win.h:365: void ProcessGestures( ui::GestureRecognizer::Gestures* gestures); remove space. Add newline before methods.
Still includes code from http://codereview.chromium.org/10134045/ This revision removes the code dependency on views. Now RenderWidgetHostViewWin defines its own LocalGestureEvent and LocalTouchEvent, which derive from ui. Q1: Where should “local” classes be defined? At point of use, or in an anonymous namespace at the top of the file? LocalGestureEvent needs to know. Q2: Better names than LocalGestureEvent/LocalTouchEvent? Q3: What’s the policy on code like: default: // Unexpected ui type mapping! (should we log message?) return WebKit::WebInputEvent::Undefined;
https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... File content/browser/renderer_host/render_widget_host_view_win.cc (right): https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:1001: template <class IINTERFACE, class PAYLOAD> I have a imprecise style-guide-kind-of-feeling that these embedded helper classes should be up at the top of the file and not mixed in with the exposed methods. https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:1056: static ui::EventType ConvertToUI( WebKit::WebGestureEvent::Type t) we have this same switch statement in more than one place. gack. You could move a static helper into GestureHelper and then use the same set of switches everywhere? https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:1065: // case WebKit::WebGestureEvent::GestureFlingStart: this will break fling. why do you want to do that? https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:1115: // Unexpected type mapping! add DCHECK instead. https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:1160: virtual ui::EventType GetEventType() const OVERRIDE { Unfortunately, these would appear to have to be virtual. Add a TODO to fix this post http://crbug.com/125937 https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:2169: //bool hasAreaInfo = remove dead code https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:2180: // Ignore this touch if there is a backlog. I don't recall you answering me as to why there would be. https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:2208: for (ui::GestureRecognizer::Gestures::iterator g_it = gestures->begin(); given the switches above, can't you just send all the gestures. They are WebGestureEvents already. I am puzzled. https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:2242: case ui::ET_GESTURE_THREE_FINGER_SWIPE: why not forward it anyway? If no events are being generated, then, you could simplify this code? https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... File content/browser/renderer_host/render_widget_host_view_win.h (left): https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.h:339: // Synthesize mouse wheel event. this removal seems not to apply here. https://chromiumcodereview.appspot.com/10365009/diff/6002/ui/aura/root_window.cc File ui/aura/root_window.cc (right): https://chromiumcodereview.appspot.com/10365009/diff/6002/ui/aura/root_window... ui/aura/root_window.cc:837: ui::TouchEvent* RootWindow::CreateTouchEvent( white space only? https://chromiumcodereview.appspot.com/10365009/diff/6002/ui/aura/root_window.h File ui/aura/root_window.h (right): https://chromiumcodereview.appspot.com/10365009/diff/6002/ui/aura/root_window... ui/aura/root_window.h:300: base::Time time, revert this change? it is white space only
Thanks for your input, Rob. Will upload a new patch shortly. https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... File content/browser/renderer_host/render_widget_host_view_win.cc (right): https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:1001: template <class IINTERFACE, class PAYLOAD> On 2012/05/09 16:48:15, rjkroege wrote: > I have a imprecise style-guide-kind-of-feeling that these embedded helper > classes should be up at the top of the file and not mixed in with the exposed > methods. Moved to the top of the file (in the local anonymous namespace.) https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:1056: static ui::EventType ConvertToUI( WebKit::WebGestureEvent::Type t) On 2012/05/09 16:48:15, rjkroege wrote: > we have this same switch statement in more than one place. gack. > > You could move a static helper into GestureHelper and then use the same set of > switches everywhere? Agree that we should combine these operations together. https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:1065: // case WebKit::WebGestureEvent::GestureFlingStart: On 2012/05/09 16:48:15, rjkroege wrote: > this will break fling. why do you want to do that? There is no ui::ET_GESTURE_FLING_START. Is it coming, or would ui::ET_SCROLL_FLING_START make sense? https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:1115: // Unexpected type mapping! On 2012/05/09 16:48:15, rjkroege wrote: > add DCHECK instead. Done. https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:2169: //bool hasAreaInfo = On 2012/05/09 16:48:15, rjkroege wrote: > remove dead code Done. https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:2180: // Ignore this touch if there is a backlog. On 2012/05/09 16:48:15, rjkroege wrote: > I don't recall you answering me as to why there would be. On my machine, sending all messages on to the GR greatly degraded the operation of the GR. It would often reverse the direction of scrolls, or lag badly, or miss gestures completely. https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:2208: for (ui::GestureRecognizer::Gestures::iterator g_it = gestures->begin(); On 2012/05/09 16:48:15, rjkroege wrote: > given the switches above, can't you just send all the gestures. They are > WebGestureEvents already. I am puzzled. Good point. This code can be removed. https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:2208: for (ui::GestureRecognizer::Gestures::iterator g_it = gestures->begin(); On 2012/05/09 16:48:15, rjkroege wrote: > given the switches above, can't you just send all the gestures. They are > WebGestureEvents already. I am puzzled. Done. https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:2242: case ui::ET_GESTURE_THREE_FINGER_SWIPE: On 2012/05/09 16:48:15, rjkroege wrote: > why not forward it anyway? If no events are being generated, then, you could > simplify this code? Forward as what kind of WebGestureEvent? Are there plans to add WebInputEvent::Type::GestureThreeFingerSwipe? https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... File content/browser/renderer_host/render_widget_host_view_win.h (left): https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.h:339: // Synthesize mouse wheel event. On 2012/05/09 16:48:15, rjkroege wrote: > this removal seems not to apply here. This is dead code. Not defined or called. https://chromiumcodereview.appspot.com/10365009/diff/6002/ui/aura/root_window.cc File ui/aura/root_window.cc (right): https://chromiumcodereview.appspot.com/10365009/diff/6002/ui/aura/root_window... ui/aura/root_window.cc:837: ui::TouchEvent* RootWindow::CreateTouchEvent( On 2012/05/09 16:48:15, rjkroege wrote: > white space only? Done. https://chromiumcodereview.appspot.com/10365009/diff/6002/ui/aura/root_window.h File ui/aura/root_window.h (right): https://chromiumcodereview.appspot.com/10365009/diff/6002/ui/aura/root_window... ui/aura/root_window.h:300: base::Time time, On 2012/05/09 16:48:15, rjkroege wrote: > revert this change? it is white space only Fist change removed a (useless) const from the function signature. The change below was forced because the time_stamp line went to 81 characters.
Thanks again for your input Rob. This patch addresses all of the issues you raised.
Some questions inline. On 2012/05/09 22:49:17, girard wrote: > Thanks for your input, Rob. Will upload a new patch shortly. > > https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... > File content/browser/renderer_host/render_widget_host_view_win.cc (right): > > https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... > content/browser/renderer_host/render_widget_host_view_win.cc:1001: template > <class IINTERFACE, class PAYLOAD> > On 2012/05/09 16:48:15, rjkroege wrote: > > I have a imprecise style-guide-kind-of-feeling that these embedded helper > > classes should be up at the top of the file and not mixed in with the exposed > > methods. > > Moved to the top of the file (in the local anonymous namespace.) > > https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... > content/browser/renderer_host/render_widget_host_view_win.cc:1056: static > ui::EventType ConvertToUI( WebKit::WebGestureEvent::Type t) > On 2012/05/09 16:48:15, rjkroege wrote: > > we have this same switch statement in more than one place. gack. > > > > You could move a static helper into GestureHelper and then use the same set of > > switches everywhere? > > Agree that we should combine these operations together. > > https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... > content/browser/renderer_host/render_widget_host_view_win.cc:1065: // case > WebKit::WebGestureEvent::GestureFlingStart: > On 2012/05/09 16:48:15, rjkroege wrote: > > this will break fling. why do you want to do that? > > There is no ui::ET_GESTURE_FLING_START. Is it coming, or would > ui::ET_SCROLL_FLING_START make sense? See web_input_event_aurax11.cc for ui:: to WebInputEvent enum equivalences. > > https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... > content/browser/renderer_host/render_widget_host_view_win.cc:1115: // Unexpected > type mapping! > On 2012/05/09 16:48:15, rjkroege wrote: > > add DCHECK instead. > > Done. > > https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... > content/browser/renderer_host/render_widget_host_view_win.cc:2169: //bool > hasAreaInfo = > On 2012/05/09 16:48:15, rjkroege wrote: > > remove dead code > > Done. > > https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... > content/browser/renderer_host/render_widget_host_view_win.cc:2180: // Ignore > this touch if there is a backlog. > On 2012/05/09 16:48:15, rjkroege wrote: > > I don't recall you answering me as to why there would be. > > On my machine, sending all messages on to the GR greatly degraded the operation > of the GR. It would often reverse the direction of scrolls, or lag badly, or > miss gestures completely. The GR is theoretically cheap. If it is not, we have a deeper issue. Let's leave this here and you can look into why this is later. > > https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... > content/browser/renderer_host/render_widget_host_view_win.cc:2208: for > (ui::GestureRecognizer::Gestures::iterator g_it = gestures->begin(); > On 2012/05/09 16:48:15, rjkroege wrote: > > given the switches above, can't you just send all the gestures. They are > > WebGestureEvents already. I am puzzled. > > Good point. This code can be removed. > > https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... > content/browser/renderer_host/render_widget_host_view_win.cc:2208: for > (ui::GestureRecognizer::Gestures::iterator g_it = gestures->begin(); > On 2012/05/09 16:48:15, rjkroege wrote: > > given the switches above, can't you just send all the gestures. They are > > WebGestureEvents already. I am puzzled. > > Done. > > https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... > content/browser/renderer_host/render_widget_host_view_win.cc:2242: case > ui::ET_GESTURE_THREE_FINGER_SWIPE: > On 2012/05/09 16:48:15, rjkroege wrote: > > why not forward it anyway? If no events are being generated, then, you could > > simplify this code? > > Forward as what kind of WebGestureEvent? Are there plans to add > WebInputEvent::Type::GestureThreeFingerSwipe? Probably not. And WebViewImpl would assert. You would have to leave those out. > > https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... > File content/browser/renderer_host/render_widget_host_view_win.h (left): > > https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... > content/browser/renderer_host/render_widget_host_view_win.h:339: // Synthesize > mouse wheel event. > On 2012/05/09 16:48:15, rjkroege wrote: > > this removal seems not to apply here. > > This is dead code. Not defined or called. It's preferable to keep unrelated code cleanup in separate CLs. (Well, I think it is.) > > https://chromiumcodereview.appspot.com/10365009/diff/6002/ui/aura/root_window.cc > File ui/aura/root_window.cc (right): > > https://chromiumcodereview.appspot.com/10365009/diff/6002/ui/aura/root_window... > ui/aura/root_window.cc:837: ui::TouchEvent* RootWindow::CreateTouchEvent( > On 2012/05/09 16:48:15, rjkroege wrote: > > white space only? > > Done. > > https://chromiumcodereview.appspot.com/10365009/diff/6002/ui/aura/root_window.h > File ui/aura/root_window.h (right): > > https://chromiumcodereview.appspot.com/10365009/diff/6002/ui/aura/root_window... > ui/aura/root_window.h:300: base::Time time, > On 2012/05/09 16:48:15, rjkroege wrote: > > revert this change? it is white space only > > Fist change removed a (useless) const from the function signature. The change > below was forced because the time_stamp line went to 81 characters.
https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... File content/browser/renderer_host/render_widget_host_view_win.cc (right): https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:2163: render_widget_host_->ForwardTouchEvent(touch_state_.touch_event()); Perhaps this should also check 'if (render_widget_host_->has_touch_handler())' before forwarding the touch-event. If the touch-event is forwarded (i.e. host has touch-handlers), then the events should be queued, otherwise they should be processed immediately. https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... content/browser/renderer_host/render_widget_host_view_win.cc:2180: // Ignore this touch if there is a backlog. On 2012/05/09 22:49:19, girard wrote: > On 2012/05/09 16:48:15, rjkroege wrote: > > I don't recall you answering me as to why there would be. > > On my machine, sending all messages on to the GR greatly degraded the operation > of the GR. It would often reverse the direction of scrolls, or lag badly, or > miss gestures completely. Note: because this is dropping some of the touch-events, the GR can generate unexpected gestures (or unexpected values for gesture parameters, e.g. fling velocities might be off).
[snip] > https://chromiumcodereview.appspot.com/10365009/diff/6002/content/browser/ren... > > content/browser/renderer_host/render_widget_host_view_win.cc:2180: // Ignore > > this touch if there is a backlog. > > On 2012/05/09 16:48:15, rjkroege wrote: > > > I don't recall you answering me as to why there would be. > > > > On my machine, sending all messages on to the GR greatly degraded the > operation > > of the GR. It would often reverse the direction of scrolls, or lag badly, or > > miss gestures completely. > > The GR is theoretically cheap. If it is not, we have a deeper issue. Let's leave > this here and you can look into why this is later. chrome://tracing/ should be useful for this.
http://codereview.chromium.org/10365009/diff/9010/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/10365009/diff/9010/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:452: class LocalTouchEvent : You did not do here what I expected. I expected you to create LocalTouchEvent objects by wrapping them around an existing WebTouchEvent is already getting made anyhow. Then you would not need to replicate storing the time_stamp, location, etc. i.e.: on receipt of TouchEvent, create a LocalTouchEvent containing a WebTouchEvent, populate (updating the TouchPoint state for the unaffected touches as necessary) send to render_widget_host_ send to gr (copy is a nop) ownership of the event is over afaik. Perhaps I am mistaken. http://codereview.chromium.org/10365009/diff/9010/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:472: // This is an experiment! Something is funny here. Perhaps you have code that you need to delete? http://codereview.chromium.org/10365009/diff/9010/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:1241: remove http://codereview.chromium.org/10365009/diff/9010/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:2736: LocalGestureEvent *local = reinterpret_cast<LocalGestureEvent *>( gesture); white space ( gesture) wrong. And we know that a gesture is a LocalGestureEvent so wouldn't a static_cast<> do? My C++ trivia is weak. http://codereview.chromium.org/10365009/diff/9010/ui/aura/root_window.h File ui/aura/root_window.h (right): http://codereview.chromium.org/10365009/diff/9010/ui/aura/root_window.h#newco... ui/aura/root_window.h:305: const gfx::Point& location, why have you not reverted the white space?
This CL incorporates a code path optimization (based on sadrul's feedback).... we now shortcut the call to the client when has_touch_handler() is false. I've also cleaned a number of items that rjkroege raised. I think that all issues have now been addressed with one exception (we still have two touch-event's being generated: a LocalTouchEvent, and a WebTouchPoint.) I will be uploading a CL shortly to clean up that redundancy. http://codereview.chromium.org/10365009/diff/6002/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/10365009/diff/6002/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:1160: virtual ui::EventType GetEventType() const OVERRIDE { On 2012/05/09 16:48:15, rjkroege wrote: > Unfortunately, these would appear to have to be virtual. Add a TODO to fix this > post http://crbug.com/125937 Done. http://codereview.chromium.org/10365009/diff/6002/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:2163: render_widget_host_->ForwardTouchEvent(touch_state_.touch_event()); On 2012/05/10 01:49:26, sadrul wrote: > Perhaps this should also check 'if (render_widget_host_->has_touch_handler())' > before forwarding the touch-event. If the touch-event is forwarded (i.e. host > has touch-handlers), then the events should be queued, otherwise they should be > processed immediately. Thanks! Done. http://codereview.chromium.org/10365009/diff/6002/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:2180: // Ignore this touch if there is a backlog. On 2012/05/10 01:49:26, sadrul wrote: > On 2012/05/09 22:49:19, girard wrote: > > On 2012/05/09 16:48:15, rjkroege wrote: > > > I don't recall you answering me as to why there would be. > > > > On my machine, sending all messages on to the GR greatly degraded the > operation > > of the GR. It would often reverse the direction of scrolls, or lag badly, or > > miss gestures completely. > > Note: because this is dropping some of the touch-events, the GR can generate > unexpected gestures (or unexpected values for gesture parameters, e.g. fling > velocities might be off). After some more testing, I determined that this issue only came up when I was running under a debugger. As such I'm removing the entire "IsQueueEmpty" subsystem. http://codereview.chromium.org/10365009/diff/6002/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_win.h (left): http://codereview.chromium.org/10365009/diff/6002/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.h:339: // Synthesize mouse wheel event. On 2012/05/09 22:49:19, girard wrote: > On 2012/05/09 16:48:15, rjkroege wrote: > > this removal seems not to apply here. > > This is dead code. Not defined or called. Removing from this CL. More precisely, I'm removing this deletion from this CL, which means I'm leaving the dead code in place. http://codereview.chromium.org/10365009/diff/9010/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/10365009/diff/9010/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:452: class LocalTouchEvent : On 2012/05/10 01:51:45, rjkroege wrote: > You did not do here what I expected. > > I expected you to create LocalTouchEvent objects by wrapping them around an > existing WebTouchEvent is already getting made anyhow. Then you would not need > to replicate storing the time_stamp, location, etc. > > i.e.: on receipt of TouchEvent, create a LocalTouchEvent containing a > WebTouchEvent, > populate (updating the TouchPoint state for the unaffected touches as necessary) > send to render_widget_host_ > send to gr (copy is a nop) ownership of the event is over afaik. > > Perhaps I am mistaken. The original code created a WebTouchEvent, which itself contains a collection of WebTouchPoint's. This (WTE/WTP) is what the client-side/javascript wants to see. The GR wants to see ui:TouchEvents, which correspond to WebTouchPoints. Because of the direct containment (WTE's contain an array of WTP's), it would be very difficult to implement a WTP that also doubles as a ui::TE. But we can certainly modify the current code to build the ui::TE's from the existing WTP's, instead of building them from scratch. This seems like a reasonable solution... comments? http://codereview.chromium.org/10365009/diff/9010/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:472: // This is an experiment! On 2012/05/10 01:51:45, rjkroege wrote: > Something is funny here. Perhaps you have code that you need to delete? Thanks for the catch! Fixed. http://codereview.chromium.org/10365009/diff/9010/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:1241: On 2012/05/10 01:51:45, rjkroege wrote: > remove Done. http://codereview.chromium.org/10365009/diff/9010/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:2736: LocalGestureEvent *local = reinterpret_cast<LocalGestureEvent *>( gesture); On 2012/05/10 01:51:45, rjkroege wrote: > white space ( gesture) wrong. And we know that a gesture is a LocalGestureEvent > so wouldn't a static_cast<> do? My C++ trivia is weak. Done. http://codereview.chromium.org/10365009/diff/9010/ui/aura/root_window.h File ui/aura/root_window.h (right): http://codereview.chromium.org/10365009/diff/9010/ui/aura/root_window.h#newco... ui/aura/root_window.h:305: const gfx::Point& location, On 2012/05/10 01:51:45, rjkroege wrote: > why have you not reverted the white space? The presubmit check was whining that this was an 81 character line.
Refactored RenderWidgetHostViewWin::WebTouchEvent to use LocalTouchEvent. Also factored WebTouchEvent out of RenderWidgetHostViewWin's public interface. http://codereview.chromium.org/10365009/diff/9010/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/10365009/diff/9010/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:452: class LocalTouchEvent : On 2012/05/11 14:21:49, girard wrote: > On 2012/05/10 01:51:45, rjkroege wrote: > > You did not do here what I expected. > > > > I expected you to create LocalTouchEvent objects by wrapping them around an > > existing WebTouchEvent is already getting made anyhow. Then you would not need > > to replicate storing the time_stamp, location, etc. > > > > i.e.: on receipt of TouchEvent, create a LocalTouchEvent containing a > > WebTouchEvent, > > populate (updating the TouchPoint state for the unaffected touches as > necessary) > > send to render_widget_host_ > > send to gr (copy is a nop) ownership of the event is over afaik. > > > > Perhaps I am mistaken. > > The original code created a WebTouchEvent, which itself contains a collection of > WebTouchPoint's. This (WTE/WTP) is what the client-side/javascript wants to > see. > > The GR wants to see ui:TouchEvents, which correspond to WebTouchPoints. Because > of the direct containment (WTE's contain an array of WTP's), it would be very > difficult to implement a WTP that also doubles as a ui::TE. > > But we can certainly modify the current code to build the ui::TE's from the > existing WTP's, instead of building them from scratch. > > This seems like a reasonable solution... comments? Done.
Touch / gesture support looks good to me. lgtm
http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:49: #include "ui/base/gestures/gesture_recognizer.h" Remove as you've got this include in the header. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:61: #include "ui/views/events/event.h" This include isn't allowed here. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:308: class WrappedObject: public IINTERFACE { space after ':' http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:309: public: space before public (and protected on 321). http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:312: WrappedObject(const PAYLOAD ©) : : on next line. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:323: }; DISALLOW_COPY_AND_ASSIGN for all your classes. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:325: WebKit::WebTouchPoint::State ConvertToWebTouchPoint(ui::EventType t) { Do other places need this code? http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:327: case ui::ET_TOUCH_PRESSED: indent case statements. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:453: public: indent http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:461: unsigned int touch_id_bitfield) : : on next line. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:561: explicit WebTouchState(const CWindowImpl<RenderWidgetHostViewWin, indentation is off. How about a typedef for this since it's so long. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:2063: CWindow, indentation is off. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_win.h (right): http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.h:29: #include "ui/base/gestures/gesture_recognizer.h" remove duplicate include. http://codereview.chromium.org/10365009/diff/13002/ui/aura/root_window.h File ui/aura/root_window.h (right): http://codereview.chromium.org/10365009/diff/13002/ui/aura/root_window.h#newc... ui/aura/root_window.h:297: virtual ui::TouchEvent* CreateTouchEvent(ui::EventType type, wrap first param to new line so all parameters are aligned.
Cleaned up code as per sky's suggestions. I currently define and use a macro for DISALLOW_COPY_AND_ASSIGN2. I'd like to move that to base/basictypes.h in a separate/subsequent CL. I've also corrected a memory leak in LocalTouchEvent::Index, which was creating a dynamic copy of the event. The caller was passing this event on to the gesture recognizer, who was immediately calling event.Copy. Index now avoids dynamic memory allocation. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:49: #include "ui/base/gestures/gesture_recognizer.h" On 2012/05/16 22:07:12, sky wrote: > Remove as you've got this include in the header. Done. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:61: #include "ui/views/events/event.h" On 2012/05/16 22:07:12, sky wrote: > This include isn't allowed here. Done. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:308: class WrappedObject: public IINTERFACE { On 2012/05/16 22:07:12, sky wrote: > space after ':' Done. (Space before ':') http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:309: public: On 2012/05/16 22:07:12, sky wrote: > space before public (and protected on 321). Done. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:312: WrappedObject(const PAYLOAD ©) : On 2012/05/16 22:07:12, sky wrote: > : on next line. Done. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:312: WrappedObject(const PAYLOAD ©) : On 2012/05/16 22:07:12, sky wrote: > : on next line. Done. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:323: }; On 2012/05/16 22:07:12, sky wrote: > DISALLOW_COPY_AND_ASSIGN for all your classes. Done. (Sidebar: Because WrappedObject is a two-param template, it can't use DISALLOW_COPY_AND_ASSIGN. I've included an extended version of the macro to address this, but it should probably live in base/basictypes.h Will move the macro in a subsequent CL). http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:325: WebKit::WebTouchPoint::State ConvertToWebTouchPoint(ui::EventType t) { On 2012/05/16 22:07:12, sky wrote: > Do other places need this code? Not that I am aware of. In fact, two of the four conversion functions aren't even used here.... I included them for completeness. If there is a better home for these, I'd be happy to move them. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:327: case ui::ET_TOUCH_PRESSED: On 2012/05/16 22:07:12, sky wrote: > indent case statements. Done. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:453: public: On 2012/05/16 22:07:12, sky wrote: > indent Done. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:461: unsigned int touch_id_bitfield) : On 2012/05/16 22:07:12, sky wrote: > : on next line. Done. http://codereview.chromium.org/10365009/diff/13002/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_view_win.cc:561: explicit WebTouchState(const CWindowImpl<RenderWidgetHostViewWin, On 2012/05/16 22:07:12, sky wrote: > indentation is off. How about a typedef for this since it's so long. Good suggestion. I actually changed this to RenderWidgetHostViewWin* (which is a class derived from the CWindowImpl<...>) and the code has become much cleaner.
http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:310: #define DISALLOW_COPY_AND_ASSIGN2(TypeName,ExtraParm) \ Are you sure we need this? Is there some way to escape the params so DISALLOW_COPY_AND_ASSIGN works? http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:328: protected: newline between 327/328 and 329/300. Also, style guide says no protected members. http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:334: // Note that this function is not currently used, and it's only included Don't include code that isn't needed. If there are no plans to use it, remove it. http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:475: : type_(type), : should be indented 4 spaces. And order of fields you're setting doesn't match declaration order. http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:484: virtual int GetLowestTouchId() const OVERRIDE { This is copied in a number of places. Move it to gesture_types as a function that takes an unsigned int so that everyone can use it. http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:496: private: newline between 495/496. http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:615: }; DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:2257: if (touch_state_->is_changed()) { no {} http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:2277: if ((gestures == 0) || gestures->empty()) 0 -> NULL http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_win.h (right): http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.h:29: #include "ui/base/gestures/gesture_recognizer.h" remove second gesture_recognizer. http://codereview.chromium.org/10365009/diff/6008/ui/aura/root_window.h File ui/aura/root_window.h (right): http://codereview.chromium.org/10365009/diff/6008/ui/aura/root_window.h#newco... ui/aura/root_window.h:297: virtual ui::TouchEvent* CreateTouchEvent(ui::EventType type, wrap type to the next line so that all args line up. Do the same thing on 289.
Cleaned up as per sky's review. http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:310: #define DISALLOW_COPY_AND_ASSIGN2(TypeName,ExtraParm) \ On 2012/05/17 15:45:46, sky wrote: > Are you sure we need this? Is there some way to escape the params so > DISALLOW_COPY_AND_ASSIGN works? AFAIK, you can't escape a comma for CPP. But I could use a typedef to rename the type. Done. http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:328: protected: On 2012/05/17 15:45:46, sky wrote: > newline between 327/328 and 329/300. Also, style guide says no protected > members. Done. http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:334: // Note that this function is not currently used, and it's only included On 2012/05/17 15:45:46, sky wrote: > Don't include code that isn't needed. If there are no plans to use it, remove > it. Done. http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:475: : type_(type), On 2012/05/17 15:45:46, sky wrote: > : should be indented 4 spaces. > And order of fields you're setting doesn't match declaration order. Done. http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:484: virtual int GetLowestTouchId() const OVERRIDE { On 2012/05/17 15:45:46, sky wrote: > This is copied in a number of places. Move it to gesture_types as a function > that takes an unsigned int so that everyone can use it. Done. http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:496: private: On 2012/05/17 15:45:46, sky wrote: > newline between 495/496. Done. http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:615: }; On 2012/05/17 15:45:46, sky wrote: > DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:2257: if (touch_state_->is_changed()) { On 2012/05/17 15:45:46, sky wrote: > no {} Done. http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.cc:2277: if ((gestures == 0) || gestures->empty()) On 2012/05/17 15:45:46, sky wrote: > 0 -> NULL Done. http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_win.h (right): http://codereview.chromium.org/10365009/diff/6008/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_win.h:29: #include "ui/base/gestures/gesture_recognizer.h" On 2012/05/17 15:45:46, sky wrote: > remove second gesture_recognizer. Agg! I blame this one on a git rebase. Done. http://codereview.chromium.org/10365009/diff/6008/ui/aura/root_window.h File ui/aura/root_window.h (right): http://codereview.chromium.org/10365009/diff/6008/ui/aura/root_window.h#newco... ui/aura/root_window.h:297: virtual ui::TouchEvent* CreateTouchEvent(ui::EventType type, On 2012/05/17 15:45:46, sky wrote: > wrap type to the next line so that all args line up. Do the same thing on 289. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/girard@chromium.org/10365009/5014
Change committed as 137920 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
