Only allow change-of-basis between transform nodes on the same ancestor chain

Only allow change-of-basis between transform nodes on the same ancestor chain

This removes the code that allowed computing a change-of-basis
transform between nodes that don't lie on the same ancestor change,
and adds DCHECKs to ensure that ComputeTransform isn't called with such nodes.

BUG=474324
TEST=no new crash caused by NOTREACHED added in the
code while running the layout tests and telemetry tests.

Committed: https://crrev.com/64948a91c39e8c5084b2389e3c6b641e2cb18603
Cr-Commit-Position: refs/heads/master@{#324492}

[jaydasika, 2015-04-09 14:19:42]

jaydasika@chromium.org changed reviewers:
+ ajuma@chromium.org, vollick@chromium.org

[ajuma, 2015-04-09 14:43:09]

This looks good. I think the IsDescendant checks can be turned into DCHECKS too
(see below).

Nit about the description: please use the present tense to describe what the CL
does. For example:

This removes the code that allowed computing a change-of-basis
transform between nodes that don't lie on the same ancestor change,
and adds a NOTREACHED to ensure that ComputeTransform isn't called
with such nodes.

https://codereview.chromium.org/1070133002/diff/20001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (left):

https://codereview.chromium.org/1070133002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:64: 
Nit: this whitespace should stay

https://codereview.chromium.org/1070133002/diff/20001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1070133002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:67: if (source_id > dest_id && IsDescendant(source_id,
dest_id))
Since we don't expect the IsDescendant check to ever return false now, it can be
changed to a DCHECK. Same thing below too.

If we're doing that, the 'if' below can go away (since if we reach there, it
must be the case that dest_id > source_id), and we can unconditionally
DCHECK(IsDescendant(dest_id, source_id) and then "return CombineInverses...".

https://codereview.chromium.org/1070133002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:74: return 0;
Style nit: use 'false' instead of 0. (But this code will go away with the
changes suggested above.)

[jaydasika, 2015-04-09 15:24:29]

https://codereview.chromium.org/1070133002/diff/20001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (left):

https://codereview.chromium.org/1070133002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:64: 
On 2015/04/09 14:43:09, ajuma wrote:
> Nit: this whitespace should stay

Done.

https://codereview.chromium.org/1070133002/diff/20001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1070133002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:67: if (source_id > dest_id && IsDescendant(source_id,
dest_id))
On 2015/04/09 14:43:09, ajuma wrote:
> Since we don't expect the IsDescendant check to ever return false now, it can
be
> changed to a DCHECK. Same thing below too.
> 
> If we're doing that, the 'if' below can go away (since if we reach there, it
> must be the case that dest_id > source_id), and we can unconditionally
> DCHECK(IsDescendant(dest_id, source_id) and then "return CombineInverses...".

Done.

https://codereview.chromium.org/1070133002/diff/20001/cc/trees/property_tree....
cc/trees/property_tree.cc:74: return 0;
On 2015/04/09 14:43:09, ajuma wrote:
> Style nit: use 'false' instead of 0. (But this code will go away with the
> changes suggested above.)

Done.

[ajuma, 2015-04-09 15:27:24]

https://codereview.chromium.org/1070133002/diff/40001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1070133002/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:68: if (source_id > dest_id && IsDescendant(source_id,
dest_id))
This IsDescendant can be moved inside a DCHECK too, e.g.:
if (source_id > dest_id) {
  DCHECK(IsDescendant(...));
  ...
}

[jaydasika, 2015-04-09 15:38:31]

https://codereview.chromium.org/1070133002/diff/40001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1070133002/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:68: if (source_id > dest_id && IsDescendant(source_id,
dest_id))
On 2015/04/09 15:27:24, ajuma wrote:
> This IsDescendant can be moved inside a DCHECK too, e.g.:
> if (source_id > dest_id) {
>   DCHECK(IsDescendant(...));
>   ...
> }

Done.

[ajuma, 2015-04-09 15:40:07]

On 2015/04/09 15:38:31, jaydasika wrote:
>
https://codereview.chromium.org/1070133002/diff/40001/cc/trees/property_tree.cc
> File cc/trees/property_tree.cc (right):
> 
>
https://codereview.chromium.org/1070133002/diff/40001/cc/trees/property_tree....
> cc/trees/property_tree.cc:68: if (source_id > dest_id &&
IsDescendant(source_id,
> dest_id))
> On 2015/04/09 15:27:24, ajuma wrote:
> > This IsDescendant can be moved inside a DCHECK too, e.g.:
> > if (source_id > dest_id) {
> >   DCHECK(IsDescendant(...));
> >   ...
> > }
> 
> Done.

Thanks! Please update the description as suggested above, and then this should
be ready to land.

[jaydasika, 2015-04-09 15:44:12]

On 2015/04/09 15:40:07, ajuma wrote:
> On 2015/04/09 15:38:31, jaydasika wrote:
> >
>
https://codereview.chromium.org/1070133002/diff/40001/cc/trees/property_tree.cc
> > File cc/trees/property_tree.cc (right):
> > 
> >
>
https://codereview.chromium.org/1070133002/diff/40001/cc/trees/property_tree....
> > cc/trees/property_tree.cc:68: if (source_id > dest_id &&
> IsDescendant(source_id,
> > dest_id))
> > On 2015/04/09 15:27:24, ajuma wrote:
> > > This IsDescendant can be moved inside a DCHECK too, e.g.:
> > > if (source_id > dest_id) {
> > >   DCHECK(IsDescendant(...));
> > >   ...
> > > }
> > 
> > Done.
> 
> Thanks! Please update the description as suggested above, and then this should
> be ready to land.

Updated.

[ajuma, 2015-04-09 15:44:48]

lgtm

[jaydasika, 2015-04-09 18:21:56]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2015-04-09 18:22:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1070133002/60001

[commit-bot: I haz the power, 2015-04-09 19:57:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-09 19:58:02]

Patchset 4 (id:??) landed as
https://crrev.com/64948a91c39e8c5084b2389e3c6b641e2cb18603
Cr-Commit-Position: refs/heads/master@{#324492}