Do not call into EventTargeter methods from ViewTargeter

Now that hit-testing for views (crbug.com/388838) and
event-targeting for views (crbug.com/391845) use
ViewTargeterDelegate to define custom behaviour for
these operations (instead of the now-obsolete
approach of subclassing ViewTargeter itself), we
should not be calling into EventTargeter for this
purpose. Changes in this CL:

* Finding the target of a scroll event is now
  performed correctly by using the logic in
  ViewTargeterDelegate and its overrides.

* The dead code
  ViewTargeter::SubtreeCanAcceptEvent(),
  ViewTargeter::EventLocationInsideBounds(),
  and ViewTargeter::BoundsForEvent() have been
  removed along with their corresponding tests.

* The class-level documentation for ViewTargeter
  and ViewTargeterDelegate has been updated
  to clarify the purpose of these classes.

BUG=395387,370579
TEST=ViewTargeterTest.*

Review URL: https://codereview.chromium.org/426443002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@287620 0039d316-1c4b-4281-b951-d872f2087c98

ui/views/view.cc

@@ -1030,6 +1030,15 @@ View::SetEventTargeter(scoped_ptr<ViewTargeter> targeter) {
   return old_targeter.Pass();
 }
 
+ViewTargeter* View::GetEffectiveViewTargeter() const {
+  DCHECK(GetWidget());
+  ViewTargeter* view_targeter = targeter();
+  if (!view_targeter)
+    view_targeter = GetWidget()->GetRootView()->targeter();
+  CHECK(view_targeter);
+  return view_targeter;
+}
+
 bool View::CanAcceptEvent(const ui::Event& event) {
   return IsDrawn();
 }
@@ -2270,14 +2279,6 @@ void View::ProcessMouseReleased(const ui::MouseEvent& event) {
   // WARNING: we may have been deleted.
 }
 
-ViewTargeter* View::GetEffectiveViewTargeter() const {
-  ViewTargeter* view_targeter = targeter();
-  if (!view_targeter)
-    view_targeter = GetWidget()->GetRootView()->targeter();
-  CHECK(view_targeter);
-  return view_targeter;
-}
-
 // Accelerators ----------------------------------------------------------------
 
 void View::RegisterPendingAccelerators() {

ui/views/view.h

@@ -704,6 +704,13 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
   // ViewTargeter.
   scoped_ptr<ViewTargeter> SetEventTargeter(scoped_ptr<ViewTargeter> targeter);
 
+  // Returns the ViewTargeter installed on |this| if one exists,
+  // otherwise returns the ViewTargeter installed on our root view.
+  // The return value is guaranteed to be non-null.
+  ViewTargeter* GetEffectiveViewTargeter() const;
+
+  ViewTargeter* targeter() const { return targeter_.get(); }
+
   // Overridden from ui::EventTarget:
   virtual bool CanAcceptEvent(const ui::Event& event) OVERRIDE;
   virtual ui::EventTarget* GetParentTarget() OVERRIDE;
@@ -712,8 +719,6 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
   virtual void ConvertEventToTarget(ui::EventTarget* target,
                                     ui::LocatedEvent* event) OVERRIDE;
 
-  ViewTargeter* targeter() const { return targeter_.get(); }
-
   // Overridden from ui::EventHandler:
   virtual void OnKeyEvent(ui::KeyEvent* event) OVERRIDE;
   virtual void OnMouseEvent(ui::MouseEvent* event) OVERRIDE;
@@ -1390,11 +1395,6 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
   bool ProcessMouseDragged(const ui::MouseEvent& event);
   void ProcessMouseReleased(const ui::MouseEvent& event);
 
-  // Returns the ViewTargeter installed on |this| if one exists,
-  // otherwise returns the ViewTargeter installed on our root view.
-  // The return value is guaranteed to be non-null.
-  ViewTargeter* GetEffectiveViewTargeter() const;
-
   // Accelerators --------------------------------------------------------------
 
   // Registers this view's keyboard accelerators that are not registered to

ui/views/view_targeter.cc

@@ -27,23 +27,17 @@ View* ViewTargeter::TargetForRect(View* root, const gfx::Rect& rect) const {
   return delegate_->TargetForRect(root, rect);
 }
 
-gfx::RectF ViewTargeter::BoundsForEvent(const ui::LocatedEvent& event) const {
-  gfx::RectF event_bounds(event.location_f(), gfx::SizeF(1, 1));
-  if (event.IsGestureEvent()) {
-    const ui::GestureEvent& gesture = *(event.AsGestureEvent());
-    event_bounds = gesture.details().bounding_box_f();
-  }
-
-  return event_bounds;
-}
-
 ui::EventTarget* ViewTargeter::FindTargetForEvent(ui::EventTarget* root,
                                                   ui::Event* event) {
   View* view = static_cast<View*>(root);
+
   if (event->IsKeyEvent())
     return FindTargetForKeyEvent(view, *static_cast<ui::KeyEvent*>(event));
-  else if (event->IsScrollEvent())
-    return EventTargeter::FindTargetForEvent(root, event);
+
+  if (event->IsScrollEvent()) {
+    return FindTargetForScrollEvent(view,
+                                    *static_cast<ui::ScrollEvent*>(event));
+  }
 
   NOTREACHED() << "ViewTargeter does not yet support this event type.";
   return NULL;
@@ -58,35 +52,27 @@ ui::EventTarget* ViewTargeter::FindNextBestTarget(
 bool ViewTargeter::SubtreeCanAcceptEvent(
     ui::EventTarget* target,
     const ui::LocatedEvent& event) const {
-  views::View* view = static_cast<views::View*>(target);
-  if (!view->visible())
-    return false;
-
-  if (!view->CanProcessEventsWithinSubtree())
-    return false;
-
-  return true;
+  NOTREACHED();
+  return false;
 }
 
 bool ViewTargeter::EventLocationInsideBounds(
     ui::EventTarget* target,
     const ui::LocatedEvent& event) const {
-  views::View* view = static_cast<views::View*>(target);
-  gfx::Rect rect(event.location(), gfx::Size(1, 1));
-  gfx::RectF rect_in_view_coords_f(rect);
-  if (view->parent())
-    View::ConvertRectToTarget(view->parent(), view, &rect_in_view_coords_f);
-  gfx::Rect rect_in_view_coords = gfx::ToEnclosingRect(rect_in_view_coords_f);
-
-  // TODO(tdanderson): Don't call into HitTestRect() directly here.
-  //                   See crbug.com/370579.
-  return view->HitTestRect(rect_in_view_coords);
+  NOTREACHED();
+  return false;
 }
 
-View* ViewTargeter::FindTargetForKeyEvent(View* view, const ui::KeyEvent& key) {
-  if (view->GetFocusManager())
-    return view->GetFocusManager()->GetFocusedView();
+View* ViewTargeter::FindTargetForKeyEvent(View* root, const ui::KeyEvent& key) {
+  if (root->GetFocusManager())
+    return root->GetFocusManager()->GetFocusedView();
   return NULL;
 }
 
-}  // namespace aura
+View* ViewTargeter::FindTargetForScrollEvent(View* root,
+                                             const ui::ScrollEvent& scroll) {
+  gfx::Rect rect(scroll.location(), gfx::Size(1, 1));
+  return root->GetEffectiveViewTargeter()->TargetForRect(root, rect);
+}
+
+}  // namespace views

ui/views/view_targeter.h

@@ -13,13 +13,8 @@ namespace views {
 class View;
 class ViewTargeterDelegate;
 
-// Contains the logic used to determine the View to which an
-// event should be dispatched. A ViewTargeter (or one of its
-// derived classes) is installed on a View to specify the
-// targeting behaviour to be used for the subtree rooted at
-// that View.
-// TODO(tdanderson): Remove overrides of all EventHandler methods except for
-//                   FindTargetForEvent() and FindNextBestTarget().
+// A ViewTargeter is installed on a View that wishes to use the custom
+// hit-testing or event-targeting behaviour defined by |delegate|.
 class VIEWS_EXPORT ViewTargeter : public ui::EventTargeter {
  public:
   explicit ViewTargeter(ViewTargeterDelegate* delegate);
@@ -32,11 +27,6 @@ class VIEWS_EXPORT ViewTargeter : public ui::EventTargeter {
   View* TargetForRect(View* root, const gfx::Rect& rect) const;
 
  protected:
-  // Returns the location of |event| represented as a rect. If |event| is
-  // a gesture event, its bounding box is returned. Otherwise, a 1x1 rect
-  // having its origin at the location of |event| is returned.
-  gfx::RectF BoundsForEvent(const ui::LocatedEvent& event) const;
-
   // ui::EventTargeter:
   virtual ui::EventTarget* FindTargetForEvent(ui::EventTarget* root,
                                               ui::Event* event) OVERRIDE;
@@ -50,7 +40,8 @@ class VIEWS_EXPORT ViewTargeter : public ui::EventTargeter {
       const ui::LocatedEvent& event) const OVERRIDE;
 
  private:
-  View* FindTargetForKeyEvent(View* view, const ui::KeyEvent& key);
+  View* FindTargetForKeyEvent(View* root, const ui::KeyEvent& key);
+  View* FindTargetForScrollEvent(View* root, const ui::ScrollEvent& scroll);
 
   // ViewTargeter does not own the |delegate_|, but |delegate_| must
   // outlive the targeter.

ui/views/view_targeter_delegate.h

@@ -15,9 +15,11 @@ class Rect;
 namespace views {
 class View;
 
-// Defines the default behaviour for hit-testing a rectangular region against
-// the bounds of a View. Subclasses of View wishing to define custom
-// hit-testing behaviour can extend this class.
+// Defines the default behaviour for hit-testing and event-targeting against a
+// View using a rectangular region representing an event's location. Views
+// wishing to define custom hit-testing or event-targeting behaviour do so by
+// extending ViewTargeterDelegate and then installing a ViewTargeter on
+// themselves.
 class VIEWS_EXPORT ViewTargeterDelegate {
  public:
   ViewTargeterDelegate() {}

ui/views/view_targeter_unittest.cc

@@ -208,140 +208,6 @@ TEST_F(ViewTargeterTest, ViewTargeterForScrollEvents) {
   EXPECT_EQ(content, static_cast<View*>(current_target));
 }
 
-// Tests the basic functionality of the method
-// ViewTargeter::SubtreeShouldBeExploredForEvent().
-TEST_F(ViewTargeterTest, SubtreeShouldBeExploredForEvent) {
-  Widget widget;
-  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
-  params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
-  params.bounds = gfx::Rect(0, 0, 650, 650);
-  widget.Init(params);
-
-  internal::RootView* root_view =
-      static_cast<internal::RootView*>(widget.GetRootView());
-  ViewTargeter* targeter = new ViewTargeter(root_view);
-  root_view->SetEventTargeter(make_scoped_ptr(targeter));
-
-  // The coordinates used for SetBounds() are in the parent coordinate space.
-  View v1, v2, v3;
-  v1.SetBounds(0, 0, 300, 300);
-  v2.SetBounds(100, 100, 100, 100);
-  v3.SetBounds(0, 0, 10, 10);
-  v3.SetVisible(false);
-  root_view->AddChildView(&v1);
-  v1.AddChildView(&v2);
-  v2.AddChildView(&v3);
-
-  // Note that the coordinates used below are in |v1|'s coordinate space,
-  // and that SubtreeShouldBeExploredForEvent() expects the event location
-  // to be in the coordinate space of the target's parent. |v1| and
-  // its parent share a common coordinate space.
-
-  // Event located within |v1| only.
-  gfx::Point point(10, 10);
-  ui::MouseEvent event(ui::ET_MOUSE_PRESSED, point, point,
-                       ui::EF_LEFT_MOUSE_BUTTON, ui::EF_LEFT_MOUSE_BUTTON);
-  EXPECT_TRUE(targeter->SubtreeShouldBeExploredForEvent(&v1, event));
-  EXPECT_FALSE(targeter->SubtreeShouldBeExploredForEvent(&v2, event));
-  v1.ConvertEventToTarget(&v2, &event);
-  EXPECT_FALSE(targeter->SubtreeShouldBeExploredForEvent(&v3, event));
-
-  // Event located within |v1| and |v2| only.
-  event.set_location(gfx::Point(150, 150));
-  EXPECT_TRUE(targeter->SubtreeShouldBeExploredForEvent(&v1, event));
-  EXPECT_TRUE(targeter->SubtreeShouldBeExploredForEvent(&v2, event));
-  v1.ConvertEventToTarget(&v2, &event);
-  EXPECT_FALSE(targeter->SubtreeShouldBeExploredForEvent(&v3, event));
-
-  // Event located within |v1|, |v2|, and |v3|. Note that |v3| is not
-  // visible, so it cannot handle the event.
-  event.set_location(gfx::Point(105, 105));
-  EXPECT_TRUE(targeter->SubtreeShouldBeExploredForEvent(&v1, event));
-  EXPECT_TRUE(targeter->SubtreeShouldBeExploredForEvent(&v2, event));
-  v1.ConvertEventToTarget(&v2, &event);
-  EXPECT_FALSE(targeter->SubtreeShouldBeExploredForEvent(&v3, event));
-
-  // Event located outside the bounds of all views.
-  event.set_location(gfx::Point(400, 400));
-  EXPECT_FALSE(targeter->SubtreeShouldBeExploredForEvent(&v1, event));
-  EXPECT_FALSE(targeter->SubtreeShouldBeExploredForEvent(&v2, event));
-  v1.ConvertEventToTarget(&v2, &event);
-  EXPECT_FALSE(targeter->SubtreeShouldBeExploredForEvent(&v3, event));
-}
-
-// Tests that FindTargetForEvent() returns the correct target when some
-// views in the view tree return false when CanProcessEventsWithinSubtree()
-// is called on them.
-TEST_F(ViewTargeterTest, CanProcessEventsWithinSubtree) {
-  Widget widget;
-  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
-  params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
-  params.bounds = gfx::Rect(0, 0, 650, 650);
-  widget.Init(params);
-
-  internal::RootView* root_view =
-      static_cast<internal::RootView*>(widget.GetRootView());
-  ViewTargeter* view_targeter = new ViewTargeter(root_view);
-  ui::EventTargeter* targeter = view_targeter;
-  root_view->SetEventTargeter(make_scoped_ptr(view_targeter));
-
-  // The coordinates used for SetBounds() are in the parent coordinate space.
-  TestingView v1, v2, v3;
-  v1.SetBounds(0, 0, 300, 300);
-  v2.SetBounds(100, 100, 100, 100);
-  v3.SetBounds(0, 0, 10, 10);
-  root_view->AddChildView(&v1);
-  v1.AddChildView(&v2);
-  v2.AddChildView(&v3);
-
-  // Note that the coordinates used below are in the coordinate space of
-  // the root view.
-
-  // Define |scroll| to be (105, 105) (in the coordinate space of the root
-  // view). This is located within all of |v1|, |v2|, and |v3|.
-  gfx::Point scroll_point(105, 105);
-  ui::ScrollEvent scroll(
-      ui::ET_SCROLL, scroll_point, ui::EventTimeForNow(), 0, 0, 3, 0, 3, 2);
-
-  // If CanProcessEventsWithinSubtree() returns true for each view,
-  // |scroll| should be targeted at the deepest view in the hierarchy,
-  // which is |v3|.
-  ui::EventTarget* current_target =
-      targeter->FindTargetForEvent(root_view, &scroll);
-  EXPECT_EQ(&v3, current_target);
-
-  // If CanProcessEventsWithinSubtree() returns |false| when called
-  // on |v3|, then |v3| cannot be the target of |scroll| (this should
-  // instead be |v2|). Note we need to reset the location of |scroll|
-  // because it may have been mutated by the previous call to
-  // FindTargetForEvent().
-  scroll.set_location(scroll_point);
-  v3.set_can_process_events_within_subtree(false);
-  current_target = targeter->FindTargetForEvent(root_view, &scroll);
-  EXPECT_EQ(&v2, current_target);
-
-  // If CanProcessEventsWithinSubtree() returns |false| when called
-  // on |v2|, then neither |v2| nor |v3| can be the target of |scroll|
-  // (this should instead be |v1|).
-  scroll.set_location(scroll_point);
-  v3.Reset();
-  v2.set_can_process_events_within_subtree(false);
-  current_target = targeter->FindTargetForEvent(root_view, &scroll);
-  EXPECT_EQ(&v1, current_target);
-
-  // If CanProcessEventsWithinSubtree() returns |false| when called
-  // on |v1|, then none of |v1|, |v2| or |v3| can be the target of |scroll|
-  // (this should instead be the root view itself).
-  scroll.set_location(scroll_point);
-  v2.Reset();
-  v1.set_can_process_events_within_subtree(false);
-  current_target = targeter->FindTargetForEvent(root_view, &scroll);
-  EXPECT_EQ(root_view, current_target);
-
-  // TODO(tdanderson): We should also test that targeting works correctly
-  //                   with gestures. See crbug.com/375822.
-}
-
 // Tests that the functions ViewTargeterDelegate::DoesIntersectRect()
 // and MaskedTargeterDelegate::DoesIntersectRect() work as intended when
 // called on views which are derived from ViewTargeterDelegate.