Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2297873002:
Remove unnecessary checks in PaintLayer::backgroundIsKnownToBeOpaqueInRect (Closed)
|
Created:
4 years, 3 months ago by Stephen Chennney Modified:
4 years, 3 months ago Reviewers:
chrishtr CC:
blink-reviews, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, slimming-paint-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove unnecessary checks in PaintLayer::backgroundIsKnownToBeOpaqueInRect
The PaintLayer::backgroundIsKnownToBeOpaque method is currently
only called by GraphicsLayers on their owning paint layer, or
by the recursive child call. The former is by definition
always called on a self painting layer, while the latter
already has a test to prevent consideration of layers that
do not paint into the backing in question. Hence we can
remove the check for self painting layer status.
Also convert a test for dirty stacking order lists to a
DCHECK, because the test never fires on any layout test.
These checks cause chicken-and-egg problems when the method
is called by the layer itself in order to identify compositing
reasons. In such cases we are not concerned with self layer
status; we are in fact trying to determine that status.
R=chrishtr@chromium.org
BUG=381840
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/a879db6c440ffd9c296b1a3f272458212eac48b5
Cr-Commit-Position: refs/heads/master@{#416628}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Fix spelling #
Total comments: 1
Patch Set 3 : Rebased. #Messages
Total messages: 37 (21 generated)
Description was changed from ========== Remove unnecessary checks in PaintLayer::backgroundIsKnownToBeOpaque The PaintLayer::backgroundIsKnownToBeOpaque method is currently only called by GraphicsLayers on their owning paint layer, or by the recursive child call. The former is by definition always called on a delf painting layer, while the latter already has a test to prevent consideration of layers that do not paint into the backing in question. Hence we can remove the check for self painting layer status. Also convert a test for dirty stacking order lists to a DCHECK, because the test never fires on any layout test. These checks cause chicken-and-egg problems when the method is called by the layer itself in order to identify compositing reasons. In such cases we are not concerned with self layer status; we are in fact trying to determine that status. R=chrishtr@chromium.org BUG=381840 ========== to ========== Remove unnecessary checks in PaintLayer::backgroundIsKnownToBeOpaque The PaintLayer::backgroundIsKnownToBeOpaque method is currently only called by GraphicsLayers on their owning paint layer, or by the recursive child call. The former is by definition always called on a delf painting layer, while the latter already has a test to prevent consideration of layers that do not paint into the backing in question. Hence we can remove the check for self painting layer status. Also convert a test for dirty stacking order lists to a DCHECK, because the test never fires on any layout test. These checks cause chicken-and-egg problems when the method is called by the layer itself in order to identify compositing reasons. In such cases we are not concerned with self layer status; we are in fact trying to determine that status. R=chrishtr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by schenney@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...
Clean up in preparation for compositing opaque scrollers. Should be no change in behavior based on local testings.
Description was changed from ========== Remove unnecessary checks in PaintLayer::backgroundIsKnownToBeOpaque The PaintLayer::backgroundIsKnownToBeOpaque method is currently only called by GraphicsLayers on their owning paint layer, or by the recursive child call. The former is by definition always called on a delf painting layer, while the latter already has a test to prevent consideration of layers that do not paint into the backing in question. Hence we can remove the check for self painting layer status. Also convert a test for dirty stacking order lists to a DCHECK, because the test never fires on any layout test. These checks cause chicken-and-egg problems when the method is called by the layer itself in order to identify compositing reasons. In such cases we are not concerned with self layer status; we are in fact trying to determine that status. R=chrishtr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Remove unnecessary checks in PaintLayer::backgroundIsKnownToBeOpaque The PaintLayer::backgroundIsKnownToBeOpaque method is currently only called by GraphicsLayers on their owning paint layer, or by the recursive child call. The former is by definition always called on a self painting layer, while the latter already has a test to prevent consideration of layers that do not paint into the backing in question. Hence we can remove the check for self painting layer status. Also convert a test for dirty stacking order lists to a DCHECK, because the test never fires on any layout test. These checks cause chicken-and-egg problems when the method is called by the layer itself in order to identify compositing reasons. In such cases we are not concerned with self layer status; we are in fact trying to determine that status. R=chrishtr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Remove unnecessary checks in PaintLayer::backgroundIsKnownToBeOpaque The PaintLayer::backgroundIsKnownToBeOpaque method is currently only called by GraphicsLayers on their owning paint layer, or by the recursive child call. The former is by definition always called on a self painting layer, while the latter already has a test to prevent consideration of layers that do not paint into the backing in question. Hence we can remove the check for self painting layer status. Also convert a test for dirty stacking order lists to a DCHECK, because the test never fires on any layout test. These checks cause chicken-and-egg problems when the method is called by the layer itself in order to identify compositing reasons. In such cases we are not concerned with self layer status; we are in fact trying to determine that status. R=chrishtr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Remove unnecessary checks in PaintLayer::backgroundIsKnownToBeOpaqueInRect The PaintLayer::backgroundIsKnownToBeOpaque method is currently only called by GraphicsLayers on their owning paint layer, or by the recursive child call. The former is by definition always called on a self painting layer, while the latter already has a test to prevent consideration of layers that do not paint into the backing in question. Hence we can remove the check for self painting layer status. Also convert a test for dirty stacking order lists to a DCHECK, because the test never fires on any layout test. These checks cause chicken-and-egg problems when the method is called by the layer itself in order to identify compositing reasons. In such cases we are not concerned with self layer status; we are in fact trying to determine that status. R=chrishtr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
https://codereview.chromium.org/2297873002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (left): https://codereview.chromium.org/2297873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2400: if (!isSelfPaintingLayer() && !hasSelfPaintingLayerDescendant()) childBackgroundIsKnownToBeOpaqueInRect calls backgroundIsKnownToBeOpaqueInRect on all stacking-children PaintLayers that paint into the same backing. They don't have to be self-painting, right?
https://codereview.chromium.org/2297873002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (left): https://codereview.chromium.org/2297873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2400: if (!isSelfPaintingLayer() && !hasSelfPaintingLayerDescendant()) On 2016/08/30 20:49:01, chrishtr wrote: > childBackgroundIsKnownToBeOpaqueInRect calls backgroundIsKnownToBeOpaqueInRect > on all stacking-children > PaintLayers that paint into the same backing. They don't have to be > self-painting, right? Right, in fact we only want the ones that are not self painting, because self painters contribute to a different backing. That check already exists in childBackgroundIsKnownToBeOpaqueInRect, in the form of childLayer->isPaintInvalidationContainer()
https://codereview.chromium.org/2297873002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (left): https://codereview.chromium.org/2297873002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2400: if (!isSelfPaintingLayer() && !hasSelfPaintingLayerDescendant()) On 2016/08/30 at 20:58:51, Stephen Chennney wrote: > On 2016/08/30 20:49:01, chrishtr wrote: > > childBackgroundIsKnownToBeOpaqueInRect calls backgroundIsKnownToBeOpaqueInRect > > on all stacking-children > > PaintLayers that paint into the same backing. They don't have to be > > self-painting, right? > > Right, in fact we only want the ones that are not self painting, because self painters contribute to a different backing. That check already exists in childBackgroundIsKnownToBeOpaqueInRect, in the form of childLayer->isPaintInvalidationContainer() No. self-painting is not the same thing as independently composited. self-painting means something sort of like stacking context.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
On 2016/08/30 21:15:39, chrishtr wrote: > https://codereview.chromium.org/2297873002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/paint/PaintLayer.cpp (left): > > https://codereview.chromium.org/2297873002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/paint/PaintLayer.cpp:2400: if > (!isSelfPaintingLayer() && !hasSelfPaintingLayerDescendant()) > On 2016/08/30 at 20:58:51, Stephen Chennney wrote: > > On 2016/08/30 20:49:01, chrishtr wrote: > > > childBackgroundIsKnownToBeOpaqueInRect calls > backgroundIsKnownToBeOpaqueInRect > > > on all stacking-children > > > PaintLayers that paint into the same backing. They don't have to be > > > self-painting, right? > > > > Right, in fact we only want the ones that are not self painting, because self > painters contribute to a different backing. That check already exists in > childBackgroundIsKnownToBeOpaqueInRect, in the form of > childLayer->isPaintInvalidationContainer() > > No. self-painting is not the same thing as independently composited. > self-painting means something > sort of like stacking context. My bad for not looking more carefully. However, the logic is still right because childLayer->isPaintInvalidationContainer() is in practice a check that the child paints into a layer other than its ancestor's: bool PaintLayer::isPaintInvalidationContainer() const { return compositingState() == PaintsIntoOwnBacking || compositingState() == PaintsIntoGroupedBacking; } This code is used to determine GraphicsLayer opacity, so whether PaintLayers in question paint into the initial caller's GraphicsLayer is all that matters beyond what area is opaque for a given layer.
The CQ bit was checked by schenney@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...
On 2016/08/31 at 17:08:51, schenney wrote: > On 2016/08/30 21:15:39, chrishtr wrote: > > https://codereview.chromium.org/2297873002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/paint/PaintLayer.cpp (left): > > > > https://codereview.chromium.org/2297873002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/paint/PaintLayer.cpp:2400: if > > (!isSelfPaintingLayer() && !hasSelfPaintingLayerDescendant()) > > On 2016/08/30 at 20:58:51, Stephen Chennney wrote: > > > On 2016/08/30 20:49:01, chrishtr wrote: > > > > childBackgroundIsKnownToBeOpaqueInRect calls > > backgroundIsKnownToBeOpaqueInRect > > > > on all stacking-children > > > > PaintLayers that paint into the same backing. They don't have to be > > > > self-painting, right? > > > > > > Right, in fact we only want the ones that are not self painting, because self > > painters contribute to a different backing. That check already exists in > > childBackgroundIsKnownToBeOpaqueInRect, in the form of > > childLayer->isPaintInvalidationContainer() > > > > No. self-painting is not the same thing as independently composited. > > self-painting means something > > sort of like stacking context. > > My bad for not looking more carefully. However, the logic is still right because childLayer->isPaintInvalidationContainer() is in practice a check that the child paints into a layer other than its ancestor's: > > bool PaintLayer::isPaintInvalidationContainer() const > { > return compositingState() == PaintsIntoOwnBacking || compositingState() == PaintsIntoGroupedBacking; > } > > This code is used to determine GraphicsLayer opacity, so whether PaintLayers in question paint into the initial caller's GraphicsLayer is all that matters beyond what area is opaque for a given layer. backgroundIsKnownToBeOpaqueInRect will still get called for non-self-painting layers that belong to the same backing. It's not valid to ask all the questions in backgroundIsKnownToBeOpaqueInRect on such layers, because they don't paint themselves, their self-painting PaintLayer ancestor does.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/08/31 18:40:41, chrishtr wrote: > On 2016/08/31 at 17:08:51, schenney wrote: > > On 2016/08/30 21:15:39, chrishtr wrote: > > > > https://codereview.chromium.org/2297873002/diff/1/third_party/WebKit/Source/c... > > > File third_party/WebKit/Source/core/paint/PaintLayer.cpp (left): > > > > > > > https://codereview.chromium.org/2297873002/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/paint/PaintLayer.cpp:2400: if > > > (!isSelfPaintingLayer() && !hasSelfPaintingLayerDescendant()) > > > On 2016/08/30 at 20:58:51, Stephen Chennney wrote: > > > > On 2016/08/30 20:49:01, chrishtr wrote: > > > > > childBackgroundIsKnownToBeOpaqueInRect calls > > > backgroundIsKnownToBeOpaqueInRect > > > > > on all stacking-children > > > > > PaintLayers that paint into the same backing. They don't have to be > > > > > self-painting, right? > > > > > > > > Right, in fact we only want the ones that are not self painting, because > self > > > painters contribute to a different backing. That check already exists in > > > childBackgroundIsKnownToBeOpaqueInRect, in the form of > > > childLayer->isPaintInvalidationContainer() > > > > > > No. self-painting is not the same thing as independently composited. > > > self-painting means something > > > sort of like stacking context. > > > > My bad for not looking more carefully. However, the logic is still right > because childLayer->isPaintInvalidationContainer() is in practice a check that > the child paints into a layer other than its ancestor's: > > > > bool PaintLayer::isPaintInvalidationContainer() const > > { > > return compositingState() == PaintsIntoOwnBacking || compositingState() == > PaintsIntoGroupedBacking; > > } > > > > This code is used to determine GraphicsLayer opacity, so whether PaintLayers > in question paint into the initial caller's GraphicsLayer is all that matters > beyond what area is opaque for a given layer. > > backgroundIsKnownToBeOpaqueInRect will still get called for non-self-painting > layers that belong to the same backing. It's not valid to ask all the questions > in backgroundIsKnownToBeOpaqueInRect on such layers, because they don't paint > themselves, their self-painting PaintLayer ancestor does. Is the problem that non-self-painting-layer's layoutObjects paint into a selfPaintingLayer that is not an ancestor in the layer tree? That implies that an ancestor layer's layoutObject is not your own layoutObject's ancestor in the layout tree, which is not inconceivable to me. Or are you concerned about the various paintsWith... checks in backgroundIsKnownToBeOpaqueInRect? Those are concerned with features that, for non-composited layers, will be true more often and hence report the child as not-opaque in more cases. We could leave this code as is and instead workaround the needsCompositedScrolling () chicken-and-egg problem using extra parameters to backgroundIsKnownToBeOpaqueInRect, or similar. But it seems worth figuring out exactly what's going on here, particularly since all our testing passes with or without the change. That in itself is concerning if this patch really is broken. In particular, a CHECK on the !isSelfpaintingLayer && !hasSelfPaintingLayerDecendent did not fire at all in testing. i.e. the code I'm removing never changed the behavior of the system in any unit or layout test.
Ok. Fair points about it being safe enough for non-self-painting layers. Last question: how does this chicken-egg bug manifest itself in your work? Which test fails? https://codereview.chromium.org/2297873002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/2297873002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2416: DCHECK(!m_stackingNode->zOrderListsDirty()); If the PaintLayer is not stacking, m_stackingNode will be null.
Patchset #3 (id:100001) has been deleted
On 2016/09/01 23:24:59, chrishtr wrote: > Ok. Fair points about it being safe enough for non-self-painting layers. > Last question: how does this chicken-egg bug manifest itself in your work? Which > test > fails? The layout tests in the composited scrolling patch all fail to composite scrollers with this sequence of calls: PaintLayerScrollableArea::updateNeedsCompositedScrolling wants to set a flag m_needsCompositedScrolling, so calls layerNeedsCompositedScrolling calls PaintLayer::backgroundIsKnownToBeOpaqueInRect calls PaintLayer::isSelfPaintingLayer calls PaintLayer::updateSelfPaintingLayer calls PaintLayer::shouldBeSelfPaintingLayer calls PaintLayer::isSelfPaintingLayerForIntrinsicOrScrollingReasons calls PaintLayer::needsCompositedScrolling calls PaintLayerScrollableArea::needsCompositedScrolling which looks at the flag and returns false because the flag has not been updated yet, so the whole chain reports false and the flag is never set for opaque backgrounds. Interestingly, without flackr's change to add the flag we would have infinite looped. > https://codereview.chromium.org/2297873002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): > > https://codereview.chromium.org/2297873002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/paint/PaintLayer.cpp:2416: > DCHECK(!m_stackingNode->zOrderListsDirty()); > If the PaintLayer is not stacking, m_stackingNode will be null. m_stackingNode is never null. updateStackingNode is called by the constructor which calls requiresStackingNode which always returns true (with a FIXME to make it smarter). All the code in PaintLayer assumes m_stackingNode exists so if we ever change it we'll need to change all the sites that use m_stackingNode. It's almost certainly faster to just create it and never have to check for its existence, which is why it has probably never been fixed.
lgtm
The CQ bit was checked by chrishtr@chromium.org
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #3 (id:120001) has been deleted
The CQ bit was checked by schenney@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/2297873002/#ps140001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Remove unnecessary checks in PaintLayer::backgroundIsKnownToBeOpaqueInRect The PaintLayer::backgroundIsKnownToBeOpaque method is currently only called by GraphicsLayers on their owning paint layer, or by the recursive child call. The former is by definition always called on a self painting layer, while the latter already has a test to prevent consideration of layers that do not paint into the backing in question. Hence we can remove the check for self painting layer status. Also convert a test for dirty stacking order lists to a DCHECK, because the test never fires on any layout test. These checks cause chicken-and-egg problems when the method is called by the layer itself in order to identify compositing reasons. In such cases we are not concerned with self layer status; we are in fact trying to determine that status. R=chrishtr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Remove unnecessary checks in PaintLayer::backgroundIsKnownToBeOpaqueInRect The PaintLayer::backgroundIsKnownToBeOpaque method is currently only called by GraphicsLayers on their owning paint layer, or by the recursive child call. The former is by definition always called on a self painting layer, while the latter already has a test to prevent consideration of layers that do not paint into the backing in question. Hence we can remove the check for self painting layer status. Also convert a test for dirty stacking order lists to a DCHECK, because the test never fires on any layout test. These checks cause chicken-and-egg problems when the method is called by the layer itself in order to identify compositing reasons. In such cases we are not concerned with self layer status; we are in fact trying to determine that status. R=chrishtr@chromium.org BUG=381840 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/a879db6c440ffd9c296b1a3f272458212eac48b5 Cr-Commit-Position: refs/heads/master@{#416628} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a879db6c440ffd9c296b1a3f272458212eac48b5 Cr-Commit-Position: refs/heads/master@{#416628}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:140001) has been created in https://codereview.chromium.org/2315693003/ by lushnikov@chromium.org. The reason for reverting is: This makes Win Tests builder to hit the DCHECK, e.g.: https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2... . |
