implTransform should be called on rootScrollLayer().

implTransform should be called on rootScrollLayer().

Currently unit tests are calling implTransform() on rootLayer(), but it is set on rootScrollLayer().

BUG=165924

[enne (OOO), 2012-12-13 16:42:25]

How were all these tests passing before?

https://codereview.chromium.org/11570008/diff/1/cc/layer_tree_host_impl_unitt...
File cc/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/11570008/diff/1/cc/layer_tree_host_impl_unitt...
cc/layer_tree_host_impl_unittest.cc:1258: EXPECT_EQ(1.0,
m_hostImpl->rootScrollLayer()->implTransform().matrix().getDouble(0, 0));
Don't we have some matrix equal macro that you can compare to identity?

[wjmaclean, 2012-12-13 16:57:05]

> How were all these tests passing before?

Not sure ... this file has seen a lot of churn recently, but presumably it has
been giving positive results. During the tests, is rootLayer == rootScrollLayer?
It wouldn't normally be in the wild ...

https://codereview.chromium.org/11570008/diff/1/cc/layer_tree_host_impl_unitt...
File cc/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/11570008/diff/1/cc/layer_tree_host_impl_unitt...
cc/layer_tree_host_impl_unittest.cc:1258: EXPECT_EQ(1.0,
m_hostImpl->rootScrollLayer()->implTransform().matrix().getDouble(0, 0));
On 2012/12/13 16:42:26, enne wrote:
> Don't we have some matrix equal macro that you can compare to identity?

You can just compare two matrices, if you can get them to be the same (which you
can't always do easily, see comment above re +0 and -0)

[wjmaclean, 2012-12-13 16:58:12]

On 2012/12/13 16:57:05, wjmaclean wrote:
> > How were all these tests passing before?
> 
> Not sure ... this file has seen a lot of churn recently, but presumably it has
> been giving positive results. During the tests, is rootLayer ==
rootScrollLayer?
> It wouldn't normally be in the wild ...
> 
>
https://codereview.chromium.org/11570008/diff/1/cc/layer_tree_host_impl_unitt...
> File cc/layer_tree_host_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/11570008/diff/1/cc/layer_tree_host_impl_unitt...
> cc/layer_tree_host_impl_unittest.cc:1258: EXPECT_EQ(1.0,
> m_hostImpl->rootScrollLayer()->implTransform().matrix().getDouble(0, 0));
> On 2012/12/13 16:42:26, enne wrote:
> > Don't we have some matrix equal macro that you can compare to identity?
> 
> You can just compare two matrices, if you can get them to be the same (which
you
> can't always do easily, see comment above re +0 and -0)

Some later tests do this, e.g. look at the diff for line 4880.

[enne (OOO), 2012-12-13 17:00:54]

On 2012/12/13 16:58:12, wjmaclean wrote:
> On 2012/12/13 16:57:05, wjmaclean wrote:
> > > How were all these tests passing before?
> > 
> > Not sure ... this file has seen a lot of churn recently, but presumably it
has
> > been giving positive results. During the tests, is rootLayer ==
> rootScrollLayer?
> > It wouldn't normally be in the wild ...

I'd really like to understand that before changing it.  Maybe these tests aren't
testing anything.

https://codereview.chromium.org/11570008/diff/1/cc/layer_tree_host_impl_unitt...
File cc/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/11570008/diff/1/cc/layer_tree_host_impl_unitt...
cc/layer_tree_host_impl_unittest.cc:1258: EXPECT_EQ(1.0,
m_hostImpl->rootScrollLayer()->implTransform().matrix().getDouble(0, 0));
On 2012/12/13 16:57:05, wjmaclean wrote:
> On 2012/12/13 16:42:26, enne wrote:
> > Don't we have some matrix equal macro that you can compare to identity?
> 
> You can just compare two matrices, if you can get them to be the same (which
you
> can't always do easily, see comment above re +0 and -0)

Then the macro should be fixed to be resilient to this, like float equals has an
epsilon.

[wjmaclean, 2012-12-13 17:12:38]

On 2012/12/13 17:00:54, enne wrote:
> On 2012/12/13 16:58:12, wjmaclean wrote:
> > On 2012/12/13 16:57:05, wjmaclean wrote:
> > > > How were all these tests passing before?
> > > 
> > > Not sure ... this file has seen a lot of churn recently, but presumably it
> has
> > > been giving positive results. During the tests, is rootLayer ==
> > rootScrollLayer?
> > > It wouldn't normally be in the wild ...
> 
> I'd really like to understand that before changing it.  Maybe these tests
aren't
> testing anything.

In some of the tests the root layer is a scrollable layer, so the two match
(e.g. scrollRootAndChangePageScaleOnMainThread). In some of the tests (e.g.
scrollChildAndChangePageScaleOnMainThread), the scrolling layer is a child.

Maybe this is why Nat had to fix results for some, but not all, tests?

It's pretty easy to just read and see which is which I think, though I don't
know if the rootLayer == rootScrollLayer case ever matches what we do in the
renderer.

>
https://codereview.chromium.org/11570008/diff/1/cc/layer_tree_host_impl_unitt...
> File cc/layer_tree_host_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/11570008/diff/1/cc/layer_tree_host_impl_unitt...
> cc/layer_tree_host_impl_unittest.cc:1258: EXPECT_EQ(1.0,
> m_hostImpl->rootScrollLayer()->implTransform().matrix().getDouble(0, 0));
> On 2012/12/13 16:57:05, wjmaclean wrote:
> > On 2012/12/13 16:42:26, enne wrote:
> > > Don't we have some matrix equal macro that you can compare to identity?
> > 
> > You can just compare two matrices, if you can get them to be the same (which
> you
> > can't always do easily, see comment above re +0 and -0)
> 
> Then the macro should be fixed to be resilient to this, like float equals has
an
> epsilon.

I would agree. The test on 4880 relies on the default matrix comparison in the
matrix class I think, so it's not the macro's fault. In either case, I would
assume that would be handled in a separate CL.

[wjmaclean, 2012-12-13 17:16:04]

As a side thought, maybe it's not the best thing to be making any assumptions
about where the implTransform() is in the tests? After, it's primarily a
property of the PinchZoomViewport, and in previous versions of the tests it was
taken directly from there (via an accessor in LTHI). But in future it's not
always going to be set on either the root scroll layer or the root layer, making
the tests harder to maintain when written this way I think.

[enne (OOO), 2012-12-13 17:37:24]

Also, just FYI, I would really like Nat's patch that touches pinch zoom tests to
land first.  https://codereview.chromium.org/11529006/

[wjmaclean, 2012-12-13 17:42:36]

On 2012/12/13 17:37:24, enne wrote:
> Also, just FYI, I would really like Nat's patch that touches pinch zoom tests
to
> land first.  https://codereview.chromium.org/11529006/

Part of the reason I put the CL up it to see if it might remove the need for
some of the changes proposed in https://codereview.chromium.org/11529006/ ... it
may be not all the changes are necessary (but I couldn't get Nat's patch applied
to my repo this morning, so I haven't been able to test that).

[nduca, 2012-12-13 18:01:07]

Can we just land my patch and then go do some of the fixes? This entire unit
tests being crappy has held up my code yellow work for 2 days so far, and I'm
getting very frustrated.

[wjmaclean, 2012-12-13 18:02:12]

On 2012/12/13 18:01:07, nduca wrote:
> Can we just land my patch and then go do some of the fixes? This entire unit
> tests being crappy has held up my code yellow work for 2 days so far, and I'm
> getting very frustrated.

OK