New issue
Advanced search Search tips

Issue 645615 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Switch to static root property tree nodes in blink

Project Member Reported by pdr@chromium.org, Sep 9 2016

Issue description

We should switch to static root property tree nodes.

See discussion in https://codereview.chromium.org/2330863003
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 23 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a518f5340802393f8bdaa6b3d376dc5482049351

commit a518f5340802393f8bdaa6b3d376dc5482049351
Author: pdr <pdr@chromium.org>
Date: Fri Sep 23 04:45:55 2016

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 

Review-Url: https://codereview.chromium.org/2359063002
Cr-Commit-Position: refs/heads/master@{#420577}

[modify] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351/third_party/WebKit/Source/core/frame/FrameView.h
[modify] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351/third_party/WebKit/Source/core/paint/FramePainter.cpp
[modify] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351/third_party/WebKit/Source/core/paint/ObjectPaintProperties.h
[modify] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351/third_party/WebKit/Source/core/paint/PaintControllerPaintTest.cpp
[modify] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h
[modify] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
[modify] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp
[modify] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351/third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp
[modify] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351/third_party/WebKit/Source/platform/graphics/paint/ClipPaintPropertyNode.h
[modify] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351/third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h
[modify] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351/third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h
[modify] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351/third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h

Comment 2 by pdr@chromium.org, Sep 23 2016

Summary: Switch to static root property tree nodes in blink (was: Switch to null root property tree nodes in blink)

Comment 3 by pdr@chromium.org, Sep 23 2016

Description: Show this description
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 24 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6dbe31ae92f52661dfa4d2af4fdf768707b5cf31

commit 6dbe31ae92f52661dfa4d2af4fdf768707b5cf31
Author: pdr <pdr@chromium.org>
Date: Sat Sep 24 02:23:28 2016

Switch the Root Layer Scrolls codepath to use root clip paint properties

This is a followup to [1] to start moving the root layer scrolls
codepath to use the new root property tree nodes. Only the clip node
has been done in this patch which unblocks [2] which will unblock
switching the rest of the root layer scrolls root nodes over.

[1] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351
[2] https://codereview.chromium.org/2370553002

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

Review-Url: https://codereview.chromium.org/2364233002
Cr-Commit-Position: refs/heads/master@{#420816}

[modify] https://crrev.com/6dbe31ae92f52661dfa4d2af4fdf768707b5cf31/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/6dbe31ae92f52661dfa4d2af4fdf768707b5cf31/third_party/WebKit/Source/platform/graphics/paint/ClipPaintPropertyNode.h

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/900408123237980d6b583c24b7e64d3b446910e8

commit 900408123237980d6b583c24b7e64d3b446910e8
Author: pdr <pdr@chromium.org>
Date: Mon Sep 26 19:40:40 2016

Add tests for PaintPropertyTreePrinter, remove path printers

This patch adds some basic testing of PaintPropertyTreePrinter which
ensures trees can be print with and without root layer scrolling.
Regular expressions have been used to focus on the tree structure and
avoid dynamic output (e.g., pointer values).

I am not aware of anyone using the path printers and have removed them
to keep PaintPropertyTreePrinter lean & mean.

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

Review-Url: https://codereview.chromium.org/2368783002
Cr-Commit-Position: refs/heads/master@{#420964}

[modify] https://crrev.com/900408123237980d6b583c24b7e64d3b446910e8/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/900408123237980d6b583c24b7e64d3b446910e8/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp
[modify] https://crrev.com/900408123237980d6b583c24b7e64d3b446910e8/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.h
[add] https://crrev.com/900408123237980d6b583c24b7e64d3b446910e8/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinterTest.cpp

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/80f2dd56da341a600b19f74b45e0aaae52fdb1d7

commit 80f2dd56da341a600b19f74b45e0aaae52fdb1d7
Author: pdr <pdr@chromium.org>
Date: Tue Sep 27 00:54:03 2016

Switch the RootLayerScrolls codepath to use root effect paint properties

This is a followup to [1] to start moving the root layer scrolls
codepath to use the new root property tree nodes. Only the effect node
has been done in this patch.

[1] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351

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

Review-Url: https://codereview.chromium.org/2366123002
Cr-Commit-Position: refs/heads/master@{#421056}

[modify] https://crrev.com/80f2dd56da341a600b19f74b45e0aaae52fdb1d7/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/80f2dd56da341a600b19f74b45e0aaae52fdb1d7/third_party/WebKit/Source/platform/graphics/paint/EffectPaintPropertyNode.h

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0abd71cfec612222822a4e4a1bb0bc89e937a9ce

commit 0abd71cfec612222822a4e4a1bb0bc89e937a9ce
Author: pdr <pdr@chromium.org>
Date: Tue Sep 27 02:43:00 2016

Switch RootLayerScrolls to use root transform and scroll properties

This is a followup to [1] to start using the root transform and scroll
properties with root layer scrolling enabled. With this patch, the
RLS/non-RLS property trees are no longer different.

A real codechange in this patch is to not create a scroll translation
for the FrameView unless it actually scrolls. This matches the logic
used for adding LayoutView scroll translation properties.

[1] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351

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

Review-Url: https://codereview.chromium.org/2364103003
Cr-Commit-Position: refs/heads/master@{#421085}

[modify] https://crrev.com/0abd71cfec612222822a4e4a1bb0bc89e937a9ce/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/0abd71cfec612222822a4e4a1bb0bc89e937a9ce/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
[modify] https://crrev.com/0abd71cfec612222822a4e4a1bb0bc89e937a9ce/third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h
[modify] https://crrev.com/0abd71cfec612222822a4e4a1bb0bc89e937a9ce/third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8dd17c6523d757f4e8075157c14cf2e661b1e1b9

commit 8dd17c6523d757f4e8075157c14cf2e661b1e1b9
Author: pdr <pdr@chromium.org>
Date: Tue Sep 27 05:05:52 2016

Revert of Add tests for PaintPropertyTreePrinter, remove path printers (patchset #2 id:20001 of https://codereview.chromium.org/2368783002/ )

Reason for revert:
This patch was correct but failed in combination with https://crrev.com/80f2dd56da341a600b19f74b45e0aaae52fdb1d7 and https://crrev.com/0abd71cfec612222822a4e4a1bb0bc89e937a9ce which landed very close to this patch. I'm going to roll this out instead of the other two to green the tree faster. I'll re-land with the simple fix tomorrow.

Original issue's description:
> Add tests for PaintPropertyTreePrinter, remove path printers
>
> This patch adds some basic testing of PaintPropertyTreePrinter which
> ensures trees can be print with and without root layer scrolling.
> Regular expressions have been used to focus on the tree structure and
> avoid dynamic output (e.g., pointer values).
>
> I am not aware of anyone using the path printers and have removed them
> to keep PaintPropertyTreePrinter lean & mean.
>
> BUG= 645615 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
>
> Committed: https://crrev.com/900408123237980d6b583c24b7e64d3b446910e8
> Cr-Commit-Position: refs/heads/master@{#420964}

TBR=chrishtr@chromium.org,szager@chromium.org,trchen@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 645615 

Review-Url: https://codereview.chromium.org/2372963002
Cr-Commit-Position: refs/heads/master@{#421112}

[modify] https://crrev.com/8dd17c6523d757f4e8075157c14cf2e661b1e1b9/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/8dd17c6523d757f4e8075157c14cf2e661b1e1b9/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp
[modify] https://crrev.com/8dd17c6523d757f4e8075157c14cf2e661b1e1b9/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.h
[delete] https://crrev.com/ce19d57eda179fe9b47d00159eb25bc025712c9b/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinterTest.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Sep 28 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a08dcd0cb471c6f7bced2291892bc1008aaa8890

commit a08dcd0cb471c6f7bced2291892bc1008aaa8890
Author: pdr <pdr@chromium.org>
Date: Tue Sep 27 21:41:10 2016

(reland) Add tests for PaintPropertyTreePrinter, remove path printers

This patch adds some basic testing of PaintPropertyTreePrinter which
ensures trees can be print with and without root layer scrolling.
Regular expressions have been used to focus on the tree structure and
avoid dynamic output (e.g., pointer values).

I am not aware of anyone using the path printers and have removed them
to keep PaintPropertyTreePrinter lean & mean.

This is a reland of [1] which was rolled out due to colliding with
root property node patches. The updated patch no longer has different
tests for root layer scrolls and non-root layer scrolls, showing that
those patches actually worked.

[1] https://crrev.com/900408123237980d6b583c24b7e64d3b446910e8

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

Review-Url: https://codereview.chromium.org/2373093002
Cr-Commit-Position: refs/heads/master@{#421344}

[modify] https://crrev.com/a08dcd0cb471c6f7bced2291892bc1008aaa8890/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/a08dcd0cb471c6f7bced2291892bc1008aaa8890/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp
[modify] https://crrev.com/a08dcd0cb471c6f7bced2291892bc1008aaa8890/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.h
[add] https://crrev.com/a08dcd0cb471c6f7bced2291892bc1008aaa8890/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinterTest.cpp

Project Member

Comment 10 by bugdroid1@chromium.org, Sep 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e61b7c42641360ac6085b7667370cb2e58b4cad2

commit e61b7c42641360ac6085b7667370cb2e58b4cad2
Author: pdr <pdr@chromium.org>
Date: Thu Sep 29 02:50:01 2016

Add paint property path printers and test them

This patch is a followup to [1] and restores the old paint property
tree path printers. Additionally, simple tests have been added to
ensure they do not break.

[1] https://crrev.com/a08dcd0cb471c6f7bced2291892bc1008aaa8890

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

Review-Url: https://codereview.chromium.org/2377183002
Cr-Commit-Position: refs/heads/master@{#421727}

[modify] https://crrev.com/e61b7c42641360ac6085b7667370cb2e58b4cad2/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp
[modify] https://crrev.com/e61b7c42641360ac6085b7667370cb2e58b4cad2/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.h
[modify] https://crrev.com/e61b7c42641360ac6085b7667370cb2e58b4cad2/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinterTest.cpp

Project Member

Comment 11 by bugdroid1@chromium.org, Sep 29 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f0ebdb54d79437c44dc8664f011aaa6a7bb49f5f

commit f0ebdb54d79437c44dc8664f011aaa6a7bb49f5f
Author: pdr <pdr@chromium.org>
Date: Thu Sep 29 21:48:40 2016

Refactor PropertyTreeState to use RefPtrs

PropertyTreeState stores the total state of the property trees at some
given time. At the moment this is only used for FrameView, but I'd like
to use PropertyTreeState in LocalBorderBoxProperties in the future.

The purpose of this patch is twofold:
1) Guard against use-after-free bugs as we add support for incremental
updates to the property trees.
2) Prepare to refactor LocalBorderBoxProperties (which uses RefPtrs) to
use PropertyTreeState.

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

Review-Url: https://codereview.chromium.org/2378883003
Cr-Commit-Position: refs/heads/master@{#421943}

[modify] https://crrev.com/f0ebdb54d79437c44dc8664f011aaa6a7bb49f5f/third_party/WebKit/Source/core/paint/FramePainter.cpp
[modify] https://crrev.com/f0ebdb54d79437c44dc8664f011aaa6a7bb49f5f/third_party/WebKit/Source/core/paint/ObjectPaintProperties.h
[modify] https://crrev.com/f0ebdb54d79437c44dc8664f011aaa6a7bb49f5f/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp

Comment 12 by pdr@chromium.org, Oct 10 2016

Status: Fixed (was: Assigned)
I think we can call this one fixed.
Project Member

Comment 13 by bugdroid1@chromium.org, Nov 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/29bb3301df59c321abee8d9d226bf38e3877a5fb

commit 29bb3301df59c321abee8d9d226bf38e3877a5fb
Author: pdr <pdr@chromium.org>
Date: Mon Nov 14 23:28:10 2016

DCHECK that paint properties are never null

This patch adds a new DCHECK that paint chunk properties are not null,
along with comments describing how to fix this situation if one is
unlucky enough to hit it. Now that we have root paint property nodes,
the properties of a paint chunk should never be null.

The asserts in this patch uncovered a bug where FrameView scrollbars
were painted with null paint property nodes. This has been fixed by
ensuring all properties, not just the transform property, are
specified in FramePainter. This fixed 18 test failures due to the new
assert.

The asserts in this patch also uncovered a case where we were using
null paint properties in SkPictureBuilder when optimizing the display
item list of the CompositingRecorder. An assert disabler has been
added because this case is safe, and comments have been added
describing why.

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

Review-Url: https://codereview.chromium.org/2480863002
Cr-Commit-Position: refs/heads/master@{#431967}

[modify] https://crrev.com/29bb3301df59c321abee8d9d226bf38e3877a5fb/third_party/WebKit/Source/core/paint/FramePainter.cpp
[modify] https://crrev.com/29bb3301df59c321abee8d9d226bf38e3877a5fb/third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp
[modify] https://crrev.com/29bb3301df59c321abee8d9d226bf38e3877a5fb/third_party/WebKit/Source/platform/graphics/paint/PaintChunker.cpp
[modify] https://crrev.com/29bb3301df59c321abee8d9d226bf38e3877a5fb/third_party/WebKit/Source/platform/graphics/paint/PaintChunker.h
[modify] https://crrev.com/29bb3301df59c321abee8d9d226bf38e3877a5fb/third_party/WebKit/Source/platform/graphics/paint/PaintChunkerTest.cpp
[modify] https://crrev.com/29bb3301df59c321abee8d9d226bf38e3877a5fb/third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp
[modify] https://crrev.com/29bb3301df59c321abee8d9d226bf38e3877a5fb/third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp

Project Member

Comment 14 by bugdroid1@chromium.org, Nov 17 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c2fccd350841d0d60631a7196c1bb9b0bf838255

commit c2fccd350841d0d60631a7196c1bb9b0bf838255
Author: trchen <trchen@chromium.org>
Date: Thu Nov 17 00:30:26 2016

[SPv2] Refactor PaintArtifactorCompositor root node creation

This CL sets up static conversion for root blink property nodes into the
hardcoded cc property nodes number 1 (a.k.a. secondary root).

Now we also enforce that every blink-created property node can never refer to
nullptr nodes.

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

Review-Url: https://codereview.chromium.org/2503193002
Cr-Commit-Position: refs/heads/master@{#432677}

[modify] https://crrev.com/c2fccd350841d0d60631a7196c1bb9b0bf838255/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
[modify] https://crrev.com/c2fccd350841d0d60631a7196c1bb9b0bf838255/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
[modify] https://crrev.com/c2fccd350841d0d60631a7196c1bb9b0bf838255/third_party/WebKit/Source/platform/testing/TestPaintArtifact.h

Project Member

Comment 15 by bugdroid1@chromium.org, Oct 25

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/29e3d2abc3fd9fd48f0dd38dccfad7ab1a00bb1a

commit 29e3d2abc3fd9fd48f0dd38dccfad7ab1a00bb1a
Author: Mason Freed <masonfreed@chromium.org>
Date: Thu Oct 25 17:27:44 2018

Cleaning up dead code.

 https://crbug.com/645615  is fixed, and due to the DCHECK, this code is
definitely dead.

Bug:  645615 
Change-Id: I95030c2f63d710ab783f34ee435d6cae04484ecd
Reviewed-on: https://chromium-review.googlesource.com/c/1298613
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602772}
[modify] https://crrev.com/29e3d2abc3fd9fd48f0dd38dccfad7ab1a00bb1a/third_party/blink/renderer/platform/graphics/compositing/property_tree_manager.cc

Sign in to add a comment