Add gesture target fuzzing to views

ui/views/widget/root_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/views/widget/root_view.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "base/message_loop.h"
@@ skipping 436 matching lines @@
   // Reset touch_pressed_handler_ to indicate that no processing is occurring.
   touch_pressed_handler_ = NULL;
 
   return status;
 }
 
 ui::GestureStatus RootView::OnGestureEvent(const GestureEvent& event) {
   GestureEvent e(event, this);
   ui::GestureStatus status = ui::GESTURE_STATUS_UNKNOWN;
 
-  if (gesture_handler_) {
+  // TODO(tdanderson): Store radius values for a ui::ET_GESTURE_LONG_PRESS
+  //                   event so that fuzzing may also be used for a long press.
+  bool useFuzzing = event.type() == ui::ET_GESTURE_TAP;

[sky, 2012/07/17 19:54:32]

Why do this here? Shouldn't we fuzz only if there is no gesture_handler_?

[tdanderson, 2012/07/18 22:35:42]

There could be a |gesture_handler_| from either GESTURE_BEGIN or
GESTURE_TAP_DOWN but this would be based only on the center of the touch region.
We want to fuzz to possibly update |gesture_handler_| to something better.

[sky, 2012/07/19 00:06:38]

Why? It seems wrong to me to change the target view midstream. I could see us
targeting multiple views for different touch points (as we do in the aura side),
but that isn't what you're doing here.

+
+  if (useFuzzing) {
+    float radius = event.details().radius_x();
+    gfx::Point adjustedLoc(event.x() - radius, event.y() - radius);
+    ConvertPointToScreen(this, &adjustedLoc);

[sadrul, 2012/07/16 22:16:14]

Why is it necessary to convert to screen coordinate? Explain in comment.

[rjkroege, 2012/07/16 23:35:48]

Aside: are screen coords in DIP or physical?

[tdanderson, 2012/07/17 19:07:07]

Please see my reply in view.cc and let me know if you agree. If we end up using
screen coordinates, I will add a comment in the code.

[tdanderson, 2012/07/18 22:35:42]

Done.

+    gfx::Rect touchRect(adjustedLoc.x(), adjustedLoc.y(), radius*2, radius*2);
+
+    gfx::Rect closestOverlappedRect;
+    FindClosestOverlappedRect(touchRect, closestOverlappedRect);
+
+    if (!closestOverlappedRect.IsEmpty()) {
+      gfx::Point newLoc(closestOverlappedRect.CenterPoint());
+      ConvertPointFromScreen(this, &newLoc);
+      e.location_ = newLoc;

[sky, 2012/07/17 19:54:32]

What is |e|?

[tdanderson, 2012/07/18 22:35:42]

This was present before I touched the code. It is a non-const copy of |event|
and does not appear to serve any purpose (replacing |e| with |event| does not
seem to change the behavior of the code in any way). I'll remove it.

+    }
+  }
+
+  if (gesture_handler_ && !useFuzzing) {

[sadrul, 2012/07/16 22:16:14]

Hm...

I think with this change here, we can end up sending some gesture events (e.g.
GESTURE_BEGIN, GESTURE_TAP_DOWN) to a view, and then retarget the gesture events
based on the fuzzing, and send subsequent gestures (GESTURE_TAP, GESTURE_END) to
a different view. That can lead to incorrect behaviour (some views, for example
buttons, do process TAP_DOWN event). So we need to be very clear and careful on
how we change the target, e.g. perhaps we need to send a GESTURE_END to an
existing gesture_handler_, and then a sequence of GESTURE_BEGIN,
GESTURE_TAP_DOWN and GESTURE_TAP to the new target (the target may very well
expect these events before a TAP).

[tdanderson, 2012/07/17 19:07:07]

Yes, I see the problem here. But if the fuzzing logic re-targets and we send a
GESTURE_END to the original target and GESTURE_BEGIN and GESTURE_TAP_DOWN,
wouldn't this incorrectly process two gesture sequences when we only wanted one?
A GESTURE_TAP_DOWN would be dispatched on the original target and a GESTURE_TAP
would be dispatched on the new target.

There are probably several problems with another approach that comes to mind,
but what if we always applied the fuzzing logic for a GESTURE_TAP_DOWN? This way
we're not processing extra gesture sequences and more gestures would
automatically get the benefit of fuzzing.

[sadrul, 2012/07/17 19:26:45]

Yes. But I do not think we can avoid this (apart from delaying sending
BEGIN/TAP_DOWN until TAP happens so that we know the target for sure, but that's
a big no-no). Once we send a view a BEGIN and TAP_DOWN, we need to also send it
END. And before we send a TAP to another target, we need to send it a BEGIN and
TAP_DOWN to make sure it is receiving an expected and well-defined sequence of
events.
This would be unexpected. We should not do this.
TAP_DOWN happens in response to a single TOUCH_DOWN event, i.e. it will have the
radius from the touch-event, but the fuzzing here won't be as effective.

[tdanderson, 2012/07/18 22:35:42]

This is the approach I have taken in my latest CL.

     // Allow |gesture_handler_| to delete this during processing.
     View* handler = gesture_handler_;
     GestureEvent handler_event(event, this, gesture_handler_);
     // TODO: should only do this for the last touch id that goes up.
     if (event.type() == ui::ET_GESTURE_END)
       gesture_handler_ = NULL;
     return handler->ProcessGestureEvent(handler_event);
   }
 
   // Walk up the tree until we find a view that wants the gesture event.
   for (gesture_handler_ = GetEventHandlerForPoint(e.location());
       gesture_handler_ && (gesture_handler_ != this);
       gesture_handler_ = gesture_handler_->parent()) {
+
     if (!gesture_handler_->enabled()) {
       // Disabled views eat events but are treated as not handled.
       return ui::GESTURE_STATUS_UNKNOWN;
     }
 
     // See if this view wants to handle the Gesture.
     GestureEvent gesture_event(e, this, gesture_handler_);
     status = gesture_handler_->ProcessGestureEvent(gesture_event);
 
     // The view could have removed itself from the tree when handling
@@ skipping 82 matching lines @@
 }
 
 void RootView::SetMouseLocationAndFlags(const MouseEvent& event) {
   last_mouse_event_flags_ = event.flags();
   last_mouse_event_x_ = event.x();
   last_mouse_event_y_ = event.y();
 }
 
 }  // namespace internal
 }  // namespace views