Refactors away method implementations in ui::EventTargeter

ui/events/event_processor_unittest.cc

Index: ui/events/event_processor_unittest.cc
diff --git a/ui/events/event_processor_unittest.cc b/ui/events/event_processor_unittest.cc
index 4bf4f0dff9122a4c426dfa067024042f6cd5637d..f09a673f5dec6ec61977c71be5a4812ff3b980e0 100644
--- a/ui/events/event_processor_unittest.cc
+++ b/ui/events/event_processor_unittest.cc
@@ -6,6 +6,7 @@
 
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/events/event.h"
+#include "ui/events/event_target_iterator.h"
 #include "ui/events/event_targeter.h"
 #include "ui/events/event_utils.h"
 #include "ui/events/test/events_test_utils.h"
@@ -18,6 +19,46 @@ typedef std::vector<std::string> HandlerSequenceRecorder;
 namespace ui {
 namespace test {
 
+class RecursiveEventTargeter : public EventTargeter {

[tdanderson, 2015/05/22 18:29:56]

I'm not sure if this class is necessary. WindowTargeterTest* should (and does)
test the logic of WindowTargeter's implementation of FindTargetForEvent() and
FindNextBestTarget(). Similarly for ViewTargeterTest*. Since the targeting logic
is tested elsewhere, it seems reasonable for RecursiveEventTargeter to be a
small test double like BubblingEventTargeter. This would avoid having to
duplicate the implementation of WindowTargeter::FindTargetForEvent(), which can
become out of date if the implementation ever changes.

What do you think of the following idea? Remove RecursiveEventTargeter and
instead change BubblingEventTargeter to a more robust class:

class TestEventTargeter : public EventTargeter {
 public:
  TestEventTargeter(TestEventTarget* initial_target, bool bubble)
    : initial_target_(initial_target),
      should_bubble_(bubble) {}

  void set_initial_target(TestEventTarget* initial_target) {
    initial_target_ = initial_target;
  }

  EventTarget* FindTargetForEvent(...) override {
    return initial_target_;
  }

  EventTarget* FindNextBestTarget(...) override {
    if (!should_bubble_)
      return nullptr;
    return previous_target->GetParentTarget();
  }

 private:
  TestEventTarget* initial_target_;
  bool should_bubble_;

  DISALLOW_COPY_AND_ASSIGN(TestEventTargeter);
};

Then in SetUp() you can initialize TestEventTargeter(nullptr, false) and call
set_initial_target() within each test as needed.

Ideally TestEventTargeter should be defined in its own file located in
ui/events/test, similar to what is done for TestEventTarget.

WDYT?

[varkha, 2015/05/22 22:18:34]

Done. I was considering it but at the time it seemed that the logic with a
targeter responding to adding and removing children should be automated in the
targeter. I think your suggestion has a net positive impact though since the
payback in the test is localized and simple and we would not need to maintain
two copies of recursive logic. Thanks!

+ public:
+  RecursiveEventTargeter() {}
+  ~RecursiveEventTargeter() override {}
+
+  // EventTargeter:
+  EventTarget* FindTargetForEvent(EventTarget* root, Event* event) override {
+    if (!event->IsLocatedEvent())
+      return root;
+    LocatedEvent* located_event = static_cast<LocatedEvent*>(event);
+
+    scoped_ptr<EventTargetIterator> iter = root->GetChildIterator();
+    if (iter) {
+      EventTarget* target = root;
+      for (EventTarget* child = iter->GetNextTarget(); child;
+           child = iter->GetNextTarget()) {
+        EventTargeter* targeter = child->GetEventTargeter();
+        if (!targeter)
+          targeter = this;
+        target->ConvertEventToTarget(child, located_event);
+        target = child;
+        EventTarget* child_target =
+            targeter->FindTargetForEvent(child, located_event);
+        if (child_target)
+          return child_target;
+      }
+      target->ConvertEventToTarget(root, located_event);
+    }
+    return root->CanAcceptEvent(*event) ? root : NULL;
+  }
+
+  EventTarget* FindNextBestTarget(EventTarget* previous_target,
+                                  Event* event) override {
+    return nullptr;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(RecursiveEventTargeter);
+};
+
 class EventProcessorTest : public testing::Test {
  public:
   EventProcessorTest() {}
@@ -27,7 +68,7 @@ class EventProcessorTest : public testing::Test {
   void SetUp() override {
     processor_.SetRoot(scoped_ptr<EventTarget>(new TestEventTarget()));
     processor_.Reset();
-    root()->SetEventTargeter(make_scoped_ptr(new EventTargeter()));
+    root()->SetEventTargeter(make_scoped_ptr(new RecursiveEventTargeter()));
   }
 
   TestEventTarget* root() {
@@ -50,6 +91,7 @@ class EventProcessorTest : public testing::Test {
 
 TEST_F(EventProcessorTest, Basic) {
   scoped_ptr<TestEventTarget> child(new TestEventTarget());
+  child->SetEventTargeter(make_scoped_ptr(new RecursiveEventTargeter()));
   root()->AddChild(child.Pass());
 
   MouseEvent mouse(ET_MOUSE_MOVED, gfx::Point(10, 10), gfx::Point(10, 10),
@@ -63,116 +105,6 @@ TEST_F(EventProcessorTest, Basic) {
   EXPECT_TRUE(root()->DidReceiveEvent(ET_MOUSE_MOVED));
 }
 
-template<typename T>
-class BoundsEventTargeter : public EventTargeter {
- public:
-  ~BoundsEventTargeter() override {}
-
- protected:
-  bool SubtreeShouldBeExploredForEvent(
-      EventTarget* target, const LocatedEvent& event) override {
-    T* t = static_cast<T*>(target);
-    return (t->bounds().Contains(event.location()));
-  }
-};
-
-class BoundsTestTarget : public TestEventTarget {
- public:
-  BoundsTestTarget() {}
-  ~BoundsTestTarget() override {}
-
-  void set_bounds(gfx::Rect rect) { bounds_ = rect; }
-  gfx::Rect bounds() const { return bounds_; }
-
-  static void ConvertPointToTarget(BoundsTestTarget* source,
-                                   BoundsTestTarget* target,
-                                   gfx::Point* location) {
-    gfx::Vector2d vector;
-    if (source->Contains(target)) {
-      for (; target && target != source;
-           target = static_cast<BoundsTestTarget*>(target->parent())) {
-        vector += target->bounds().OffsetFromOrigin();
-      }
-      *location -= vector;
-    } else if (target->Contains(source)) {
-      for (; source && source != target;
-           source = static_cast<BoundsTestTarget*>(source->parent())) {
-        vector += source->bounds().OffsetFromOrigin();
-      }
-      *location += vector;
-    } else {
-      NOTREACHED();
-    }
-  }
-
- private:
-  // EventTarget:
-  void ConvertEventToTarget(EventTarget* target, LocatedEvent* event) override {
-    event->ConvertLocationToTarget(this,
-                                   static_cast<BoundsTestTarget*>(target));
-  }
-
-  gfx::Rect bounds_;
-
-  DISALLOW_COPY_AND_ASSIGN(BoundsTestTarget);
-};
-
-TEST_F(EventProcessorTest, Bounds) {
-  scoped_ptr<BoundsTestTarget> parent(new BoundsTestTarget());
-  scoped_ptr<BoundsTestTarget> child(new BoundsTestTarget());
-  scoped_ptr<BoundsTestTarget> grandchild(new BoundsTestTarget());
-
-  parent->set_bounds(gfx::Rect(0, 0, 30, 30));
-  child->set_bounds(gfx::Rect(5, 5, 20, 20));
-  grandchild->set_bounds(gfx::Rect(5, 5, 5, 5));
-
-  child->AddChild(scoped_ptr<TestEventTarget>(grandchild.Pass()));
-  parent->AddChild(scoped_ptr<TestEventTarget>(child.Pass()));
-  root()->AddChild(scoped_ptr<TestEventTarget>(parent.Pass()));
-
-  ASSERT_EQ(1u, root()->child_count());
-  ASSERT_EQ(1u, root()->child_at(0)->child_count());
-  ASSERT_EQ(1u, root()->child_at(0)->child_at(0)->child_count());
-
-  TestEventTarget* parent_r = root()->child_at(0);
-  TestEventTarget* child_r = parent_r->child_at(0);
-  TestEventTarget* grandchild_r = child_r->child_at(0);
-
-  // Dispatch a mouse event that falls on the parent, but not on the child. When
-  // the default event-targeter used, the event will still reach |grandchild|,
-  // because the default targeter does not look at the bounds.
-  MouseEvent mouse(ET_MOUSE_MOVED, gfx::Point(1, 1), gfx::Point(1, 1),
-                   EventTimeForNow(), EF_NONE, EF_NONE);
-  DispatchEvent(&mouse);
-  EXPECT_FALSE(root()->DidReceiveEvent(ET_MOUSE_MOVED));
-  EXPECT_FALSE(parent_r->DidReceiveEvent(ET_MOUSE_MOVED));
-  EXPECT_FALSE(child_r->DidReceiveEvent(ET_MOUSE_MOVED));
-  EXPECT_TRUE(grandchild_r->DidReceiveEvent(ET_MOUSE_MOVED));
-  grandchild_r->ResetReceivedEvents();
-
-  // Now install a targeter on the parent that looks at the bounds and makes
-  // sure the event reaches the target only if the location of the event within
-  // the bounds of the target.
-  MouseEvent mouse2(ET_MOUSE_MOVED, gfx::Point(1, 1), gfx::Point(1, 1),
-                    EventTimeForNow(), EF_NONE, EF_NONE);
-  parent_r->SetEventTargeter(scoped_ptr<EventTargeter>(
-      new BoundsEventTargeter<BoundsTestTarget>()));
-  DispatchEvent(&mouse2);
-  EXPECT_FALSE(root()->DidReceiveEvent(ET_MOUSE_MOVED));
-  EXPECT_TRUE(parent_r->DidReceiveEvent(ET_MOUSE_MOVED));
-  EXPECT_FALSE(child_r->DidReceiveEvent(ET_MOUSE_MOVED));
-  EXPECT_FALSE(grandchild_r->DidReceiveEvent(ET_MOUSE_MOVED));
-  parent_r->ResetReceivedEvents();
-
-  MouseEvent second(ET_MOUSE_MOVED, gfx::Point(12, 12), gfx::Point(12, 12),
-                    EventTimeForNow(), EF_NONE, EF_NONE);
-  DispatchEvent(&second);
-  EXPECT_FALSE(root()->DidReceiveEvent(ET_MOUSE_MOVED));
-  EXPECT_FALSE(parent_r->DidReceiveEvent(ET_MOUSE_MOVED));
-  EXPECT_FALSE(child_r->DidReceiveEvent(ET_MOUSE_MOVED));
-  EXPECT_TRUE(grandchild_r->DidReceiveEvent(ET_MOUSE_MOVED));
-}
-
 // ReDispatchEventHandler is used to receive mouse events and forward them
 // to a specified EventProcessor. Verifies that the event has the correct
 // target and phase both before and after the nested event processing. Also
@@ -222,7 +154,7 @@ TEST_F(EventProcessorTest, NestedEventProcessing) {
   scoped_ptr<EventTarget> second_root_scoped(new TestEventTarget());
   TestEventTarget* second_root =
       static_cast<TestEventTarget*>(second_root_scoped.get());
-  second_root->SetEventTargeter(make_scoped_ptr(new EventTargeter()));
+  second_root->SetEventTargeter(make_scoped_ptr(new RecursiveEventTargeter()));
   scoped_ptr<TestEventTarget> second_child(new TestEventTarget());
   second_root->AddChild(second_child.Pass());
 
@@ -319,43 +251,6 @@ TEST_F(EventProcessorTest, OnEventProcessingStarted) {
   EXPECT_EQ(1, processor()->num_times_processing_finished());
 }
 
-class IgnoreEventTargeter : public EventTargeter {
- public:
-  IgnoreEventTargeter() {}
-  ~IgnoreEventTargeter() override {}
-
- private:
-  // EventTargeter:
-  bool SubtreeShouldBeExploredForEvent(EventTarget* target,
-                                       const LocatedEvent& event) override {
-    return false;
-  }
-};
-
-// Verifies that the EventTargeter installed on an EventTarget can dictate
-// whether the target itself can process an event.
-TEST_F(EventProcessorTest, TargeterChecksOwningEventTarget) {
-  scoped_ptr<TestEventTarget> child(new TestEventTarget());
-  root()->AddChild(child.Pass());
-
-  MouseEvent mouse(ET_MOUSE_MOVED, gfx::Point(10, 10), gfx::Point(10, 10),
-                   EventTimeForNow(), EF_NONE, EF_NONE);
-  DispatchEvent(&mouse);
-  EXPECT_TRUE(root()->child_at(0)->DidReceiveEvent(ET_MOUSE_MOVED));
-  EXPECT_FALSE(root()->DidReceiveEvent(ET_MOUSE_MOVED));
-  root()->child_at(0)->ResetReceivedEvents();
-
-  // Install an event handler on |child| which always prevents the target from
-  // receiving event.
-  root()->child_at(0)->SetEventTargeter(
-      scoped_ptr<EventTargeter>(new IgnoreEventTargeter()));
-  MouseEvent mouse2(ET_MOUSE_MOVED, gfx::Point(10, 10), gfx::Point(10, 10),
-                    EventTimeForNow(), EF_NONE, EF_NONE);
-  DispatchEvent(&mouse2);
-  EXPECT_FALSE(root()->child_at(0)->DidReceiveEvent(ET_MOUSE_MOVED));
-  EXPECT_TRUE(root()->DidReceiveEvent(ET_MOUSE_MOVED));
-}
-
 // An EventTargeter which is used to allow a bubbling behaviour in event
 // dispatch: if an event is not handled after being dispatched to its
 // initial target, the event is dispatched to the next-best target as