cc: Consider scroll offset in CalcDrawProperties

cc: Consider scroll offset in CalcDrawProperties

Previously, any updates to scroll offset on the main thread updated both
scroll offset and layer position.  CalcDrawProperties only used position
and ignored scroll offset.  This patch changes that to consider scroll
offset equally with scroll delta when computing a layer's tranform.

Although this patch doesn't do anything about it yet, the end result of
this disentangling is that it will become possible to early out in
Layer::SetScrollOffset and not require a SetNeedsCommit when Blink
updates the scroll offset to the value that it already is on the
compositor thread.

This is step 3 of a patch series to disentangle layer position and
scroll offset.  This patch depends on both
https://codereview.chromium.org/18405003/ and
https://codereview.chromium.org/18187004/.

R=danakj@chromium.org
BUG=256381
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211176

[commit-bot: I haz the power, 2013-07-08 17:52:29]

No comments yet.

[danakj, 2013-07-08 21:53:42]

https://chromiumcodereview.appspot.com/18400003/diff/2001/cc/trees/layer_tree...
File cc/trees/layer_tree_host_common_unittest.cc (left):

https://chromiumcodereview.appspot.com/18400003/diff/2001/cc/trees/layer_tree...
cc/trees/layer_tree_host_common_unittest.cc:397: TEST(LayerTreeHostCommonTest,
TransformsAboutScrollOffset) {
Maybe this should be named DoesNotTransformAbountScrollPositionOrOffset now?

https://chromiumcodereview.appspot.com/18400003/diff/2001/cc/trees/layer_tree...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/18400003/diff/2001/cc/trees/layer_tree...
cc/trees/layer_tree_host_impl_unittest.cc:1897:
host_impl_->ScrollBegin(gfx::Point(),
Why this? That's the point where the finger is sitting.

https://chromiumcodereview.appspot.com/18400003/diff/2001/cc/trees/layer_tree...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://chromiumcodereview.appspot.com/18400003/diff/2001/cc/trees/layer_tree...
cc/trees/layer_tree_host_unittest_scroll.cc:145:
EXPECT_GE(impl->SourceAnimationFrameNumber(), 2);
Does this mean we do the above elseif{} block (sourceframe=0,
sourceanimationframe=2) and then also this block (sourceframe=1,
sourceanimationframe=2)? Ie, new source frame, but the source animation frame
number didn't change?

Or is the block above being skipped and some of this test isn't testing
anything?

[enne (OOO), 2013-07-08 22:03:46]

https://chromiumcodereview.appspot.com/18400003/diff/2001/cc/trees/layer_tree...
File cc/trees/layer_tree_host_common_unittest.cc (left):

https://chromiumcodereview.appspot.com/18400003/diff/2001/cc/trees/layer_tree...
cc/trees/layer_tree_host_common_unittest.cc:397: TEST(LayerTreeHostCommonTest,
TransformsAboutScrollOffset) {
On 2013/07/08 21:53:42, danakj wrote:
> Maybe this should be named DoesNotTransformAbountScrollPositionOrOffset now?

Only the inputs to this test have changed, in parallel with the reverted code in
WebLayerImpl.  This test hasn't changed what it's testing, so I'm confused about
what you're suggesting to rename it to.

https://chromiumcodereview.appspot.com/18400003/diff/2001/cc/trees/layer_tree...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/18400003/diff/2001/cc/trees/layer_tree...
cc/trees/layer_tree_host_impl_unittest.cc:1897:
host_impl_->ScrollBegin(gfx::Point(),
On 2013/07/08 21:53:42, danakj wrote:
> Why this? That's the point where the finger is sitting.

This test previously wasn't implemented correctly (or, at least in a way that
simulated the normal flow of scroll offset).  Because these scroll offsets don't
go back to Blink, the position of the layers never changed even though the
scroll offsets changed.

Changing the scroll offset to the origin maintains the original set of layers
that get scrolled via hit testing for this test now that the layers move.

https://chromiumcodereview.appspot.com/18400003/diff/2001/cc/trees/layer_tree...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://chromiumcodereview.appspot.com/18400003/diff/2001/cc/trees/layer_tree...
cc/trees/layer_tree_host_unittest_scroll.cc:145:
EXPECT_GE(impl->SourceAnimationFrameNumber(), 2);
On 2013/07/08 21:53:42, danakj wrote:
> Does this mean we do the above elseif{} block (sourceframe=0,
> sourceanimationframe=2) and then also this block (sourceframe=1,
> sourceanimationframe=2)? Ie, new source frame, but the source animation frame
> number didn't change?

All three blocks run in order on different "draws".  The third time the draw may
not occur because there's no damage, as the comment implies.  I could force some
damage if you think that'd make it clearer.

> Or is the block above being skipped and some of this test isn't testing
> anything?

Nope.  The above block is also running.

[enne (OOO), 2013-07-09 00:49:45]

https://chromiumcodereview.appspot.com/18400003/diff/2001/cc/trees/layer_tree...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://chromiumcodereview.appspot.com/18400003/diff/2001/cc/trees/layer_tree...
cc/trees/layer_tree_host_unittest_scroll.cc:145:
EXPECT_GE(impl->SourceAnimationFrameNumber(), 2);
On 2013/07/08 22:03:46, enne wrote:
> On 2013/07/08 21:53:42, danakj wrote:
> > Does this mean we do the above elseif{} block (sourceframe=0,
> > sourceanimationframe=2) and then also this block (sourceframe=1,
> > sourceanimationframe=2)? Ie, new source frame, but the source animation
frame
> > number didn't change?
> 
> All three blocks run in order on different "draws".  The third time the draw
may
> not occur because there's no damage, as the comment implies.  I could force
some
> damage if you think that'd make it clearer.
> 
> > Or is the block above being skipped and some of this test isn't testing
> > anything?
> 
> Nope.  The above block is also running.

I fixed this case by resizing the root.  Now that scroll offsets move layers
(via the large initial scroll offset in this test), this layer got scrolled
offscreen and so its damage no longer affected the viewport.

[enne (OOO), 2013-07-09 01:45:43]

https://codereview.chromium.org/18400003/diff/14001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/18400003/diff/14001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_scroll.cc:260:
child_layer_->SetPosition(gfx::Point(5, 5));
This reverts the child layer positioning when we're scrolling the child layer. 
I think this preserves the hit testing under high DPI testing that the original
test had?

https://codereview.chromium.org/18400003/diff/14001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_scroll.cc:263:
child_layer_->SetPosition(gfx::Point(60, 5));
In this case we're trying to miss the child layer.  The problem is that as the
root scrolls it scrolls underneath the hit target for scrolls.  I don't think
any test coverage is lost by moving it over here.

[danakj, 2013-07-09 01:52:45]

LGTM

https://codereview.chromium.org/18400003/diff/2001/cc/trees/layer_tree_host_c...
File cc/trees/layer_tree_host_common_unittest.cc (left):

https://codereview.chromium.org/18400003/diff/2001/cc/trees/layer_tree_host_c...
cc/trees/layer_tree_host_common_unittest.cc:397: TEST(LayerTreeHostCommonTest,
TransformsAboutScrollOffset) {
On 2013/07/08 22:03:46, enne wrote:
> On 2013/07/08 21:53:42, danakj wrote:
> > Maybe this should be named DoesNotTransformAbountScrollPositionOrOffset now?
> 
> Only the inputs to this test have changed, in parallel with the reverted code
in
> WebLayerImpl.  This test hasn't changed what it's testing, so I'm confused
about
> what you're suggesting to rename it to.

Mmm.. right, the scroll offset/delta are still used directly, just not included
in the layer's position anymore. Nevermind!

https://codereview.chromium.org/18400003/diff/2001/cc/trees/layer_tree_host_i...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/18400003/diff/2001/cc/trees/layer_tree_host_i...
cc/trees/layer_tree_host_impl_unittest.cc:1897:
host_impl_->ScrollBegin(gfx::Point(),
On 2013/07/08 22:03:46, enne wrote:
> On 2013/07/08 21:53:42, danakj wrote:
> > Why this? That's the point where the finger is sitting.
> 
> This test previously wasn't implemented correctly (or, at least in a way that
> simulated the normal flow of scroll offset).  Because these scroll offsets
don't
> go back to Blink, the position of the layers never changed even though the
> scroll offsets changed.
> 
> Changing the scroll offset to the origin maintains the original set of layers
> that get scrolled via hit testing for this test now that the layers move.

Cool, thanks for the explanation. I see, this would hit the child now otherwise.

https://codereview.chromium.org/18400003/diff/14001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/18400003/diff/14001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_scroll.cc:260:
child_layer_->SetPosition(gfx::Point(5, 5));
On 2013/07/09 01:45:43, enne wrote:
> This reverts the child layer positioning when we're scrolling the child layer.

> I think this preserves the hit testing under high DPI testing that the
original
> test had?

Thanks.

https://codereview.chromium.org/18400003/diff/14001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest_scroll.cc:263:
child_layer_->SetPosition(gfx::Point(60, 5));
On 2013/07/09 01:45:43, enne wrote:
> In this case we're trying to miss the child layer.  The problem is that as the
> root scrolls it scrolls underneath the hit target for scrolls.  I don't think
> any test coverage is lost by moving it over here.

Agree.

[jamesr, 2013-07-09 23:45:27]

https://codereview.chromium.org/18400003/diff/18001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/18400003/diff/18001/build/common.gypi#newcode...
build/common.gypi:1838: # TODO(enne): Remove this once #ifdefs using this have
been removed.
oh yuck - i don't think you want to set a define for every project in the
universe, just have a gyp variable and map that to a #define in the targets that
need it

[enne (OOO), 2013-07-10 00:41:21]

https://codereview.chromium.org/18400003/diff/18001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/18400003/diff/18001/build/common.gypi#newcode...
build/common.gypi:1838: # TODO(enne): Remove this once #ifdefs using this have
been removed.
On 2013/07/09 23:45:27, jamesr wrote:
> oh yuck - i don't think you want to set a define for every project in the
> universe, just have a gyp variable and map that to a #define in the targets
that
> need it

Thanks for the suggestion.  I changed this to a gyp variable instead, PTAL.

[jamesr, 2013-07-10 04:08:36]

lgtm

[commit-bot: I haz the power, 2013-07-10 19:55:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/18400003/22001

[commit-bot: I haz the power, 2013-07-10 20:48:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/18400003/22001

[commit-bot: I haz the power, 2013-07-11 18:09:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/18400003/22001

[commit-bot: I haz the power, 2013-07-11 18:45:48]

Change committed as 211176