Rewrite Layer::SetScrollOffset{FromImplSide} property tree fast-path

Cleanup the Layer::SetScrollOffset{FromImplSide} property tree fast-path

This patch updates Layer::SetScrollOffset{FromImplSide} to clarify how the
scroll offset fast-path works:
1) Existing property nodes are updated if they exist without invalidating
the property trees.
2) If no existing property nodes exist, the property trees should already be
marked as needig a full rebuild which will update the scroll offset.

DCHECKs have been added to clarify:
* SetScrollOffset should not be called for non-scrollable Layers.
* The scroll transform node does not need to be looked up using layer ids
because it is available via the Layer's transform_node_index().

This patch removes two users of transform node's owning layer id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2877673002
Cr-Commit-Position: refs/heads/master@{#471490}
Committed: https://chromium.googlesource.com/chromium/src/+/64b691c1dfc3e667a1c615e29abf08304c8d5734

[pdr., 2017-05-11 04:28:56]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 04:29:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 05:41:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 05:41:58]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pdr., 2017-05-11 17:25:34]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, enne@chromium.org, wkorman@chromium.org

[pdr., 2017-05-11 17:25:52]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 17:26:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pdr., 2017-05-11 17:33:11]

Description was changed from

==========
Rewrite Layer::SetScrollOffset{FromImplSide} property tree fast-path

This patch updates Layer::SetScrollOffset{FromImplSide} to clarify how the
scroll offset fast-path works:
1) Existing property nodes are updated if they exist without invalidating
the property trees.
2) If no existing property nodes exist, the property trees should already be
marked as needig a full rebuild which will update the scroll offset.

DCHECKs have been added to clarify:
* SetScrollOffset should not be called for non-scrollable Layers.
* The scroll transform node does not need to be looked up using layer ids
because it is available via the Layer's transform_node_index().

This patch removes two users of transform node's owning layer id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Cleanup the Layer::SetScrollOffset{FromImplSide} property tree fast-path

This patch updates Layer::SetScrollOffset{FromImplSide} to clarify how the
scroll offset fast-path works:
1) Existing property nodes are updated if they exist without invalidating
the property trees.
2) If no existing property nodes exist, the property trees should already be
marked as needig a full rebuild which will update the scroll offset.

DCHECKs have been added to clarify:
* SetScrollOffset should not be called for non-scrollable Layers.
* The scroll transform node does not need to be looked up using layer ids
because it is available via the Layer's transform_node_index().

This patch removes two users of transform node's owning layer id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[wkorman, 2017-05-11 17:42:14]

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

https://codereview.chromium.org/2877673002/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:814: bool Layer::UpdateExistingScrollOffsetPropertyTreeNodes(
Add unit test for this?

https://codereview.chromium.org/2877673002/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:817: return false;
Why not put DCHECK in two places above in here, and then do we still need
retval?

https://codereview.chromium.org/2877673002/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:822: PropertyTrees* property_trees =
layer_tree_host_->property_trees();
Use auto& like you did in other patch?

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

https://codereview.chromium.org/2877673002/diff/20001/cc/layers/layer.h#newco...
cc/layers/layer.h:512: bool UpdateExistingScrollOffsetPropertyTreeNodes(const
gfx::ScrollOffset&);
What about just "UpdateScrollOffset"? The rest seems implicit.

[enne (OOO), 2017-05-11 17:44:28]

Looks good in general.

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

https://codereview.chromium.org/2877673002/diff/20001/cc/layers/layer.h#newco...
cc/layers/layer.h:512: bool UpdateExistingScrollOffsetPropertyTreeNodes(const
gfx::ScrollOffset&);
I assume this dance here is because you can SetScrollOffset before the first
commit including this layer where property trees are built?

This is just a minor nit, but I'm not sure why the caller needs to care about
the return value.  For simplicity, could this function just be void, and just
DCHECK that it needs a rebuild internally instead of returning false?

[pdr., 2017-05-11 18:03:37]

Please take another look

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

https://codereview.chromium.org/2877673002/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:814: bool Layer::UpdateExistingScrollOffsetPropertyTreeNodes(
On 2017/05/11 at 17:42:13, wkorman wrote:
> Add unit test for this?

This function is really just a refactoring of the common functionality from
SetScrollOffset and SetScrollOffsetFromImplSide into a helper. This area is
tested in many tests. Do you think we need an additional test for just the
helper?

https://codereview.chromium.org/2877673002/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:817: return false;
On 2017/05/11 at 17:42:13, wkorman wrote:
> Why not put DCHECK in two places above in here, and then do we still need
retval?

Done.

https://codereview.chromium.org/2877673002/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:822: PropertyTrees* property_trees =
layer_tree_host_->property_trees();
On 2017/05/11 at 17:42:13, wkorman wrote:
> Use auto& like you did in other patch?

Done

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

https://codereview.chromium.org/2877673002/diff/20001/cc/layers/layer.h#newco...
cc/layers/layer.h:512: bool UpdateExistingScrollOffsetPropertyTreeNodes(const
gfx::ScrollOffset&);
On 2017/05/11 at 17:42:13, wkorman wrote:
> What about just "UpdateScrollOffset"? The rest seems implicit.

Good idears. Done.

[enne (OOO), 2017-05-11 18:05:37]

lgtm

[wkorman, 2017-05-11 18:21:38]

lgtm

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

https://codereview.chromium.org/2877673002/diff/20001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:814: bool Layer::UpdateExistingScrollOffsetPropertyTreeNodes(
On 2017/05/11 18:03:37, pdr. wrote:
> On 2017/05/11 at 17:42:13, wkorman wrote:
> > Add unit test for this?
> 
> This function is really just a refactoring of the common functionality from
> SetScrollOffset and SetScrollOffsetFromImplSide into a helper. This area is
> tested in many tests. Do you think we need an additional test for just the
> helper?

It looks like a public method on layer, so I would say yes, but I am not an
OWNER and much less familiar with scroll logic so am fine with your thought.

[pdr., 2017-05-11 18:22:26]

On 2017/05/11 at 18:21:38, wkorman wrote:
> lgtm
> 
> https://codereview.chromium.org/2877673002/diff/20001/cc/layers/layer.cc
> File cc/layers/layer.cc (right):
> 
>
https://codereview.chromium.org/2877673002/diff/20001/cc/layers/layer.cc#newc...
> cc/layers/layer.cc:814: bool
Layer::UpdateExistingScrollOffsetPropertyTreeNodes(
> On 2017/05/11 18:03:37, pdr. wrote:
> > On 2017/05/11 at 17:42:13, wkorman wrote:
> > > Add unit test for this?
> > 
> > This function is really just a refactoring of the common functionality from
> > SetScrollOffset and SetScrollOffsetFromImplSide into a helper. This area is
> > tested in many tests. Do you think we need an additional test for just the
> > helper?
> 
> It looks like a public method on layer, so I would say yes, but I am not an
OWNER and much less familiar with scroll logic so am fine with your thought.

Oh no, it is a private method

[pdr., 2017-05-12 15:07:58]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-05-12 15:08:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 17:43:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 17:43:14]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[pdr., 2017-05-12 22:09:01]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2017-05-12 22:10:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 23:38:02]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494626941296030,
"parent_rev": "5cd5e67224e946eeb92bebfc52fbf38573cc26c3", "commit_rev":
"64b691c1dfc3e667a1c615e29abf08304c8d5734"}

[commit-bot: I haz the power, 2017-05-12 23:38:14]

Description was changed from

==========
Cleanup the Layer::SetScrollOffset{FromImplSide} property tree fast-path

This patch updates Layer::SetScrollOffset{FromImplSide} to clarify how the
scroll offset fast-path works:
1) Existing property nodes are updated if they exist without invalidating
the property trees.
2) If no existing property nodes exist, the property trees should already be
marked as needig a full rebuild which will update the scroll offset.

DCHECKs have been added to clarify:
* SetScrollOffset should not be called for non-scrollable Layers.
* The scroll transform node does not need to be looked up using layer ids
because it is available via the Layer's transform_node_index().

This patch removes two users of transform node's owning layer id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Cleanup the Layer::SetScrollOffset{FromImplSide} property tree fast-path

This patch updates Layer::SetScrollOffset{FromImplSide} to clarify how the
scroll offset fast-path works:
1) Existing property nodes are updated if they exist without invalidating
the property trees.
2) If no existing property nodes exist, the property trees should already be
marked as needig a full rebuild which will update the scroll offset.

DCHECKs have been added to clarify:
* SetScrollOffset should not be called for non-scrollable Layers.
* The scroll transform node does not need to be looked up using layer ids
because it is available via the Layer's transform_node_index().

This patch removes two users of transform node's owning layer id.

BUG=693740
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2877673002
Cr-Commit-Position: refs/heads/master@{#471490}
Committed:
https://chromium.googlesource.com/chromium/src/+/64b691c1dfc3e667a1c615e29abf...
==========

[commit-bot: I haz the power, 2017-05-12 23:38:15]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/64b691c1dfc3e667a1c615e29abf...