Refined DCHECK preventing multiple event forwarders in VA tree path

Refined DCHECK preventing multiple event forwarders in VA tree path

A ViewAndroid tree path from the top to a leaf node can have only
a single EventForwarder instance. Rewrote helper methods used for
DCHECK that enforces the requirement to match the actual job with
the intention.

BUG=717271
Review-Url: https://codereview.chromium.org/2861413002
Cr-Commit-Position: refs/heads/master@{#470814}
Committed: https://chromium.googlesource.com/chromium/src/+/1a116ecfdf829702b653ded84a0e1cdb8bdfd7d6

[Jinsuk Kim, 2017-05-08 04:59:22]

jinsukkim@chromium.org changed reviewers:
+ boliu@chromium.org

[Jinsuk Kim, 2017-05-08 04:59:23]

To illustrate the point, following WindowAndroid can have as many as 3
EventForwarder, one per each WCVA ViewAndroid in its subtree. So |AddChild|
called on WindowAndroid should not be DCHECKED against unique EventForwarder
requirement. I think I forgot to update the DCHECK condition when backing off
from the design where WindowAndroid is not ViewAndroid.

    o (WindowAndroid)
    |
o --o---o  ViewAndroid (for WCVA, has EventForwarder)
|   |   | 
o   o   o  ViewAndroid (RWHVA)

[boliu, 2017-05-08 16:54:55]

https://codereview.chromium.org/2861413002/diff/20001/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2861413002/diff/20001/ui/android/view_android...
ui/android/view_android.cc:119: !ViewTreeHasEventForwarder(this))
the previous check is too strict. check should be there's at most one forwarder
from node to root path, but previous code is checking there's only one forwarder
in the entire tree

but I have no idea what this new check does now

[Jinsuk Kim, 2017-05-10 00:53:32]

Patchset #3 (id:40001) has been deleted

[Jinsuk Kim, 2017-05-10 01:40:55]

Description was changed from

==========
Allow WindowAndroid to have multiple event forwarders in subtrees

ViewAndroid tree can have only a single EventForwarder instance
present in the tree hierarchy. WindowAndroid which is also ViewAndroid
however should be allowed to have multiple EventForwarders in its subtrees
since current design positions EventForwarder in the ViewAndroid
associated with WebContentsViewAndroid. This CL loosens the DCHECK
in ViewAndroid::AddChild for that.

BUG=717271
==========

to

==========
Refined DCHECK preventing multiple event forwarders in VA tree path

A ViewAndroid tree path from the top to a leaf node can have only
a single EventForwarder instance. Rewrote helper methods used for
DCHECK that enforces the requirement to match the actual job with
the intention.

BUG=717271
==========

[Jinsuk Kim, 2017-05-10 01:42:10]

https://codereview.chromium.org/2861413002/diff/20001/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2861413002/diff/20001/ui/android/view_android...
ui/android/view_android.cc:119: !ViewTreeHasEventForwarder(this))
On 2017/05/08 16:54:55, boliu wrote:
> the previous check is too strict. check should be there's at most one
forwarder
> from node to root path, but previous code is checking there's only one
forwarder
> in the entire tree
> 
> but I have no idea what this new check does now

Updated the DCHECK to do what's actually intended.

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

https://codereview.chromium.org/2861413002/diff/60001/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2861413002/diff/60001/ui/android/view_android...
ui/android/view_android.cc:130: bool
ViewAndroid::HasMultipleEventForwarder(ViewAndroid* node,
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 04:32:27]

Patchset #4 (id:80001) has been deleted

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

https://codereview.chromium.org/2861413002/diff/60001/ui/android/view_android.cc
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2861413002/diff/60001/ui/android/view_android...
ui/android/view_android.cc:130: bool
ViewAndroid::HasMultipleEventForwarder(ViewAndroid* node,
On 2017/05/10 01:57:16, boliu wrote:
> 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

Fixed the bug, and simplified the logic.

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

https://codereview.chromium.org/2861413002/diff/100001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2861413002/diff/100001/ui/android/view_androi...
ui/android/view_android.cc:118:
DCHECK(!SubtreeOrParentsHaveEventForwarder(child) || !has_event_forwarder())
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:58]

https://codereview.chromium.org/2861413002/diff/100001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2861413002/diff/100001/ui/android/view_androi...
ui/android/view_android.cc:118:
DCHECK(!SubtreeOrParentsHaveEventForwarder(child) || !has_event_forwarder())
On 2017/05/10 17:19:16, boliu wrote:
> has_event_forwarder is not enough. you need to check all nodes along this node
> to root node path
> 
|has_event_forwarder| only checks this node, and Subtree.. checks all the
parents plus the new subtree as its name indicates.


> 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

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]

https://codereview.chromium.org/2861413002/diff/100001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2861413002/diff/100001/ui/android/view_androi...
ui/android/view_android.cc:118:
DCHECK(!SubtreeOrParentsHaveEventForwarder(child) || !has_event_forwarder())
On 2017/05/10 21:57:57, Jinsuk Kim wrote:
> On 2017/05/10 17:19:16, boliu wrote:
> > has_event_forwarder is not enough. you need to check all nodes along this
node
> > to root node path
> > 
> |has_event_forwarder| only checks this node, and Subtree.. checks all the
> parents plus the new subtree as its name indicates.

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

> 
> 
> > 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
> 
> It doesn't check child's root path but only walk child and the children nodes
> below it. 

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.

> 
> 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.

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

https://codereview.chromium.org/2861413002/diff/100001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2861413002/diff/100001/ui/android/view_androi...
ui/android/view_android.cc:118:
DCHECK(!SubtreeOrParentsHaveEventForwarder(child) || !has_event_forwarder())
On 2017/05/10 22:09:40, boliu wrote:
> On 2017/05/10 21:57:57, Jinsuk Kim wrote:
> > On 2017/05/10 17:19:16, boliu wrote:
> > > has_event_forwarder is not enough. you need to check all nodes along this
> node
> > > to root node path
> > > 
> > |has_event_forwarder| only checks this node, and Subtree.. checks all the
> > parents plus the new subtree as its name indicates.
> 
> 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
> 

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.

> > 
> > 
> > > 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
> > 
> > It doesn't check child's root path but only walk child and the children
nodes
> > below it. 
> 
> 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.
> 

child's parent is not checked. Adding the child is fine in this case and DCHECK
doesn't get fired.

> > 
> > 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.

I'll probably split the method to avoid the confusion.

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

https://codereview.chromium.org/2861413002/diff/100001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2861413002/diff/100001/ui/android/view_androi...
ui/android/view_android.cc:118:
DCHECK(!SubtreeOrParentsHaveEventForwarder(child) || !has_event_forwarder())
On 2017/05/10 22:21:24, Jinsuk Kim wrote:
> On 2017/05/10 22:09:40, boliu wrote:
> > On 2017/05/10 21:57:57, Jinsuk Kim wrote:
> > > On 2017/05/10 17:19:16, boliu wrote:
> > > > has_event_forwarder is not enough. you need to check all nodes along
this
> > node
> > > > to root node path
> > > > 
> > > |has_event_forwarder| only checks this node, and Subtree.. checks all the
> > > parents plus the new subtree as its name indicates.
> > 
> > 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
> > 
> 
> 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.
> 
> > > 
> > > 
> > > > 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
> > > 
> > > It doesn't check child's root path but only walk child and the children
> nodes
> > > below it. 
> > 
> > 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.
> > 
> 
> child's parent is not checked. Adding the child is fine in this case and
DCHECK
> doesn't get fired.
> 
> > > 
> > > 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.
> 
> I'll probably split the method to avoid the confusion.

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

[Jinsuk Kim, 2017-05-10 22:36:37]

PTAL

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

https://codereview.chromium.org/2861413002/diff/120001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2861413002/diff/120001/ui/android/view_androi...
ui/android/view_android.cc:132: return ParentsHaveEventForwarder() ||
SubtreeHasEventForwarder(tree);
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:57]

https://codereview.chromium.org/2861413002/diff/120001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2861413002/diff/120001/ui/android/view_androi...
ui/android/view_android.cc:132: return ParentsHaveEventForwarder() ||
SubtreeHasEventForwarder(tree);
On 2017/05/10 22:48:43, boliu wrote:
> 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

Done.

[boliu, 2017-05-10 23:55:14]

https://codereview.chromium.org/2861413002/diff/140001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2861413002/diff/140001/ui/android/view_androi...
ui/android/view_android.cc:133: view = view->parent_;
start with view, not view->parent_, and then fix caller accordingly

https://codereview.chromium.org/2861413002/diff/140001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/2861413002/diff/140001/ui/android/view_androi...
ui/android/view_android.h:182: // or in the node paths down to each leaf of
subtree.
two methods, two comments

[Jinsuk Kim, 2017-05-11 00:17:14]

https://codereview.chromium.org/2861413002/diff/140001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2861413002/diff/140001/ui/android/view_androi...
ui/android/view_android.cc:133: view = view->parent_;
On 2017/05/10 23:55:13, boliu wrote:
> start with view, not view->parent_, and then fix caller accordingly

Done.

https://codereview.chromium.org/2861413002/diff/140001/ui/android/view_android.h
File ui/android/view_android.h (right):

https://codereview.chromium.org/2861413002/diff/140001/ui/android/view_androi...
ui/android/view_android.h:182: // or in the node paths down to each leaf of
subtree.
On 2017/05/10 23:55:13, boliu wrote:
> two methods, two comments

Done.

[Jinsuk Kim, 2017-05-11 00:23:26]

Made DCHECK for |AddChild| a bit simpler. PTAL

[boliu, 2017-05-11 00:30:27]

lgtm

https://codereview.chromium.org/2861413002/diff/180001/ui/android/view_androi...
File ui/android/view_android.cc (right):

https://codereview.chromium.org/2861413002/diff/180001/ui/android/view_androi...
ui/android/view_android.cc:107: DCHECK(!RootPathHasEventForwarder(parent_) &&
nit: maybe split this into two DCHECKs

[Jinsuk Kim, 2017-05-11 04:11:34]

The CQ bit was checked by jinsukkim@chromium.org

[Jinsuk Kim, 2017-05-11 04:11:34]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2861413002/#ps200001
(title: "nit")

[commit-bot: I haz the power, 2017-05-11 04:12:40]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 05:04:38]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1494475894312710,
"parent_rev": "3e77d17c672252b07ff0a065d238c3fbe4f149dd", "commit_rev":
"1a116ecfdf829702b653ded84a0e1cdb8bdfd7d6"}

[commit-bot: I haz the power, 2017-05-11 05:04:54]

Description was changed from

==========
Refined DCHECK preventing multiple event forwarders in VA tree path

A ViewAndroid tree path from the top to a leaf node can have only
a single EventForwarder instance. Rewrote helper methods used for
DCHECK that enforces the requirement to match the actual job with
the intention.

BUG=717271
==========

to

==========
Refined DCHECK preventing multiple event forwarders in VA tree path

A ViewAndroid tree path from the top to a leaf node can have only
a single EventForwarder instance. Rewrote helper methods used for
DCHECK that enforces the requirement to match the actual job with
the intention.

BUG=717271

Review-Url: https://codereview.chromium.org/2861413002
Cr-Commit-Position: refs/heads/master@{#470814}
Committed:
https://chromium.googlesource.com/chromium/src/+/1a116ecfdf829702b653ded84a0e...
==========

[commit-bot: I haz the power, 2017-05-11 05:04:55]

Committed patchset #9 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/1a116ecfdf829702b653ded84a0e...

[sebsg, 2017-05-11 13:20:03]

A revert of this CL (patchset #9 id:200001) has been created in
https://codereview.chromium.org/2880583002/ by sebsg@chromium.org.

The reason for reverting is: Primary suspect making
ViewAndroidTest.ChecksMultipleEventForwarders fails consistently

https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Tests.