Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2540043002:
Fix descendant-dependent PaintLayer flags update to not early-out when dirty. (Closed)
|
Created:
4 years ago by chrishtr Modified:
4 years ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix descendant-dependent PaintLayer flags update to not early-out when dirty.
Previously, the iteration over children would early-out if any of them had
visible content or visible child content. This is incorrect, because other children
might still have dirty bits that need to be cleared.
Fixing this allows us to remove the force-recompute hammer added in
https://codereview.chromium.org/455963002 and adjusted in
https://codereview.chromium.org/1149303002 to add a dirty bit to avoid walking
the entire PaintLayer tree on every frame.
BUG=646188
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/cce865745dd419cdbbdd9ba6c1e8e8669c91518b
Cr-Commit-Position: refs/heads/master@{#435307}
Patch Set 1 #Patch Set 2 : none #Patch Set 3 : none #
Total comments: 2
Messages
Total messages: 25 (20 generated)
Description was changed from ========== none BUG= ========== to ========== none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Description was changed from ========== none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix descendant-dependent PaintLayer flags update to not early-out when dirty. Previously, the iteration over children would early-out if any of them had visible content or visible child content. This is incorrect, because other children might still have dirty bits that need to be cleared. Fixing this allows us to remove the force-recompute hammer added BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Fix descendant-dependent PaintLayer flags update to not early-out when dirty. Previously, the iteration over children would early-out if any of them had visible content or visible child content. This is incorrect, because other children might still have dirty bits that need to be cleared. Fixing this allows us to remove the force-recompute hammer added BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix descendant-dependent PaintLayer flags update to not early-out when dirty. Previously, the iteration over children would early-out if any of them had visible content or visible child content. This is incorrect, because other children might still have dirty bits that need to be cleared. Fixing this allows us to remove the force-recompute hammer added in https://codereview.chromium.org/455963002 and adjusted in https://codereview.chromium.org/1149303002 to add a dirty bit to avoid walking the entire PaintLayer tree on every frame. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Fix descendant-dependent PaintLayer flags update to not early-out when dirty. Previously, the iteration over children would early-out if any of them had visible content or visible child content. This is incorrect, because other children might still have dirty bits that need to be cleared. Fixing this allows us to remove the force-recompute hammer added in https://codereview.chromium.org/455963002 and adjusted in https://codereview.chromium.org/1149303002 to add a dirty bit to avoid walking the entire PaintLayer tree on every frame. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix descendant-dependent PaintLayer flags update to not early-out when dirty. Previously, the iteration over children would early-out if any of them had visible content or visible child content. This is incorrect, because other children might still have dirty bits that need to be cleared. Fixing this allows us to remove the force-recompute hammer added in https://codereview.chromium.org/455963002 and adjusted in https://codereview.chromium.org/1149303002 to add a dirty bit to avoid walking the entire PaintLayer tree on every frame. BUG=646188 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
chrishtr@chromium.org changed reviewers: + pdr@chromium.org, wangxianzhu@chromium.org
Side note: Next up after this CL is moving all descendant-dependent PaintLayer flags into this walk, and removing them from CompositingInputsUpdater. In SPv2 mode, ancestor-dependent properties and compositing triggers will be computed during the pre-paint tree walk. But descendant-dependent properties can't all be done there, because they can affect the structure of the property trees, and we don't want to allow modifying the property trees on the way back up the tree walk. https://codereview.chromium.org/2540043002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp (left): https://codereview.chromium.org/2540043002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:192: // actually cleaning all the dirty bits in a subtree. I thought about it for a long time and could not come up with an explanation for what this comment is talking about. I think the bit clearing was simply incorrect because of the invalid early-out of the child for loop. https://codereview.chromium.org/2540043002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (left): https://codereview.chromium.org/2540043002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:709: break; Here is where it used to incorrectly early-out of the iteration over children. There are about 10 layout tests that fail if the "whole subtree" hammer is removed but this break remains.
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wangxianzhu@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480526036462700,
"parent_rev": "607a6852d6b523a0c5c09253ef4766f0d682200c", "commit_rev":
"b2a26471f6fab87e36607e33b2cf36eb76fd44bc"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix descendant-dependent PaintLayer flags update to not early-out when dirty. Previously, the iteration over children would early-out if any of them had visible content or visible child content. This is incorrect, because other children might still have dirty bits that need to be cleared. Fixing this allows us to remove the force-recompute hammer added in https://codereview.chromium.org/455963002 and adjusted in https://codereview.chromium.org/1149303002 to add a dirty bit to avoid walking the entire PaintLayer tree on every frame. BUG=646188 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix descendant-dependent PaintLayer flags update to not early-out when dirty. Previously, the iteration over children would early-out if any of them had visible content or visible child content. This is incorrect, because other children might still have dirty bits that need to be cleared. Fixing this allows us to remove the force-recompute hammer added in https://codereview.chromium.org/455963002 and adjusted in https://codereview.chromium.org/1149303002 to add a dirty bit to avoid walking the entire PaintLayer tree on every frame. BUG=646188 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/cce865745dd419cdbbdd9ba6c1e8e8669c91518b Cr-Commit-Position: refs/heads/master@{#435307} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cce865745dd419cdbbdd9ba6c1e8e8669c91518b Cr-Commit-Position: refs/heads/master@{#435307} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
