Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2266103002:
Repaint when background switches painting from/to scrolling contents layer. (Closed)
|
Created:
4 years, 4 months ago by flackr Modified:
4 years, 3 months 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRepaint when background switches painting from/to scrolling contents layer.
BUG=639886
TEST=paint/invalidation/composited-overflow-local-background-removed.html
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/7f4aaf6e67cb051586ab6f028bfed6411b1893a3
Cr-Commit-Position: refs/heads/master@{#414571}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Update flag and use setNeedsDisplay to dirty scrollingContentsLayer. #
Total comments: 1
Patch Set 3 : Mark scrolling contents layer no longer opaque and setNeedsRepaint and add content to test. #
Total comments: 4
Patch Set 4 : Merge with master. #
Total comments: 4
Patch Set 5 : Rename updateBackgroundPaintingLayer to updateBackgroundPaintsOntoScrollingContentsLayer. #Patch Set 6 : Fix comparison. #Patch Set 7 : Speculative fix for scrollbar drawing difference by compositing reference scroller. #Patch Set 8 : Remove setNeedsRepaint #
Messages
Total messages: 39 (10 generated)
Description was changed from ========== Repaint when background switches painting from/to scrolling contents layer. BUG=639886 TEST=paint/invalidation/composited-overflow-local-background-removed.html ========== to ========== Repaint when background switches painting from/to scrolling contents layer. BUG=639886 TEST=paint/invalidation/composited-overflow-local-background-removed.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
flackr@chromium.org changed reviewers: + schenney@chromium.org
PTAL, this handles repainting when the background becomes no longer painted into the scrolling contents layer.
https://codereview.chromium.org/2266103002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2266103002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:300: if (backgroundLayerPaintsOntoScrollingContentsLayer != m_backgroundPaintsOntoScrollingContentsLayer) You never set m_backgroundPaintsOntoScrollingContentsLayer, as far as I can see. Do it here? I test that switched once then back again would fail, I suspect.
On 2016/08/22 19:16:04, Stephen Chennney wrote: > https://codereview.chromium.org/2266103002/diff/1/third_party/WebKit/Source/c... > File > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp > (right): > > https://codereview.chromium.org/2266103002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:300: > if (backgroundLayerPaintsOntoScrollingContentsLayer != > m_backgroundPaintsOntoScrollingContentsLayer) > You never set m_backgroundPaintsOntoScrollingContentsLayer, as far as I can see. > Do it here? > > I test that switched once then back again would fail, I suspect. Sorry, "A test that switched ..."
https://codereview.chromium.org/2266103002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2266103002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:300: if (backgroundLayerPaintsOntoScrollingContentsLayer != m_backgroundPaintsOntoScrollingContentsLayer) On 2016/08/22 at 19:16:04, Stephen Chennney wrote: > You never set m_backgroundPaintsOntoScrollingContentsLayer, as far as I can see. Do it here? > > I test that switched once then back again would fail, I suspect. Oops, thank you for catching this. I suspect the other tests would have failed as well. It now sets the flag and I've discovered I need to use setNeedsDisplay (similar to the other update* methods in CLM) to dirty the layer.
On 2016/08/22 19:33:25, flackr wrote: > https://codereview.chromium.org/2266103002/diff/1/third_party/WebKit/Source/c... > File > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp > (right): > > https://codereview.chromium.org/2266103002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:300: > if (backgroundLayerPaintsOntoScrollingContentsLayer != > m_backgroundPaintsOntoScrollingContentsLayer) > On 2016/08/22 at 19:16:04, Stephen Chennney wrote: > > You never set m_backgroundPaintsOntoScrollingContentsLayer, as far as I can > see. Do it here? > > > > I test that switched once then back again would fail, I suspect. > > Oops, thank you for catching this. I suspect the other tests would have failed > as well. It now sets the flag and I've discovered I need to use setNeedsDisplay > (similar to the other update* methods in CLM) to dirty the layer. The comment in the code suggests that layer()->compositedLayerMapping()->setScrollingContentsNeedDisplayInRect does the same thing as m_scrollingContentsLayer->setNeedsDisplay(). Is that right? Sorry I didn't ask these things the first time.
Replying doesn't publish comments. How annoying. Append this to the previous comment. https://codereview.chromium.org/2266103002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2266103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:442: layer()->setNeedsRepaint(); I'm concerned that we are missing a layer()->setNeedsRepaint() in the case where we stop painting the background into the scrolling contents. But if seems the test catches that. Do you know why it's not necessary?
On 2016/08/22 at 19:51:47, schenney wrote: > The comment in the code suggests that layer()->compositedLayerMapping()->setScrollingContentsNeedDisplayInRect does the same thing as m_scrollingContentsLayer->setNeedsDisplay(). Is that right? Yes, this is correct. > Replying doesn't publish comments. How annoying. Append this to the previous comment. Also seems not to be attached to the code :( > > https://codereview.chromium.org/2266103002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): > > https://codereview.chromium.org/2266103002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:442: layer()->setNeedsRepaint(); > I'm concerned that we are missing a layer()->setNeedsRepaint() in the case where we stop painting the background into the scrolling contents. But if seems the test catches that. Do you know why it's not necessary? The test behaves differently with content where it still needs to draw something into the layer. We also need to mark the scrolling contents layer as no longer opaque (see updateContentsOpaque), but I don't know why setNeedsRepaint still seems to be unnecessary. I can add this just to be safe in case it is accidentally marked in my case.
ping, ideally I'd like to fix this before M54 branches or else we'll have to merge this or a patch to disable painting into the composited scrolling contents layer back.
chrishtr@chromium.org changed reviewers: + chrishtr@chromium.org
https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:306: if (hasScrollingLayer() && !backgroundLayerPaintsOntoScrollingContentsLayer) { This is not the right place for this invalidation. Instead it should be done via a style update on the PaintLayer, and use the usual mechanisms. Does that not work?
https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:306: if (hasScrollingLayer() && !backgroundLayerPaintsOntoScrollingContentsLayer) { On 2016/08/24 at 00:33:26, chrishtr wrote: > This is not the right place for this invalidation. Instead it should be done via a style update on the PaintLayer, and use > the usual mechanisms. Does that not work? Hmm, PaintLayer::styleDidChange could detect most of the reasons for not being able to paint onto the scrolling contents layer but would it be able to tell when hasNegativeZOrderList had changed? I'm not sure I understand why invalidating here when the background moves between the background layer and scrolling contents layer is different than the setNeedsDisplay calls that happen in CompositedLayerMapping::updateScrollingLayerGeometry. Does the composited layer mapping update happen too late to invalidate?
On 2016/08/24 at 01:34:52, flackr wrote: > https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): > > https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:306: if (hasScrollingLayer() && !backgroundLayerPaintsOntoScrollingContentsLayer) { > On 2016/08/24 at 00:33:26, chrishtr wrote: > > This is not the right place for this invalidation. Instead it should be done via a style update on the PaintLayer, and use > > the usual mechanisms. Does that not work? > > Hmm, PaintLayer::styleDidChange could detect most of the reasons for not being able to paint onto the scrolling contents layer but would it be able to tell when hasNegativeZOrderList had changed? Maybe not.. > I'm not sure I understand why invalidating here when the background moves between the background layer and scrolling contents layer is different than the setNeedsDisplay calls that happen in CompositedLayerMapping::updateScrollingLayerGeometry. Does the composited layer mapping update happen too late to invalidate? No it doesn't. I was hoping to find a way to avoid these complications. Let me look at the CL again and think some more.
https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:300: if (backgroundLayerPaintsOntoScrollingContentsLayer != m_backgroundPaintsOntoScrollingContentsLayer) { What if backgroundLayerPaintsOntoScrollingContentsLayer starts to become true? Doesn't m_owningLayer need to be cleared out then?
https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:300: if (backgroundLayerPaintsOntoScrollingContentsLayer != m_backgroundPaintsOntoScrollingContentsLayer) { On 2016/08/24 at 20:50:35, chrishtr wrote: > What if backgroundLayerPaintsOntoScrollingContentsLayer starts to become true? Doesn't m_owningLayer > need to be cleared out then? It does, but in that case it's handled by LayoutBoxModelObject::setBackingNeedsPaintInvalidationInRect because when we know we're painting the background into the scrolling contents layer we invalidate both layers.
https://codereview.chromium.org/2266103002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2266103002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:298: void CompositedLayerMapping::updateBackgroundPaintingLayer() updateBackgroundPaintsOntoScrollingContentsLayer https://codereview.chromium.org/2266103002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:344: m_scrollingContentsLayer->setContentsOpaque(false); Is this technically an unrelated fix? Is it tested?
https://codereview.chromium.org/2266103002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2266103002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:298: void CompositedLayerMapping::updateBackgroundPaintingLayer() On 2016/08/24 at 23:35:32, chrishtr wrote: > updateBackgroundPaintsOntoScrollingContentsLayer Done. https://codereview.chromium.org/2266103002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:344: m_scrollingContentsLayer->setContentsOpaque(false); On 2016/08/24 at 23:35:32, chrishtr wrote: > Is this technically an unrelated fix? No, this is necessary to remove contents opaque flag when we switch from having a background painted onto the scrolling contents layer to not. > > Is it tested? Yes, in the added test the scrolling contents layer remains opaque (and turns cyan in debug and usually black in release) when the background is no longer painted into it if we don't call setContentsOpaque.
lgtm Ok. I thought about the invalidation, and concluded that your approach is the best one. Thanks for the patience. :)
The CQ bit was checked by flackr@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_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by flackr@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/2266103002/#ps120001 (title: "Speculative fix for scrollbar drawing difference by compositing reference scroller.")
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 chrishtr@chromium.org
Sorry, late update. I just diagnosed a related bug with carets that has the same flavor of problem. I think we might want a different solution. Will update in a few minutes.
I realized after discussing with pdr that there is an additional bug: needsRepaint is set on a per-PaintLayer basis, but in this case the PaintLayer paints into two GraphicsLayers. Thus, needsRepaint may have been reset when painting the first GraphicsLayer before painting the second. I think you need to edit the shouldCreateSubsequence method to disallow subsequence caching when shouldPaintBackgroundOntoScrollingContentsLayer is true.
On 2016/08/25 at 18:07:46, chrishtr wrote: > I realized after discussing with pdr that there is an additional bug: needsRepaint is set > on a per-PaintLayer basis, but in this case the PaintLayer paints into two GraphicsLayers. Thus, needsRepaint may have been reset when painting the first GraphicsLayer before painting the > second. > > I think you need to edit the shouldCreateSubsequence method to disallow subsequence caching > when shouldPaintBackgroundOntoScrollingContentsLayer is true. Naive question: Would this break the optimization that allows us to accelerate gmail's lowdpi overflow scroll if a background is set?
On 2016/08/25 at 18:15:31, pdr wrote: > On 2016/08/25 at 18:07:46, chrishtr wrote: > > I realized after discussing with pdr that there is an additional bug: needsRepaint is set > > on a per-PaintLayer basis, but in this case the PaintLayer paints into two GraphicsLayers. Thus, needsRepaint may have been reset when painting the first GraphicsLayer before painting the > > second. > > > > I think you need to edit the shouldCreateSubsequence method to disallow subsequence caching > > when shouldPaintBackgroundOntoScrollingContentsLayer is true. Hmm, is this not already handled by the compositingState() check? We only paint into the scrolling contents layer on composited scrolling contents. > > Naive question: Would this break the optimization that allows us to accelerate gmail's lowdpi overflow scroll if a background is set? I'm not sure I understand shouldPaintSubsequence, I'm guessing it has to do with caching paints? This change is part of enabling promoting opaque background scrolling contents on low dpi by painting those backgrounds into the scrolling contents layer so we can do subpixel text aa. Here we make sure when we can no longer promote that we restore the scrolling contents layer.
On 2016/08/25 at 18:28:30, flackr wrote: > On 2016/08/25 at 18:15:31, pdr wrote: > > On 2016/08/25 at 18:07:46, chrishtr wrote: > > > I realized after discussing with pdr that there is an additional bug: needsRepaint is set > > > on a per-PaintLayer basis, but in this case the PaintLayer paints into two GraphicsLayers. Thus, needsRepaint may have been reset when painting the first GraphicsLayer before painting the > > > second. > > > > > > I think you need to edit the shouldCreateSubsequence method to disallow subsequence caching > > > when shouldPaintBackgroundOntoScrollingContentsLayer is true. > > Hmm, is this not already handled by the compositingState() check? We only paint into the scrolling contents layer on composited scrolling contents. No it isn't. That state just differentiates between self-composited and squashed. > > > > > Naive question: Would this break the optimization that allows us to accelerate gmail's lowdpi overflow scroll if a background is set? > > I'm not sure I understand shouldPaintSubsequence, I'm guessing it has to do with caching paints? This change is part of enabling promoting opaque background scrolling contents on low dpi by painting those backgrounds into the scrolling contents layer so we can do subpixel text aa. Here we make sure when we can no longer promote that we restore the scrolling contents layer. It has to do with caching paint subsequences. The problem is that the same PaintLayer is painted into two composited layers, and so shouldPaintSubsequence is being called twice. On the second call, withotuo my suggested fix, shouldRepaintSubsequence will be false, since needsRepaint() will have been set to false. This is incorrect, since both need repaint
On 2016/08/25 at 18:42:24, chrishtr wrote: > On 2016/08/25 at 18:28:30, flackr wrote: > > On 2016/08/25 at 18:15:31, pdr wrote: > > > On 2016/08/25 at 18:07:46, chrishtr wrote: > > > > I realized after discussing with pdr that there is an additional bug: needsRepaint is set > > > > on a per-PaintLayer basis, but in this case the PaintLayer paints into two GraphicsLayers. Thus, needsRepaint may have been reset when painting the first GraphicsLayer before painting the > > > > second. > > > > > > > > I think you need to edit the shouldCreateSubsequence method to disallow subsequence caching > > > > when shouldPaintBackgroundOntoScrollingContentsLayer is true. > > > > Hmm, is this not already handled by the compositingState() check? We only paint into the scrolling contents layer on composited scrolling contents. > > No it isn't. That state just differentiates between self-composited and squashed. > > > > > > > > > Naive question: Would this break the optimization that allows us to accelerate gmail's lowdpi overflow scroll if a background is set? > > > > I'm not sure I understand shouldPaintSubsequence, I'm guessing it has to do with caching paints? This change is part of enabling promoting opaque background scrolling contents on low dpi by painting those backgrounds into the scrolling contents layer so we can do subpixel text aa. Here we make sure when we can no longer promote that we restore the scrolling contents layer. > > It has to do with caching paint subsequences. The problem is that the same PaintLayer is painted into two composited layers, and so shouldPaintSubsequence is being called twice. > On the second call, withotuo my suggested fix, shouldRepaintSubsequence will be false, since needsRepaint() will have been set to false. This is incorrect, since both need repaint As discussed, we already return false for composited scrollers (they paint into their own backings), so we don't actually need the setNeedsRepaint call. I have removed this.
The CQ bit was checked by chrishtr@chromium.org
lgtm
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 #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Repaint when background switches painting from/to scrolling contents layer. BUG=639886 TEST=paint/invalidation/composited-overflow-local-background-removed.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Repaint when background switches painting from/to scrolling contents layer. BUG=639886 TEST=paint/invalidation/composited-overflow-local-background-removed.html CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/7f4aaf6e67cb051586ab6f028bfed6411b1893a3 Cr-Commit-Position: refs/heads/master@{#414571} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/7f4aaf6e67cb051586ab6f028bfed6411b1893a3 Cr-Commit-Position: refs/heads/master@{#414571} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
