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..75156d58b9c46d65c2f9d8bfe56bff3b90a4ffc2 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(!HasMultipleEventForwarder(this, true, this))
+        << "The view tree path already has an event handler.";
     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(!HasMultipleEventForwarder(this, has_event_forwarder(), child))
+      << "Some view tree path will have more than one event handler after "
+         "the child is added.";
 
   // The new child goes to the top, which is the end of the list.
   children_.push_back(child);
@@ -126,22 +127,35 @@ void ViewAndroid::AddChild(ViewAndroid* child) {
 }
 
 // static
-bool ViewAndroid::ViewTreeHasEventForwarder(ViewAndroid* view) {
-  ViewAndroid* v = view;
-  do {
-    if (v->has_event_forwarder())
+bool ViewAndroid::HasMultipleEventForwarder(ViewAndroid* node,

[boliu, 2017/05/10 01:57:16]

this is not correct afaict, if there's a single forwarder in the node to root
path, then it will be counted twice, once going up to root and once coming down

also this is totally not required. you should be able to do all the checks just
by chekcing if there are forwarders or not, instead of having this complicated
the thing that tries to find if it's less that two or not

[Jinsuk Kim, 2017/05/10 06:36:12]

Fixed the bug, and simplified the logic.

+                                            bool has_forwarder,
+                                            ViewAndroid* tree) {
+  ViewAndroid* v = node->parent_;
+  bool forwarder_found = has_forwarder;
+
+  // Checks if there are multiple event forwarders from here to the root node.
+  while (v) {
+    if (forwarder_found && v->has_event_forwarder())
       return true;
+    forwarder_found |= v->has_event_forwarder();
     v = v->parent_;
-  } while (v);
-  return SubtreeHasEventForwarder(view);
+  }
+
+  // Checks of there is any path from |tree| to leaf nodes that has multiple
+  // event forwarders.
+  return HasMultipleEventForwarderInAnyPath(tree, forwarder_found);
 }
 
 // static
-bool ViewAndroid::SubtreeHasEventForwarder(ViewAndroid* view) {
-  if (view->has_event_forwarder())
+bool ViewAndroid::HasMultipleEventForwarderInAnyPath(ViewAndroid* node,
+                                                     bool forwarder_found) {
+  bool has_forwarder = node->has_event_forwarder();
+  if (forwarder_found && has_forwarder)
     return true;
-  for (auto* child : view->children_) {
-    if (SubtreeHasEventForwarder(child))
+
+  forwarder_found |= has_forwarder;
+  for (auto* child : node->children_) {
+    if (HasMultipleEventForwarderInAnyPath(child, forwarder_found))
       return true;
   }
   return false;