Refined DCHECK preventing multiple event forwarders in VA tree path

ui/android/view_android.cc

Index: ui/android/view_android.cc
diff --git a/ui/android/view_android.cc b/ui/android/view_android.cc
index 15e51a0042ca48b6eb5c29d0bbbc6cafdb9541a3..e37772bbe89b6659b006d8a8a6053fc0260fa861 100644
--- a/ui/android/view_android.cc
+++ b/ui/android/view_android.cc
@@ -104,8 +104,8 @@ float ViewAndroid::GetDipScale() {
 
 ScopedJavaLocalRef<jobject> ViewAndroid::GetEventForwarder() {
   if (!event_forwarder_) {
-    DCHECK(!ViewTreeHasEventForwarder(this))
-        << "Root of the ViewAndroid can have at most one handler.";
+    DCHECK(!OtherNodesHaveEventForwarder(this))
+        << "The view tree path already has an event forwarder.";
     event_forwarder_.reset(new EventForwarder(this));
   }
   return event_forwarder_->GetJavaObject();
@@ -115,8 +115,9 @@ void ViewAndroid::AddChild(ViewAndroid* child) {
   DCHECK(child);
   DCHECK(std::find(children_.begin(), children_.end(), child) ==
          children_.end());
-  DCHECK(!SubtreeHasEventForwarder(child) || !ViewTreeHasEventForwarder(this))
-      << "Only one event handler is allowed.";
+  DCHECK(!OtherNodesHaveEventForwarder(child) || !has_event_forwarder())
+      << "Some view tree path will have more than one event forwarder "
+         "if the child is added.";
 
   // The new child goes to the top, which is the end of the list.
   children_.push_back(child);
@@ -125,21 +126,27 @@ void ViewAndroid::AddChild(ViewAndroid* child) {
   child->parent_ = this;
 }
 
-// static
-bool ViewAndroid::ViewTreeHasEventForwarder(ViewAndroid* view) {
-  ViewAndroid* v = view;
-  do {
-    if (v->has_event_forwarder())
+bool ViewAndroid::OtherNodesHaveEventForwarder(ViewAndroid* tree) {
+  // Note that it's |this| node's parents that are being checked here,
+  // not those of the |tree|.
+  return ParentsHaveEventForwarder() || SubtreeHasEventForwarder(tree);

[boliu, 2017/05/10 22:48:43]

this method is still super confusing, and can easily be improved

make all of these tree traversal methods static so caller always have to
explicitly pass in the node. then have two separate checks, the "parent path"
check and the "subtree" check, and have DCHECKs directly call them

[Jinsuk Kim, 2017/05/10 23:17:56]

Done.

+}
+
+bool ViewAndroid::ParentsHaveEventForwarder() {
+  ViewAndroid* view = parent_;
+  while (view) {
+    if (view->has_event_forwarder())
       return true;
-    v = v->parent_;
-  } while (v);
-  return SubtreeHasEventForwarder(view);
+    view = view->parent_;
+  }
+  return false;
 }
 
 // static
 bool ViewAndroid::SubtreeHasEventForwarder(ViewAndroid* view) {
   if (view->has_event_forwarder())
     return true;
+
   for (auto* child : view->children_) {
     if (SubtreeHasEventForwarder(child))
       return true;