Generalize scroll parent work in CalculateDrawProperties

Generalize scroll parent work in CalculateDrawProperties

Previously, CDP would fail if the scroll child wasn't the immediate
descendant of the lowest common ancestor of the scroll parent and child.

With this patch I've fixed up two things so that CDP can handle these
cases.

1. Dumber sorting.
  I've made sorting simpler by removing assumptions about the tree
  topology. Instead, I assigned "sort weights" to the layers in
  PreCalculateMetaInformation. The approach is pretty naive. As I
  walk the tree I assign a monotonically increasing weight. If I ever
  hit a scroll child before its parent, I jump up to the lowest common
  ancestor and make sure that the child of the LCA is assigned the
  smallest weight in the tree so far (the negative of the next
  monotonically increasing weight). This ensures that the scroll parent
  will be visited first. Here's a picture of a layer tree and the
  weights that would be assigned.

  lca(0)
  + filler(1)
  | + scroll child(2)
  + more-filler(-3)
    + scroll parent(4)

  It would be a huge bummer if we had to sort all the time, though, so
  I've taken care to flag the LCA if I ever have to assign a negative
  weight so that we only sort in that case.

  Here's my argument for the correctness of the algorithm. Clearly,
  we force correct ordering of a particular scroll child/parent pair,
  but the worry is that by setting a negative weight, we'll force an
  illegal ordering (i.e., we'll "undo" an ordering we've forced
  earlier in the algorithm). Here's a diagram.

  lca
  + subtree with scroll parent #1
  + subtree with scroll child #1 and scroll_parent #2
  + subtree with scroll child #2.

  Now, say we're processing scroll child #2. We want to be sure that
  we don't go reordering subtrees and mess up the relative ordering of
  scroll child #1 and scroll parent #1's subtrees.

  But we know that this is impossible. Since we've already visited the
  subtree with scroll child #1, scroll child #2 will necessarily get a
  higher sort weight, so we'll never need to force any reordering in
  this case; we won't assign any negative weights here.

  Say the ordering were changed such that when processing scroll child
  #2, we haven't seen scroll child #1's subtree. We're also fine in
  this case because we will do any ordering fixups required when we do
  eventually visit scroll child #1.

  The only case where we get into trouble is when we have a cycle. For
  example,

  lca
   + scroll parent 1
   | + scroll child 2
   + scroll parent 2
     + scroll child 1

  No ordering could satisfy a situation with a cycle, but this is
  an impossible situation as CSS stacking defines a well defined, non
  circular order.

2. Proper clip transformations.
  We sometimes need to put the scroll parent's clip into the space of
  the scroll child's immediate parent. This needs to go through the
  LCA. Previously, we'd assumed that the LCA was going to be the scroll
  child's immediate parent, but we no longer make that assumption. We
  made a similar assumption for clip children that we also had to
  generalize

BUG=403866, 424064
> Was previously landed but reverted. NB - it was reverted because it
> exposed an unrelated bug, the bug was not with the patch itself.
> Committed: https://crrev.com/7faedca87705b714845842e595bca262c4090fa4
> Cr-Commit-Position: refs/heads/master@{#294521}

[Ian Vollick, 2014-09-08 04:08:28]

vollick@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2014-09-09 19:02:31]

lca
  + subtree with scroll child #2.
  + subtree with scroll child #1 and scroll_parent #2
  + subtree with scroll parent #1

In this case the subtree of scrollchild #2 is given a negative weight to be
sorted last. Then the subtree of child #1 is given a negative weight to be
sorted laster. Then the parent of #2 will come after the child #2?

[Ian Vollick, 2014-09-09 19:17:31]

On 2014/09/09 19:02:31, danakj wrote:
> lca
>   + subtree with scroll child #2.
>   + subtree with scroll child #1 and scroll_parent #2
>   + subtree with scroll parent #1
> 
> In this case the subtree of scrollchild #2 is given a negative weight to be
> sorted last. Then the subtree of child #1 is given a negative weight to be
> sorted laster. Then the parent of #2 will come after the child #2?

Thanks for taking a look! In the case you mentioned, the algorithm should
proceed as follows (simplified a little bit because we've referred to subtrees
and don't have exact layers in them. Should still illustrate how the algorithm
works, though).

Step 1. Visit lca.

 [0] lca
   + subtree with scroll child #2.
   + subtree with scroll child #1 and scroll_parent #2
   + subtree with scroll parent #1

Step 2. Visit "subtree with scroll child #2". We're forced to set a negative
weight here so that sc #2's parent is processed first.

 [0] lca
   + [1] subtree with scroll child #2.
   + [-2] subtree with scroll child #1 and scroll_parent #2
   + subtree with scroll parent #1

Step 3. Visit "subtree with scroll child #1". We're again forced to set a
negative weight so that sc #1's scroll parent is visited first.

 [0] lca
   + [1] subtree with scroll child #2.
   + [-2] subtree with scroll child #1 and scroll_parent #2
   + [-3] subtree with scroll parent #1

The net result is that lca's children will be processed in the following order:
subtree with scroll parent #1,
subtree with scroll child #1 and scroll_parent #2,
subtree with scroll child #2,

Which is what we want.

[danakj, 2014-09-09 19:21:20]

On 2014/09/09 19:17:31, vollick wrote:
> On 2014/09/09 19:02:31, danakj wrote:
> > lca
> >   + subtree with scroll child #2.
> >   + subtree with scroll child #1 and scroll_parent #2
> >   + subtree with scroll parent #1
> > 
> > In this case the subtree of scrollchild #2 is given a negative weight to be
> > sorted last. Then the subtree of child #1 is given a negative weight to be
> > sorted laster. Then the parent of #2 will come after the child #2?
> 
> Thanks for taking a look! In the case you mentioned, the algorithm should
> proceed as follows (simplified a little bit because we've referred to subtrees
> and don't have exact layers in them. Should still illustrate how the algorithm
> works, though).
> 
> Step 1. Visit lca.
> 
>  [0] lca
>    + subtree with scroll child #2.
>    + subtree with scroll child #1 and scroll_parent #2
>    + subtree with scroll parent #1
> 
> Step 2. Visit "subtree with scroll child #2". We're forced to set a negative
> weight here so that sc #2's parent is processed first.
> 
>  [0] lca
>    + [1] subtree with scroll child #2.
>    + [-2] subtree with scroll child #1 and scroll_parent #2
>    + subtree with scroll parent #1
> 
> Step 3. Visit "subtree with scroll child #1". We're again forced to set a
> negative weight so that sc #1's scroll parent is visited first.
> 
>  [0] lca
>    + [1] subtree with scroll child #2.
>    + [-2] subtree with scroll child #1 and scroll_parent #2
>    + [-3] subtree with scroll parent #1
> 
> The net result is that lca's children will be processed in the following
order:
> subtree with scroll parent #1,
> subtree with scroll child #1 and scroll_parent #2,
> subtree with scroll child #2,
> 
> Which is what we want.

Is it because we visit children in lowest sort weight to heighest (ie back to
front if they were sorted?)

[Ian Vollick, 2014-09-09 19:37:16]

On 2014/09/09 19:21:20, danakj wrote:
> On 2014/09/09 19:17:31, vollick wrote:
> > On 2014/09/09 19:02:31, danakj wrote:
> > > lca
> > >   + subtree with scroll child #2.
> > >   + subtree with scroll child #1 and scroll_parent #2
> > >   + subtree with scroll parent #1
> > > 
> > > In this case the subtree of scrollchild #2 is given a negative weight to
be
> > > sorted last. Then the subtree of child #1 is given a negative weight to be
> > > sorted laster. Then the parent of #2 will come after the child #2?
> > 
> > Thanks for taking a look! In the case you mentioned, the algorithm should
> > proceed as follows (simplified a little bit because we've referred to
subtrees
> > and don't have exact layers in them. Should still illustrate how the
algorithm
> > works, though).
> > 
> > Step 1. Visit lca.
> > 
> >  [0] lca
> >    + subtree with scroll child #2.
> >    + subtree with scroll child #1 and scroll_parent #2
> >    + subtree with scroll parent #1
> > 
> > Step 2. Visit "subtree with scroll child #2". We're forced to set a negative
> > weight here so that sc #2's parent is processed first.
> > 
> >  [0] lca
> >    + [1] subtree with scroll child #2.
> >    + [-2] subtree with scroll child #1 and scroll_parent #2
> >    + subtree with scroll parent #1
> > 
> > Step 3. Visit "subtree with scroll child #1". We're again forced to set a
> > negative weight so that sc #1's scroll parent is visited first.
> > 
> >  [0] lca
> >    + [1] subtree with scroll child #2.
> >    + [-2] subtree with scroll child #1 and scroll_parent #2
> >    + [-3] subtree with scroll parent #1
> > 
> > The net result is that lca's children will be processed in the following
> order:
> > subtree with scroll parent #1,
> > subtree with scroll child #1 and scroll_parent #2,
> > subtree with scroll child #2,
> > 
> > Which is what we want.
> 
> Is it because we visit children in lowest sort weight to heighest (ie back to
> front if they were sorted?)

I may have drawn my trees incorrectly, but in the example above I'd intended to
show us visiting the nodes in paint order during the algorithm, but producing an
ordering (due to the negative weights) that forces the reverse order when we
iterate during CDP.

We need to visit scroll parents before scroll children during CDP because the
children need to use their clip information. Of course, this can result in the
contributions to the RSLL being out of order, so we fix that up after recurring
where necessary.

Please let me know if that doesn't address your question. I think I may not have
:)

[danakj, 2014-09-10 18:20:47]

https://codereview.chromium.org/548963002/diff/40001/cc/layers/draw_properties.h
File cc/layers/draw_properties.h (right):

https://codereview.chromium.org/548963002/diff/40001/cc/layers/draw_propertie...
cc/layers/draw_properties.h:152: // Determines the order the layer should be
processed in CDP wrt to its
s/CDP/CalculateDrawProperties/

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:218: // much check if we need to transform
from the lca's space to our parent's.
must check

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1231: // This case is easy. If we haven't
visited the scroll parent, we just have
Is it worth mentioning this is true because we visit layers in a DFS order?

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1233: while (scroll_parent->parent() &&
why the check for ->parent()? You're worried about hitting the root? This could
only be a problem if you called this for scroll_parent = root while not having
visited the root yet, right? Which can't happen? Can you DCHECK
scroll_parent->parent() once instead?

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1239: CHECK_NE(scroll_child,
scroll_parent->parent());
can these be DCHECKs please

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1249: int delta =
scroll_child->draw_properties().depth -
nit: i think it'd read simpler with "bool child_deeper =
scroll_child->draw_properties().depth > scroll_parent->draw_properties().depth;"

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1258: // same ancestor chain, which is an
error.
why? because then we should have been in the other case, right? Can you say why?

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1294: depth(other.depth + 1),
This is a weird copy constructor. Can you DISALLOW_COPY_AND_ASSIGN and turn this
into a static method instead?

actually, these 3 new values don't seem to really fit in this class. Can they
just be recursive arguments to PreCalcMetaInfo? And pass them in to
AssignSortWeights directly? wdyt? That seems like it would remove a bit of
indirection and make things a bit more clear.

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1497: struct ScrollParentComparator {
Can you mention this produces a descending sort order in this name?

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:2514: static size_t sequence_number = 0;
can you use the render_surface_layer_list_id instead of a static?

one thing tho
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:2547: int initial_sort_weight = 0;
nit: s/initial/last/ <- point out that this storage is used for more than an
initial value

[danakj, 2014-09-10 18:21:05]

> Generalize scroll parent work in CDP

Please no acronyms in the patch title/description without defining them first

[Ian Vollick, 2014-09-11 14:43:13]

Patchset #4 (id:60001) has been deleted

[Ian Vollick, 2014-09-11 14:57:31]

Patchset #4 (id:80001) has been deleted

[Ian Vollick, 2014-09-11 15:10:12]

Patchset #4 (id:100001) has been deleted

[Ian Vollick, 2014-09-11 15:21:11]

Thanks for the review!

https://codereview.chromium.org/548963002/diff/40001/cc/layers/draw_properties.h
File cc/layers/draw_properties.h (right):

https://codereview.chromium.org/548963002/diff/40001/cc/layers/draw_propertie...
cc/layers/draw_properties.h:152: // Determines the order the layer should be
processed in CDP wrt to its
On 2014/09/10 18:20:46, danakj wrote:
> s/CDP/CalculateDrawProperties/

Done.

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:218: // much check if we need to transform
from the lca's space to our parent's.
On 2014/09/10 18:20:46, danakj wrote:
> must check

Done.

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1231: // This case is easy. If we haven't
visited the scroll parent, we just have
On 2014/09/10 18:20:46, danakj wrote:
> Is it worth mentioning this is true because we visit layers in a DFS order?

Done.

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1233: while (scroll_parent->parent() &&
On 2014/09/10 18:20:46, danakj wrote:
> why the check for ->parent()? You're worried about hitting the root? This
could
> only be a problem if you called this for scroll_parent = root while not having
> visited the root yet, right? Which can't happen? Can you DCHECK
> scroll_parent->parent() once instead?

Done.

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1239: CHECK_NE(scroll_child,
scroll_parent->parent());
On 2014/09/10 18:20:46, danakj wrote:
> can these be DCHECKs please

Done.

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1249: int delta =
scroll_child->draw_properties().depth -
On 2014/09/10 18:20:46, danakj wrote:
> nit: i think it'd read simpler with "bool child_deeper =
> scroll_child->draw_properties().depth >
scroll_parent->draw_properties().depth;"

Done.

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1258: // same ancestor chain, which is an
error.
On 2014/09/10 18:20:46, danakj wrote:
> why? because then we should have been in the other case, right? Can you say
why?

Done.

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1294: depth(other.depth + 1),
On 2014/09/10 18:20:46, danakj wrote:
> This is a weird copy constructor. Can you DISALLOW_COPY_AND_ASSIGN and turn
this
> into a static method instead?
> 
> actually, these 3 new values don't seem to really fit in this class. Can they
> just be recursive arguments to PreCalcMetaInfo? And pass them in to
> AssignSortWeights directly? wdyt? That seems like it would remove a bit of
> indirection and make things a bit more clear.
I took your suggestion to plumb the new fields I added here as arguments to
PreCalculateKitchenSink, so this is no longer applicable.

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1497: struct ScrollParentComparator {
On 2014/09/10 18:20:46, danakj wrote:
> Can you mention this produces a descending sort order in this name?

Done.

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:2514: static size_t sequence_number = 0;
On 2014/09/10 18:20:46, danakj wrote:
> can you use the render_surface_layer_list_id instead of a static?
> 
> one thing tho
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

As you pointed out, I needed to ensure that we had an increasing id on the main
thread, but I've also switched the storage to a size_t so for all practical
purposes, it won't roll over.

https://codereview.chromium.org/548963002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:2547: int initial_sort_weight = 0;
On 2014/09/10 18:20:46, danakj wrote:
> nit: s/initial/last/ <- point out that this storage is used for more than an
> initial value

Done.

[danakj, 2014-09-11 19:22:05]

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1246: // First get the layers to the same
depth.
nit: I think this comment was clearer before. Now it sounds like you're mutating
layers.

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1259: // child relationships in blink
precisely because the scroll child is not a
oh thanks for this comment, that's helpful. but can you do this without
mentioning blink? like "scroll parents and children are only meant to be used
when..." or something

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1938: layer,
combined_transform.Preserves2dAxisAlignment());
these unrelated format changes may make future merging more difficuly

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2518:
inputs->current_render_surface_layer_list_id + 1,
why +1?

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2554:
inputs->current_render_surface_layer_list_id + 1,
why +1?

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:382: size_t render_surface_layer_list_id_;
This kinda violates my reading of the chromium style guide. It's a sneaky way of
trying to use unsigned, when this is not in fact the size of a container or
something, where size_t is explicitly allowed.
<quote>
Use an unsigned integer type if your variable represents a bit pattern (e.g. a
hash value or a bitmask).

Use unsigned integer types (preferably size_t) for object and allocation sizes,
object counts, array and pointer offsets, vector indices, and so on.

Do not use unsigned types to mean "this value should never be < 0". For that,
use assertions or run-time checks (as appropriate).
</quote>

It doesn't discuss "if your value can wrap around" but that loosely falls close
to "it can't be < 0", and we should handle wrap around by checking for max() if
it's a problem in practice. Given this hasn't been a problem on the impl side
which generates >= # of frames as the main thread side, I think you should just
leave this as integer and not worry about it?

[Ian Vollick, 2014-09-11 19:58:08]

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1246: // First get the layers to the same
depth.
On 2014/09/11 19:22:05, danakj wrote:
> nit: I think this comment was clearer before. Now it sounds like you're
mutating
> layers.
Yeah, I've switched it up for something that's hopefully clearer.

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1259: // child relationships in blink
precisely because the scroll child is not a
On 2014/09/11 19:22:05, danakj wrote:
> oh thanks for this comment, that's helpful. but can you do this without
> mentioning blink? like "scroll parents and children are only meant to be used
> when..." or something

Done.

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1938: layer,
combined_transform.Preserves2dAxisAlignment());
On 2014/09/11 19:22:05, danakj wrote:
> these unrelated format changes may make future merging more difficuly
I put 'em back. (This was the work of git cl format)

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2518:
inputs->current_render_surface_layer_list_id + 1,
On 2014/09/11 19:22:05, danakj wrote:
> why +1?
0 doesn't work as a sequence number. Although no real caller of this fn passes 0
as a RSLL-id, the tests did. I'd added one here to avoid test churn, but it
turned out to be trivial to update the tests, so I've just done that.

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2554:
inputs->current_render_surface_layer_list_id + 1,
On 2014/09/11 19:22:05, danakj wrote:
> why +1?

Zero doesn't work here. In practice, no real caller of CalcDrawProps passes
zero, but the tests do. If you'd prefer that I modify the tests and add an
assertion, I'd be happy to do that.

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/548963002/diff/120001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.h:382: size_t render_surface_layer_list_id_;
On 2014/09/11 19:22:05, danakj wrote:
> This kinda violates my reading of the chromium style guide. It's a sneaky way
of
> trying to use unsigned, when this is not in fact the size of a container or
> something, where size_t is explicitly allowed.
> <quote>
> Use an unsigned integer type if your variable represents a bit pattern (e.g. a
> hash value or a bitmask).
> 
> Use unsigned integer types (preferably size_t) for object and allocation
sizes,
> object counts, array and pointer offsets, vector indices, and so on.
> 
> Do not use unsigned types to mean "this value should never be < 0". For that,
> use assertions or run-time checks (as appropriate).
> </quote>
> 
> It doesn't discuss "if your value can wrap around" but that loosely falls
close
> to "it can't be < 0", and we should handle wrap around by checking for max()
if
> it's a problem in practice. Given this hasn't been a problem on the impl side
> which generates >= # of frames as the main thread side, I think you should
just
> leave this as integer and not worry about it?

Done.

[danakj, 2014-09-11 20:02:42]

https://codereview.chromium.org/548963002/diff/140001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (left):

https://codereview.chromium.org/548963002/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:2853:
SingularTransformDoesNotPreventClearingDrawProperties) {
This test doesn't test anything anymore. Can you make it still test something or
remove it?

[Ian Vollick, 2014-09-11 20:02:49]

> Zero doesn't work here. In practice, no real caller of CalcDrawProps passes
> zero, but the tests do. If you'd prefer that I modify the tests and add an
> assertion, I'd be happy to do that.

Ack that was a stale comment. Please disregard it.

[Ian Vollick, 2014-09-11 20:22:53]

https://codereview.chromium.org/548963002/diff/140001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (left):

https://codereview.chromium.org/548963002/diff/140001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:2853:
SingularTransformDoesNotPreventClearingDrawProperties) {
On 2014/09/11 20:02:42, danakj wrote:
> This test doesn't test anything anymore. Can you make it still test something
or
> remove it?

Whoops! Yeah, it turns out that sequence_number can be used to check what
sorted_for_recursion was testing for here. Updated.

[danakj, 2014-09-11 20:31:13]

LGTM

[danakj, 2014-09-11 20:31:22]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-09-11 20:39:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/548963002/160001

[commit-bot: I haz the power, 2014-09-12 02:02:57]

Committed patchset #6 (id:160001) as 4c5596de55b5206cae8779bd6a58d107a972a78d

[commit-bot: I haz the power, 2014-09-12 02:05:47]

Patchset 6 (id:??) landed as
https://crrev.com/7faedca87705b714845842e595bca262c4090fa4
Cr-Commit-Position: refs/heads/master@{#294521}

[Ian Vollick, 2014-09-12 17:43:52]

A revert of this CL (patchset #6 id:160001) has been created in
https://codereview.chromium.org/563313002/ by vollick@chromium.org.

The reason for reverting is: This patch seems to be exposing some bugs. They
bugs appear not to be directly related to this patch, but I'm going to revert
for now until I can investigate..

[Ian Vollick, 2014-10-17 18:49:53]

On 2014/09/12 at 17:43:52, vollick wrote:
> A revert of this CL (patchset #6 id:160001) has been created in
https://codereview.chromium.org/563313002/ by vollick@chromium.org.
> 
> The reason for reverting is: This patch seems to be exposing some bugs. They
bugs appear not to be directly related to this patch, but I'm going to revert
for now until I can investigate..

As of (https://codereview.chromium.org/572483002), the bug exposed by this patch
when it was last applied should be fixed.

I'd thought that this patch was unnecessary, though, so I'd let it sit closed.
Issue 424064 shows that it's actually still required.

I've made two significant changes since this patch was last landed that need
approval.

1) It turns out that clip parents may not always be on the ancestor chain, so
I've had to make it possible to transform the clip through the LCA in that case.
2) It's possible for a scroll parent to not be axis aligned wrt to its target,
so I've made it possible for clip transformation to fail in that case (and I've
logged crbug.com/424684 to describe what will be needed to fix this for real).

I've also added tests for these cases.

[Ian Vollick, 2014-10-20 23:16:44]

On 2014/10/17 at 18:49:53, vollick wrote:
> On 2014/09/12 at 17:43:52, vollick wrote:
> > A revert of this CL (patchset #6 id:160001) has been created in
https://codereview.chromium.org/563313002/ by vollick@chromium.org.
> > 
> > The reason for reverting is: This patch seems to be exposing some bugs. They
bugs appear not to be directly related to this patch, but I'm going to revert
for now until I can investigate..
> 
> As of (https://codereview.chromium.org/572483002), the bug exposed by this
patch when it was last applied should be fixed.
> 
> I'd thought that this patch was unnecessary, though, so I'd let it sit closed.
Issue 424064 shows that it's actually still required.
> 
> I've made two significant changes since this patch was last landed that need
approval.
> 
> 1) It turns out that clip parents may not always be on the ancestor chain, so
I've had to make it possible to transform the clip through the LCA in that case.
> 2) It's possible for a scroll parent to not be axis aligned wrt to its target,
so I've made it possible for clip transformation to fail in that case (and I've
logged crbug.com/424684 to describe what will be needed to fix this for real).
> 
> I've also added tests for these cases.

ping.

[danakj, 2014-10-20 23:34:36]

On 2014/10/20 23:16:44, vollick wrote:
> On 2014/10/17 at 18:49:53, vollick wrote:
> > On 2014/09/12 at 17:43:52, vollick wrote:
> > > A revert of this CL (patchset #6 id:160001) has been created in
> https://codereview.chromium.org/563313002/ by mailto:vollick@chromium.org.
> > > 
> > > The reason for reverting is: This patch seems to be exposing some bugs.
They
> bugs appear not to be directly related to this patch, but I'm going to revert
> for now until I can investigate..
> > 
> > As of (https://codereview.chromium.org/572483002), the bug exposed by this
> patch when it was last applied should be fixed.
> > 
> > I'd thought that this patch was unnecessary, though, so I'd let it sit
closed.
> Issue 424064 shows that it's actually still required.
> > 
> > I've made two significant changes since this patch was last landed that need
> approval.
> > 
> > 1) It turns out that clip parents may not always be on the ancestor chain,
so
> I've had to make it possible to transform the clip through the LCA in that
case.
> > 2) It's possible for a scroll parent to not be axis aligned wrt to its
target,
> so I've made it possible for clip transformation to fail in that case (and
I've
> logged crbug.com/424684 to describe what will be needed to fix this for real).
> > 
> > I've also added tests for these cases.
> 
> ping.

Oh sorry didn't realize this needs review.

[Ian Vollick, 2014-10-23 15:31:18]

On 2014/10/20 at 23:34:36, danakj wrote:
> On 2014/10/20 23:16:44, vollick wrote:
> > On 2014/10/17 at 18:49:53, vollick wrote:
> > > On 2014/09/12 at 17:43:52, vollick wrote:
> > > > A revert of this CL (patchset #6 id:160001) has been created in
> > https://codereview.chromium.org/563313002/ by mailto:vollick@chromium.org.
> > > > 
> > > > The reason for reverting is: This patch seems to be exposing some bugs.
They
> > bugs appear not to be directly related to this patch, but I'm going to
revert
> > for now until I can investigate..
> > > 
> > > As of (https://codereview.chromium.org/572483002), the bug exposed by this
> > patch when it was last applied should be fixed.
> > > 
> > > I'd thought that this patch was unnecessary, though, so I'd let it sit
closed.
> > Issue 424064 shows that it's actually still required.
> > > 
> > > I've made two significant changes since this patch was last landed that
need
> > approval.
> > > 
> > > 1) It turns out that clip parents may not always be on the ancestor chain,
so
> > I've had to make it possible to transform the clip through the LCA in that
case.
> > > 2) It's possible for a scroll parent to not be axis aligned wrt to its
target,
> > so I've made it possible for clip transformation to fail in that case (and
I've
> > logged crbug.com/424684 to describe what will be needed to fix this for
real).
> > > 
> > > I've also added tests for these cases.
> > 
> > ping.
> 
> Oh sorry didn't realize this needs review.

ping.