Move MaxScrollOffset to property_trees

Move MaxScrollOffset to property_trees

MaxScrollOffset is removed from LayerImpl so that updating scrolling info
does not need to touch LayerImpl unless a non-fast scrollable region is met
(this is also planned to be removed from LayerImpl when hit testing list
is available).

ScrollTree now stores information about layer bounds, scroll clip layer
bounds, bounds_delta and inner/outer viewport container/scroller layer info.

Also needs to push page_scale_factor to TransformTree when updated.

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/cfccd1b3bb04f45527c34f6f52d3052faab0e1b2
Cr-Commit-Position: refs/heads/master@{#374809}

[sunxd, 2016-02-06 02:57:20]

Description was changed from

==========
Move MaxScrollOffset to property_trees

BUG=568830
==========

to

==========
Move MaxScrollOffset to property_trees

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunxd, 2016-02-09 00:10:12]

Description was changed from

==========
Move MaxScrollOffset to property_trees

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Move MaxScrollOffset to property_trees

MaxScrollOffset is removed from LayerImpl so that updating scrolling info
does not need to touch LayerImpl unless a non-fast scrollable region is met
(this is also planned to be removed from LayerImpl when hit testing list 
is available).

ScrollTree now stores information about layer bounds, scroll clip layer
bounds, bounds_delta and inner/outer viewport container/scroller layer info.

Also needs to push page_scale_factor to TransformTree when updated.

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunxd, 2016-02-09 00:10:12]

sunxd@chromium.org changed reviewers:
+ ajuma@chromium.org, jaydasika@chromium.org, vollick@chromium.org,
weiliangc@chromium.org

[sunxd, 2016-02-09 00:11:32]

Move MaxScrollOffset to scroll tree from layer impl.

I will delete the dead code soon.

https://codereview.chromium.org/1675963002/diff/20001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1675963002/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:379: scroll_node->data.bounds = size;
I will delete this part, as we now force a rebuild.

https://codereview.chromium.org/1675963002/diff/20001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/1675963002/diff/20001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1038: void LayerImpl::SetBoundsForTesting(const
gfx::Size& bounds) {
Dead code.

https://codereview.chromium.org/1675963002/diff/20001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1629: return
layer_tree_impl()->property_trees()->scroll_tree.MaxScrollOffset(
This will be moved in the next CL.

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

https://codereview.chromium.org/1675963002/diff/20001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:462: void SetBoundsForTesting(const gfx::Size& bounds);
This is dead code, will be deleted.

https://codereview.chromium.org/1675963002/diff/20001/cc/test/fake_layer_tree...
File cc/test/fake_layer_tree_host.cc (right):

https://codereview.chromium.org/1675963002/diff/20001/cc/test/fake_layer_tree...
cc/test/fake_layer_tree_host.cc:77:
active_tree()->property_trees()->SetupInternalPointers();
This is to make the
active_tree()->property_trees()->transform_tree.property_tree pointer point to
the property tree in active tree instead of that in pending tree.

[sunxd, 2016-02-09 15:47:23]

I removed the dead code mentioned in patch#2 and attempted to fix a trybot
failure (because we move bounds deltas to property tree and these tests only
instantiate the transform tree).

[ajuma, 2016-02-09 16:27:13]

https://codereview.chromium.org/1675963002/diff/40001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/1675963002/diff/40001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:374: }
The block above should be removed too (no need to update the existing ClipNode
if we're going to rebuild property trees).

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

https://codereview.chromium.org/1675963002/diff/40001/cc/layers/layer.h#newco...
cc/layers/layer.h:132: // This function is for property tree builder
Maybe point out this is because of templating? (Or, even better, make property
tree builder not depend on this, see comment there.)

https://codereview.chromium.org/1675963002/diff/40001/cc/layers/layer.h#newco...
cc/layers/layer.h:278: int scroll_clip_layer_id() const { return
scroll_clip_layer_id_; }
It looks like this isn't called anywhere. Is it needed?

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

https://codereview.chromium.org/1675963002/diff/40001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:459: 
Unintentional whitespace change?

https://codereview.chromium.org/1675963002/diff/40001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1675963002/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:489: void
LayerTreeImpl::SetPageScaleOnActiveTreeForTesting(
Can callers of this just call SetPageScaleOnActiveTree instead (so we don't need
this testing-only method)?

https://codereview.chromium.org/1675963002/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:546: // factor in transform tree.
I'm not sure I follow this comment. We could be pushing to the pending tree
here.

https://codereview.chromium.org/1675963002/diff/40001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:558: }
What goes wrong if we wait till UpdateDrawProperties time for the property trees
to find out about the page scale factor? (Since page scale factor is a
SyncedProperty, changes on the active tree immediately affect the value seen by
the pending tree, even though nothing gets called on the pending tree, so
pulling the value at UpdateDrawProperties time seems to be the least complex way
to make sure we use the correct value when computing draw properties. If there
are callers that need property trees to have an up-to-date page scale even
before we updated draw properties, it might be better for those callers to
ensure this.)

https://codereview.chromium.org/1675963002/diff/40001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1675963002/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.cc:1269:
property_trees()->outer_viewport_container_bounds_delta().y());
Are the inner/outer viewport container layers ever actually scrollable
themselves? I'd expect them to show up as scroll clip layers but not as
scrollable layers (in which case we don't need this
is_inner_viewport_container_layer and is_outer_viewport_container_layer bools).

https://codereview.chromium.org/1675963002/diff/40001/cc/trees/property_tree.h
File cc/trees/property_tree.h (right):

https://codereview.chromium.org/1675963002/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.h:275: bool envelope_page_scale_layer_with_clip_layer;
This variable could use a comment (or perhaps a clearer name).

https://codereview.chromium.org/1675963002/diff/40001/cc/trees/property_tree....
cc/trees/property_tree.h:551: scroll_tree.SetPropertyTrees(this);
How about making the constructor and assignment operator take care of this? (so
we don't depend on callers remembering to call this)

https://codereview.chromium.org/1675963002/diff/40001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1675963002/diff/40001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:712: -layer->bounds_delta().y());
Since bounds_delta is always 0 on Layer, the only reason to take bounds_delta
into account is for tests. But what if we ignore them here, and instead just
make sure that the bounds_delta values we store elsewhere are up to date?

[sunxd, 2016-02-09 21:17:49]

I applied the comments to the CL.

[ajuma, 2016-02-09 22:35:05]

This mostly looks good, just a few more comments.

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:8416:
host_impl_->SetViewportSize(gfx::Size(100, 100));
If you build property trees here, you shouldn't need the additional changes to
bounds_delta below (we should be able to handle the changes in
UpdateTopControlsState without another rebuild).

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:308: outer_container->bounds_delta());
Do we need to do the same for the inner viewport scroll layer? It's a good idea
to add a test for this too (something like a layer tree host test that sets a
bounds delta on the inner viewport scroll layer just before a commit/activation
happens, and then verifies property trees are correctly taking the bounds delta
into account after activation).

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:496:
ClampPageScaleFactorToLimits(active_page_scale),
I think you can use current_page_scale_factor() here without having to clamp
again.

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:497: device_scale_factor(), identity_transform);
Instead of identity_transform, this should use the actual device transform
(layer_tree_host_impl_->DrawTransform()).

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:530:
ClampPageScaleFactorToLimits(*page_scale_factor),
If you move this to the end of this method then you can use
current_page_scale_factor() here.

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:531: device_scale_factor(), identity_transform);
Instead of identity_transform, this should be
layer_tree_host_impl_->DrawTransform().

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:534:
ClampPageScaleFactorToLimits(*page_scale_factor));
Is this code needed? It seems wrong to have a non-1 page scale factor without a
page scale layer, and if we don't have a page scale layer then the property
trees we just pushed should already have page scale 1 in the transform tree.
Perhaps just DCHECK(*page_scale_factor == 1) here?

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/property_tree.cc
File cc/trees/property_tree.cc (right):

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/property_tree....
cc/trees/property_tree.cc:1234: if (!scroll_node->data.scrollable ||
scroll_node->data.bounds.IsEmpty())
Do we need to take bounds_delta into account before checking if bounds are
empty? This could use a test case too.

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:687: kInvalidPropertyTreeNodeId) {
Are there cases where this doesn't hold? What if we DCHECK instead?

[sunxd, 2016-02-09 23:03:57]

Will work on resolving comments. And here are some comments about cc_unittests
related to property_trees.

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:534:
ClampPageScaleFactorToLimits(*page_scale_factor));
On 2016/02/09 22:35:05, ajuma wrote:
> Is this code needed? It seems wrong to have a non-1 page scale factor without
a
> page scale layer, and if we don't have a page scale layer then the property
> trees we just pushed should already have page scale 1 in the transform tree.
> Perhaps just DCHECK(*page_scale_factor == 1) here? 

This is also for testing, LayerTreeHostCommonTestBase::
    ExecuteCalculateDrawPropertiesWithPropertyTrees(Layer* root_layer)
(https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/layer_tree...)
sets page_scale_layer to be nullptr even though the layer tree contains a valid
page_scale_layer. Should I change that code? I was afraid of breaking other
stuff if I use layer_tree_host()->page_scale_layer() here.

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:687: kInvalidPropertyTreeNodeId) {
On 2016/02/09 22:35:05, ajuma wrote:
> Are there cases where this doesn't hold? What if we DCHECK instead?

In LayerTreeHostImplTest.ScrollUnknownScrollAncestorMismatch, child_scroll_clip
layer is created but not added to the layer tree, so when building property
trees here, child_scroll layer's clip layer does not have a valid
transform_tree_index.

[ajuma, 2016-02-10 00:15:06]

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:534:
ClampPageScaleFactorToLimits(*page_scale_factor));
On 2016/02/09 23:03:56, sunxd wrote:
> On 2016/02/09 22:35:05, ajuma wrote:
> > Is this code needed? It seems wrong to have a non-1 page scale factor
without
> a
> > page scale layer, and if we don't have a page scale layer then the property
> > trees we just pushed should already have page scale 1 in the transform tree.
> > Perhaps just DCHECK(*page_scale_factor == 1) here? 
> 
> This is also for testing, LayerTreeHostCommonTestBase::
>     ExecuteCalculateDrawPropertiesWithPropertyTrees(Layer* root_layer)
>
(https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/layer_tree...)
> sets page_scale_layer to be nullptr even though the layer tree contains a
valid
> page_scale_layer. Should I change that code? 

Checking for a non-null PageScaleLayer() is fine, but in the case where it's
null, page_scale_factor should be 1, so property trees should already have the
right value. That is, doing
if (PageScaleLayer()) {
  ...
} else {
  DCHECK(*page_scale_factor == 1)
}
should work.

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/property_tree_...
File cc/trees/property_tree_builder.cc (right):

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/property_tree_...
cc/trees/property_tree_builder.cc:687: kInvalidPropertyTreeNodeId) {
On 2016/02/09 23:03:56, sunxd wrote:
> On 2016/02/09 22:35:05, ajuma wrote:
> > Are there cases where this doesn't hold? What if we DCHECK instead?
> 
> In LayerTreeHostImplTest.ScrollUnknownScrollAncestorMismatch,
child_scroll_clip
> layer is created but not added to the layer tree, so when building property
> trees here, child_scroll layer's clip layer does not have a valid
> transform_tree_index.

What if we modify the test so that the child_scroll_clip_layer is added to the
tree (in between scroll_layer and child_scroll)?

[sunxd, 2016-02-10 16:56:36]

Just one problem about the comments:

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:8416:
host_impl_->SetViewportSize(gfx::Size(100, 100));
On 2016/02/09 22:35:05, ajuma wrote:
> If you build property trees here, you shouldn't need the additional changes to
> bounds_delta below (we should be able to handle the changes in
> UpdateTopControlsState without another rebuild).

DrawFrame() forces a rebuild of property trees here, so it will still overwrite
the built tree with the one affected by bounds_delta.

[ajuma, 2016-02-10 17:54:58]

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:8416:
host_impl_->SetViewportSize(gfx::Size(100, 100));
On 2016/02/10 16:56:36, sunxd wrote:
> On 2016/02/09 22:35:05, ajuma wrote:
> > If you build property trees here, you shouldn't need the additional changes
to
> > bounds_delta below (we should be able to handle the changes in
> > UpdateTopControlsState without another rebuild).
> 
> DrawFrame() forces a rebuild of property trees here, so it will still
overwrite
> the built tree with the one affected by bounds_delta.

How about making a version of DrawFrame that doesn't rebuild? (Ideally, we'd
make DrawFrame itself not always rebuild, and instead make tests that need to
rebuild ask for this explicitly, but doing that is probably beyond the scope of
this CL.)

[sunxd, 2016-02-10 17:59:26]

On 2016/02/10 17:54:58, ajuma wrote:
>
https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/1675963002/diff/60001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl_unittest.cc:8416:
> host_impl_->SetViewportSize(gfx::Size(100, 100));
> On 2016/02/10 16:56:36, sunxd wrote:
> > On 2016/02/09 22:35:05, ajuma wrote:
> > > If you build property trees here, you shouldn't need the additional
changes
> to
> > > bounds_delta below (we should be able to handle the changes in
> > > UpdateTopControlsState without another rebuild).
> > 
> > DrawFrame() forces a rebuild of property trees here, so it will still
> overwrite
> > the built tree with the one affected by bounds_delta.
> 
> How about making a version of DrawFrame that doesn't rebuild? (Ideally, we'd
> make DrawFrame itself not always rebuild, and instead make tests that need to
> rebuild ask for this explicitly, but doing that is probably beyond the scope
of
> this CL.)

Sure, I can deal with that in the next CL.

[ajuma, 2016-02-10 19:08:14]

https://codereview.chromium.org/1675963002/diff/100001/cc/trees/layer_tree_im...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/1675963002/diff/100001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:496: if (PageScaleLayer() &&
!property_trees()->needs_rebuild)
We shouldn't be rebuilding on the compositor thread outside of test code, so the
check for needs_rebuild here can be removed.

https://codereview.chromium.org/1675963002/diff/100001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:538: if (page_scale_factor &&
!property_trees()->needs_rebuild) {
Here too, we shouldn't be checking for needs_rebuild.

https://codereview.chromium.org/1675963002/diff/100001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:546: }
This block needs to happen after the next one (since clamping happens in
DidUpdatePageScale).

[ajuma, 2016-02-10 22:30:19]

Thanks! Just one more nit, but otherwise lgtm.

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

https://codereview.chromium.org/1675963002/diff/120001/cc/trees/layer_tree_im...
cc/trees/layer_tree_impl.cc:544: if (page_scale_factor) {  // &&
!property_trees()->needs_rebuild) {
The commented-out code needs to be removed.

[sunxd, 2016-02-10 22:44:30]

The CQ bit was checked by sunxd@chromium.org

[sunxd, 2016-02-10 22:44:31]

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/1675963002/#ps140001
(title: "Remove comment.")

[commit-bot: I haz the power, 2016-02-10 22:48:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675963002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675963002/140001

[commit-bot: I haz the power, 2016-02-11 00:54:30]

Description was changed from

==========
Move MaxScrollOffset to property_trees

MaxScrollOffset is removed from LayerImpl so that updating scrolling info
does not need to touch LayerImpl unless a non-fast scrollable region is met
(this is also planned to be removed from LayerImpl when hit testing list 
is available).

ScrollTree now stores information about layer bounds, scroll clip layer
bounds, bounds_delta and inner/outer viewport container/scroller layer info.

Also needs to push page_scale_factor to TransformTree when updated.

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Move MaxScrollOffset to property_trees

MaxScrollOffset is removed from LayerImpl so that updating scrolling info
does not need to touch LayerImpl unless a non-fast scrollable region is met
(this is also planned to be removed from LayerImpl when hit testing list 
is available).

ScrollTree now stores information about layer bounds, scroll clip layer
bounds, bounds_delta and inner/outer viewport container/scroller layer info.

Also needs to push page_scale_factor to TransformTree when updated.

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-02-11 00:54:31]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-02-16 22:33:19]

Description was changed from

==========
Move MaxScrollOffset to property_trees

MaxScrollOffset is removed from LayerImpl so that updating scrolling info
does not need to touch LayerImpl unless a non-fast scrollable region is met
(this is also planned to be removed from LayerImpl when hit testing list 
is available).

ScrollTree now stores information about layer bounds, scroll clip layer
bounds, bounds_delta and inner/outer viewport container/scroller layer info.

Also needs to push page_scale_factor to TransformTree when updated.

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Move MaxScrollOffset to property_trees

MaxScrollOffset is removed from LayerImpl so that updating scrolling info
does not need to touch LayerImpl unless a non-fast scrollable region is met
(this is also planned to be removed from LayerImpl when hit testing list
is available).

ScrollTree now stores information about layer bounds, scroll clip layer
bounds, bounds_delta and inner/outer viewport container/scroller layer info.

Also needs to push page_scale_factor to TransformTree when updated.

BUG=568830
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/cfccd1b3bb04f45527c34f6f52d3052faab0e1b2
Cr-Commit-Position: refs/heads/master@{#374809}
==========

[commit-bot: I haz the power, 2016-02-16 22:33:20]

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