[cc] Fold more update calls into updateDrawProperties

[cc] Fold more update calls into updateDrawProperties

Now that we have a clean update system, we can remove the manual
calling of updateRootScrollLayerImplTransform and other similar kinds of "I'm going to do something manually in a half dozen places because I can." This removes some surface area
on LTHI which helps with the LTHI->LTI push.

BUG=155209
R=enne,skyostil

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173406

[nduca, 2012-12-11 08:21:32]

Does this make sense, you two?

[Sami, 2012-12-11 11:51:54]

I think this also improves things by making the tests behave more like the real
thing and not keeping LTHI in an inconsistent state.

https://codereview.chromium.org/11529006/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11529006/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:403:
rootScrollLayer()->setImplTransform(implTransform());
I guess this file is still in WebKit style so the indent looks off here. Also,
no need for braces.

[danakj, 2012-12-11 15:49:24]

https://codereview.chromium.org/11529006/diff/1/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/11529006/diff/1/cc/layer_tree_host_impl.h#new...
cc/layer_tree_host_impl.h:259: void updateDrawProperties();
You shouldn't need to make this public, we already have public API that ends up
calling this, as well as calcRSLL()

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

https://codereview.chromium.org/11529006/diff/1/cc/layer_tree_host_impl_unitt...
cc/layer_tree_host_impl_unittest.cc:1297: m_hostImpl->updateDrawProperties();
You could call prepareToDraw() instead to get an even more real test. (And
drawLayers(); didDrawAllLayers() after to clean up).

Or you could call calculateRenderSurfaceLayerList which is already exposed for
testing, if needed (I prefer the other one).

[enne (OOO), 2012-12-11 17:09:02]

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

https://codereview.chromium.org/11529006/diff/1/cc/layer_tree_host_impl_unitt...
cc/layer_tree_host_impl_unittest.cc:1297: m_hostImpl->updateDrawProperties();
On 2012/12/11 15:49:24, danakj wrote:
> You could call prepareToDraw() instead to get an even more real test. (And
> drawLayers(); didDrawAllLayers() after to clean up).
> 
> Or you could call calculateRenderSurfaceLayerList which is already exposed for
> testing, if needed (I prefer the other one).

Yeah, I'd prefer if prepare/draw/didDraw was the interface to LTHI rather than
reaching into the internals to call things like either calcRSLL or
updateDrawProperties.

[nduca, 2012-12-12 08:30:30]

Okay. @enne, PTAL.

Dana, your help is needed to resolve an issue in the pinchZoomPan unit tests.
Namely, they send scrollBy's as Gesture, but send their deltas untransformed
into viewport space [whatever that is]. This works now because we never run
calcDraw, and thus the screenspace transforms on all the layers used by
scrollLayerWithViewportSpaceDelta is identity. With this patch, the layers have
legit screenspace transforms, so now "scrollBy(5,0)" is transformed by the
screenSpaceTransform and thus the test fails.

Help?!

The remaining issue is that these tests fail:
[  FAILED  ] 3 tests, listed below:
[  FAILED  ]
LayerTreeHostImplTests/LayerTreeHostImplTest.pinchZoomPanViewportWithDeviceScaleFactor1/1,
where GetParam() = true
[  FAILED  ]
LayerTreeHostImplTests/LayerTreeHostImplTest.pinchZoomPanViewportAndScrollWithDeviceScaleFactor/1,
where GetParam() = true
[  FAILED  ]
LayerTreeHostImplTests/LayerTreeHostImplTest.pinchZoomPanViewportAndScrollBoundaryWithDeviceScaleFactor/1,
where GetParam() = true

[nduca, 2012-12-12 10:43:00]

thanks to some (HUGE) help from @shawnsingh, I present patchet #3, which passes
tests but is ... odd. Dana, @wj, what do you two think? Please advise on how to
get this to a landable state, as this is sort of a baby step in a larger patch
series that we're trying to move along as part of CY.

[enne (OOO), 2012-12-12 18:44:24]

https://codereview.chromium.org/11529006/diff/9008/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11529006/diff/9008/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:1189: // TODO(nduca): Try to delete this before
landing.
I just checked.  This function can just call updateDrawProperties() and it'll do
the right thing, thanks to all your test changes.  :)

https://codereview.chromium.org/11529006/diff/9008/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/11529006/diff/9008/cc/layer_tree_host_impl.h#...
cc/layer_tree_host_impl.h:55: class PinchZoomViewport {
Needs CC_EXPORT now that you're calling totalPageScaleFactor in tests.

[wjmaclean, 2012-12-12 21:55:35]

On 2012/12/12 18:44:24, enne wrote:
> https://codereview.chromium.org/11529006/diff/9008/cc/layer_tree_host_impl.cc
> File cc/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/11529006/diff/9008/cc/layer_tree_host_impl.cc...
> cc/layer_tree_host_impl.cc:1189: // TODO(nduca): Try to delete this before
> landing.
> I just checked.  This function can just call updateDrawProperties() and it'll
do
> the right thing, thanks to all your test changes.  :)
> 
> https://codereview.chromium.org/11529006/diff/9008/cc/layer_tree_host_impl.h
> File cc/layer_tree_host_impl.h (right):
> 
>
https://codereview.chromium.org/11529006/diff/9008/cc/layer_tree_host_impl.h#...
> cc/layer_tree_host_impl.h:55: class PinchZoomViewport {
> Needs CC_EXPORT now that you're calling totalPageScaleFactor in tests.

I'm not sure why the tests are failing the way they are, though I build with
this patch, and manually tested the things that are being guarded by those
tests, and it seems fine. I would say go-ahead and if we find anything's changed
for the worse we can file a bug and assign it to me (at least for the
pinch-zoom/implTransform stuff). If I come up with a good explanation, I'll add
it here.

[nduca, 2012-12-12 22:11:00]

Okay. Dana, Enne, need your approval plz.

[danakj, 2012-12-12 22:11:37]

lookin :)

On Wed, Dec 12, 2012 at 5:11 PM,  <nduca@chromium.org> wrote:
> Okay. Dana, Enne, need your approval plz.
>
> https://codereview.chromium.org/11529006/

[enne (OOO), 2012-12-12 22:22:33]

lgtm, but I'd definitely like extra eyes from danakj on the math

[danakj, 2012-12-13 00:03:37]

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:769: DCHECK(canDraw());
I don't like this.. I assume this is for the tests. Can they just set a viewport
size or whatever is making canDraw() false?

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:1193: // TODO(enne): See http://crbug.com/164949. 
This function should really
This is super yay.

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:897: if (m_resourceProvider)
If we initialze the host properly with a FakeOutputSurface this shouldn't be
needed?

The Setup() function in LTHImplTests does this.

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl.h...
cc/layer_tree_host_impl.h:302: float totalPageScaleFactor() const { return
m_pinchZoomViewport.totalPageScaleFactor(); }
totalPageScaleFactorForTesting() (can probly drop the comment then)

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl.h...
cc/layer_tree_host_impl.h:321: void resetNeedsUpdateDrawPropertiesForTesting() {
m_needsUpdateDrawProperties = false; }
private fortesting method. can this be removed then?

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl_u...
File cc/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl_u...
cc/layer_tree_host_impl_unittest.cc:213: void forcePrepareToDraw() {
why is this called force?

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl_u...
cc/layer_tree_host_impl_unittest.cc:4651: gfx::Vector2d
scrollDeltaInViewportSpace = ToFlooredVector2d(gfx::ScaleVector2d(scrollDelta,
m_hostImpl->totalPageScaleFactor()));
these would be the scrollDeltaInZoomedViewport

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_unitte...
File cc/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_unitte...
cc/layer_tree_host_unittest.cc:1334: mockImpl->didDrawAllLayers(frameData);
can you move the didDraw down to the end, so we use the frameData before calling
didDraw in case it destroys the frameData

[enne (OOO), 2012-12-13 00:18:07]

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:769: DCHECK(canDraw());
On 2012/12/13 00:03:37, danakj wrote:
> I don't like this.. I assume this is for the tests. Can they just set a
viewport
> size or whatever is making canDraw() false?

I'm going to disagree here.  I like this a lot.  The amount of setup you need to
do to get a test on a layer tree seems overly burdensome.  We call prepareToDraw
in exactly two non-test locations, so I'm not that sure this assert is buying us
very much.

[danakj, 2012-12-13 00:20:14]

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:769: DCHECK(canDraw());
On 2012/12/13 00:18:07, enne wrote:
> On 2012/12/13 00:03:37, danakj wrote:
> > I don't like this.. I assume this is for the tests. Can they just set a
> viewport
> > size or whatever is making canDraw() false?
> 
> I'm going to disagree here.  I like this a lot.  The amount of setup you need
to
> do to get a test on a layer tree seems overly burdensome.  We call
prepareToDraw
> in exactly two non-test locations, so I'm not that sure this assert is buying
us
> very much.

I am motivated by the fact that we had a lot of crashes in the past which is why
it's here :/

Maybe we need a TestLayerTreeHostImpl that lets you force canDraw to always be
true or something.

[nduca, 2012-12-13 00:27:05]

I've still got the earlyOut on canDraw, its just slightly later. I think we
should use review culture to think through crashes in the future that harden
individual items.

[danakj, 2012-12-13 00:30:47]

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl_u...
File cc/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl_u...
cc/layer_tree_host_impl_unittest.cc:4651: gfx::Vector2d
scrollDeltaInViewportSpace = ToFlooredVector2d(gfx::ScaleVector2d(scrollDelta,
m_hostImpl->totalPageScaleFactor()));
On further thought I think this shouldn't be needed, the bug is in scrollBy.

The input should be in (unzoomed) viewport space (ie webkit's view).

The scrollBy function should be moving the input to zoomed device viewport space
but it is not.

[danakj, 2012-12-13 00:56:33]

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl_u...
File cc/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/11529006/diff/10016/cc/layer_tree_host_impl_u...
cc/layer_tree_host_impl_unittest.cc:4651: gfx::Vector2d
scrollDeltaInViewportSpace = ToFlooredVector2d(gfx::ScaleVector2d(scrollDelta,
m_hostImpl->totalPageScaleFactor()));
On 2012/12/13 00:30:47, danakj wrote:
> On further thought I think this shouldn't be needed, the bug is in scrollBy.
> 
> The input should be in (unzoomed) viewport space (ie webkit's view).
> 
> The scrollBy function should be moving the input to zoomed device viewport
space
> but it is not.

Ok no, this shouldn't be done. Don't scale the scrollDelta at all.

Moving 5 pixels in the viewport if the layer is all zoomed in makes it scroll
more. Instead, scale up the expectedTranslation below.

[nduca, 2012-12-13 06:30:00]

Okay so clearly this patch just unearths some brain damage in pinch path that
was, likely, always broken, or if not, then very hard for experts to understand.
Personally,  this is really eye opener for me.

I had an offline conversation with dana about changing from pre-scaling the
viewport to dividing the expected translation. However, even just simply
tweaking the expectations from (0, 5) to (0, 6) to handle rounding errors ---
was equally fragile. Basically, I'm convinced that the code is fragile, and the
test more so.

Right now, making progress on impl-side painting is an overall need. So, we face
a dillemma: do I put another day of work into this [we're +1 day in by now] to
fix this? Or do I file a bug for pinch being broken per Dana's notes, and we
plan to give this whole system a big pile of love and care to make it
understandable and maintainable in the future?

I'm leaning toward the latter: impl side painting needs to move forward.

[danakj, 2012-12-13 07:02:23]

On Wed, Dec 12, 2012 at 10:30 PM,  <nduca@chromium.org> wrote:
> Okay so clearly this patch just unearths some brain damage in pinch path
> that
> was, likely, always broken, or if not, then very hard for experts to
> understand.
> Personally,  this is really eye opener for me.
>
> I had an offline conversation with dana about changing from pre-scaling the
> viewport to dividing the expected translation. However, even just simply
> tweaking the expectations from (0, 5) to (0, 6) to handle rounding errors
> ---
> was equally fragile. Basically, I'm convinced that the code is fragile, and
> the
> test more so.
>
> Right now, making progress on impl-side painting is an overall need. So, we
> face
> a dillemma: do I put another day of work into this [we're +1 day in by now]
> to
> fix this? Or do I file a bug for pinch being broken per Dana's notes, and we
> plan to give this whole system a big pile of love and care to make it
> understandable and maintainable in the future?

I think that scrollBy is okay, I was confused. The test's expectations
of the result of scrolling a layer when it's zoomed is wrong.

When the layer is 2x as big on the screen, moving your finger 5 pixels
on the screen only moves 5/2 pixels from the point of view of the
layer, so it should scroll that much.

We can fix the expected scroll amount in these tests, or we can set
pageScaleFactor to 1 in them, which is what it was actually testing
before (but was lying about it).

> I'm leaning toward the latter: impl side painting needs to move forward.
>
> https://codereview.chromium.org/11529006/

[wjmaclean, 2012-12-13 16:10:37]

Not sure if this is related:

https://codereview.chromium.org/11570008/

[nduca, 2012-12-13 17:56:27]

folks, 2 days 9 hours on this patch. Whats holding up final review approval.

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

On 2012/12/13 17:56:27, nduca wrote:
> folks, 2 days 9 hours on this patch. Whats holding up final review approval.

Not that you need my OK, but I already indicated that I'm fine to land this and
fix bugs later (see #8).

[danakj, 2012-12-13 18:12:27]

Oh, sorry Nat. Without a "PTAL" I didn't know to look for a new version.

LGTM with a TODO and a bug saying these tests are wrong.

https://codereview.chromium.org/11529006/diff/23001/cc/layer_tree_host_impl_u...
File cc/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/11529006/diff/23001/cc/layer_tree_host_impl_u...
cc/layer_tree_host_impl_unittest.cc:4651: gfx::Vector2d
scrollDeltaInZoomedViewport = ToFlooredVector2d(gfx::ScaleVector2d(scrollDelta,
m_hostImpl->totalPageScaleFactorForTesting()));
At the least I'd like a TODO here saying that the expectedTranslation needs to
be scaled, not the scroll input.

https://codereview.chromium.org/11529006/diff/23001/cc/test/fake_layer_tree_h...
File cc/test/fake_layer_tree_host_impl.h (right):

https://codereview.chromium.org/11529006/diff/23001/cc/test/fake_layer_tree_h...
cc/test/fake_layer_tree_host_impl.h:19: void forcePrepareToDraw() {
nit: prepareToDrawEtc()

force means something specific around the draw code (draw when prepareToDraw is
false)

[enne (OOO), 2012-12-13 18:17:31]

https://codereview.chromium.org/11529006/diff/23001/cc/test/fake_layer_tree_h...
File cc/test/fake_layer_tree_host_impl.h (right):

https://codereview.chromium.org/11529006/diff/23001/cc/test/fake_layer_tree_h...
cc/test/fake_layer_tree_host_impl.h:19: void forcePrepareToDraw() {
On 2012/12/13 18:12:28, danakj wrote:
> nit: prepareToDrawEtc()
> 
> force means something specific around the draw code (draw when prepareToDraw
is
> false)

What about "drawOneFrame"?

[commit-bot: I haz the power, 2012-12-16 23:21:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nduca@chromium.org/11529006/41002

[commit-bot: I haz the power, 2012-12-17 01:16:50]

Change committed as 173406