Chromium compositor change implementing page-scale driven pinch-zoom.

Chromium compositor changes to enable a pinch-zoom path that uses page-scale instead of a CSS transformation.

The plan is to maintain the existing CSS-driven pinch behaviour, and submit this change with the functionality hidden behind a flag.

Corresponding WK change:  https://bugs.webkit.org/show_bug.cgi?id=93292

BUG=148816
TEST=None

[enne (OOO), 2012-09-13 19:40:54]

https://codereview.chromium.org/10916279/diff/1/cc/CCLayerTreeHost.cpp
File cc/CCLayerTreeHost.cpp (right):

https://codereview.chromium.org/10916279/diff/1/cc/CCLayerTreeHost.cpp#newcod...
cc/CCLayerTreeHost.cpp:465: if (false /*layer->boundsContainPageScale()*/)
I assume this can go away once with the corresponding NCCH changes on the WebKit
bug?

https://codereview.chromium.org/10916279/diff/1/cc/CCLayerTreeHostImpl.cpp
File cc/CCLayerTreeHostImpl.cpp (right):

https://codereview.chromium.org/10916279/diff/1/cc/CCLayerTreeHostImpl.cpp#ne...
cc/CCLayerTreeHostImpl.cpp:1175: #if 0
?

https://codereview.chromium.org/10916279/diff/1/cc/LayerChromium.h
File cc/LayerChromium.h (right):

https://codereview.chromium.org/10916279/diff/1/cc/LayerChromium.h#newcode151
cc/LayerChromium.h:151: FloatSize localOffset() const { return FloatSize(); }
This seriously needs a comment describing what this variable is.  localOffset is
also not a particularly intuitive name.

https://codereview.chromium.org/10916279/diff/1/cc/LayerChromium.h#newcode154
cc/LayerChromium.h:154: float pageScaleFactor() const { return 1; }
Why is the page scale factor 1 on the main thread? I'm guessing this is due to
your previous concerns about the wrong tiles getting invalidated.  I'd like to
lean on the prepainting system here to paint the correct set of tiles rather
than having a fake scale on the main thread.

[aelias_OOO_until_Jul13, 2012-09-13 19:47:41]

https://codereview.chromium.org/10916279/diff/1/cc/LayerChromium.h
File cc/LayerChromium.h (right):

https://codereview.chromium.org/10916279/diff/1/cc/LayerChromium.h#newcode151
cc/LayerChromium.h:151: FloatSize localOffset() const { return FloatSize(); }
On 2012/09/13 19:40:54, enne wrote:
> This seriously needs a comment describing what this variable is.  localOffset
is
> also not a particularly intuitive name.

In the WebKit bug, I proposed collapsing m_localOffset, m_pageScaleDelta and
m_pageScaleFactor (in CCLayerImpl) into a new matrix called "m_implTransform" (a
transformation which is only computed and applied on the impl-side).  What do
you think?

https://codereview.chromium.org/10916279/diff/1/cc/LayerChromium.h#newcode154
cc/LayerChromium.h:154: float pageScaleFactor() const { return 1; }
On 2012/09/13 19:40:54, enne wrote:
> Why is the page scale factor 1 on the main thread? I'm guessing this is due to
> your previous concerns about the wrong tiles getting invalidated.  I'd like to
> lean on the prepainting system here to paint the correct set of tiles rather
> than having a fake scale on the main thread.

I would also like us to get the right scale on the main thread to make sure we
paint the right set of tiles.  This will be a blocker to ship this on Android,
and I suspect you won't be happy with the overpainting on low-end Windows
tablets and Chromebooks either.  It might be easier to solve this problem on the
first pass instead of rejiggering the math in the future.

[Jeff Timanus, 2012-09-13 19:58:49]

Thx for having a look.

At the moment, I'm trying to figure out how to preserve the old pinch behaviour
via a command line switch.

Once I have the switch in place, I'll address the code clean-up suggestions that
have been offered.

https://codereview.chromium.org/10916279/diff/1/cc/CCLayerTreeHost.cpp
File cc/CCLayerTreeHost.cpp (right):

https://codereview.chromium.org/10916279/diff/1/cc/CCLayerTreeHost.cpp#newcod...
cc/CCLayerTreeHost.cpp:465: if (false /*layer->boundsContainPageScale()*/)
On 2012/09/13 19:40:54, enne wrote:
> I assume this can go away once with the corresponding NCCH changes on the
WebKit
> bug?
Yes, this hack can go away once the NCCH change in wk has landed.

https://codereview.chromium.org/10916279/diff/1/cc/CCLayerTreeHostImpl.cpp
File cc/CCLayerTreeHostImpl.cpp (right):

https://codereview.chromium.org/10916279/diff/1/cc/CCLayerTreeHostImpl.cpp#ne...
cc/CCLayerTreeHostImpl.cpp:1175: #if 0
On 2012/09/13 19:40:54, enne wrote:
> ?
This code needs to be retained so that the android path still works.  

Expect a patch shortly that adds necessary flags to switch between this
implementation, and the previous approach.

https://codereview.chromium.org/10916279/diff/1/cc/LayerChromium.h
File cc/LayerChromium.h (right):

https://codereview.chromium.org/10916279/diff/1/cc/LayerChromium.h#newcode151
cc/LayerChromium.h:151: FloatSize localOffset() const { return FloatSize(); }
On 2012/09/13 19:47:41, aelias wrote:
> On 2012/09/13 19:40:54, enne wrote:
> > This seriously needs a comment describing what this variable is. 
localOffset
> is
> > also not a particularly intuitive name.
> 
> In the WebKit bug, I proposed collapsing m_localOffset, m_pageScaleDelta and
> m_pageScaleFactor (in CCLayerImpl) into a new matrix called "m_implTransform"
(a
> transformation which is only computed and applied on the impl-side).  What do
> you think?

I'm working on wrapping all of these transforms as Alexandre suggests.

https://codereview.chromium.org/10916279/diff/1/cc/LayerChromium.h#newcode154
cc/LayerChromium.h:154: float pageScaleFactor() const { return 1; }
On 2012/09/13 19:47:41, aelias wrote:
> On 2012/09/13 19:40:54, enne wrote:
> > Why is the page scale factor 1 on the main thread? I'm guessing this is due
to
> > your previous concerns about the wrong tiles getting invalidated.  I'd like
to
> > lean on the prepainting system here to paint the correct set of tiles rather
> > than having a fake scale on the main thread.

The page scale factor is 1 because of the change suggested by Dana on the wk
patch.  Previously, I was passing the scale and blue-box offset to the main
thread so that only those tiles that are visible from the pinched viewport would
be painted.

By passing a scale factor of 1, all of the tiles in the 'red box' are being
prepared at the scaled resolution.

The intent was to land the patch as is, and then correct the pre-painting
behaviour to only prepare the necessary tiles.

> 
> I would also like us to get the right scale on the main thread to make sure we
> paint the right set of tiles.  This will be a blocker to ship this on Android,
> and I suspect you won't be happy with the overpainting on low-end Windows
> tablets and Chromebooks either.  It might be easier to solve this problem on
the
> first pass instead of rejiggering the math in the future.

I'm fine with fixing this in this patch.  It would be a straight-forward follow
up patch as well.

[Jeff Timanus, 2012-09-14 04:22:26]

Please take another look.

I added a command-line switch --enable-pinch-in-compositor, and plumbed it
through to the various settings objects to enable the new path.  When the switch
is not present, the old clank zooming path should remain unchanged.

I also addressed most of the comments from the previous wk issue.  One dangling
issue is moving m_pageScaleDelta to the PinchZoomViewport class.

There are still some remaining issues that will need to be fixed once this work
lands:
 - Scaling will not work for NPAPI plug-ins.  (Discussed on the wk issue.)
 - Main-thread scrolling will not update the pinch-zoom viewport.  This is
demonstrated presently on tech-crunch.  
    1) Navigate to techcrunch.com. (With --enable-pinch-in-compositor)
    2) Zoom in.
    3) Attempt to scroll to the top.
  All of the scrolling is taking place on the main thread, so the
CCLayerTreeHostImpl never receives notification of the scrolls.  A listener that
pushes this scrolling information back to the impl thread will be required to
correct this problem.
 - The main thread is preparing too many tiles.  I think there is a failure
condition when the copositor texture cache is exhausted at maximal zoom.  This
needs to be correct.

Appropriate bugs will be filed when the initial revision lands.

[Jeff Timanus, 2012-09-21 18:10:01]

I updated the change so that only the tiles that are necessary are prepared.  

Unit-tests for the new path are forthcoming.

https://codereview.chromium.org/10916279/diff/15001/cc/CCSettings.cpp
File cc/CCSettings.cpp (right):

https://codereview.chromium.org/10916279/diff/15001/cc/CCSettings.cpp#newcode32
cc/CCSettings.cpp:32: switches::kEnablePinchInCompositor);
Using the command line directly from within CC is exposing some gyp problems, I
believe.  

I get the following warning when building:
../../third_party/gold/gold64: warning: hidden symbol
'switches::kEnablePinchInCompositor' in
obj/chrome/libinstaller_util.a(obj/chrome/../content/public/common/installer_util.content_switches.o)
is referenced by DSO lib/libglue.so

The issue is related to the CONTENT_EXPORT macro in content_switches.h.  When CC
is pulled into glue.so, I don't think the macro is being set properly.  Sound
familiar?

https://codereview.chromium.org/10916279/diff/15001/cc/CCThreadProxy.cpp
File cc/CCThreadProxy.cpp (right):

https://codereview.chromium.org/10916279/diff/15001/cc/CCThreadProxy.cpp#newc...
cc/CCThreadProxy.cpp:470: m_pendingBeginFrameRequest->implTransform =
m_layerTreeHostImpl->implTransform();
I pass the impl-transform from the pinch state to the main thread at this point.
 This transform is then installed on the fist scrollable layer before the call
to CalculateDrawTransforms(...) in CCLayerTreeHost.

This ensures that only those tiles that are needed are prepared, as opposed to
all the tiles covering the red box.

I think my variable naming here is not the best.  I chose impl-transform,
because that's where this transformation is installed on the impl side, but I
think for the main thread, it would be clearer if this name indicated that it
was specific to the pinch zoom state, and only applied to the root scroll layer.
 Thoughts?

[aelias_OOO_until_Jul13, 2012-09-24 23:46:45]

I'm trying this patch locally.  I notice there's a bug that when you let go of
the pinch after zooming out, scroll Y position jumps upward.

[aelias_OOO_until_Jul13, 2012-09-25 03:53:29]

http://codereview.chromium.org/10916279/diff/31001/cc/CCLayerTreeHostImpl.cpp
File cc/CCLayerTreeHostImpl.cpp (right):

http://codereview.chromium.org/10916279/diff/31001/cc/CCLayerTreeHostImpl.cpp...
cc/CCLayerTreeHostImpl.cpp:55: scaledBounds.scale(1 / m_pageScaleFactor);
On Android, < 1 pageScaleFactors are common.

I think the correct way to write this to support them is:

scaledBounds.scale(m_minPageScaleFactor / m_pageScaleFactor);

http://codereview.chromium.org/10916279/diff/31001/cc/CCLayerTreeHostImpl.cpp...
cc/CCLayerTreeHostImpl.cpp:1141:
m_pinchZoomViewport.setTotalPageScaleFactor(m_pageScaleFactor *
m_pageScaleDelta);
This needs to be updated whenever m_pageScaleFactor or m_pageScaleDelta changes,
not just when a pinch gesture happens.  I recommend avoiding the issue by just
moving all the page-scale related values to CCPinchZoomViewport and not holding
a copy in CCLayerTreeHostImpl at all.

[Jeff Timanus, 2012-09-25 18:01:05]

On 2012/09/24 23:46:45, aelias wrote:
> I'm trying this patch locally.  I notice there's a bug that when you let go of
> the pinch after zooming out, scroll Y position jumps upward.

On what platform are you observing the scroll y jump?  I do not see it when
testing on linux.

Are you certain that you also patched in the corresponding webkit change? 
https://bugs.webkit.org/show_bug.cgi?id=93292

[aelias_OOO_until_Jul13, 2012-09-25 19:51:26]

On 2012/09/25 18:01:05, Jeff Timanus wrote:
> On 2012/09/24 23:46:45, aelias wrote:
> > I'm trying this patch locally.  I notice there's a bug that when you let go
of
> > the pinch after zooming out, scroll Y position jumps upward.
> 
> On what platform are you observing the scroll y jump?  I do not see it when
> testing on linux.
> 
> Are you certain that you also patched in the corresponding webkit change? 
> https://bugs.webkit.org/show_bug.cgi?id=93292

Yes.  There may be something config-specific about it, I can't repro on Android
either.  This is on Linux non-Aura build with both your patches with flags:

out/Release/chrome --force-compositing-mode --enable-threaded-compositing
--simulate-touch-screen-with-mouse --enable-pinch-in-compositor

Scroll down to the bottom of the page on http://nytimes.com/, zoom in with the
right mouse button, let go, then zoom out with the right mouse button.

[Jeff Timanus, 2012-09-25 23:50:31]

On 2012/09/25 19:51:26, aelias wrote:
> On 2012/09/25 18:01:05, Jeff Timanus wrote:
> > On 2012/09/24 23:46:45, aelias wrote:
> > > I'm trying this patch locally.  I notice there's a bug that when you let
go
> of
> > > the pinch after zooming out, scroll Y position jumps upward.
> > 
> > On what platform are you observing the scroll y jump?  I do not see it when
> > testing on linux.
> > 
> > Are you certain that you also patched in the corresponding webkit change? 
> > https://bugs.webkit.org/show_bug.cgi?id=93292
> 
> Yes.  There may be something config-specific about it, I can't repro on
Android
> either.  This is on Linux non-Aura build with both your patches with flags:
> 
> out/Release/chrome --force-compositing-mode --enable-threaded-compositing
> --simulate-touch-screen-with-mouse --enable-pinch-in-compositor
> 
> Scroll down to the bottom of the page on http://nytimes.com/, zoom in with the
> right mouse button, let go, then zoom out with the right mouse button.

Patch set #9, and the corresponding update in the webkit patch resolve this
issue.  The problem was that WebViewImpl::setPageScaleFactor(...) was applying
the page scale delta to the scroll offsets.  In the new pinch zoom path, all
scroll offsets remain in pre-scaled space.  

See wk patch:  https://bugs.webkit.org/attachment.cgi?id=165699

[Jeff Timanus, 2012-09-25 23:51:05]

On 2012/09/25 23:50:31, Jeff Timanus wrote:
> On 2012/09/25 19:51:26, aelias wrote:
> > On 2012/09/25 18:01:05, Jeff Timanus wrote:
> > > On 2012/09/24 23:46:45, aelias wrote:
> > > > I'm trying this patch locally.  I notice there's a bug that when you let
> go
> > of
> > > > the pinch after zooming out, scroll Y position jumps upward.
> > > 
> > > On what platform are you observing the scroll y jump?  I do not see it
when
> > > testing on linux.
> > > 
> > > Are you certain that you also patched in the corresponding webkit change? 
> > > https://bugs.webkit.org/show_bug.cgi?id=93292
> > 
> > Yes.  There may be something config-specific about it, I can't repro on
> Android
> > either.  This is on Linux non-Aura build with both your patches with flags:
> > 
> > out/Release/chrome --force-compositing-mode --enable-threaded-compositing
> > --simulate-touch-screen-with-mouse --enable-pinch-in-compositor
> > 
> > Scroll down to the bottom of the page on http://nytimes.com/, zoom in with
the
> > right mouse button, let go, then zoom out with the right mouse button.
> 
> Patch set #9, and the corresponding update in the webkit patch resolve this
> issue.  

Correction:  Patch set 10.

The problem was that WebViewImpl::setPageScaleFactor(...) was applying
> the page scale delta to the scroll offsets.  In the new pinch zoom path, all
> scroll offsets remain in pre-scaled space.  
> 
> See wk patch:  https://bugs.webkit.org/attachment.cgi?id=165699

[Jeff Timanus, 2012-09-26 21:54:20]

PT(another)L

Patch set #12 pushes the kEnablePinchInCompositor flag to CCSettings via the
same model as the other fields in this structure.

My previous patches were referencing content_common directly from cc.  This
approach built successfully on linux, but failed on Windows.  The libglue.dll
target would complain of missing symbols (the kEnablePinchInCompositor string). 
Adding content_common as a dependency of cc caused gyp to report a cyclic
dependency.

I figured that this flag can remain consistent with the pre-existing ones, and
they can all be converted in a separate change that also addresses the gyp
dependencies.

[aelias_OOO_until_Jul13, 2012-09-26 23:15:08]

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cpp
File cc/CCLayerTreeHostImpl.cpp (right):

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:60: float finalMagnifyScale = m_pageScaleFactor *
delta;
Rename "finalMagnifyScale" to "totalPageScaleFactor" for consistency.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:74: if (!pageScaleFactor)
Replace this with ASSERT(pageScaleFactor); since it would be a bug in WebKit if
this happens.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:90: float totalScale = totalPageScaleFactor();
No need for this local variable.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:91: if (totalScale > 1)
Remove this if().  It's correct in the == 1 or < 1 case as well.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:95: bounds.setSize(scaledBounds);
Use the FloatRect constructor instead.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:109: overflow.setWidth(pinchedBounds.x());
Looks like a bug here.  Seems overflow values should be set before mutating
pinchedBounds, not after.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:1270: // If pinch zoom is applied in the impl thread,
then there is no need to
There is still a need for this in some form.  It's to repaint at a different
scale.   Replace this comment with // FIXME(aelias): Make these painting
optimizations compatible with compositor-side scaling.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:1288: WebTransformationMatrix
CCLayerTreeHostImpl::implTransform() const
Move this method into CCPinchZoomViewport.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.h
File cc/CCLayerTreeHostImpl.h (right):

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.h#...
cc/CCLayerTreeHostImpl.h:53: : m_pageScaleFactor(1)
Move this constructor to the .cpp file.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.h#...
cc/CCLayerTreeHostImpl.h:56: , m_minPageScaleFactor(0)
Hmm, this default of zero is not very sane particularly since nobody asserts
that it was initialized.  Let's default the min and max to 1 as well.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.h#...
cc/CCLayerTreeHostImpl.h:99: FloatSize m_unpinchedBounds;
Rename this to "m_unpinchedViewportSize" since it's not a rect.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImplTes...
File cc/CCLayerTreeHostImplTest.cpp (right):

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImplTes...
cc/CCLayerTreeHostImplTest.cpp:509: TEST_F(CCLayerTreeHostImplTest,
pinchGesture)
Hmm... all of these tests should really be run in the new coordinate space. 
Your coverage is poor with only the above test.

I'm thinking we can run every test in both modes using parametrized tests.  Look
at CCResourceProviderTest.cpp for an example.

[aelias_OOO_until_Jul13, 2012-09-26 23:19:04]

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cpp
File cc/CCLayerTreeHostImpl.cpp (right):

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:91: if (totalScale > 1)
On 2012/09/26 23:15:09, aelias wrote:
> Remove this if().  It's correct in the == 1 or < 1 case as well.

Actually, maybe it's not.  As I suggested in my comment a few days ago, is what
we want really:
scaledBounds.scale(m_minPageScaleFactor / totalScale);

?

(since the "red box" will be at m_minPageScaleFactor scale).

[Jeff Timanus, 2012-10-01 21:42:36]

I think this version of the patch is getting close to ready to land.

I addressed all of the previous comments, and added the suggested testing.

For now, I propose:
 - Landing the wk side of this patch.
 - Alexandre and I had an offline discussion on changing the space in which the
blue-box resides so that the red and blue box are in the same coordinate system.
 I will add this change.
 - Submit this change once the wk patch is in, and rolled, and the above change
is done, as well as any other fall out.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cpp
File cc/CCLayerTreeHostImpl.cpp (right):

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:60: float finalMagnifyScale = m_pageScaleFactor *
delta;
On 2012/09/26 23:15:09, aelias wrote:
> Rename "finalMagnifyScale" to "totalPageScaleFactor" for consistency.

Done.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:74: if (!pageScaleFactor)
On 2012/09/26 23:15:09, aelias wrote:
> Replace this with ASSERT(pageScaleFactor); since it would be a bug in WebKit
if
> this happens.

Done.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:90: float totalScale = totalPageScaleFactor();
On 2012/09/26 23:15:09, aelias wrote:
> No need for this local variable.

Done.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:95: bounds.setSize(scaledBounds);
On 2012/09/26 23:15:09, aelias wrote:
> Use the FloatRect constructor instead.

Done.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:109: overflow.setWidth(pinchedBounds.x());
On 2012/09/26 23:15:09, aelias wrote:
> Looks like a bug here.  Seems overflow values should be set before mutating
> pinchedBounds, not after.

Good catch.  I'm surprised that this wasn't causing issues previously.

Done.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:109: overflow.setWidth(pinchedBounds.x());
On 2012/09/26 23:15:09, aelias wrote:
> Looks like a bug here.  Seems overflow values should be set before mutating
> pinchedBounds, not after.

Good catch.  Fixed.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:1270: // If pinch zoom is applied in the impl thread,
then there is no need to
On 2012/09/26 23:15:09, aelias wrote:
> There is still a need for this in some form.  It's to repaint at a different
> scale.   Replace this comment with // FIXME(aelias): Make these painting
> optimizations compatible with compositor-side scaling.

Done.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.cp...
cc/CCLayerTreeHostImpl.cpp:1288: WebTransformationMatrix
CCLayerTreeHostImpl::implTransform() const
On 2012/09/26 23:15:09, aelias wrote:
> Move this method into CCPinchZoomViewport.

Done.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.h
File cc/CCLayerTreeHostImpl.h (right):

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.h#...
cc/CCLayerTreeHostImpl.h:53: : m_pageScaleFactor(1)
On 2012/09/26 23:15:09, aelias wrote:
> Move this constructor to the .cpp file.

Done.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.h#...
cc/CCLayerTreeHostImpl.h:56: , m_minPageScaleFactor(0)
On 2012/09/26 23:15:09, aelias wrote:
> Hmm, this default of zero is not very sane particularly since nobody asserts
> that it was initialized.  Let's default the min and max to 1 as well.

I tried to make this change, but it introduced subtle regressions.  It turns out
that these are tested for 0.  See CCPinchZoomViewport::setPageScaleDelta, and
the original code in CCLayerTreeHostImpl::setPageScaleDelta.

I left the initialization at 0, 0, because that change isn't directly required
by my work.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImpl.h#...
cc/CCLayerTreeHostImpl.h:99: FloatSize m_unpinchedBounds;
On 2012/09/26 23:15:09, aelias wrote:
> Rename this to "m_unpinchedViewportSize" since it's not a rect.

Done.

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImplTes...
File cc/CCLayerTreeHostImplTest.cpp (right):

https://codereview.chromium.org/10916279/diff/39009/cc/CCLayerTreeHostImplTes...
cc/CCLayerTreeHostImplTest.cpp:509: TEST_F(CCLayerTreeHostImplTest,
pinchGesture)
On 2012/09/26 23:15:09, aelias wrote:
> Hmm... all of these tests should really be run in the new coordinate space. 
> Your coverage is poor with only the above test.
> 
> I'm thinking we can run every test in both modes using parametrized tests. 
Look
> at CCResourceProviderTest.cpp for an example.

I converted all of these tests to parametrized, and corrected them for both the
new pinch path and the old one.

I kept the tests that I had added around, but only execute them when testing the
new path.

[aelias_OOO_until_Jul13, 2012-10-02 06:04:44]

LGTM.  Looking pretty clean now, let's get this landed.  James, could you give
it a final look?

[jamesr, 2012-10-03 19:48:56]

One naming nit: "implTransform" is really vague. It looks like this is either
the pinchZoomTransform or pageScaleTransform - can we name it something like
that?

It looks like one of the test files has some leftover/intermediate stuff left in
it.

Otherwise lgtm.

https://chromiumcodereview.appspot.com/10916279/diff/63002/cc/CCLayerTreeHost...
File cc/CCLayerTreeHostImplTest.cpp (right):

https://chromiumcodereview.appspot.com/10916279/diff/63002/cc/CCLayerTreeHost...
cc/CCLayerTreeHostImplTest.cpp:54: /*                                public
::testing::WithParamInterface<bool>,*/
hm, what's this? leftovers?

https://chromiumcodereview.appspot.com/10916279/diff/63002/cc/CCLayerTreeHost...
cc/CCLayerTreeHostImplTest.cpp:471: if
(!CCSettings::pageScalePinchZoomEnabled())
not sure I grok this with line 474 - it looks like it's testing if the setting
is false and returning if so, then setting the value to true. hm?

https://chromiumcodereview.appspot.com/10916279/diff/63002/content/browser/we...
File content/browser/web_contents/web_contents_impl.cc (right):

https://chromiumcodereview.appspot.com/10916279/diff/63002/content/browser/we...
content/browser/web_contents/web_contents_impl.cc:677:
GpuProcessHost::gpu_enabled() &&
this check seems unnecessary

[aelias_OOO_until_Jul13, 2012-10-03 20:07:57]

For implTransform, it contains impl-thread page scale delta and scroll offset
delta, but not normal page scale, so I think your suggested names are
misleading.

I like "implTransform" because it represents the transformation owned by the
impl thread.  It's possible that it could be extended to other kinds of
impl-thread-owned transformations in the future.  But, as there are no current
plans for this, we could go with "implSidePinchZoomTransform", if we prefer
being verbose.

[Jeff Timanus, 2012-10-03 20:56:04]

Thanks for the review.  I addressed most of your suggestions.

Given the recent discussion here, do we feel that there is a need to change the
name of implTransform?

https://codereview.chromium.org/10916279/diff/63002/cc/CCLayerTreeHostImplTes...
File cc/CCLayerTreeHostImplTest.cpp (right):

https://codereview.chromium.org/10916279/diff/63002/cc/CCLayerTreeHostImplTes...
cc/CCLayerTreeHostImplTest.cpp:54: /*                                public
::testing::WithParamInterface<bool>,*/
On 2012/10/03 19:48:57, jamesr wrote:
> hm, what's this? leftovers?

Yes.  Old cruft.  Removed.

https://codereview.chromium.org/10916279/diff/63002/cc/CCLayerTreeHostImplTes...
cc/CCLayerTreeHostImplTest.cpp:471: if
(!CCSettings::pageScalePinchZoomEnabled())
On 2012/10/03 19:48:57, jamesr wrote:
> not sure I grok this with line 474 - it looks like it's testing if the setting
> is false and returning if so, then setting the value to true. hm?

This is a good catch.  Previous patches (before I added the parametrization)
assigned the setting explicitly.  I can remove the explicit assignment of the
state.  However, the early out is still required, because these new tests are
only relevant in the new-pinch-world.

https://codereview.chromium.org/10916279/diff/63002/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/10916279/diff/63002/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:677:
GpuProcessHost::gpu_enabled() &&
On 2012/10/03 19:48:57, jamesr wrote:
> this check seems unnecessary

I followed the model of all of the other preference checks in this file.  All
the compositor related checks also check to see if the gpu is enabled. (For
example accelerated_compositing_enabled.)

[jamesr, 2012-10-03 20:58:11]

I suppose implTransform is reasonable given that.

Re the gpu check - you can leave it there but Alex will have to remove it at
some point to support software compositing.  Easier to leave it out from the
start.  We'll have to consider what the other flags do sooner or later as well.

[commit-bot: I haz the power, 2012-10-04 20:19:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/twiz@chromium.org/10916279/52034

[commit-bot: I haz the power, 2012-10-04 20:19:32]

Failed to apply patch for cc/CCSettings.h:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file cc/CCSettings.h
  Hunk #1 FAILED at 14.
  1 out of 1 hunk FAILED -- saving rejects to file cc/CCSettings.h.rej

Patch:       cc/CCSettings.h
Index: cc/CCSettings.h
===================================================================
--- cc/CCSettings.h	(revision 160197)
+++ cc/CCSettings.h	(working copy)
@@ -14,12 +14,14 @@
     static bool perTilePaintingEnabled();
     static bool partialSwapEnabled();
     static bool acceleratedAnimationEnabled();
+    static bool pageScalePinchZoomEnabled();
 
     // These setters should only be used on the main thread before the layer
     // renderer is initialized.
     static void setPerTilePaintingEnabled(bool);
     static void setPartialSwapEnabled(bool);
     static void setAcceleratedAnimationEnabled(bool);
+    static void setPageScalePinchZoomEnabled(bool);
 
     // These settings are meant to be set only once, and only read thereafter.
     // This function is only for resetting settings in tests.

[Jeff Timanus, 2012-10-04 23:31:21]

Antoine, can you please OWNERS-stamp the changes in these directories:

    content/browser/renderer_host
    content/common
    content/browser/web_contents
    ui/compositor
    content/public

[piman, 2012-10-05 00:01:08]

https://chromiumcodereview.appspot.com/10916279/diff/59003/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10916279/diff/59003/content/renderer/r...
content/renderer/render_view_impl.cc:3170:
webview()->setPageScaleFactorLimits(1, 4);
note: this is possibly racy. isAcceleratedCompositingActive() may not be true
now, but could change later. Should we do this as well on
didActivateCompositor()?

https://chromiumcodereview.appspot.com/10916279/diff/59003/ui/compositor/comp...
File ui/compositor/compositor.cc (right):

https://chromiumcodereview.appspot.com/10916279/diff/59003/ui/compositor/comp...
ui/compositor/compositor.cc:190:
command_line->HasSwitch(switches::kEnablePinchInCompositor));
Do we really want to pinch-zoom the UI?

[Jeff Timanus, 2012-10-05 00:05:34]

Thx for the review.

https://chromiumcodereview.appspot.com/10916279/diff/59003/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10916279/diff/59003/content/renderer/r...
content/renderer/render_view_impl.cc:3170:
webview()->setPageScaleFactorLimits(1, 4);
On 2012/10/05 00:01:08, piman wrote:
> note: this is possibly racy. isAcceleratedCompositingActive() may not be true
> now, but could change later. Should we do this as well on
> didActivateCompositor()?

If this is racey, are the other conditions also racey?  They are also gated on
isAccelratedCompositingActive().

https://chromiumcodereview.appspot.com/10916279/diff/59003/ui/compositor/comp...
File ui/compositor/compositor.cc (right):

https://chromiumcodereview.appspot.com/10916279/diff/59003/ui/compositor/comp...
ui/compositor/compositor.cc:190:
command_line->HasSwitch(switches::kEnablePinchInCompositor));
On 2012/10/05 00:01:08, piman wrote:
> Do we really want to pinch-zoom the UI?

I'm not sure if I understand your question.  This particular call pushes the
switch state for the new flag to the compositor, which is used to selectively
enable my new pinch patch.

As I understand the implementation, the browser UI should never be scaled, only
the contents of the root scroll layer.

[piman, 2012-10-05 00:25:51]

https://chromiumcodereview.appspot.com/10916279/diff/59003/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10916279/diff/59003/content/renderer/r...
content/renderer/render_view_impl.cc:3170:
webview()->setPageScaleFactorLimits(1, 4);
On 2012/10/05 00:05:34, Jeff Timanus wrote:
> On 2012/10/05 00:01:08, piman wrote:
> > note: this is possibly racy. isAcceleratedCompositingActive() may not be
true
> > now, but could change later. Should we do this as well on
> > didActivateCompositor()?
> 
> If this is racey, are the other conditions also racey?  They are also gated on
> isAccelratedCompositingActive().
> 

Most likely, yes.

https://chromiumcodereview.appspot.com/10916279/diff/59003/ui/compositor/comp...
File ui/compositor/compositor.cc (right):

https://chromiumcodereview.appspot.com/10916279/diff/59003/ui/compositor/comp...
ui/compositor/compositor.cc:190:
command_line->HasSwitch(switches::kEnablePinchInCompositor));
On 2012/10/05 00:05:34, Jeff Timanus wrote:
> On 2012/10/05 00:01:08, piman wrote:
> > Do we really want to pinch-zoom the UI?
> 
> I'm not sure if I understand your question.  This particular call pushes the
> switch state for the new flag to the compositor, which is used to selectively
> enable my new pinch patch.
> 
> As I understand the implementation, the browser UI should never be scaled,
only
> the contents of the root scroll layer.

This is the UI's compositor. Why are we doing anything to it?

[Jeff Timanus, 2012-10-05 15:37:49]

https://chromiumcodereview.appspot.com/10916279/diff/59003/content/renderer/r...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10916279/diff/59003/content/renderer/r...
content/renderer/render_view_impl.cc:3170:
webview()->setPageScaleFactorLimits(1, 4);
On 2012/10/05 00:25:51, piman wrote:
> On 2012/10/05 00:05:34, Jeff Timanus wrote:
> > On 2012/10/05 00:01:08, piman wrote:
> > > note: this is possibly racy. isAcceleratedCompositingActive() may not be
> true
> > > now, but could change later. Should we do this as well on
> > > didActivateCompositor()?
> > 
> > If this is racey, are the other conditions also racey?  They are also gated
on
> > isAccelratedCompositingActive().
> > 
> 
> Most likely, yes.

I'm not sure if fixing these races is appropriate for this change.  I've already
touched a lot of files here.  I think filing a bug and a follow-up change would
be better.  Agree?

https://chromiumcodereview.appspot.com/10916279/diff/59003/ui/compositor/comp...
File ui/compositor/compositor.cc (right):

https://chromiumcodereview.appspot.com/10916279/diff/59003/ui/compositor/comp...
ui/compositor/compositor.cc:190:
command_line->HasSwitch(switches::kEnablePinchInCompositor));
On 2012/10/05 00:25:51, piman wrote:
> On 2012/10/05 00:05:34, Jeff Timanus wrote:
> > On 2012/10/05 00:01:08, piman wrote:
> > > Do we really want to pinch-zoom the UI?
> > 
> > I'm not sure if I understand your question.  This particular call pushes the
> > switch state for the new flag to the compositor, which is used to
selectively
> > enable my new pinch patch.
> > 
> > As I understand the implementation, the browser UI should never be scaled,
> only
> > the contents of the root scroll layer.
> 
> This is the UI's compositor. Why are we doing anything to it?

Ah.  It's possible that I did not understand the different compositor
environments.  I followed the existing flag behaviour to push the pinch state to
the compositor.

I don't think pinch should be relevant for the UI.  If I remove this call, and
rely on the similar path in content/renderer/render_thread_impl.cc, should that
suffice for only modifying the settings state of the web-contents compositor?

[piman, 2012-10-05 16:04:10]

On Fri, Oct 5, 2012 at 8:37 AM, <twiz@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/10916279/diff/**
>
59003/content/renderer/render_**view_impl.cc<https://chromiumcodereview.appspot.com/10916279/diff/59003/content/renderer/render_view_impl.cc>
> File content/renderer/render_view_**impl.cc (right):
>
> https://chromiumcodereview.**appspot.com/10916279/diff/**
>
59003/content/renderer/render_**view_impl.cc#newcode3170<https://chromiumcodereview.appspot.com/10916279/diff/59003/content/renderer/render_view_impl.cc#newcode3170>
> content/renderer/render_view_**impl.cc:3170:
> webview()->**setPageScaleFactorLimits(1, 4);
> On 2012/10/05 00:25:51, piman wrote:
>
>> On 2012/10/05 00:05:34, Jeff Timanus wrote:
>> > On 2012/10/05 00:01:08, piman wrote:
>> > > note: this is possibly racy. isAcceleratedCompositingActive**() may
>>
> not be
>
>> true
>> > > now, but could change later. Should we do this as well on
>> > > didActivateCompositor()?
>> >
>> > If this is racey, are the other conditions also racey?  They are
>>
> also gated on
>
>> > isAccelratedCompositingActive(**).
>> >
>>
>
>  Most likely, yes.
>>
>
> I'm not sure if fixing these races is appropriate for this change.  I've
> already touched a lot of files here.  I think filing a bug and a
> follow-up change would be better.  Agree?


You should at least fix the race for the code you're adding. But honestly,
it looks like just a couple of lines.


>
>
> https://chromiumcodereview.**appspot.com/10916279/diff/**
>
59003/ui/compositor/**compositor.cc<https://chromiumcodereview.appspot.com/10916279/diff/59003/ui/compositor/compositor.cc>
> File ui/compositor/compositor.cc (right):
>
> https://chromiumcodereview.**appspot.com/10916279/diff/**
>
59003/ui/compositor/**compositor.cc#newcode190<https://chromiumcodereview.appspot.com/10916279/diff/59003/ui/compositor/compositor.cc#newcode190>
> ui/compositor/compositor.cc:**190:
> command_line->HasSwitch(**switches::**kEnablePinchInCompositor));
> On 2012/10/05 00:25:51, piman wrote:
>
>> On 2012/10/05 00:05:34, Jeff Timanus wrote:
>> > On 2012/10/05 00:01:08, piman wrote:
>> > > Do we really want to pinch-zoom the UI?
>> >
>> > I'm not sure if I understand your question.  This particular call
>>
> pushes the
>
>> > switch state for the new flag to the compositor, which is used to
>>
> selectively
>
>> > enable my new pinch patch.
>> >
>> > As I understand the implementation, the browser UI should never be
>>
> scaled,
>
>> only
>> > the contents of the root scroll layer.
>>
>
>  This is the UI's compositor. Why are we doing anything to it?
>>
>
> Ah.  It's possible that I did not understand the different compositor
> environments.  I followed the existing flag behaviour to push the pinch
> state to the compositor.
>
> I don't think pinch should be relevant for the UI.  If I remove this
> call, and rely on the similar path in
> content/renderer/render_**thread_impl.cc, should that suffice for only
> modifying the settings state of the web-contents compositor?
>

Modifying this code has no effect on the page compositor.
If your code works without it, you should be good.


>
>
https://chromiumcodereview.**appspot.com/10916279/<https://chromiumcodereview...
>

[Jeff Timanus, 2012-10-05 17:54:40]

On 2012/10/05 16:04:10, piman wrote:
> On Fri, Oct 5, 2012 at 8:37 AM, <mailto:twiz@chromium.org> wrote:
> 
> >
> > https://chromiumcodereview.**appspot.com/10916279/diff/**
> >
>
59003/content/renderer/render_**view_impl.cc<https://chromiumcodereview.appspot.com/10916279/diff/59003/content/renderer/render_view_impl.cc>
> > File content/renderer/render_view_**impl.cc (right):
> >
> > https://chromiumcodereview.**appspot.com/10916279/diff/**
> >
>
59003/content/renderer/render_**view_impl.cc#newcode3170<https://chromiumcodereview.appspot.com/10916279/diff/59003/content/renderer/render_view_impl.cc#newcode3170>
> > content/renderer/render_view_**impl.cc:3170:
> > webview()->**setPageScaleFactorLimits(1, 4);
> > On 2012/10/05 00:25:51, piman wrote:
> >
> >> On 2012/10/05 00:05:34, Jeff Timanus wrote:
> >> > On 2012/10/05 00:01:08, piman wrote:
> >> > > note: this is possibly racy. isAcceleratedCompositingActive**() may
> >>
> > not be
> >
> >> true
> >> > > now, but could change later. Should we do this as well on
> >> > > didActivateCompositor()?
> >> >
> >> > If this is racey, are the other conditions also racey?  They are
> >>
> > also gated on
> >
> >> > isAccelratedCompositingActive(**).
> >> >
> >>
> >
> >  Most likely, yes.
> >>
> >
> > I'm not sure if fixing these races is appropriate for this change.  I've
> > already touched a lot of files here.  I think filing a bug and a
> > follow-up change would be better.  Agree?
> 
> 
> You should at least fix the race for the code you're adding. But honestly,
> it looks like just a couple of lines.

I changed the code so that the command line is checked again when the compositor
is active.  This should resolve the race condition.

> 
> 
> >
> >
> > https://chromiumcodereview.**appspot.com/10916279/diff/**
> >
>
59003/ui/compositor/**compositor.cc<https://chromiumcodereview.appspot.com/10916279/diff/59003/ui/compositor/compositor.cc>
> > File ui/compositor/compositor.cc (right):
> >
> > https://chromiumcodereview.**appspot.com/10916279/diff/**
> >
>
59003/ui/compositor/**compositor.cc#newcode190<https://chromiumcodereview.appspot.com/10916279/diff/59003/ui/compositor/compositor.cc#newcode190>
> > ui/compositor/compositor.cc:**190:
> > command_line->HasSwitch(**switches::**kEnablePinchInCompositor));
> > On 2012/10/05 00:25:51, piman wrote:
> >
> >> On 2012/10/05 00:05:34, Jeff Timanus wrote:
> >> > On 2012/10/05 00:01:08, piman wrote:
> >> > > Do we really want to pinch-zoom the UI?
> >> >
> >> > I'm not sure if I understand your question.  This particular call
> >>
> > pushes the
> >
> >> > switch state for the new flag to the compositor, which is used to
> >>
> > selectively
> >
> >> > enable my new pinch patch.
> >> >
> >> > As I understand the implementation, the browser UI should never be
> >>
> > scaled,
> >
> >> only
> >> > the contents of the root scroll layer.
> >>
> >
> >  This is the UI's compositor. Why are we doing anything to it?
> >>
> >
> > Ah.  It's possible that I did not understand the different compositor
> > environments.  I followed the existing flag behaviour to push the pinch
> > state to the compositor.
> >
> > I don't think pinch should be relevant for the UI.  If I remove this
> > call, and rely on the similar path in
> > content/renderer/render_**thread_impl.cc, should that suffice for only
> > modifying the settings state of the web-contents compositor?
> >
> 
> Modifying this code has no effect on the page compositor.
> If your code works without it, you should be good.

I tested my path without this flag, and the new zoom still worked.  I removed
this flag assignment.

> 
> 
> >
> >
>
https://chromiumcodereview.**appspot.com/10916279/%3Chttps://chromiumcoderevi...>
> >

[piman, 2012-10-05 18:03:48]

lgtm

[commit-bot: I haz the power, 2012-10-05 18:05:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/twiz@chromium.org/10916279/54039

[commit-bot: I haz the power, 2012-10-05 18:14:00]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-05 18:59:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/twiz@chromium.org/10916279/58007

[commit-bot: I haz the power, 2012-10-05 20:07:42]

Retried try job too often for step(s) browser_tests

[aelias_OOO_until_Jul13, 2012-10-08 17:42:07]

I'll try to land this.

[commit-bot: I haz the power, 2012-10-08 17:42:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/twiz@chromium.org/10916279/58007

[commit-bot: I haz the power, 2012-10-08 17:42:27]

Failed to apply patch for cc/CCLayerTreeHostCommon.cpp:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file cc/CCLayerTreeHostCommon.cpp
  Hunk #2 FAILED at 468.
  1 out of 2 hunks FAILED -- saving rejects to file
cc/CCLayerTreeHostCommon.cpp.rej

Patch:       cc/CCLayerTreeHostCommon.cpp
Index: cc/CCLayerTreeHostCommon.cpp
===================================================================
--- cc/CCLayerTreeHostCommon.cpp	(revision 160422)
+++ cc/CCLayerTreeHostCommon.cpp	(working copy)
@@ -271,7 +271,7 @@
     //
 
     WebTransformationMatrix partialLayerOriginTransform = parentMatrix;
-    partialLayerOriginTransform.scale(scrollingLayer->pageScaleDelta());
+    partialLayerOriginTransform.multiply(scrollingLayer->implTransform());
 
     WebTransformationMatrix scrollCompensationForThisLayer =
partialLayerOriginTransform; // Step 3
    
scrollCompensationForThisLayer.translate(scrollingLayer->scrollDelta().width(),
scrollingLayer->scrollDelta().height()); // Step 2
@@ -468,13 +468,13 @@
     float centerOffsetY = (0.5 - anchorPoint.y()) * bounds.height();
 
     WebTransformationMatrix layerLocalTransform;
-    // LT = S[pageScaleDelta]
-    layerLocalTransform.scale(layer->pageScaleDelta());
-    // LT = S[pageScaleDelta] * Tr[origin] * Tr[origin2anchor]
+    // LT = M[impl transformation]
+    layerLocalTransform.multiply(layer->implTransform());
+    // LT = M[impl transformation] * Tr[origin] * Tr[origin2anchor]
     layerLocalTransform.translate3d(position.x() + anchorPoint.x() *
bounds.width(), position.y() + anchorPoint.y() * bounds.height(),
layer->anchorPointZ());
-    // LT = S[pageScaleDelta] * Tr[origin] * Tr[origin2anchor] * M[layer]
+    // LT = M[impl transformation] * Tr[origin] * Tr[origin2anchor] * M[layer]
     layerLocalTransform.multiply(layer->transform());
-    // LT = S[pageScaleDelta] * Tr[origin] * Tr[origin2anchor] * M[layer] *
Tr[anchor2center]
+    // LT = M[impl transformation] * Tr[origin] * Tr[origin2anchor] * M[layer]
* Tr[anchor2center]
     layerLocalTransform.translate3d(centerOffsetX, centerOffsetY,
-layer->anchorPointZ());
 
     WebTransformationMatrix combinedTransform = parentMatrix;

[commit-bot: I haz the power, 2012-10-08 17:46:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/twiz@chromium.org/10916279/58007

[commit-bot: I haz the power, 2012-10-08 17:46:46]

Failed to apply patch for cc/CCLayerTreeHostCommon.cpp:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  patching file cc/CCLayerTreeHostCommon.cpp
  Hunk #2 FAILED at 468.
  1 out of 2 hunks FAILED -- saving rejects to file
cc/CCLayerTreeHostCommon.cpp.rej

Patch:       cc/CCLayerTreeHostCommon.cpp
Index: cc/CCLayerTreeHostCommon.cpp
===================================================================
--- cc/CCLayerTreeHostCommon.cpp	(revision 160422)
+++ cc/CCLayerTreeHostCommon.cpp	(working copy)
@@ -271,7 +271,7 @@
     //
 
     WebTransformationMatrix partialLayerOriginTransform = parentMatrix;
-    partialLayerOriginTransform.scale(scrollingLayer->pageScaleDelta());
+    partialLayerOriginTransform.multiply(scrollingLayer->implTransform());
 
     WebTransformationMatrix scrollCompensationForThisLayer =
partialLayerOriginTransform; // Step 3
    
scrollCompensationForThisLayer.translate(scrollingLayer->scrollDelta().width(),
scrollingLayer->scrollDelta().height()); // Step 2
@@ -468,13 +468,13 @@
     float centerOffsetY = (0.5 - anchorPoint.y()) * bounds.height();
 
     WebTransformationMatrix layerLocalTransform;
-    // LT = S[pageScaleDelta]
-    layerLocalTransform.scale(layer->pageScaleDelta());
-    // LT = S[pageScaleDelta] * Tr[origin] * Tr[origin2anchor]
+    // LT = M[impl transformation]
+    layerLocalTransform.multiply(layer->implTransform());
+    // LT = M[impl transformation] * Tr[origin] * Tr[origin2anchor]
     layerLocalTransform.translate3d(position.x() + anchorPoint.x() *
bounds.width(), position.y() + anchorPoint.y() * bounds.height(),
layer->anchorPointZ());
-    // LT = S[pageScaleDelta] * Tr[origin] * Tr[origin2anchor] * M[layer]
+    // LT = M[impl transformation] * Tr[origin] * Tr[origin2anchor] * M[layer]
     layerLocalTransform.multiply(layer->transform());
-    // LT = S[pageScaleDelta] * Tr[origin] * Tr[origin2anchor] * M[layer] *
Tr[anchor2center]
+    // LT = M[impl transformation] * Tr[origin] * Tr[origin2anchor] * M[layer]
* Tr[anchor2center]
     layerLocalTransform.translate3d(centerOffsetX, centerOffsetY,
-layer->anchorPointZ());
 
     WebTransformationMatrix combinedTransform = parentMatrix;