Add static root property tree nodes [spv2]

Add static root property tree nodes [spv2]

Slimming paint v2 (SPV2) initially had root property tree nodes on the
FrameView. This approach required some contortions when the root-layer-
scrolls codepath was added because LayoutView needed to store the root
nodes.

This patch adds static root paint property tree nodes but only uses
them on FrameView. A followup patch will switch the root-layer-scrolls
codepath to use this approach as well.

Because the root nodes can no longer be looked up off the FrameView,
a cache of the entire property tree state has been added to FrameView
(totalPropertyTreeStateForContents). This is similar to
LocalBorderBoxProperties on ObjectPaintProperties, and a followup patch
will refactor LocalBorderBoxProperties to use PropertyTreeState as well.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=645615
Committed: https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351
Cr-Commit-Position: refs/heads/master@{#420577}

[pdr., 2016-09-22 06:12:09]

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

[commit-bot: I haz the power, 2016-09-22 06:12:24]

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

[pdr., 2016-09-22 06:39:37]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org, jbroman@chromium.org, szager@chromium.org,
trchen@chromium.org

[commit-bot: I haz the power, 2016-09-22 07:29:19]

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

[commit-bot: I haz the power, 2016-09-22 07:29:20]

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

[trchen, 2016-09-22 08:03:20]

lgtm.

I was thinking of having a static "invalid" node instead of static root node and
use nullptr to denote root node. Seeing this I'm convinced static root nodes
would be a cleaner approach.

[jbroman, 2016-09-22 14:20:52]

overall this approach looks good to me

https://codereview.chromium.org/2359063002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2359063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:63:
rootScrollNode()->clearMainThreadScrollingReasons();
This one confuses me a little. If there's a global root, presumably it should be
treated as immutable? Modifying it seems bad given that it's shared by several
FrameViews.

[chrishtr, 2016-09-22 16:36:58]

https://codereview.chromium.org/2359063002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/ClipPaintPropertyNode.h
(right):

https://codereview.chromium.org/2359063002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/ClipPaintPropertyNode.h:45:
bool isRoot() const { return !m_parent; }
You never use this method outside of tests. why add it?

[pdr., 2016-09-22 19:48:21]

https://codereview.chromium.org/2359063002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/FramePainter.cpp (right):

https://codereview.chromium.org/2359063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/FramePainter.cpp:49: if (const
PropertyTreeState* contentsState =
m_frameView->totalPropertyTreeStateForContents()) {
This change caught a real bug where FramePainter was using a null scrollNode
here. I've fixed the test
(PaintControllerPaintTestForSlimmingPaintV2.ChunkIdClientCacheFlag) and added a
check specifically that we don't have a null node.

https://codereview.chromium.org/2359063002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2359063002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:63:
rootScrollNode()->clearMainThreadScrollingReasons();
On 2016/09/22 at 14:20:52, jbroman wrote:
> This one confuses me a little. If there's a global root, presumably it should
be treated as immutable? Modifying it seems bad given that it's shared by
several FrameViews.

This patch was just matching the existing behavior here, this was previously
hidden in "rootFrame.rootScroll()->update(...)" but is now explicit. The reason
this occurs is we propagate the "has background attachment fixed" main thread
scroll reason all the way to the root, even though the root isn't used for
scrolling. We could change this to make the root nodes immutable
(dcheck(!isRoot() in all prop tree node update(...) functions), but I'd like to
defer that until the RLS and non-RLS codepaths both use these root nodes.

https://codereview.chromium.org/2359063002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/ClipPaintPropertyNode.h
(right):

https://codereview.chromium.org/2359063002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/ClipPaintPropertyNode.h:45:
bool isRoot() const { return !m_parent; }
On 2016/09/22 at 16:36:57, chrishtr wrote:
> You never use this method outside of tests. why add it?

It's also used in the property tree printer. I think we'll start using it for
real in the followup when root layer scrolls begins using the root nodes. For
example, we can dcheck that the root nodes aren't modified. We could do a
private/friend approach but I don't think we will want to hide the root node
concept.

[pdr., 2016-09-22 19:48:24]

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

[commit-bot: I haz the power, 2016-09-22 19:49:05]

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

[chrishtr, 2016-09-22 21:14:51]

lgtm

[commit-bot: I haz the power, 2016-09-22 23:16:53]

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

[commit-bot: I haz the power, 2016-09-22 23:16:54]

Dry run: This issue passed the CQ dry run.

[szager1, 2016-09-23 00:35:48]

lgtm with nit.

https://codereview.chromium.org/2359063002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp (right):

https://codereview.chromium.org/2359063002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp:244: 
You need something here to output information about the root nodes.

[pdr., 2016-09-23 03:23:06]

On 2016/09/23 at 00:35:48, szager wrote:
> lgtm with nit.
> 
>
https://codereview.chromium.org/2359063002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp
(right):
> 
>
https://codereview.chromium.org/2359063002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp:244: 
> You need something here to output information about the root nodes.

Fixed!

In we go

[pdr., 2016-09-23 03:23:12]

The CQ bit was checked by pdr@chromium.org

[pdr., 2016-09-23 03:23:12]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org,
chrishtr@chromium.org, szager@chromium.org
Link to the patchset: https://codereview.chromium.org/2359063002/#ps40001
(title: "Restore PaintPropertyTreeGraphBuilder root node printing")

[commit-bot: I haz the power, 2016-09-23 03:23: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, 2016-09-23 04:46:16]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-23 04:48:27]

Description was changed from

==========
Add static root property tree nodes [spv2]

Slimming paint v2 (SPV2) initially had root property tree nodes on the
FrameView. This approach required some contortions when the root-layer-
scrolls codepath was added because LayoutView needed to store the root
nodes.

This patch adds static root paint property tree nodes but only uses
them on FrameView. A followup patch will switch the root-layer-scrolls
codepath to use this approach as well.

Because the root nodes can no longer be looked up off the FrameView,
a cache of the entire property tree state has been added to FrameView
(totalPropertyTreeStateForContents). This is similar to
LocalBorderBoxProperties on ObjectPaintProperties, and a followup patch
will refactor LocalBorderBoxProperties to use PropertyTreeState as well.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=645615
==========

to

==========
Add static root property tree nodes [spv2]

Slimming paint v2 (SPV2) initially had root property tree nodes on the
FrameView. This approach required some contortions when the root-layer-
scrolls codepath was added because LayoutView needed to store the root
nodes.

This patch adds static root paint property tree nodes but only uses
them on FrameView. A followup patch will switch the root-layer-scrolls
codepath to use this approach as well.

Because the root nodes can no longer be looked up off the FrameView,
a cache of the entire property tree state has been added to FrameView
(totalPropertyTreeStateForContents). This is similar to
LocalBorderBoxProperties on ObjectPaintProperties, and a followup patch
will refactor LocalBorderBoxProperties to use PropertyTreeState as well.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
BUG=645615

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

[commit-bot: I haz the power, 2016-09-23 04:48:28]

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