Incrementally build main thread scrolling reasons [spv2]

Incrementally build main thread scrolling reasons [spv2]

This patch re-implements the scroll property's main thread scrolling
reasons so they can be built incrementally. Instead of propagating
main thread scrolling reasons up the scroll property tree, we now
calculate them when building each scroll node. With this change it is
simple to invalidate and rebuild scroll properties when the main
thread scrolling reasons change.

This patch trades performance for simplicity. FrameView already
maintains a set of LayoutObjects with background attachment fixed (a
main thread scrolling trigger). For every scroll node we iterate over
this set and check if any of the objects are descendants of the
scrolling object. In the common case, the set is empty.

This patch also cleans up ScrollPaintPropertyNode so it has a const
parent pointer like all other paint property nodes.

BUG=664672, 645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/3797c08066b335c81847266d1f16dafc9b21e886
Cr-Commit-Position: refs/heads/master@{#433367}

[pdr., 2016-11-17 05:39:39]

Description was changed from

==========
Incrementally build main thread scrolling reasons [spv2]

This patch re-implements the scroll property's main thread scrolling
reasons so they can be built incrementally. Instead of propagating
main thread scrolling reasons up the scroll property tree, we now
calculate them when building each scroll node. With this change it is
simple to invalidate and rebuild scroll properties when the main
thread scrolling reasons change.

This patch trades performance for implementation simplicity. FrameView
maintains a set of LayoutObjects with background attachment fixed (a
main thread scrolling trigger). For every scroll node we iterate over
this set and check if any of the objects are descendants of the
scrolling object.

This patch also cleans up ScrollPaintPropertyNode so it has a const
parent pointer like all other paint property nodes.

BUG=664672,645667
==========

to

==========
Incrementally build main thread scrolling reasons [spv2]

This patch re-implements the scroll property's main thread scrolling
reasons so they can be built incrementally. Instead of propagating
main thread scrolling reasons up the scroll property tree, we now
calculate them when building each scroll node. With this change it is
simple to invalidate and rebuild scroll properties when the main
thread scrolling reasons change.

This patch trades performance for implementation simplicity. FrameView
maintains a set of LayoutObjects with background attachment fixed (a
main thread scrolling trigger). For every scroll node we iterate over
this set and check if any of the objects are descendants of the
scrolling object.

This patch also cleans up ScrollPaintPropertyNode so it has a const
parent pointer like all other paint property nodes.

BUG=664672,645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2016-11-17 05:41:45]

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

[commit-bot: I haz the power, 2016-11-17 05:42:10]

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, 2016-11-17 05:45:35]

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

[commit-bot: I haz the power, 2016-11-17 05:45:36]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pdr., 2016-11-17 06:16:39]

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

[commit-bot: I haz the power, 2016-11-17 06:17:09]

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, 2016-11-17 07:33:54]

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

[commit-bot: I haz the power, 2016-11-17 07:33:54]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-11-17 18:34:26]

Description was changed from

==========
Incrementally build main thread scrolling reasons [spv2]

This patch re-implements the scroll property's main thread scrolling
reasons so they can be built incrementally. Instead of propagating
main thread scrolling reasons up the scroll property tree, we now
calculate them when building each scroll node. With this change it is
simple to invalidate and rebuild scroll properties when the main
thread scrolling reasons change.

This patch trades performance for implementation simplicity. FrameView
maintains a set of LayoutObjects with background attachment fixed (a
main thread scrolling trigger). For every scroll node we iterate over
this set and check if any of the objects are descendants of the
scrolling object.

This patch also cleans up ScrollPaintPropertyNode so it has a const
parent pointer like all other paint property nodes.

BUG=664672,645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Incrementally build main thread scrolling reasons [spv2]

This patch re-implements the scroll property's main thread scrolling
reasons so they can be built incrementally. Instead of propagating
main thread scrolling reasons up the scroll property tree, we now
calculate them when building each scroll node. With this change it is
simple to invalidate and rebuild scroll properties when the main
thread scrolling reasons change.

This patch trades performance for simplicity. FrameView already
maintains a set of LayoutObjects with background attachment fixed (a
main thread scrolling trigger). For every scroll node we iterate over
this set and check if any of the objects are descendants of the
scrolling object. In the common case, the set is empty.

This patch also cleans up ScrollPaintPropertyNode so it has a const
parent pointer like all other paint property nodes.

BUG=664672,645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[pdr., 2016-11-17 18:34:31]

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

[pdr., 2016-11-17 18:34:41]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, wangxianzhu@chromium.org

[commit-bot: I haz the power, 2016-11-17 18:35:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2016-11-17 19:03:04]

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:79: void
updateFrameViewScroll(
Nit not necessarily to be addressed in this CL: we should put these global
functions into anonymous namespace or prefix them with 'static'. BTW, can we
also remove the declarations of private functions in PaintPropertyTreeBuilder.h
and make them also file-visibility functions?

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:152:
frameView.hasBackgroundAttachmentFixedObjects();
Can the above code be moved into updateFrameViewScroll() to reduce the number of
parameters? It seems that we can just pass two parameters (frameView, context).

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2616: //
TODO(pdr): The main thread scrolling setting should invalidate properties.
The TODO is fine. Just curious, should we make it a RuntimeEnabledFlag for
simplicity?

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h
(right):

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h:145:
bool hasBackgroundAttachmentFixedDescendants)
Can we use named bit flags to replace the multiple bool parameters? I think
flags are clearer at callsites and can avoid errors of wrong parameters.

[commit-bot: I haz the power, 2016-11-17 20:38:50]

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

[commit-bot: I haz the power, 2016-11-17 20:38:50]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[pdr., 2016-11-17 21:01:36]

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:79: void
updateFrameViewScroll(
On 2016/11/17 at 19:03:04, Xianzhu wrote:
> Nit not necessarily to be addressed in this CL: we should put these global
functions into anonymous namespace or prefix them with 'static'. BTW, can we
also remove the declarations of private functions in PaintPropertyTreeBuilder.h
and make them also file-visibility functions?

Good ideas all around. I'll do this in a followup CL.

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:152:
frameView.hasBackgroundAttachmentFixedObjects();
On 2016/11/17 at 19:03:04, Xianzhu wrote:
> Can the above code be moved into updateFrameViewScroll() to reduce the number
of parameters? It seems that we can just pass two parameters (frameView,
context).

I'd prefer if we leave them here because this code matches the pattern used in
updateScrollAndScrollTranslation. Do you think that's okay?

Fortunately, most of the FrameView property code will go away when
RootLayerScrolls launches.

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:2616: //
TODO(pdr): The main thread scrolling setting should invalidate properties.
On 2016/11/17 at 19:03:04, Xianzhu wrote:
> The TODO is fine. Just curious, should we make it a RuntimeEnabledFlag for
simplicity?

I want to remove it entirely but that was shot down. It is only used for testing
though, see: https://bugs.chromium.org/p/chromium/issues/detail?id=410974#c45

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h
(right):

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h:145:
bool hasBackgroundAttachmentFixedDescendants)
On 2016/11/17 at 19:03:04, Xianzhu wrote:
> Can we use named bit flags to replace the multiple bool parameters? I think
flags are clearer at callsites and can avoid errors of wrong parameters.

+1, this looks much better. Done.

[Xianzhu, 2016-11-17 21:25:02]

lgtm

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:152:
frameView.hasBackgroundAttachmentFixedObjects();
On 2016/11/17 21:01:36, pdr. wrote:
> On 2016/11/17 at 19:03:04, Xianzhu wrote:
> > Can the above code be moved into updateFrameViewScroll() to reduce the
number
> of parameters? It seems that we can just pass two parameters (frameView,
> context).
> 
> I'd prefer if we leave them here because this code matches the pattern used in
> updateScrollAndScrollTranslation. Do you think that's okay?
> 
> Fortunately, most of the FrameView property code will go away when
> RootLayerScrolls launches.

Sounds good.

[pdr., 2016-11-17 21:26:50]

Thanks!

@Chris, did you want to take a look too? This is a different solution to the
main thread scrolling approach we talked about a long time ago.

[pdr., 2016-11-17 22:30:04]

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2506353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:79: void
updateFrameViewScroll(
On 2016/11/17 at 21:01:36, pdr. wrote:
> On 2016/11/17 at 19:03:04, Xianzhu wrote:
> > Nit not necessarily to be addressed in this CL: we should put these global
functions into anonymous namespace or prefix them with 'static'. BTW, can we
also remove the declarations of private functions in PaintPropertyTreeBuilder.h
and make them also file-visibility functions?
> 
> Good ideas all around. I'll do this in a followup CL.

I started this in parallel and found out why we didn't do this: We use c++
friendship to allow PaintPropertyTreeBuilder to access MutableForPainting in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La....
I think these have to be setup the way they are for that to work.

[pdr., 2016-11-18 00:26:39]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-11-18 00:28:36]

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, 2016-11-18 02:00:17]

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

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

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pdr., 2016-11-18 19:44:37]

On 2016/11/18 at 02:00:18, commit-bot wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

This failure was a simple mistake I made in
PaintArtifactCompositorTestWithPropertyTrees.NestedScrollNodes when switching
from two bools in ScrollPaintPropertyNode to an enum. Re-CQ'in with that fixed.

[pdr., 2016-11-18 19:44:44]

The CQ bit was checked by pdr@chromium.org

[pdr., 2016-11-18 19:44:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/2506353002/#ps100001
(title: "Fix test mistake")

[commit-bot: I haz the power, 2016-11-18 19:45:37]

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, 2016-11-19 00:22:25]

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

[commit-bot: I haz the power, 2016-11-19 00:22:27]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pdr., 2016-11-19 00:23:28]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-11-19 00:24: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, 2016-11-19 01:12:53]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-19 01:17:00]

Description was changed from

==========
Incrementally build main thread scrolling reasons [spv2]

This patch re-implements the scroll property's main thread scrolling
reasons so they can be built incrementally. Instead of propagating
main thread scrolling reasons up the scroll property tree, we now
calculate them when building each scroll node. With this change it is
simple to invalidate and rebuild scroll properties when the main
thread scrolling reasons change.

This patch trades performance for simplicity. FrameView already
maintains a set of LayoutObjects with background attachment fixed (a
main thread scrolling trigger). For every scroll node we iterate over
this set and check if any of the objects are descendants of the
scrolling object. In the common case, the set is empty.

This patch also cleans up ScrollPaintPropertyNode so it has a const
parent pointer like all other paint property nodes.

BUG=664672,645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Incrementally build main thread scrolling reasons [spv2]

This patch re-implements the scroll property's main thread scrolling
reasons so they can be built incrementally. Instead of propagating
main thread scrolling reasons up the scroll property tree, we now
calculate them when building each scroll node. With this change it is
simple to invalidate and rebuild scroll properties when the main
thread scrolling reasons change.

This patch trades performance for simplicity. FrameView already
maintains a set of LayoutObjects with background attachment fixed (a
main thread scrolling trigger). For every scroll node we iterate over
this set and check if any of the objects are descendants of the
scrolling object. In the common case, the set is empty.

This patch also cleans up ScrollPaintPropertyNode so it has a const
parent pointer like all other paint property nodes.

BUG=664672,645667
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/3797c08066b335c81847266d1f16dafc9b21e886
Cr-Commit-Position: refs/heads/master@{#433367}
==========

[commit-bot: I haz the power, 2016-11-19 01:17:01]

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