third_party/WebKit/Source/core/paint/BoxPainter.cpp - Issue 2254893005: Fix visual rect for background box painting in composited scrollers.

Unified Diff: third_party/WebKit/Source/core/paint/BoxPainter.cpp

Issue 2254893005: Fix visual rect for background box painting in composited scrollers. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Minor cleanup. Created 4 years, 4 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« no previous file with comments | « third_party/WebKit/LayoutTests/fast/scroll-behavior/overflow-scroll-with-local-background-and-text-expected.html ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: third_party/WebKit/Source/core/paint/BoxPainter.cpp

diff --git a/third_party/WebKit/Source/core/paint/BoxPainter.cpp b/third_party/WebKit/Source/core/paint/BoxPainter.cpp

index 15c4f102bfa1a638d8c264b848300c684fbaba0b..b6612b49a3b5fdcef6f958668968f9535e676054 100644

--- a/third_party/WebKit/Source/core/paint/BoxPainter.cpp

+++ b/third_party/WebKit/Source/core/paint/BoxPainter.cpp

@@ -109,11 +109,11 @@ void BoxPainter::paintBoxDecorationBackgroundWithRect(const PaintInfo& paintInfo

if (style.appearance() == MediaSliderPart)

cacheSkipper.emplace(paintInfo.context);

- if (LayoutObjectDrawingRecorder::useCachedDrawingIfPossible(paintInfo.context, m_layoutBox, DisplayItem::BoxDecorationBackground))

+ const DisplayItemClient& displayItemClient = paintingOverflowContents ? static_cast<const DisplayItemClient&>(*m_layoutBox.layer()->compositedLayerMapping()->scrollingContentsLayer()) : m_layoutBox;

Xianzhu 2016/08/17 23:28:48 Can you make sure if paint invalidation work for t

wkorman 2016/08/18 17:26:23 Hmm, it does not, and it does at ToT. Paint flashi

wkorman 2016/08/18 18:02:02 Invalidation not working is specific to 'local' be

flackr 2016/08/18 19:03:39 Invalidations were supposed to be fixed by https:/

wkorman 2016/08/18 19:46:00 OK, that change is helpful re: involved code, than

wkorman 2016/08/18 21:16:59 So it looks like we're invalidating the correct re

On 2016/08/18 at 19:46:00, wkorman wrote: > On 2016/08/18 at 19:03:39, flackr wrote: > > On 2016/08/18 at 18:02:02, wkorman wrote: > > > On 2016/08/18 at 17:26:23, wkorman wrote: > > > > On 2016/08/17 at 23:28:48, Xianzhu wrote: > > > > > Can you make sure if paint invalidation work for this change? For example, when m_layoutBox's background changes? > > > > > > > > Hmm, it does not, and it does at ToT. Paint flashing does look like it's invalidating the right areas and logging doesn't seem to show we're using a cached item improperly. > > > > > > > > Also, the paint flashing for the composited scroller is now the full 550px height of the graphics layer whereas at ToT it's only the ~200px height of the visible area, though I'm not sure this is a big deal as things probably clip. > > > > > > > > Looking further but if you have thoughts on cause or alternate approach, do tell! > > > > > > Invalidation not working is specific to 'local' being set in 'background' style. If one only specifies a color and leaves attachment as default, invalidation works. So, perhaps related to flackr@ work in http://crrev.com/2196583002. > > > > Invalidations were supposed to be fixed by https://cr-rev.appspot.com/411441 which adds a test to verify it invalidates the entire region of the composited overflow scroller. > > OK, that change is helpful re: involved code, thanks. The added test paint/invalidation/composited-overflow-with-local-background.html passes with a release build with this patch but is visually incorrect. The scroller content remains red when it should have been repainted as green. The text expectations must match however, and the repaint test doesn't include pixel expectations, which we may want to change. > > That test does fail on the trybots or debug build with under-invalidation check enabled, see sample output: > > https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r... > > So, like, probably we should actually look into the details of that under-invalidation. :) Proceeding...

So it looks like we're invalidating the correct rect on correct layer via setScrollingContentsNeedDisplayInRect() which ends up in GraphicsLayer::setNeedsDisplayInRect() and is in fact calling m_layer->layer()->invalidateRect(rect). But we're failing under-invalidation, excerpt as: [1:1:0818/141139:1395034311807:ERROR:PaintController.cpp(526)] New display item: "{client: \"0x7d549542d20 Scrolling Contents Layer\", type: \"DrawingBoxDecorationBackground\", rect: [0.000000,0.000000 185.000000x300.000000]}" [1:1:0818/141139:1395034311975:ERROR:PaintController.cpp(527)] Old display item: "{client: \"0x7d549542d20 Scrolling Contents Layer\", type: \"DrawingBoxDecorationBackground\", rect: [0.000000,0.000000 185.000000x300.000000]}" and the picture bitmaps don't match due to pixel color change -- the old pixels are red and the new are green, which is what we would expect since we've changed the background color. I'm not sure how rect invalidation interacts with object/layer invalidation, so am still researching.

+ if (DrawingRecorder::useCachedDrawingIfPossible(paintInfo.context, displayItemClient, DisplayItem::BoxDecorationBackground))

return;

- LayoutObjectDrawingRecorder recorder(paintInfo.context, m_layoutBox, DisplayItem::BoxDecorationBackground, boundsForDrawingRecorder(paintInfo, paintOffset));

+ DrawingRecorder recorder(paintInfo.context, displayItemClient, DisplayItem::BoxDecorationBackground, FloatRect(boundsForDrawingRecorder(paintInfo, paintOffset)));

BoxDecorationData boxDecorationData(m_layoutBox);

GraphicsContextStateSaver stateSaver(paintInfo.context, false);