Support bottom-right anchored fixed-position elements during a pinch gesture

Implement pinch zoom for bottom-right fixed-position elements

This patch adds a fixed container size compensation matrix to fixed-position
layers, so fixed-position layers can be anchored to right / bottom edge during
a pinch gesture.

WebKit side: https://bugs.webkit.org/show_bug.cgi?id=111670

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

[trchen, 2013-03-07 03:49:03]

This is the chromium-side patch for
https://bugs.webkit.org/show_bug.cgi?id=111670

[shawnsingh, 2013-03-07 09:57:06]

I'm pretty sure this patch won't work as-is, because the sizeDelta is not being
re-scaled according to layers' transforms as it is passed down the hierarchy.

But, if you do try to do that, you'll find it requires nasty inverse transforms
and will quickly become a bit of a mess.

The scroll compensation matrix is actually computed in a better way, and this
sizeDelta compensation should follow exactly the same pattern - and it should
actually be folded into the same code for efficiency and code re-use.  

Here's how it works:

(1) the compensation matrix is a matrix that represents the following sequence
of transforms (reading the math from right-to-left)
  -- transform from target-surface space to the container layer's space
  -- in that space, we can easily apply the compensation according to the
container's scrollDelta (or sizeDelta)
  -- transform back to target-surface space.

The nice property about this matrix is that it receives objects in
target-surface space, and outputs them also in target-surface space, and the
only effect is has is to compensate for the scrollDelta (or sizeDelta)
accounting for any scales, rotations, etc that may have occurred along the way. 
It does so in the correct space and for the most part will do fewer matrix
multiplications (and only one matrix inverse) to compute it all.  Furthermore,
folding this computation in with the scrollDelta will allow us to avoid
computing those matrices twice in separate places in code, too.

(2) for any layer that is fixed to a container, we pre-multiply (i.e. in the
math, multiply from the left;  in the code, it's called Concat instead of
Preconcat) this matrix to the combinedTransform.  The combinedTransform outputs
objects into target surface space.  So from right-to-left, everything seams
together perfectly.


I hope that made sense?  I'm happy to explain more offline.  Maybe I should just
create a doc that explains the transform mechanism of the scroll compensation.


And last important thing - ideally we should have several (probably 7-10) unit
tests that cover the various scenarios here.  You might be able to mimic the
testing pattern used for scroll compensation unit tests and adapt them to
sizeDelta computations, and possibly add another test or two that cover the
combination of scroll and sizeDelta.

I know that is a lot of work, I'll be happy to help you write the unit tests if
this patch can wait until next week... my preference would be that we don't land
this patch however without unit tests.

Thanks!

https://codereview.chromium.org/12552004/diff/3005/cc/layer_impl.h
File cc/layer_impl.h (right):

https://codereview.chromium.org/12552004/diff/3005/cc/layer_impl.h#newcode402
cc/layer_impl.h:402: // Currently the only fixed container that resizes is the
main frame
I imagine that it will be easy to overlook updating this comment if we ever have
to change it.   Also, we should be able to make this patch work for any
container layer that resizes (even though that wouldn't happen yet).  So my vote
is to remove this second sentence in this comment.

https://codereview.chromium.org/12552004/diff/3005/cc/layer_tree_host_common.cc
File cc/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12552004/diff/3005/cc/layer_tree_host_common....
cc/layer_tree_host_common.cc:720:
combinedTransform.PreconcatTransform(bottomRightAnchorCompensation);
Chances are, once we do this correctly, that this will be Concat instead of
Preconcat.  Better yet, I think the right way to do this will be to fold this
computation into the scroll compensation, and re-name scrollCompensation to
something more generic.  More explanation at the beginning of this review =)

[shawnsingh, 2013-03-07 10:14:49]

So, for all the verbosity I just spewed out,  I should point out that all that
transform math is already implemented and you probably don't need to change it. 
All we would need for this patch is:

(1) add your sizeDelta compensation translation inside of
computeScrollCompensationForThisLayer, as part of Step 2

(2) Update the conditions for which we early exit that code path, so that we
don't early exit when we need to compute sizeDelta compensation

(3) all comments and symbol names need to be updated to reflect that it's not
just "scroll" compensation anymore, but a more generic
fixedPositionCompensation, perhaps.

That should be it, i think =)  The unit tests would be the actual hard part.

[trchen, 2013-03-07 11:54:46]

I'm aware of the transformation issue you're saying, but that actually won't be
a problem for WebKit generated layers. There is a rule that any transformation
will result in a fixed container, which means there will be only simple
translate between the fixed-position layer and its fixed container.

We can do what you suggested, just pass another 2 compensation matrix for
right-edge anchor and bottom-edge anchor compensation in the same manner the
scroll delta compensation is passed. However IMO that adds extra complexity to
the code and I don't see any real benefits.

I believe your justification is that we can support fixed-position layers that
has arbitrary transform between its fixed container, so we won't have to be
limited by Apple's de facto standard. I would suggest lets keep it simple until
there is real needs. We can change how bottom-right compensation matrix is
computed later if needed.

BTW I will definitely write an unit test. Before that I just want to make sure
everybody accept it is a good enough implementation. :)

[trchen, 2013-03-07 12:09:50]

Another question: Does your approach work for flattened 3D layer with
perspective? Say there is a flattened 3D layer with perspective in between the
fixed-position layer and the fixed container, what if we scrolled the fixed
container and the fixed-position layer is now out of the horizon line?

before scroll:
+-----------------------+
|                      ^|
|    +-----+           ||
|   /  fixed\          ||
|  /     O   \         ||
| /           \        ||
|+-------------+       ||
|                      v|
+-----------------------+

after scroll:
+-----------------------+
|                      ^|
|                      ||
|      fixed           ||
|        O             ||
|  vanish point        ||
|=======*==============||== horizon
|      / \             v|
+-----------------------+
     +-----+           
    /       \          
   /         \         
  /           \        
 +-------------+

[shawnsingh, 2013-03-07 19:31:49]

I think it's actually better to avoid making assumptions in our compositor based
on WebKit behavior and W3C spec - as long as we comply to those =)   I think the
rule you're talking about seems to have some w3c bugzilla issues filed against
it anyway, so who knows if it might still evolve a little bit...  And most
likely there are deviations from spec that are hard to find, just because it is
difficult for the transforms spec to clarify every single possible situation the
way they've set it up =)  and finally, if the layer has no CSS transform, that
doesn't mean it would not have additional transforms from some other source,
such as contents scale, or if there's a renderSurface between the fixed-pos
element and its container that re-parents the drawTransform in between.

As for your perspective example, I think it would actually still work where the
perspective flattened fixed pos element stays where it is originally.  Maybe we
could construct a quick html test case to try it out?

[enne (OOO), 2013-03-07 21:53:09]

What happened to the idea of a pre-pass where we reformulated all constraints as
a delta from the top left?

[jamesr, 2013-03-07 21:56:01]

https://codereview.chromium.org/12552004/diff/3005/cc/layer.h
File cc/layer.h (right):

https://codereview.chromium.org/12552004/diff/3005/cc/layer.h#newcode123
cc/layer.h:123: void setFixedToRightEdge(bool);
Do we need this today?  I think it's nice to have general code but I can't think
of a use-case for fixed to the right edge today.

[trchen, 2013-03-07 22:21:00]

On 2013/03/07 19:31:49, shawnsingh wrote:
> I think it's actually better to avoid making assumptions in our compositor
based
> on WebKit behavior and W3C spec - as long as we comply to those =)   I think
the
> rule you're talking about seems to have some w3c bugzilla issues filed against
> it anyway, so who knows if it might still evolve a little bit...  And most
> likely there are deviations from spec that are hard to find, just because it
is
> difficult for the transforms spec to clarify every single possible situation
the
> way they've set it up =)  and finally, if the layer has no CSS transform, that
> doesn't mean it would not have additional transforms from some other source,
> such as contents scale, or if there's a renderSurface between the fixed-pos
> element and its container that re-parents the drawTransform in between.
> 
> As for your perspective example, I think it would actually still work where
the
> perspective flattened fixed pos element stays where it is originally.  Maybe
we
> could construct a quick html test case to try it out?

I don't think it will work, since the flattened layer will have its own
RenderSurface, and no point on the RenderSurface will be projected to the point
the fixed-position element is supposed to be.

We can't construct a quick html test unfortunately, since the flattened layer
will become the fixed container of the element...

My opinion is that, even if we do want to support transformed fixed position
element, I don't think applying an undo matrix to all scroll deltas is an ideal
way to do it. We should never have applied scroll deltas in the first place.

It doesn't solve the RenderSurface with perspective issue either though, the
only way is probably to make the fixed-position element a separate
RenderSurface, and split the original RenderSurface into two subtrees...

[trchen, 2013-03-07 22:27:12]

On 2013/03/07 21:56:01, jamesr wrote:
> https://codereview.chromium.org/12552004/diff/3005/cc/layer.h
> File cc/layer.h (right):
> 
> https://codereview.chromium.org/12552004/diff/3005/cc/layer.h#newcode123
> cc/layer.h:123: void setFixedToRightEdge(bool);
> Do we need this today?  I think it's nice to have general code but I can't
think
> of a use-case for fixed to the right edge today.

The bug is targeting M27. The only use case today is pinch zooming. I doubt if
there is any practical use. (like supporting resize corner in the compositor?
separate page scale factor for subframes?)

[trchen, 2013-03-07 22:35:19]

On 2013/03/07 21:53:09, enne wrote:
> What happened to the idea of a pre-pass where we reformulated all constraints
as
> a delta from the top left?

I'd be happy to do it, but it will break the transformation-in-between issue
Shawn mentioned.

My current implementation is a trade off, so we still support transformed fixed
position but won't have proper bottom-right compensation. WebKit doesn't
generate such layers anyway.

I vote for Enne's proposal to add a layout phase to calculate total offset from
the top left of the fixed container to the fixed-position layer. However we will
have to give up transformed fixed-position layer.

[enne (OOO), 2013-03-07 22:40:36]

On 2013/03/07 22:35:19, trchen wrote:
> On 2013/03/07 21:53:09, enne wrote:
> > What happened to the idea of a pre-pass where we reformulated all
constraints
> as
> > a delta from the top left?
> 
> I'd be happy to do it, but it will break the transformation-in-between issue
> Shawn mentioned.

CSS transforms create a new fixed position container, so I'm not sure what the
problem is other than needing to implicitly enforce that in cc.

[shawnsingh, 2013-03-07 23:38:28]

In reply to trchen comment #9:

"never having applied the scroll deltas in the first place" would mean that we
have to rearrange how the entire layer tree is constructed before it is given to
us.

As for the surface case, what I would have expected is that the surface size
would change size to encompass the fixed-position element.  The "flattening"
that occurs would be after that surface size is computed.

I still feel that the compensation approach will be relatively simple change to
existing code, and we don't have to make any assumptions about how CSS / W3C /
users of our compositor will use it.

In reply to enne@ comment #7

I'm not totally aware of what the pre-pass conversation was, but it sounds to me
like simply computing deltas with respect to top-left won't change the fact that
we still have to apply the compensation in the correct space - which is what all
that scroll compensation code is for now.

[trchen, 2013-03-07 23:51:41]

On 2013/03/07 23:38:28, shawnsingh wrote:
> In reply to trchen comment #9:
> 
> "never having applied the scroll deltas in the first place" would mean that we
> have to rearrange how the entire layer tree is constructed before it is given
to
> us.

Not necessary. We can simply pass another draw matrix that doesn't include the
scroll delta. Again there will be problem with intermediate RenderSurface, but I
think it should be considered as a class or more general problem. 

> As for the surface case, what I would have expected is that the surface size
> would change size to encompass the fixed-position element.  The "flattening"
> that occurs would be after that surface size is computed.

The problem is no matter how large the surface size is, the fixed-position
element can't possibly be covered. The projected point approaches the vanish
point as the original point on the surface approaches (0, -inf).

> I still feel that the compensation approach will be relatively simple change
to
> existing code, and we don't have to make any assumptions about how CSS / W3C /
> users of our compositor will use it.
> 
> I'm not totally aware of what the pre-pass conversation was, but it sounds to
me
> like simply computing deltas with respect to top-left won't change the fact
that
> we still have to apply the compensation in the correct space - which is what
all
> that scroll compensation code is for now.

I think it is trade off between code simplicity and support for transformed
fixed layer. I suggest we have an offline meeting to decide what is the best for
the compositor.

[shawnsingh, 2013-03-08 00:13:02]

> The problem is no matter how large the surface size is, the fixed-position
> element can't possibly be covered. The projected point approaches the vanish
> point as the original point on the surface approaches (0, -inf).

The scroll compensation should account for this.  If it doesn't that's a bug =) 
but it's not missing from the intended design.

> I think it is trade off between code simplicity and support for transformed
> fixed layer. I suggest we have an offline meeting to decide what is the best
for
> the compositor.

Sounds good to me, as long as you don't mind waiting until next week? =(  I fell
pretty sick this week, will not be coming to office.

[trchen, 2013-03-13 00:24:37]

Had a offline discussion with shawnsingh@ and enne@ just now. Let me sum up the
conclusion here to make sure there is not misunderstanding:

1. We should change the Layer::isContainerForFixedPositionLayers() accessor so
that if a layer has non-translate-scale transform or its parent has
non-translate-scale sub-layer transform, it implicitly gets promoted to a fixed
container.

2. We don't do the impl-side layout phase in this patch, for the sake of
simplicity.

3. Compute the bottom-right compensation in existing scroll compensation
function, so we hide the complexity from calculateDrawPropertiesInternal.

4. The bottom-right compensation needs to be computed in the fixed container's
space, taking the scale transform on the way into consideration.
----
I'm not sure about my memory with 1. though. Please correct me if I'm wrong. I
have vague memory that we should promote a layer to fixed container if it has
any non-translate transform, but then that contradicts with 4., since we can
simply concat the size delta for bottom-right compensation if there is no
non-translate transform in between the fixed layer and the container.

[shawnsingh, 2013-03-13 00:32:25]

I don't think it's a contradiction, just an emergent property that happens to
simplify things in the cases we expect to see.  Personally I feel it's not
something we want to assume so that we can make tighter code, and really the
code may not necessarily be any tighter anyway if we try to take advantage of
such an assumption.  Ultimately I feel that fewer assumptions == safer code,
especially when those fewer assumptions don't cost anything.

[trchen, 2013-03-13 01:04:32]

On 2013/03/13 00:32:25, shawnsingh wrote:
> I don't think it's a contradiction, just an emergent property that happens to
> simplify things in the cases we expect to see.  Personally I feel it's not
> something we want to assume so that we can make tighter code, and really the
> code may not necessarily be any tighter anyway if we try to take advantage of
> such an assumption.  Ultimately I feel that fewer assumptions == safer code,
> especially when those fewer assumptions don't cost anything.

Okay, now I think it is not that difficult for the case when there is no
RenderSurface after the fixed container. The only things needed to compute
correct compensation are the size delta and the draw transform of the container
layer.

It would be tricky for case when there is one or more RenderSurface between the
fixed container and the fixed layer. We have do a few hops to convert the
compensation to the nearest RenderSurface. We can probably make use of
renderSurfaceLayerList to avoid traversing the ancestor chain. However I
wouldn't say the code will be tight and won't cost anything. :/

[shawnsingh, 2013-03-13 01:21:44]

I don't think that will be necessary.  But let's see what the next patch looks
like, maybe i'm missing something that you're seeing =)

[Sami, 2013-03-13 11:06:52]

On 2013/03/13 00:24:37, trchen wrote:
> 1. We should change the Layer::isContainerForFixedPositionLayers() accessor so
> that if a layer has non-translate-scale transform or its parent has
> non-translate-scale sub-layer transform, it implicitly gets promoted to a
fixed
> container.

Do you mean the compositor would figure out which layer is a container instead
doing it WebKit like today?  That sounds a little error-prone (e.g., because of
page scale), especially since I think CSS3 requires that any element with a
transform other than none becomes a container
(http://dev.w3.org/csswg/css3-transforms/#transform-rendering).

[trchen, 2013-03-13 21:51:13]

On 2013/03/13 11:06:52, Sami wrote:
> On 2013/03/13 00:24:37, trchen wrote:
> > 1. We should change the Layer::isContainerForFixedPositionLayers() accessor
so
> > that if a layer has non-translate-scale transform or its parent has
> > non-translate-scale sub-layer transform, it implicitly gets promoted to a
> fixed
> > container.
> 
> Do you mean the compositor would figure out which layer is a container instead
> doing it WebKit like today?  That sounds a little error-prone (e.g., because
of
> page scale), especially since I think CSS3 requires that any element with a
> transform other than none becomes a container
> (http://dev.w3.org/csswg/css3-transforms/#transform-rendering).

Nope, WebKit can still explicitly promote a layer to a fixed container. However
if we see non-trivial transform on a non-container layer, we promote it as well.

Personally I'd prefer a DCHECK instead of implicit promotion, but either are
acceptable to me.

BTW I forgot to brought up animation yesterday. Should we also promote a layer
to fixed container if it has an animation? Currently we don't compensate for
animation.

[enne (OOO), 2013-03-13 22:02:00]

On 2013/03/13 21:51:13, trchen wrote:

> Personally I'd prefer a DCHECK instead of implicit promotion, but either are
> acceptable to me.

I think I'd prefer the implicit behavior and some documentation in the Layer
header, since a DCHECK might go unnoticed.

> BTW I forgot to brought up animation yesterday. Should we also promote a layer
> to fixed container if it has an animation? Currently we don't compensate for
> animation.

Can you explain how animation enters into this?

[trchen, 2013-03-13 22:13:38]

On 2013/03/13 22:02:00, enne wrote:
> On 2013/03/13 21:51:13, trchen wrote:
> 
> > Personally I'd prefer a DCHECK instead of implicit promotion, but either are
> > acceptable to me.
> 
> I think I'd prefer the implicit behavior and some documentation in the Layer
> header, since a DCHECK might go unnoticed.
> 
> > BTW I forgot to brought up animation yesterday. Should we also promote a
layer
> > to fixed container if it has an animation? Currently we don't compensate for
> > animation.
> 
> Can you explain how animation enters into this?

Layer animation move a layer and its sub-layers. A fixed-position sub-layer is
not "fixed" anymore if it gets moved by some ancestor animation.

[jamesr, 2013-03-13 22:24:04]

On 2013/03/13 22:13:38, trchen wrote:
> On 2013/03/13 22:02:00, enne wrote:
> > On 2013/03/13 21:51:13, trchen wrote:
> > 
> > > Personally I'd prefer a DCHECK instead of implicit promotion, but either
are
> > > acceptable to me.
> > 
> > I think I'd prefer the implicit behavior and some documentation in the Layer
> > header, since a DCHECK might go unnoticed.
> > 
> > > BTW I forgot to brought up animation yesterday. Should we also promote a
> layer
> > > to fixed container if it has an animation? Currently we don't compensate
for
> > > animation.
> > 
> > Can you explain how animation enters into this?
> 
> Layer animation move a layer and its sub-layers. A fixed-position sub-layer is
> not "fixed" anymore if it gets moved by some ancestor animation.

We support animations on two properties: opacity and transform.  If the
transform is being animated, then it clearly has a transform set and so it's a
fixed position container.  If opacity is being animated, it's not moving.

[trchen, 2013-03-13 22:26:53]

On 2013/03/13 22:24:04, jamesr wrote:
> On 2013/03/13 22:13:38, trchen wrote:
> > On 2013/03/13 22:02:00, enne wrote:
> > > On 2013/03/13 21:51:13, trchen wrote:
> > > 
> > > > Personally I'd prefer a DCHECK instead of implicit promotion, but either
> are
> > > > acceptable to me.
> > > 
> > > I think I'd prefer the implicit behavior and some documentation in the
Layer
> > > header, since a DCHECK might go unnoticed.
> > > 
> > > > BTW I forgot to brought up animation yesterday. Should we also promote a
> > layer
> > > > to fixed container if it has an animation? Currently we don't compensate
> for
> > > > animation.
> > > 
> > > Can you explain how animation enters into this?
> > 
> > Layer animation move a layer and its sub-layers. A fixed-position sub-layer
is
> > not "fixed" anymore if it gets moved by some ancestor animation.
> 
> We support animations on two properties: opacity and transform.  If the
> transform is being animated, then it clearly has a transform set and so it's a
> fixed position container.  If opacity is being animated, it's not moving.

Yep, I'm talking about what if a layer has transform animation but isn't
explicitly promoted to a fixed container. Do we DCHECK or implicitly promote it
to a fixed container?

[trchen, 2013-03-16 04:30:21]

Changes from last patch:

* Layer::isContainerForFixedPositionLayers() implicitly promotes layers to fixed
container if the layer has non-translate transform or its parent has
non-translate sub-layer transform.

* Moved fixed-position code in LayerTreeHostCommon to a new function
applyPositionAdjustment()

* Now bottom-right compensation matrix is applied in the container layer's
coordinate.

[trchen, 2013-03-21 23:05:51]

Hey James, do you think the patch is in good shape or you still think we need to
add a layout phase? Let me know if I need to revise anything. Thanks!

[jamesr, 2013-03-22 01:49:50]

I think Enne or Shawn or someone else more familiar with LayerTreeHostCommon is
going to have to review that part of the code - I'm just not up on the transform
math enough to do a good review.

https://codereview.chromium.org/12552004/diff/44001/cc/layers/layer_position_...
File cc/layers/layer_position_constraint.h (right):

https://codereview.chromium.org/12552004/diff/44001/cc/layers/layer_position_...
cc/layers/layer_position_constraint.h:9: struct WebLayerPositionConstraint;
NAK on this.  Things in cc/ should not depend on things in WebKit. Add any
translation code you need between WebKit and cc types in
webkit/compositor_bindings

https://codereview.chromium.org/12552004/diff/44001/cc/layers/layer_position_...
cc/layers/layer_position_constraint.h:26: operator
WebKit::WebLayerPositionConstraint() const;
NAK

[trchen, 2013-03-22 03:49:32]

On 2013/03/22 01:49:50, jamesr wrote:
> I think Enne or Shawn or someone else more familiar with LayerTreeHostCommon
is
> going to have to review that part of the code - I'm just not up on the
transform
> math enough to do a good review.

Shawn do you think the math looks legit?
If there is no design issues I'll start to write unit tests now.

https://codereview.chromium.org/12552004/diff/44001/cc/layers/layer_position_...
> File cc/layers/layer_position_constraint.h (right):
> 
>
https://codereview.chromium.org/12552004/diff/44001/cc/layers/layer_position_...
> cc/layers/layer_position_constraint.h:9: struct WebLayerPositionConstraint;
> NAK on this.  Things in cc/ should not depend on things in WebKit. Add any
> translation code you need between WebKit and cc types in
> webkit/compositor_bindings
done
>
https://codereview.chromium.org/12552004/diff/44001/cc/layers/layer_position_...
> cc/layers/layer_position_constraint.h:26: operator
> WebKit::WebLayerPositionConstraint() const;
> NAK
done

[shawnsingh, 2013-03-22 04:42:11]

Sorry trchen@ this isn't the design that I was looking for.  But it's close, I
think it shouldn't take much more work to get there.

https://codereview.chromium.org/12552004/diff/48001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12552004/diff/48001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:369:
combinedTransform->Translate(positionOffset.x(), positionOffset.y());
This Translate is applied the same way as PreConcat, instead of Concat.  So this
isn't placing the translation in the place you might think it's being placed.

What you want is   Step3*Step2*Step1 * combinedTransform

but what seems to be computed here is Step3 * Step1 * combinedTransform * Step2.

But actually that's a moot point.  To repeat our conversation from last week - I
still feel you should not be repeating this math at all.  It's just an exact
duplicate of the scroll compensation math 50 lines below this, right?   Not only
will this be less efficient, but it will also be duplicated code that becomes
more error prone and maintenance bugs WILL occur with duplicated math code =) 
Is there some subtle difference in the math that requires a separate version? 
If so, let's discuss again offline.  Otherwise, let's please add the offset
below in the existing scroll compensation code (I'm putting another comment
where it should go).

To do that, you can add a few args to the scroll compensation code as needed,
and eventually (this patch ideally, or a follow-up patch) we should re-name
"scroll compensation" to fixedPositionCompensation or something more general.

Once you do that, this function would only exist to conditionally
Concat(compensationMatrix).   The actual computation of the scroll compensation
is invoked elsewhere in layer tree host common in the correct place, and you
would want to create another wrapper function there to add the offset
computation.

https://codereview.chromium.org/12552004/diff/48001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:401:
scrollCompensationForThisLayer.Translate(scrollingLayer->scroll_delta().x(),
scrollingLayer->scroll_delta().y()); // Step 2
This is where you would apply the offset Translation.  The offset can be passed
as an arg through the scroll compensation code.

https://codereview.chromium.org/12552004/diff/48001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:954: gfx::Transform
nextScrollCompensationMatrix = computeScrollCompensationMatrixForChildren(layer,
parentMatrix, currentScrollCompensationMatrix);;
computeScrollCompensationMatrixForChildren is the function you want to pass
whatever offset information is needed.   If you think it's appropriate, you can
create a wrapper function that (a) computes the offset for the appropriate
container and (b) calls computeScrollCompensationMatrixForChildren, and call
that here instead.

[trchen, 2013-03-22 08:11:20]

On 2013/03/22 04:42:11, shawnsingh wrote:
> Sorry trchen@ this isn't the design that I was looking for.  But it's close, I
> think it shouldn't take much more work to get there.
> 
>
https://codereview.chromium.org/12552004/diff/48001/cc/trees/layer_tree_host_...
> File cc/trees/layer_tree_host_common.cc (right):
> 
>
https://codereview.chromium.org/12552004/diff/48001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_common.cc:369:
> combinedTransform->Translate(positionOffset.x(), positionOffset.y());
> This Translate is applied the same way as PreConcat, instead of Concat.  So
this
> isn't placing the translation in the place you might think it's being placed.
> 
> What you want is   Step3*Step2*Step1 * combinedTransform
> 
> but what seems to be computed here is Step3 * Step1 * combinedTransform *
Step2.

Argh! Thank you for pointing that out!

> But actually that's a moot point.  To repeat our conversation from last week -
I
> still feel you should not be repeating this math at all.  It's just an exact
> duplicate of the scroll compensation math 50 lines below this, right?   Not
only
> will this be less efficient, but it will also be duplicated code that becomes
> more error prone and maintenance bugs WILL occur with duplicated math code =) 
> Is there some subtle difference in the math that requires a separate version? 
> If so, let's discuss again offline.  Otherwise, let's please add the offset
> below in the existing scroll compensation code (I'm putting another comment
> where it should go).

No there is no subtle difference in the math and I can indeed do that in the
same function.
The only issue is that then we need to pass 3 or 4 matrices for position
compensation instead of just 1.
(1 for reverting scroll delta, 1 for right anchor, 1 for bottom anchor,
or 4 matrices for fixed elements that fix to different corners.)
I don't mind to do that, but that's different from what we discussed last time.

> To do that, you can add a few args to the scroll compensation code as needed,
> and eventually (this patch ideally, or a follow-up patch) we should re-name
> "scroll compensation" to fixedPositionCompensation or something more general.

I prefer to make it an opaque structure say PositionCompensationParameters or
something,
so later we can extend that structure in case we need to add more values for
other needs. (for example, sticky elements might need to pass a bounding box
along the compensation matrices)

> Once you do that, this function would only exist to conditionally
> Concat(compensationMatrix).   The actual computation of the scroll
compensation
> is invoked elsewhere in layer tree host common in the correct place, and you
> would want to create another wrapper function there to add the offset
> computation.
> 
>
https://codereview.chromium.org/12552004/diff/48001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_common.cc:401:
> scrollCompensationForThisLayer.Translate(scrollingLayer->scroll_delta().x(),
> scrollingLayer->scroll_delta().y()); // Step 2
> This is where you would apply the offset Translation.  The offset can be
passed
> as an arg through the scroll compensation code.
> 
>
https://codereview.chromium.org/12552004/diff/48001/cc/trees/layer_tree_host_...
> cc/trees/layer_tree_host_common.cc:954: gfx::Transform
> nextScrollCompensationMatrix =
computeScrollCompensationMatrixForChildren(layer,
> parentMatrix, currentScrollCompensationMatrix);;
> computeScrollCompensationMatrixForChildren is the function you want to pass
> whatever offset information is needed.   If you think it's appropriate, you
can
> create a wrapper function that (a) computes the offset for the appropriate
> container and (b) calls computeScrollCompensationMatrixForChildren, and call
> that here instead.

[shawnsingh, 2013-03-22 10:47:24]

> No there is no subtle difference in the math and I can indeed do that in the
> same function.
> The only issue is that then we need to pass 3 or 4 matrices for position
> compensation instead of just 1.
> (1 for reverting scroll delta, 1 for right anchor, 1 for bottom anchor,
> or 4 matrices for fixed elements that fix to different corners.)
> I don't mind to do that, but that's different from what we discussed last
time.

Again, I believe the correct patch will not need any matrix math at all, except
for one translate() for the sizeDelta offset.  

We're going in circles here.  Let's just sit-down tomorrow and code this part
together; I think it will take only 20 minutes.

[shawnsingh, 2013-03-22 21:45:13]

trchen@ and I discussed offline, and he has totally convinced me that his
current approach is actually the most reasonable thing to do for now =)  I'm
ready to give his next patch a stamp for layer_tree_host_common math part. 
Great job for getting this complicated thing implemented quite nicely.

[trchen, 2013-03-22 22:18:40]

On 2013/03/22 21:45:13, shawnsingh wrote:
> trchen@ and I discussed offline, and he has totally convinced me that his
> current approach is actually the most reasonable thing to do for now =)  I'm
> ready to give his next patch a stamp for layer_tree_host_common math part. 
> Great job for getting this complicated thing implemented quite nicely.

Thanks man! Pair programming is really helpful when things involve recursion and
complicated math. :p

Just revised the code as discussed. Please let me know if there are other design
issues. Thank.

[shawnsingh, 2013-03-23 00:07:03]

math in layer_tree_host_common --> LGTM.   Once we add unit tests, I think this
will be pretty solid.

https://codereview.chromium.org/12552004/diff/33002/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12552004/diff/33002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:327: gfx::Transform
computeTranslateInContainerLayerSpace(LayerImpl* layer, LayerImpl* container,
const gfx::Vector2dF& positionOffset)
can we rename this to something like computeSizeDeltaCompensation, instead?  (1)
to self-document the similarity between scroll compensation and sizeDelta
compensation being similar.  (2) the existing name in my opinion is actually a
bit misleading, "in container layer space" doesn't actually indicate what the
spaces are at the input and output of applying the transform.

https://codereview.chromium.org/12552004/diff/33002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:338: gfx::Transform
targetSurfaceSpaceToContainerLayerSpace;
There's some inconsistency in transform names in layer_tree_host_common.  Some
other places in code use left-to-right naming at the moment (to be changed
later) ... so it's a good idea to put a comment somewhere that says these
transforms are named the way they are interpreted from right-to-left (with
matrix*columnVector ordering).

https://codereview.chromium.org/12552004/diff/33002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:344:
targetSurfaceSpaceToContainerLayerSpace.ConcatTransform(currentTargetSurface->render_surface()->draw_transform());
A comment about using Concat would be nice here, too, since Concat is so rarely
used.  Also for Concat used a few lines below, too.

[Sami, 2013-03-25 11:41:37]

I think this is looking good. Just a question below about the dynamic promotion
of layers to containers and some style nits.

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:417: bool Layer::is_container_for_fixed_position_layers()
const {
Any reason why this is on Layer instead LayerImpl? It seems like this bit should
be updated any time the transforms change and that can happen on the impl side
too.

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:420: if (parent_ &&
!parent_->sublayer_transform_.IsIdentityOrTranslation())
Should we also check sublayer_transform_ for this layer?

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer.h#newcode127
cc/layers/layer.h:127: bool is_container_for_fixed_position_layers() const;
This should be renamed to IsContainerForFixedPositionLayers() now that it's not
a simple getter anymore.

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer_impl.h#ne...
cc/layers/layer_impl.h:488: // This property is effective when
m_isContainerForFixedPositionLayers == true,
s/m_isContainerForFixedPositionLayers/is_container_for_fixed_position_layers_/.
Also, line length.

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer_position_...
File cc/layers/layer_position_constraint.h (right):

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer_position_...
cc/layers/layer_position_constraint.h:11: public:
Indent labels by 1.

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer_position_...
cc/layers/layer_position_constraint.h:14: void SetIsFixedPosition(bool fixed) {
is_fixed_position_ = fixed; }
These setters should be set_foo() style.

[trchen, 2013-03-26 01:45:28]

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:417: bool Layer::is_container_for_fixed_position_layers()
const {
On 2013/03/25 11:41:37, Sami wrote:
> Any reason why this is on Layer instead LayerImpl? It seems like this bit
should
> be updated any time the transforms change and that can happen on the impl side
> too.

Extra transform may be applied in LayerImpl but transform_ itself never changes
I believe. I do need to push the adjusted flag to LayerImpl though. (which I
forgot. thanks Sami!)

I do agree that promoting a layer with transform animation to fixed container
sounds like the right thing to do. Unfortunately I don't know a good way to
detect that.

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:420: if (parent_ &&
!parent_->sublayer_transform_.IsIdentityOrTranslation())
On 2013/03/25 11:41:37, Sami wrote:
> Should we also check sublayer_transform_ for this layer?

It is arguable but I would vote not. The point of the implicit promotion is that
scroll delta compensation can be easily applied after all intermediate
transforms. (i.e. scroll delta compensation is commutative with translation-only
transform)

             scroll delta sub-layer transform transform
PARENT LAYER <----------- <------------------ <-------- CURRENT LAYER

I ideally we want to apply scroll compensation (anti scroll delta) at the place
scroll delta is applied, it is actually what we do now (well not that simple, we
have to inverse all intermediate transforms and apply again). If we guarantee
all transform will be commutative, then we can simply apply a combined scroll
compensation at last.

Let's see the case current layer with sub-layer transform:

              scroll delta sub-layer transform transform
CURRENT LAYER <----------- <------------------ <-------- SUB LAYER(PROMOTED)

No scroll delta will be ever applied BEFORE the sub-layer transform, as the
sub-layer gets promoted to a fixed-container already, absorbing all scroll delta
after it. That means an anti scroll delta transform never needs commutate past
the sub-layer transform.

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer.h#newcode127
cc/layers/layer.h:127: bool is_container_for_fixed_position_layers() const;
On 2013/03/25 11:41:37, Sami wrote:
> This should be renamed to IsContainerForFixedPositionLayers() now that it's
not
> a simple getter anymore.

Done.

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer_impl.h#ne...
cc/layers/layer_impl.h:488: // This property is effective when
m_isContainerForFixedPositionLayers == true,
On 2013/03/25 11:41:37, Sami wrote:
>
s/m_isContainerForFixedPositionLayers/is_container_for_fixed_position_layers_/.
> Also, line length.

Done.

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer_position_...
File cc/layers/layer_position_constraint.h (right):

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer_position_...
cc/layers/layer_position_constraint.h:11: public:
On 2013/03/25 11:41:37, Sami wrote:
> Indent labels by 1.

Done.

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer_position_...
cc/layers/layer_position_constraint.h:14: void SetIsFixedPosition(bool fixed) {
is_fixed_position_ = fixed; }
On 2013/03/25 11:41:37, Sami wrote:
> These setters should be set_foo() style.

Done.

https://codereview.chromium.org/12552004/diff/33002/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/12552004/diff/33002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:327: gfx::Transform
computeTranslateInContainerLayerSpace(LayerImpl* layer, LayerImpl* container,
const gfx::Vector2dF& positionOffset)
On 2013/03/23 00:07:03, shawnsingh wrote:
> can we rename this to something like computeSizeDeltaCompensation, instead? 
(1)
> to self-document the similarity between scroll compensation and sizeDelta
> compensation being similar.  (2) the existing name in my opinion is actually a
> bit misleading, "in container layer space" doesn't actually indicate what the
> spaces are at the input and output of applying the transform.

Done.

https://codereview.chromium.org/12552004/diff/33002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:338: gfx::Transform
targetSurfaceSpaceToContainerLayerSpace;
On 2013/03/23 00:07:03, shawnsingh wrote:
> There's some inconsistency in transform names in layer_tree_host_common.  Some
> other places in code use left-to-right naming at the moment (to be changed
> later) ... so it's a good idea to put a comment somewhere that says these
> transforms are named the way they are interpreted from right-to-left (with
> matrix*columnVector ordering).

I don't understand your suggestion here. No matter column vector or row vector
is used, the transformation converts a vector in target surface space to the
container layer space. The way I see it is that Transform as an opaque object,
which contains a pipeline of operations that will be applied on a vector.
Preconcat means we insert an operation at the front and concat means we append
an operation at the end of the pipeline.

I don't mind to change it to "containerLayerSpaceFromTargetSurfaceSpace". I
think it's a better practice (Hungarian notation) actually. Just want to confirm
your intention first.

https://codereview.chromium.org/12552004/diff/33002/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_common.cc:344:
targetSurfaceSpaceToContainerLayerSpace.ConcatTransform(currentTargetSurface->render_surface()->draw_transform());
On 2013/03/23 00:07:03, shawnsingh wrote:
> A comment about using Concat would be nice here, too, since Concat is so
rarely
> used.  Also for Concat used a few lines below, too.

Done.

[Sami, 2013-03-26 11:05:23]

Thanks for the clarifications Tien Ren. lgtm once it comes with tests.

[shawnsingh, 2013-03-27 19:46:39]

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:420: if (parent_ &&
!parent_->sublayer_transform_.IsIdentityOrTranslation())

In reply to Sami asking "Should we also check sublayer_transform_ for this
layer" --> I don't think we should be checking both layer and parent for a
non-identity sublayer transform.  Otherwise we'll either potentially get two
consecutive containers where clearly only one of them should have been the
container.  So I think the way trchen has it now is very reasonable =)

However, trchen@ I didn't totally understand your reply.  Also I think the
diagrams you've made are actually wrong.

First, we should rarely ever bring up the concept of matrix commutativity. 
Because matrices are generally not commutative, making decisions based on
assumptions about commutativity will very likely result in bad sequences of
transforms that are very hard to debug.

Second, sublayer transform doesn't fit in the diagram where you've drawn it. 
the scroll delta of a layer is applied to the right hand side (in matrix*vector
math ordering) of the parent's hierarchy matrix passed down the recursion.   the
parent's sublayer transform is inside that hierarchy matrix.   The current
layer's sublayer transform is appended to the right hand side after the layer's
current transform.

Just wanted to set some concepts straight =)  But it doesn't change anything
about the code as it's written here.

[trchen, 2013-03-27 22:22:12]

Unit tests added.

On 2013/03/27 19:46:39, shawnsingh wrote:
> https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer.cc
> File cc/layers/layer.cc (right):
> 
>
https://codereview.chromium.org/12552004/diff/33002/cc/layers/layer.cc#newcod...
> cc/layers/layer.cc:420: if (parent_ &&
> !parent_->sublayer_transform_.IsIdentityOrTranslation())
> 
> In reply to Sami asking "Should we also check sublayer_transform_ for this
> layer" --> I don't think we should be checking both layer and parent for a
> non-identity sublayer transform.  Otherwise we'll either potentially get two
> consecutive containers where clearly only one of them should have been the
> container.  So I think the way trchen has it now is very reasonable =)
> 
> However, trchen@ I didn't totally understand your reply.  Also I think the
> diagrams you've made are actually wrong.
> 
> First, we should rarely ever bring up the concept of matrix commutativity. 
> Because matrices are generally not commutative, making decisions based on
> assumptions about commutativity will very likely result in bad sequences of
> transforms that are very hard to debug.

Agree. Just to clarify, we don't rely on communtativity assumption in LayerImpl
or any calculation in LayerTreeHostCommon. In LayerTreeHostCommon we try to
support any kind of transformation in the layer tree. That's another reason why
the implicit fixed-container promotion is done in the Layer side.

> Second, sublayer transform doesn't fit in the diagram where you've drawn it. 
> the scroll delta of a layer is applied to the right hand side (in
matrix*vector
> math ordering) of the parent's hierarchy matrix passed down the recursion.  
the
> parent's sublayer transform is inside that hierarchy matrix.   The current
> layer's sublayer transform is appended to the right hand side after the
layer's
> current transform.

Ah, I didn't draw the diagram to be consistent with the written-order of matrix
multiplication. Anyway I won't try to confuse things further by drawing another
transposed diagram. Your explanation is clearer. ;) 

> Just wanted to set some concepts straight =)  But it doesn't change anything
> about the code as it's written here.

[shawnsingh, 2013-04-01 18:12:36]

I would like to participate reviewing the unit tests, but I will probably be a
bit slow looking at it very carefully.  Please give me a day or two =) thanks.

[trchen, 2013-04-01 22:13:17]

On 2013/04/01 18:12:36, shawnsingh wrote:
> I would like to participate reviewing the unit tests, but I will probably be a
> bit slow looking at it very carefully.  Please give me a day or two =) thanks.

Sure, take your time!

FYI, besides moving the existing tests to a separate file, most existing code
are identical to original. Specifically, these are the only changes I made:
1. Make a test fixture and deduplicate common initialization code.
2. For all the tests, add case 3 that changes the fixed-position layer to
bottom-right anchored, and case 4 that enlarges the fixed-container so now the
fixed-position layer should move.

[shawnsingh, 2013-04-04 10:15:14]

Summary:  (1) one issue that we need to resolve where the expected sequence of
matrices is backwards and I don't udnerstand why it works, and (2) most of the
case 4 scenarios for the more complicated tests need to be more explicit about
the expected sequence of transforms.

Thanks!

https://codereview.chromium.org/12552004/diff/93012/cc/layers/layer_position_...
File cc/layers/layer_position_constraint_unittest.cc (right):

https://codereview.chromium.org/12552004/diff/93012/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:18: void
SetLayerPropertiesForTestingInternal(
Since this is only used by LayerImpl side, let's get rid of this wrapper and
combine it with SetLayerPropertiesForTesting below.

Also, we should make a note to follow-up: the occlusion tracker has a similar
function, and we should consider moving all this to test/layer_test_common in a
separate clean-up patch.

https://codereview.chromium.org/12552004/diff/93012/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:76: void
ExecuteCalculateDrawProperties(LayerType* root_layer) {
And this probably doesn't need to be templated here.

https://codereview.chromium.org/12552004/diff/93012/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:216: // gfx::Transforms are in
general non-commutative; using something like a
gfx::Transforms --> transforms.  I think this was a search-replace typo of mine
from a few months ago, but it's a good opportunity to fix it =)

https://codereview.chromium.org/12552004/diff/93012/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:275:
expected_grand_child_transform.Translate(20.0, 20.0);
Something isn't right here, but I can't explain why this test would be passing
even though it's wrong.

The way we're defining our expected transform:

scrollDeltaCompensation * nonUniformScale * sizeDeltaCompensation

but the correct expected transform should be:

scrollDeltaCompensation * sizeDeltaCompensation * nonUniformScale.

the whole point of choosing nonUniformScale was to make sure it is not
commutative, and yet somehow this test is passing with the backwards expected
transform sequence =)

Do you see what I mean?  Unfortunately, we need to investigate this before
landing, and get a solid explanation and fix.

By the way, this issue is actually true of all "case 4" tests here.  It just so
happens that for the simpler tests, the additional matrix does not exist in the
sequence (the nonUniformScale in this case), so it happens to be correct.

https://codereview.chromium.org/12552004/diff/93012/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:447:
expected_great_grand_child_transform.Translate(20.0, -20.0);

this comment also applies to all tests, but this is the first test where the
issue is obvious.

I wondered why it's a translation of +20, -20, the reason is hidden the way the
test is written - there are more transforms that need to be shown explicitly
here.

If we don't do that, we run the high risk of "putting a test value just because
that's what this patch outputs", and not really knowing to ourselves if it's
correct.  spelling out the transforms here will force us to make sure we
understand the scenario and expected result, in addition to testing the code.

https://codereview.chromium.org/12552004/diff/93012/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:549: // Case 4: Bottom-right
fixed-position layer.
I feel like the case 3 and case 4 here are actually not very meaningful.  This
test case is about testing multiple scroll deltas in-between a fixed-position
element and its container.   By the definition of size deltas we won't ever have
that issue.  So instead of these additional no-op cases, perhaps we should have
a brief comment that mentions, "testing size-delta here is not meaningful since
we cannot have multiple size-deltas in the hierarchy."

https://codereview.chromium.org/12552004/diff/93012/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:665:
expected_great_grand_child_transform.Translate(20.0, -20.0);
same issue here, we need to see the spelled-out explicit seuqence of transforms
that would have given us this expected result.

https://codereview.chromium.org/12552004/diff/93012/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:841:
expected_fixed_position_child_transform.Translate(-20.0, -20.0);
and again here, same issue as above =)

[trchen, 2013-04-04 11:18:04]

https://codereview.chromium.org/12552004/diff/93012/cc/layers/layer_position_...
File cc/layers/layer_position_constraint_unittest.cc (right):

https://codereview.chromium.org/12552004/diff/93012/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:18: void
SetLayerPropertiesForTestingInternal(
On 2013/04/04 10:15:14, shawnsingh wrote:
> Since this is only used by LayerImpl side, let's get rid of this wrapper and
> combine it with SetLayerPropertiesForTesting below.

Done.

> Also, we should make a note to follow-up: the occlusion tracker has a similar
> function, and we should consider moving all this to test/layer_test_common in
a
> separate clean-up patch.

Agree.

https://codereview.chromium.org/12552004/diff/93012/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:76: void
ExecuteCalculateDrawProperties(LayerType* root_layer) {
On 2013/04/04 10:15:14, shawnsingh wrote:
> And this probably doesn't need to be templated here.

Done.

https://codereview.chromium.org/12552004/diff/93012/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:216: // gfx::Transforms are in
general non-commutative; using something like a
On 2013/04/04 10:15:14, shawnsingh wrote:
> gfx::Transforms --> transforms.  I think this was a search-replace typo of
mine
> from a few months ago, but it's a good opportunity to fix it =)

Done.

https://codereview.chromium.org/12552004/diff/93012/cc/layers/layer_position_...
cc/layers/layer_position_constraint_unittest.cc:275:
expected_grand_child_transform.Translate(20.0, 20.0);
On 2013/04/04 10:15:14, shawnsingh wrote:
> Something isn't right here, but I can't explain why this test would be passing
> even though it's wrong.
> 
> The way we're defining our expected transform:
> 
> scrollDeltaCompensation * nonUniformScale * sizeDeltaCompensation
> 
> but the correct expected transform should be:
> 
> scrollDeltaCompensation * sizeDeltaCompensation * nonUniformScale.

There is no sizeDeltaCompensation in the tests at all.

I never meant to verify the size delta compensation matrix in the first place.
Doing that falls exactly into the problem of "putting a test value just because
that's what this patch outputs". The test shouldn't know too much about internal
computation in LayerTreeHostCommon.

Instead, I did all the values by hand, by drawing the layers on a paper and
figure out the bottom-right offset in the local space. By comparing the output
from a different method, actually I get more confidence that the patch is doing
the right thing here.

Anyway I agree that plugging mysterious values everywhere makes the tests
difficult to read and maintain if we ever change the tests later. I'll try to
make it looks less of a puzzle.

> the whole point of choosing nonUniformScale was to make sure it is not
> commutative, and yet somehow this test is passing with the backwards expected
> transform sequence =)
> 
> Do you see what I mean?  Unfortunately, we need to investigate this before
> landing, and get a solid explanation and fix.

This works correctly because .Translate is a preconcat. No matter what transform
is applied on the container, in local space the fixed-position layer moves exact
sizeDelta (given that no transformation in between). If you want it to be a
concat, then the translation offset should become (40, 160).

> By the way, this issue is actually true of all "case 4" tests here.  It just
so
> happens that for the simpler tests, the additional matrix does not exist in
the
> sequence (the nonUniformScale in this case), so it happens to be correct.

[shawnsingh, 2013-04-04 18:39:25]

> There is no sizeDeltaCompensation in the tests at all.
> 
> I never meant to verify the size delta compensation matrix in the first place.
> Doing that falls exactly into the problem of "putting a test value just
because
> that's what this patch outputs". The test shouldn't know too much about
internal
> computation in LayerTreeHostCommon.
> 
> Instead, I did all the values by hand, by drawing the layers on a paper and
> figure out the bottom-right offset in the local space. By comparing the output
> from a different method, actually I get more confidence that the patch is
doing
> the right thing here.
> 

I disagree with this; I've actually been bitten by this exact approach.  I made
mistakes computing things by hand which actually reflected similar mistakes I
made in the code.  By forcing myself to list out the explicit sequence of
transforms in the tests, I found it helped my understanding incredibly, without
having to rely on error prone manual work.  There are still more than enough
differences between the code and the conceptual sequence of transforms that I
don't feel the explicit sequence is just repeating the implementation in the
code.

> Anyway I agree that plugging mysterious values everywhere makes the tests
> difficult to read and maintain if we ever change the tests later. I'll try to
> make it looks less of a puzzle.
> 

great thanks =)

> This works correctly because .Translate is a preconcat. No matter what
transform
> is applied on the container, in local space the fixed-position layer moves
exact
> sizeDelta (given that no transformation in between). If you want it to be a
> concat, then the translation offset should become (40, 160).
> 

First, we cannot just blindly change a transform between preconcat and concat. 
That places the transform in a different place in the sequence, which means it
is applying the transform in a different space.

Second --> "given that no transformation in between" --> but there is actually a
transform in between, it's the nonUniformScale in this test.

So yeah, let's discuss offline to get this resolved faster, and then come back
here =)

[shawnsingh, 2013-04-08 20:04:45]

new case 3 and case 4 unit tests on the position contraints LGTM =)  I think the
math on this patch is solid and good-to-go.

[commit-bot: I haz the power, 2013-04-08 20:25:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/12552004/111002

[commit-bot: I haz the power, 2013-04-08 20:25:57]

Presubmit check for 12552004-111002 failed and returned exit status 1.

INFO:root:Found 15 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/cc/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    webkit/compositor_bindings/web_layer_impl.cc
    webkit/compositor_bindings/web_layer_impl.h

Presubmit checks took 2.6s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[jamesr, 2013-04-08 20:40:02]

lgtm for webkit/

[commit-bot: I haz the power, 2013-04-08 20:58:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/trchen@chromium.org/12552004/111002

[commit-bot: I haz the power, 2013-04-09 04:26:38]

Change committed as 192999