Repaint when background switches painting from/to scrolling contents layer. (issue 2266103002 by flackr@chromium.org)

fla...@chromium.org

unread,

Aug 22, 2016, 2:59:19 PM8/22/16

to sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

Reviewers: Stephen Chennney
CL: https://codereview.chromium.org/2266103002/

Message:
PTAL, this handles repainting when the background becomes no longer painted into
the scrolling contents layer.

Description:
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

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+37, -8 lines):
A + third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-local-background-removed.html
A third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-local-background-removed-expected.html
M third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
M third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
M third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp

Index: third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-local-background-removed-expected.html
diff --git a/third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-local-background-removed-expected.html b/third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-local-background-removed-expected.html
new file mode 100644
index 0000000000000000000000000000000000000000..89a57b207b990e351125143cd13bed2700afa5c3
--- /dev/null
+++ b/third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-local-background-removed-expected.html
@@ -0,0 +1,15 @@
+<!DOCTYPE html>
+<style>
+#scroller {
+ overflow: scroll;
+ width: 200px;
+ height: 200px;
+}
+
+.spacer {
+ height: 300px;
+}
+</style>
+<div id="scroller">
+ <div class="spacer"></div>
+</div>
Index: third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-local-background-removed.html
diff --git a/third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-local-background.html b/third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-local-background-removed.html
similarity index 72%
copy from third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-local-background.html
copy to third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-local-background-removed.html
index 1f3dd06c2d6ad1ec10dff7363459d535f0858608..9c24e6ac6789840d058d8bae5e2027b603d29831 100644
--- a/third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-local-background.html
+++ b/third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-local-background-removed.html
@@ -2,13 +2,12 @@
<script src="../../fast/repaint/resources/text-based-repaint.js"></script>
<script>
function repaintTest() {
- scroller.style.background = 'green local';
+ scroller.style.background = '';
}
onload = runRepaintAndPixelTest;
</script>
<style>
#scroller {
- background: red local;
overflow: scroll;
width: 200px;
height: 200px;
@@ -20,7 +19,7 @@ onload = runRepaintAndPixelTest;
}
</style>

-<div id="scroller">
+ scrolling contents layer. When the color is removed it should be cleared. -->
+<div id="scroller" style="background: red local;">
<div class="spacer"></div>
</div>
Index: third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
diff --git a/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp b/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
index d83f1633504c0f04c338ede0370b2395be6c27fd..d52fa0529b284cb454a5617c79ef0793aa0f768a 100644
--- a/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
+++ b/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
@@ -434,7 +434,7 @@ void LayoutBoxModelObject::setBackingNeedsPaintInvalidationInRect(const LayoutRe
} else if (object.compositedScrollsWithRespectTo(*this)) {
layer()->compositedLayerMapping()->setScrollingContentsNeedDisplayInRect(r, invalidationReason, object);
} else if (usesCompositedScrolling()) {
- if (layer()->compositedLayerMapping()->shouldPaintBackgroundOntoScrollingContentsLayer()) {
+ if (layer()->compositedLayerMapping()->backgroundPaintsOntoScrollingContentsLayer()) {
// TODO(flackr): Get a correct rect in the context of the scrolling contents layer to update
// rather than updating the entire rect.
const LayoutRect& scrollingContentsRect = toLayoutBox(this)->layoutOverflowRect();
Index: third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
diff --git a/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp b/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
index c070f17b691aaa55202e27bd01b0c065aeb2c101..0e7f4cff92544fb4bd6c322bbb9369d3185b1af8 100644
--- a/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
+++ b/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
@@ -173,6 +173,7 @@ CompositedLayerMapping::CompositedLayerMapping(PaintLayer& layer)
, m_isMainFrameLayoutViewLayer(false)
, m_backgroundLayerPaintsFixedRootBackground(false)
, m_scrollingContentsAreEmpty(false)
+ , m_backgroundPaintsOntoScrollingContentsLayer(false)
{
if (layer.isRootLayer() && layoutObject()->frame()->isMainFrame())
m_isMainFrameLayoutViewLayer = true;
@@ -293,9 +294,15 @@ void CompositedLayerMapping::updateIsRootForIsolatedGroup()
m_graphicsLayer->setIsRootForIsolatedGroup(isolate);
}

+void CompositedLayerMapping::updateBackgroundPaintingLayer()
+{
+ bool backgroundLayerPaintsOntoScrollingContentsLayer = shouldPaintBackgroundOntoScrollingContentsLayer();
+ if (backgroundLayerPaintsOntoScrollingContentsLayer != m_backgroundPaintsOntoScrollingContentsLayer)
+ m_owningLayer.setNeedsRepaint();
+}
+
void CompositedLayerMapping::updateContentsOpaque()
{
- ASSERT(m_isMainFrameLayoutViewLayer || !m_backgroundLayer);
if (isAcceleratedCanvas(layoutObject())) {
// Determine whether the rendering context's external texture layer is opaque.
CanvasRenderingContext* context = toHTMLCanvasElement(layoutObject()->node())->renderingContext();
@@ -311,7 +318,7 @@ void CompositedLayerMapping::updateContentsOpaque()
} else {
// For non-root layers, background is painted by the scrolling contents layer if all backgrounds
// are background attachment local, otherwise background is painted by the primary graphics layer.
- if (hasScrollingLayer() && shouldPaintBackgroundOntoScrollingContentsLayer()) {
+ if (hasScrollingLayer() && m_backgroundPaintsOntoScrollingContentsLayer) {
// Backgrounds painted onto the foreground are clipped by the padding box rect.
// TODO(flackr): This should actually check the entire overflow rect within the
// scrolling contents layer but since we currently only trigger this for solid
@@ -768,6 +775,7 @@ void CompositedLayerMapping::updateGraphicsLayerGeometry(const PaintLayer* compo
updateBackgroundColor();
updateDrawsContent();
updateElementIdAndCompositorMutableProperties();
+ updateBackgroundPaintingLayer();
updateContentsOpaque();
updateAfterPartResize();
updateRenderingContext();
@@ -2460,7 +2468,7 @@ void CompositedLayerMapping::paintContents(const GraphicsLayer* graphicsLayer, G
|| graphicsLayer == m_childClippingMaskLayer.get()
|| graphicsLayer == m_scrollingContentsLayer.get()) {

- bool paintRootBackgroundOntoScrollingContentsLayer = shouldPaintBackgroundOntoScrollingContentsLayer();
+ bool paintRootBackgroundOntoScrollingContentsLayer = m_backgroundPaintsOntoScrollingContentsLayer;
DCHECK(!paintRootBackgroundOntoScrollingContentsLayer || (!m_backgroundLayer && !m_foregroundLayer));
if (paintRootBackgroundOntoScrollingContentsLayer) {
if (graphicsLayer == m_scrollingContentsLayer.get())
Index: third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
diff --git a/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h b/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
index bd7baaed155b2d2f464dc65f7a080baf4aa48cbb..6afcfbc014411007dc9d79d12102f1aae8dcc39f 100644
--- a/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
+++ b/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.h
@@ -87,6 +87,9 @@ public:
bool updateGraphicsLayerConfiguration();
void updateGraphicsLayerGeometry(const PaintLayer* compositingContainer, const PaintLayer* compositingStackingContext, Vector<PaintLayer*>& layersNeedingPaintInvalidation);

+ // Update whether background paints onto scrolling contents layer.
+ void updateBackgroundPaintingLayer();
+
// Update whether layer needs blending.
void updateContentsOpaque();

@@ -223,6 +226,7 @@ public:
// opaque this allows us to composite the scroller even on low DPI as we can
// draw with subpixel anti-aliasing.
bool shouldPaintBackgroundOntoScrollingContentsLayer() const;
+ bool backgroundPaintsOntoScrollingContentsLayer() { return m_backgroundPaintsOntoScrollingContentsLayer; }

private:
IntRect recomputeInterestRect(const GraphicsLayer*) const;
@@ -461,6 +465,9 @@ private:
unsigned m_backgroundLayerPaintsFixedRootBackground : 1;
unsigned m_scrollingContentsAreEmpty : 1;

+ // Keep track of whether the background is painted onto the scrolling contents layer for invalidations.
+ unsigned m_backgroundPaintsOntoScrollingContentsLayer : 1;
+
friend class CompositedLayerMappingTest;
};

sche...@chromium.org

unread,

Aug 22, 2016, 3:16:05 PM8/22/16

to fla...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

https://codereview.chromium.org/2266103002/diff/1/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2266103002/diff/1/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode300
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.

https://codereview.chromium.org/2266103002/

sche...@chromium.org

unread,

Aug 22, 2016, 3:16:33 PM8/22/16

to fla...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

Sorry, "A test that switched ..."

https://codereview.chromium.org/2266103002/

fla...@chromium.org

unread,

Aug 22, 2016, 3:33:25 PM8/22/16

to sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

https://codereview.chromium.org/2266103002/diff/1/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2266103002/diff/1/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode300
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.

https://codereview.chromium.org/2266103002/

sche...@chromium.org

unread,

Aug 22, 2016, 3:51:08 PM8/22/16

to fla...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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.

https://codereview.chromium.org/2266103002/

sche...@chromium.org

unread,

Aug 22, 2016, 3:51:48 PM8/22/16

to fla...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

Replying doesn't publish comments. How annoying. Append this to the previous
comment.

https://codereview.chromium.org/2266103002/diff/20001/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp
(right):

https://codereview.chromium.org/2266103002/diff/20001/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp#newcode442
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?

https://codereview.chromium.org/2266103002/

fla...@chromium.org

unread,

Aug 23, 2016, 9:10:07 AM8/23/16

to sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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/Source/core/layout/LayoutBoxModelObject.cpp
> File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):
>
>
https://codereview.chromium.org/2266103002/diff/20001/third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp#newcode442
> 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.

https://codereview.chromium.org/2266103002/

fla...@chromium.org

unread,

Aug 23, 2016, 2:53:54 PM8/23/16

to sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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.

https://codereview.chromium.org/2266103002/

chri...@chromium.org

unread,

Aug 23, 2016, 8:33:26 PM8/23/16

to fla...@chromium.org, sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode306
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/

fla...@chromium.org

unread,

Aug 23, 2016, 9:34:52 PM8/23/16

to chri...@chromium.org, sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode306
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?

https://codereview.chromium.org/2266103002/

chri...@chromium.org

unread,

Aug 24, 2016, 12:30:47 PM8/24/16

to fla...@chromium.org, sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

On 2016/08/24 at 01:34:52, flackr wrote:
>
https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
> File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):
>
>
https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode306
>
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/

chri...@chromium.org

unread,

Aug 24, 2016, 4:50:36 PM8/24/16

to fla...@chromium.org, sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode300

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/

fla...@chromium.org

unread,

Aug 24, 2016, 5:01:11 PM8/24/16

to chri...@chromium.org, sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2266103002/diff/40001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode300
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/

chri...@chromium.org

unread,

Aug 24, 2016, 7:35:32 PM8/24/16

to fla...@chromium.org, sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

https://codereview.chromium.org/2266103002/diff/60001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2266103002/diff/60001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode298
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:298:
void CompositedLayerMapping::updateBackgroundPaintingLayer()
updateBackgroundPaintsOntoScrollingContentsLayer

https://codereview.chromium.org/2266103002/diff/60001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode344
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/

fla...@chromium.org

unread,

Aug 25, 2016, 11:21:53 AM8/25/16

to chri...@chromium.org, sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

https://codereview.chromium.org/2266103002/diff/60001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
File
third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
(right):

https://codereview.chromium.org/2266103002/diff/60001/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode298
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/Source/core/layout/compositing/CompositedLayerMapping.cpp#newcode344
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.

https://codereview.chromium.org/2266103002/

chri...@chromium.org

unread,

Aug 25, 2016, 11:25:15 AM8/25/16

to fla...@chromium.org, sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

lgtm

Ok. I thought about the invalidation, and concluded that your approach is the
best one.
Thanks for the patience. :)

https://codereview.chromium.org/2266103002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Aug 25, 2016, 11:26:12 AM8/25/16

to fla...@chromium.org, chri...@chromium.org, sche...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2266103002/80001

https://codereview.chromium.org/2266103002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Aug 25, 2016, 12:03:18 PM8/25/16

to fla...@chromium.org, chri...@chromium.org, sche...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.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_ng/builds/281102)

https://codereview.chromium.org/2266103002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Aug 25, 2016, 1:04:23 PM8/25/16

to fla...@chromium.org, chri...@chromium.org, sche...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2266103002/120001

https://codereview.chromium.org/2266103002/

chri...@chromium.org

unread,

Aug 25, 2016, 1:47:09 PM8/25/16

to fla...@chromium.org, sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.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.

https://codereview.chromium.org/2266103002/

chri...@chromium.org

unread,

Aug 25, 2016, 2:07:47 PM8/25/16

to fla...@chromium.org, sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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.

https://codereview.chromium.org/2266103002/

p...@chromium.org

unread,

Aug 25, 2016, 2:15:32 PM8/25/16

to fla...@chromium.org, chri...@chromium.org, sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

Naive question: Would this break the optimization that allows us to accelerate
gmail's lowdpi overflow scroll if a background is set?

https://codereview.chromium.org/2266103002/

fla...@chromium.org

unread,

Aug 25, 2016, 2:28:31 PM8/25/16

to chri...@chromium.org, sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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.

https://codereview.chromium.org/2266103002/

chri...@chromium.org

unread,

Aug 25, 2016, 2:42:24 PM8/25/16

to fla...@chromium.org, sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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

https://codereview.chromium.org/2266103002/

fla...@chromium.org

unread,

Aug 25, 2016, 2:55:40 PM8/25/16

to chri...@chromium.org, sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

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.

https://codereview.chromium.org/2266103002/

chri...@chromium.org

unread,

Aug 25, 2016, 2:58:34 PM8/25/16

to fla...@chromium.org, sche...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

lgtm

https://codereview.chromium.org/2266103002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Aug 25, 2016, 3:04:31 PM8/25/16

to fla...@chromium.org, chri...@chromium.org, sche...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2266103002/140001

https://codereview.chromium.org/2266103002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Aug 25, 2016, 6:55:33 PM8/25/16

to fla...@chromium.org, chri...@chromium.org, sche...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

Committed patchset #8 (id:140001)

https://codereview.chromium.org/2266103002/

commit-bot@chromium.org via codereview.chromium.org

unread,

Aug 25, 2016, 6:57:20 PM8/25/16

to fla...@chromium.org, chri...@chromium.org, sche...@chromium.org, commi...@chromium.org, blink-...@chromium.org, blink-revi...@chromium.org, chromium...@chromium.org, eae+bli...@chromium.org, jchaffraix...@chromium.org, leviw+re...@chromium.org, pdr+renderi...@chromium.org, szager+la...@chromium.org, zol...@webkit.org

Patchset 8 (id:??) landed as
https://crrev.com/7f4aaf6e67cb051586ab6f028bfed6411b1893a3
Cr-Commit-Position: refs/heads/master@{#414571}

https://codereview.chromium.org/2266103002/

Reply all

Reply to author

Forward