Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2543913002:
Generalize visible descendant dirty bits to prepare for more properties. (Closed)
|
Created:
4 years ago by chrishtr Modified:
4 years ago CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, 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. |
DescriptionGeneralize visible descendant dirty bits to prepare for more properties.
This will allow for a subsequent patch to move other descendant-dependent
properties into the same tree walk.
Also remove several redundant calls to update descendant-dependent flags
(it should happen in exactly one tree walk per lifecycle update) and
avoid reading stale state in potentiallyDirtyVisibleContentStatus.
BUG=646188
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/6bbd055d68a54c04c41e87814d0d0be323a09ad0
Cr-Commit-Position: refs/heads/master@{#436045}
Patch Set 1 #Patch Set 2 : none #Patch Set 3 : none #
Total comments: 15
Patch Set 4 : none #Patch Set 5 : none #Patch Set 6 : none #Messages
Total messages: 45 (30 generated)
Description was changed from ========== none BUG= ========== to ========== none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Generalize visible descendant dirty bits to prepare for more properties. This will allow for a subsequent patch to move other descendant-dependent properties into the same tree walk. Also remove several redundant calls to update descendant-dependent flags (it should happen in exactly one tree walk per lifecycle update) and avoid reading stale state in potentiallyDirtyVisibleContentStatus. 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: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
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...
Description was changed from ========== Generalize visible descendant dirty bits to prepare for more properties. This will allow for a subsequent patch to move other descendant-dependent properties into the same tree walk. Also remove several redundant calls to update descendant-dependent flags (it should happen in exactly one tree walk per lifecycle update) and avoid reading stale state in potentiallyDirtyVisibleContentStatus. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Generalize visible descendant dirty bits to prepare for more properties. This will allow for a subsequent patch to move other descendant-dependent properties into the same tree walk. Also remove several redundant calls to update descendant-dependent flags (it should happen in exactly one tree walk per lifecycle update) and avoid reading stale state in potentiallyDirtyVisibleContentStatus. BUG=646188 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
chrishtr@chromium.org changed reviewers: + pdr@chromium.org, wangxianzhu@chromium.org
https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/contents-opaque/hidden-with-visible-text-expected.txt (right): https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/contents-opaque/hidden-with-visible-text-expected.txt:1: Caption Caption is now visible.. https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/contents-opaque/hidden-with-visible-text.html (right): https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/contents-opaque/hidden-with-visible-text.html:30: <span class="inner">Caption</span></div> My CL actually fixes this test, as it was previously written. See: https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r... The "caption" composited layer should not be present, because it's entirely invisible. I verified this by, at ToT, putting visibility: hidden on the "caption" element via inline style (vs modifying with script), and as expected the composited layer disappears from the output. The old code failed to invalidate the visibility status when adding visibility: hidden to the "caption" element. However, the test as named is intended to check that there is still a composited layer if there is visible text below an invisible element. It was added here: https://codereview.chromium.org/15925012 by copying two WebKit tests added here: http://trac.webkit.org/changeset/149084 The test in WebKit was wrong in the first place, because there is no element with class "inner" in the page. So I added it to the test here, and rebaselined. https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2848: frameView.layoutView()->layer()->updateDescendantDependentFlags(); This is necessary because I removed the old ones in PaintLayer, and SPv2 does not run PaintLayerCompositor::updateIfNeeded. Also, this is an intended tree walk required for SPv2, to compute descendant-dependent flags in advance of the pre-paint tree walk. https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (left): https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:317: updateDescendantDependentFlags(); This is not needed because it will happen later during the compositing update step. I also verified that there are no callers outside of compositing or later in the lifecycle that need any of these flags. See the attached bug, which links to a doc that investigates all call sites. https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:655: if (hasVisibleContent() == (visibility == EVisibility::Visible)) This is cleanup for what looks like unnecessary complexity. abarth added this code in this patch: https://codereview.chromium.org/453113002 which was trying to avoid reading dirty state, by adding a potentiallyDirtyVisibleContentStatus method which bailed if the value was dirty. The code in LayoutObject that is the call site has since been simplified to avoid conditionally dirtying visible content state based on current visible state, so we can now just remove this code and unconditionally dirty the state in LayoutObject. https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:1285: child->updateDescendantDependentFlags(); Same here - not necessary. https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:3009: updateDescendantDependentFlags(); Same here - not necessary.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Also added a fix exhibited to two browser tests, in which the destructor for CompositedLayerMapping would read stale state.
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...
https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:639: markAncestorChainForDescendantDependentFlagsUpdate(); This will also set m_needsDescendantDependentFlagsUpdate for |this|. Is this expected? https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.h:1133: unsigned m_needsDescendantDependentFlagsUpdate : 1; I'm not sure if the renaming is a good idea. updateDescendantDependentFlags() updated both m_isVisibleContentDirty and m_isVisibleDescendantDirty, so both of them seemed descendant (object and layer) dependent flags. If we want m_needsDescendantDependentFlagsUpdate cover both conditions (like in this patch dirtyVisibleContentStatus unconditionally calling markAncestorChainForDescendantDependentFlagsUpdate), we will do an extra level of traverse into child layers in updateDescendantDependentFlags().
https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:639: markAncestorChainForDescendantDependentFlagsUpdate(); On 2016/12/01 at 17:59:32, Xianzhu wrote: > This will also set m_needsDescendantDependentFlagsUpdate for |this|. Is this expected? It's intended yes. It's because visible contents updated for |this| in updateDescendantDependentFlags. https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.h:1133: unsigned m_needsDescendantDependentFlagsUpdate : 1; On 2016/12/01 at 17:59:32, Xianzhu wrote: > I'm not sure if the renaming is a good idea. > > updateDescendantDependentFlags() updated both m_isVisibleContentDirty and m_isVisibleDescendantDirty, so both of them seemed descendant (object and layer) dependent flags. I could update the patch to try removing m_isVisibleContentDirty.. > If we want m_needsDescendantDependentFlagsUpdate cover both conditions (like in this patch dirtyVisibleContentStatus unconditionally calling markAncestorChainForDescendantDependentFlagsUpdate), we will do an extra level of traverse into child layers in updateDescendantDependentFlags(). The new code for dirtyVisibleContentStatus will indeed cause recursion from the root PaintLayer down to the one whose visible contents status may have changed. I don't think that is a significant problem, do you?
https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.h:1133: unsigned m_needsDescendantDependentFlagsUpdate : 1; Can this be m_childNeedsDecendantDependengFlagsUpdate?
https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.h:1133: unsigned m_needsDescendantDependentFlagsUpdate : 1; On 2016/12/01 at 18:05:25, Xianzhu wrote: > Can this be m_childNeedsDecendantDependengFlagsUpdate? Seem my comment in response to your one in PaintLayer.cpp about why it's not just child any more.
https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.h:1133: unsigned m_needsDescendantDependentFlagsUpdate : 1; On 2016/12/01 18:06:01, chrishtr wrote: > On 2016/12/01 at 18:05:25, Xianzhu wrote: > > Can this be m_childNeedsDecendantDependengFlagsUpdate? > > Seem my comment in response to your one in PaintLayer.cpp about why it's not > just child any more. Removing m_isVisibleContentDirty SGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/01 at 18:07:03, wangxianzhu wrote: > https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/paint/PaintLayer.h (right): > > https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintLayer.h:1133: unsigned m_needsDescendantDependentFlagsUpdate : 1; > On 2016/12/01 18:06:01, chrishtr wrote: > > On 2016/12/01 at 18:05:25, Xianzhu wrote: > > > Can this be m_childNeedsDecendantDependengFlagsUpdate? > > > > Seem my comment in response to your one in PaintLayer.cpp about why it's not > > just child any more. > > Removing m_isVisibleContentDirty SGTM. LGTM
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...
https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.h (right): https://codereview.chromium.org/2543913002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.h:1133: unsigned m_needsDescendantDependentFlagsUpdate : 1; On 2016/12/01 at 18:07:03, Xianzhu wrote: > On 2016/12/01 18:06:01, chrishtr wrote: > > On 2016/12/01 at 18:05:25, Xianzhu wrote: > > > Can this be m_childNeedsDecendantDependengFlagsUpdate? > > > > Seem my comment in response to your one in PaintLayer.cpp about why it's not > > just child any more. > > Removing m_isVisibleContentDirty SGTM. Done.
lgtm
The CQ bit was unchecked by chrishtr@chromium.org
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2543913002/#ps80001 (title: "none")
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
Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org, wangxianzhu@chromium.org Link to the patchset: https://codereview.chromium.org/2543913002/#ps100001 (title: "none")
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": 100001, "attempt_start_ts": 1480711109758430,
"parent_rev": "4724b14c0fdf956012ce3571cf28943f21a96cab", "commit_rev":
"7d9c17f4150b9bec92ed664f91e5b9338025132a"}
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Generalize visible descendant dirty bits to prepare for more properties. This will allow for a subsequent patch to move other descendant-dependent properties into the same tree walk. Also remove several redundant calls to update descendant-dependent flags (it should happen in exactly one tree walk per lifecycle update) and avoid reading stale state in potentiallyDirtyVisibleContentStatus. BUG=646188 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Generalize visible descendant dirty bits to prepare for more properties. This will allow for a subsequent patch to move other descendant-dependent properties into the same tree walk. Also remove several redundant calls to update descendant-dependent flags (it should happen in exactly one tree walk per lifecycle update) and avoid reading stale state in potentiallyDirtyVisibleContentStatus. BUG=646188 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/6bbd055d68a54c04c41e87814d0d0be323a09ad0 Cr-Commit-Position: refs/heads/master@{#436045} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6bbd055d68a54c04c41e87814d0d0be323a09ad0 Cr-Commit-Position: refs/heads/master@{#436045} |
