Move page scale ownership to LayerTreeImpl.

Move page scale ownership to LayerTreeImpl.

Page scale is conceptually very similar to scroll offset, just
multiplicative instead of additive.  It has the exact same problems of
state synchronization between WebKit and the CC impl thread, and the
solution is also the same.  In this patch I closely imitated the three-way
sentScrollDelta logic.  I also deleted PinchZoomViewport and moved
everything to LayerTreeImpl because it turned out there was almost no
logic that could be isolated there.

This resolves many graphical glitches, including the white flashes and
blurriness while pinching or double-tap zooming, and the jumping around
during navigation.

NOTRY=true
BUG=171914
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179607

[aelias_OOO_until_Jul13, 2013-01-28 09:58:57]

This patch depends on https://codereview.chromium.org/12045002 landing first
(which is itself blocked on a WebKit change).  Note that I left some commented
out code in the uploaded version (will remove before submitting) corresponding
to the equivalent scroll offset manipulation, which I imitated, and I thought it
might be useful for reviewers to see that as well.

[danakj, 2013-01-28 20:29:03]

I like this CL so much, thanks for taking this on. One question..

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_host.cc#newcode302
cc/layer_tree_host.cc:302: if (m_settings.implSidePainting) {
how about pulling out the appropriate page_scale_delta and sent_page_scale_delta
into variables based on if. Then the syncTree->set() stuff can be
non-conditional.

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_host.cc#newcode305
cc/layer_tree_host.cc:305:
syncTree->SetPageScaleDelta(hostImpl->activeTree()->page_scale_delta() /
hostImpl->activeTree()->sent_page_scale_delta());
Can you add a comment here similar to the scroll code to explain this is due to
the active layer doing stuff before the pending layer was created?

That said, is this still true and needed in either place now that we sync the
pending tree instead of recreating it?

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_host_impl.cc#ol...
cc/layer_tree_host_impl.cc:1446: static void
collectScrollDeltas(ScrollAndScaleSet* scrollInfo, LayerImpl* layerImpl)
didn't need to move this function did you?

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_impl.cc#newcode128
cc/layer_tree_impl.cc:128: {
nit: { on previous line

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_impl.cc#newcode129
cc/layer_tree_impl.cc:129: LayerTreeImpl* pending_tree =
layer_tree_host_impl_->pendingTree();
2 space indents and 80 col

[enne (OOO), 2013-01-28 21:03:45]

Thanks for this.  Removing the PinchZoomViewport is exciting to see.

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_host.cc#newcode305
cc/layer_tree_host.cc:305:
syncTree->SetPageScaleDelta(hostImpl->activeTree()->page_scale_delta() /
hostImpl->activeTree()->sent_page_scale_delta());
On 2013/01/28 20:29:03, danakj wrote:
> That said, is this still true and needed in either place now that we sync the
> pending tree instead of recreating it?

Yeah, this will still be needed given that we don't sync updates to the recycle
tree (i.e. the pending tree post-activation).  Maybe that assumption should
change, but that's the state of the code now.

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

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:957:
m_activeTree->SetPageScaleFactorAndLimits(m_pendingTree->page_scale_factor(),
m_pendingTree->min_page_scale_factor(), m_pendingTree->max_page_scale_factor());
Can you make a LayerTreeImpl -> LayerTreeImpl pushPropertiesTo function and move
all this there?

[aelias_OOO_until_Jul13, 2013-01-29 06:05:13]

OK, all suggestions applied.

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_host.cc#newcode302
cc/layer_tree_host.cc:302: if (m_settings.implSidePainting) {
On 2013/01/28 20:29:03, danakj wrote:
> how about pulling out the appropriate page_scale_delta and
sent_page_scale_delta
> into variables based on if. Then the syncTree->set() stuff can be
> non-conditional.

Done.

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_host.cc#newcode305
cc/layer_tree_host.cc:305:
syncTree->SetPageScaleDelta(hostImpl->activeTree()->page_scale_delta() /
hostImpl->activeTree()->sent_page_scale_delta());
Added comment.

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (left):

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_host_impl.cc#ol...
cc/layer_tree_host_impl.cc:1446: static void
collectScrollDeltas(ScrollAndScaleSet* scrollInfo, LayerImpl* layerImpl)
On 2013/01/28 20:29:03, danakj wrote:
> didn't need to move this function did you?

Done.

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

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_host_impl.cc#ne...
cc/layer_tree_host_impl.cc:957:
m_activeTree->SetPageScaleFactorAndLimits(m_pendingTree->page_scale_factor(),
m_pendingTree->min_page_scale_factor(), m_pendingTree->max_page_scale_factor());
On 2013/01/28 21:03:46, enne wrote:
> Can you make a LayerTreeImpl -> LayerTreeImpl pushPropertiesTo function and
move
> all this there?

Done.

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_impl.cc
File cc/layer_tree_impl.cc (right):

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_impl.cc#newcode128
cc/layer_tree_impl.cc:128: {
On 2013/01/28 20:29:03, danakj wrote:
> nit: { on previous line

Done.

https://codereview.chromium.org/12093015/diff/1/cc/layer_tree_impl.cc#newcode129
cc/layer_tree_impl.cc:129: LayerTreeImpl* pending_tree =
layer_tree_host_impl_->pendingTree();
On 2013/01/28 20:29:03, danakj wrote:
> 2 space indents and 80 col

Done.

[enne (OOO), 2013-01-29 06:07:20]

lgtm

[commit-bot: I haz the power, 2013-01-30 03:31:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/12093015/9003

[commit-bot: I haz the power, 2013-01-30 12:10:53]

Change committed as 179607