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(!HasMultipleEventForwarder(this, true, this))
+        << "The view tree path already has an event handler.";
     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(!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);
   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::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;
 }
 
 void ViewAndroid::MoveToFront(ViewAndroid* child) {
   DCHECK(child);
   auto it = std::find(children_.begin(), children_.end(), child);
   DCHECK(it != children_.end());
 
@@ skipping 177 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