Refined DCHECK preventing multiple event forwarders in VA tree path

ui/android/view_android.cc

 // Copyright 2016 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/android/view_android.h"
 
 #include <algorithm>
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
@@ skipping 86 matching lines @@
   JNIEnv* env = base::android::AttachCurrentThread();
   delegate_ = JavaObjectWeakGlobalRef(env, delegate);
 }
 
 float ViewAndroid::GetDipScale() {
   return ui::GetScaleFactorForNativeView(this);
 }
 
 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();
 }
 
 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);
   if (child->parent_)
     child->RemoveFromParent();
   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;
   }
   return false;
 }
 
 void ViewAndroid::MoveToFront(ViewAndroid* child) {
   DCHECK(child);
   auto it = std::find(children_.begin(), children_.end(), child);
@@ skipping 179 matching lines @@
         matched = bound.Contains(e->GetX(0), e->GetY(0));
       }
       if (matched && child->HitTest(send_to_client, *e))
         return true;
     }
   }
   return false;
 }
 
 }  // namespace ui