Account for fixed positioning once when computing parent offset

Account for fixed positioning once when computing parent offset

I'd updated the code for updating the parent offset for fixed
positioned layers in crrev.com/967163002, but there was a bug in that
cl that resulted in the offset being updated twice.

This caused incorrect visible content rects in the layout test
compositing/squashing/squashed-clip-layer.html.

This cl simplifies the parent offset computation code to handle
three cases explicitly
 1. has a scroll parent (must account for tree parent positioning)
 2. is fixed position (must account for the offset between the
    fixed container and the parent if they're different), and
 3. the usual case - inherits the parent offset.

BUG=386810
Committed: https://crrev.com/8c8247428f92db3a4c8a5a4ac28da093d27e2f7f
Cr-Commit-Position: refs/heads/master@{#321653}

[Ian Vollick, 2015-03-10 05:55:56]

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

[ajuma, 2015-03-20 18:15:34]

ajuma@chromium.org changed reviewers:
+ ajuma@chromium.org

[ajuma, 2015-03-20 18:15:35]

lgtm

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

https://codereview.chromium.org/994843002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:8909: gfx::Transform translateZ;
nit: translate_z

https://codereview.chromium.org/994843002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:8939:
ExecuteCalculateDrawProperties(root.get());
It'd be good to add 'EXPECT' lines here so this continues to be a meaningful
test once property trees are on by default.

[Ian Vollick, 2015-03-20 20:23:21]

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

https://codereview.chromium.org/994843002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:8909: gfx::Transform translateZ;
On 2015/03/20 18:15:34, ajuma wrote:
> nit: translate_z

Done.

https://codereview.chromium.org/994843002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:8939:
ExecuteCalculateDrawProperties(root.get());
On 2015/03/20 18:15:35, ajuma wrote:
> It'd be good to add 'EXPECT' lines here so this continues to be a meaningful
> test once property trees are on by default.

Done.

[ajuma, 2015-03-20 20:29:36]

https://codereview.chromium.org/994843002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/994843002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common_unittest.cc:8892:
fixed->visible_rect_from_property_trees().ToString());
Any reason not to compare gfx::Rects here rather than strings?

[Ian Vollick, 2015-03-20 20:32:04]

On 2015/03/20 20:29:36, ajuma wrote:
>
https://codereview.chromium.org/994843002/diff/60001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_common_unittest.cc (right):
> 
>
https://codereview.chromium.org/994843002/diff/60001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_common_unittest.cc:8892:
> fixed->visible_rect_from_property_trees().ToString());
> Any reason not to compare gfx::Rects here rather than strings?

Yeah, this is common practice in cc because it gives nicer error messages
(nicely stringified rects). See picture_pile_unittest.cc for other examples.

[ajuma, 2015-03-20 20:42:30]

On 2015/03/20 20:32:04, vollick wrote:
> On 2015/03/20 20:29:36, ajuma wrote:
> >
>
https://codereview.chromium.org/994843002/diff/60001/cc/trees/layer_tree_host...
> > File cc/trees/layer_tree_host_common_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/994843002/diff/60001/cc/trees/layer_tree_host...
> > cc/trees/layer_tree_host_common_unittest.cc:8892:
> > fixed->visible_rect_from_property_trees().ToString());
> > Any reason not to compare gfx::Rects here rather than strings?
> 
> Yeah, this is common practice in cc because it gives nicer error messages
> (nicely stringified rects). See picture_pile_unittest.cc for other examples.

Hmm. EXPECT_EQ(actual, expected) seems to give equally nicely stringified rects
when actual and expected are gfx::Rects. For example:
Value of: actual
Actual: 0,0 20x20
Expected: expected
Which is: 0,0 20x21

That said, you're certainly right that there are other places where we do call
ToString and compare strings. Perhaps at some point in that past EXPECT_EQ
didn't do what we needed it to?

[ajuma, 2015-03-20 20:42:52]

On 2015/03/20 20:42:30, ajuma wrote:
> On 2015/03/20 20:32:04, vollick wrote:
> > On 2015/03/20 20:29:36, ajuma wrote:
> > >
> >
>
https://codereview.chromium.org/994843002/diff/60001/cc/trees/layer_tree_host...
> > > File cc/trees/layer_tree_host_common_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/994843002/diff/60001/cc/trees/layer_tree_host...
> > > cc/trees/layer_tree_host_common_unittest.cc:8892:
> > > fixed->visible_rect_from_property_trees().ToString());
> > > Any reason not to compare gfx::Rects here rather than strings?
> > 
> > Yeah, this is common practice in cc because it gives nicer error messages
> > (nicely stringified rects). See picture_pile_unittest.cc for other examples.
> 
> Hmm. EXPECT_EQ(actual, expected) seems to give equally nicely stringified
rects
> when actual and expected are gfx::Rects. For example:
> Value of: actual
> Actual: 0,0 20x20
> Expected: expected
> Which is: 0,0 20x21
> 
> That said, you're certainly right that there are other places where we do call
> ToString and compare strings. Perhaps at some point in that past EXPECT_EQ
> didn't do what we needed it to?

In any case, lgtm either way.

[Ian Vollick, 2015-03-20 20:45:15]

On 2015/03/20 20:42:30, ajuma wrote:
> On 2015/03/20 20:32:04, vollick wrote:
> > On 2015/03/20 20:29:36, ajuma wrote:
> > >
> >
>
https://codereview.chromium.org/994843002/diff/60001/cc/trees/layer_tree_host...
> > > File cc/trees/layer_tree_host_common_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/994843002/diff/60001/cc/trees/layer_tree_host...
> > > cc/trees/layer_tree_host_common_unittest.cc:8892:
> > > fixed->visible_rect_from_property_trees().ToString());
> > > Any reason not to compare gfx::Rects here rather than strings?
> > 
> > Yeah, this is common practice in cc because it gives nicer error messages
> > (nicely stringified rects). See picture_pile_unittest.cc for other examples.
> 
> Hmm. EXPECT_EQ(actual, expected) seems to give equally nicely stringified
rects
> when actual and expected are gfx::Rects. For example:
> Value of: actual
> Actual: 0,0 20x20
> Expected: expected
> Which is: 0,0 20x21
> 
> That said, you're certainly right that there are other places where we do call
> ToString and compare strings. Perhaps at some point in that past EXPECT_EQ
> didn't do what we needed it to?

Oh, neat. Didn't know that. I bet you're right (or there was another type that
didn't stringify nicely that set a precedent). I'll switch to comparing rects in
that case.

[Ian Vollick, 2015-03-20 20:48:18]

On 2015/03/20 20:45:15, vollick wrote:
> On 2015/03/20 20:42:30, ajuma wrote:
> > On 2015/03/20 20:32:04, vollick wrote:
> > > On 2015/03/20 20:29:36, ajuma wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/994843002/diff/60001/cc/trees/layer_tree_host...
> > > > File cc/trees/layer_tree_host_common_unittest.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/994843002/diff/60001/cc/trees/layer_tree_host...
> > > > cc/trees/layer_tree_host_common_unittest.cc:8892:
> > > > fixed->visible_rect_from_property_trees().ToString());
> > > > Any reason not to compare gfx::Rects here rather than strings?
> > > 
> > > Yeah, this is common practice in cc because it gives nicer error messages
> > > (nicely stringified rects). See picture_pile_unittest.cc for other
examples.
> > 
> > Hmm. EXPECT_EQ(actual, expected) seems to give equally nicely stringified
> rects
> > when actual and expected are gfx::Rects. For example:
> > Value of: actual
> > Actual: 0,0 20x20
> > Expected: expected
> > Which is: 0,0 20x21
> > 
> > That said, you're certainly right that there are other places where we do
call
> > ToString and compare strings. Perhaps at some point in that past EXPECT_EQ
> > didn't do what we needed it to?
> 
> Oh, neat. Didn't know that. I bet you're right (or there was another type that
> didn't stringify nicely that set a precedent). I'll switch to comparing rects
in
> that case.

rectified. (sorry).

[Ian Vollick, 2015-03-20 20:49:24]

The CQ bit was checked by vollick@chromium.org

[Ian Vollick, 2015-03-20 20:49:25]

The patchset sent to the CQ was uploaded after l-g-t-m from ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/994843002/#ps80001 (title:
"compare rects, not strings.")

[commit-bot: I haz the power, 2015-03-20 20:49:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/994843002/80001

[commit-bot: I haz the power, 2015-03-20 22:21:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-20 22:21:50]

Patchset 5 (id:??) landed as
https://crrev.com/8c8247428f92db3a4c8a5a4ac28da093d27e2f7f
Cr-Commit-Position: refs/heads/master@{#321653}