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..5da0c35fdcc722dc195a7d4f89d3860d08c48a56 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(!SubtreeOrParentsHaveEventForwarder(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(!SubtreeOrParentsHaveEventForwarder(child) || !has_event_forwarder())

[boliu, 2017/05/10 17:19:16]

has_event_forwarder is not enough. you need to check all nodes along this node
to root node path

also there's no need to walk child's root path, since we are going to remove it
from parent anyway, so in that sense, this is too strict

[Jinsuk Kim, 2017/05/10 21:57:57]

|has_event_forwarder| only checks this node, and Subtree.. checks all the
parents plus the new subtree as its name indicates.
It doesn't check child's root path but only walk child and the children nodes
below it. 

Maybe the parameter and the method name gives you the confusion. Would it be
better if I pass Subtree...(parent_, child)? I've tried it as well, but removed
|parent_| since it will always walk |this|'s parent.

[boliu, 2017/05/10 22:09:40]

so what if both are true:
this->parent_->has_event_forwarder()
child->has_event_forwarder()
but this->has_event_forwarder() is false

this case is not allowed, but DCHECK doesn't fire
What if child->parent_->has_event_forwarder() is true
SubtreeHasEventForwarder(child) is false, and this->has_event_forwarder() is
true. In this case, adding the child is fine, since child be removed from its
previous parent, but DCHECK actually fires.

[Jinsuk Kim, 2017/05/10 22:21:24]

I'm afraid you don't get the change right. See the line 131:

ViewAndroid* view = parent_;  // it's not child's parent but |this|'s parent.

Subtree... checks both |this|'s parent and child, so DCHECK detects it.
child's parent is not checked. Adding the child is fine in this case and DCHECK
doesn't get fired.
I'll probably split the method to avoid the confusion.

[boliu, 2017/05/10 22:24:34]

Oh yeah. That's just confusing. Rewrite it.

+      << "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,22 @@ 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::SubtreeOrParentsHaveEventForwarder(ViewAndroid* tree) {
+  // Checks from |parent_| to the root node if there's event forwarder.
+  ViewAndroid* view = parent_;
+  while (view) {
+    if (view->has_event_forwarder())
       return true;
-    v = v->parent_;
-  } while (v);
-  return SubtreeHasEventForwarder(view);
+    view = view->parent_;
+  }
+  return SubtreeHasEventForwarder(tree);
 }
 
 // static
 bool ViewAndroid::SubtreeHasEventForwarder(ViewAndroid* view) {
   if (view->has_event_forwarder())
     return true;
+
   for (auto* child : view->children_) {
     if (SubtreeHasEventForwarder(child))
       return true;