Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2254893005:
Fix visual rect for background box painting in composited scrollers. (Closed)
|
Created:
4 years, 4 months ago by wkorman Modified:
4 years, 4 months ago CC:
blink-reviews, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, slimming-paint-reviews_chromium.org, Xianzhu Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix visual rect for background box painting in composited scrollers.
Use the scrolling contents GraphicsLayer as the DisplayItemClient for the
background drawing when we're painting the full background of a composited
scroller.
BUG=638611
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/ff27367de0553a7aa38f078e528d19024f2d3668
Cr-Commit-Position: refs/heads/master@{#413340}
Patch Set 1 #Patch Set 2 : Add tests. #Patch Set 3 : Minor cleanup. #
Total comments: 6
Patch Set 4 : Invalidate scrolling contents layer and mark paint layer setNeedsRepaint. #
Total comments: 8
Patch Set 5 : Fix up reference test. #Patch Set 6 : Deal with delayed-invalidation object. #
Total comments: 6
Messages
Total messages: 52 (28 generated)
Description was changed from ========== Fix visual rect for background box painting in composited scrollers. BUG=638611 ========== to ========== Fix visual rect for background box painting in composited scrollers. BUG=638611 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Fix visual rect for background box painting in composited scrollers. BUG=638611 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix visual rect for background box painting in composited scrollers. Use the scrolling contents GraphicsLayer as the DisplayItemClient for the background drawing when we're painting the full background of a composited scroller. BUG=638611 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
wkorman@chromium.org changed reviewers: + flackr@chromium.org, wangxianzhu@chromium.org
For discussion. This fixes the visual rect and so flackr@'s test but my
familiarity with all common scroller permutations and implementation details is
still limited, so open to other suggestions.
The cull rect for the background drawing does not match the visual rect. It has
an x/y offset presumably from the border, which seems wrong to me in the
composited case. See #3 below. Perhaps we should fix this, but drawing cull
rects could be obsolete with rtree queries performing same function, in which
case this won't matter.
current display item list: [{index: 0, client: "0x22d235342d20 Scrolling
Contents Layer", type: "BeginTransform", transform:
[1.000000,0.000000,0.000000,1.000000,-10.000000,-10.000000], cacheIsValid: true,
visualRect: [0,0 185x550]},
{index: 1, client: "0x22d235342d20 Scrolling Contents Layer", type:
"DrawingBoxDecorationBackground", rect: [10.000000,10.000000
185.000000x550.000000], cacheIsValid: true, visualRect: [0,0 185x550]},
{index: 2, client: "0x2ffc91224248 LayoutBlockFlow DIV id='scroller'", type:
"ScrollPaintPhaseForeground", currentOffset: [0,0], cacheIsValid: false,
visualRect: [-10,-10 220x220]},
{index: 3, client: "0x2ffc91240010 InlineTextBox 'Text content'", type:
"DrawingPaintPhaseForeground", rect: [10.000000,526.000000 80.000000x17.000000],
cacheIsValid: true, visualRect: [0,516 80x17]},
{index: 4, client: "0x2ffc91224248 LayoutBlockFlow DIV id='scroller'", type:
"EndScrollPaintPhaseForeground", cacheIsValid: false, visualRect: [-10,-10
220x220]},
{index: 5, client: "0x22d235342d20 Scrolling Contents Layer", type:
"EndTransform", cacheIsValid: true, visualRect: [0,0 185x550]}]
The CQ bit was checked by wkorman@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/2254893005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right): https://codereview.chromium.org/2254893005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/BoxPainter.cpp:112: const DisplayItemClient& displayItemClient = paintingOverflowContents ? static_cast<const DisplayItemClient&>(*m_layoutBox.layer()->compositedLayerMapping()->scrollingContentsLayer()) : m_layoutBox; Can you make sure if paint invalidation work for this change? For example, when m_layoutBox's background changes?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2254893005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right): https://codereview.chromium.org/2254893005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/BoxPainter.cpp:112: const DisplayItemClient& displayItemClient = paintingOverflowContents ? static_cast<const DisplayItemClient&>(*m_layoutBox.layer()->compositedLayerMapping()->scrollingContentsLayer()) : m_layoutBox; 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!
https://codereview.chromium.org/2254893005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right): https://codereview.chromium.org/2254893005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/BoxPainter.cpp:112: const DisplayItemClient& displayItemClient = paintingOverflowContents ? static_cast<const DisplayItemClient&>(*m_layoutBox.layer()->compositedLayerMapping()->scrollingContentsLayer()) : m_layoutBox; 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.
This lgtm, and if it fixes the test should fix the problem I was seeing. I'm not sure why invalidations aren't working - see my comment. https://codereview.chromium.org/2254893005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right): https://codereview.chromium.org/2254893005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/BoxPainter.cpp:112: const DisplayItemClient& displayItemClient = paintingOverflowContents ? static_cast<const DisplayItemClient&>(*m_layoutBox.layer()->compositedLayerMapping()->scrollingContentsLayer()) : m_layoutBox; 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.
https://codereview.chromium.org/2254893005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right): https://codereview.chromium.org/2254893005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/BoxPainter.cpp:112: const DisplayItemClient& displayItemClient = paintingOverflowContents ? static_cast<const DisplayItemClient&>(*m_layoutBox.layer()->compositedLayerMapping()->scrollingContentsLayer()) : m_layoutBox; 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...
https://codereview.chromium.org/2254893005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right): https://codereview.chromium.org/2254893005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/BoxPainter.cpp:112: const DisplayItemClient& displayItemClient = paintingOverflowContents ? static_cast<const DisplayItemClient&>(*m_layoutBox.layer()->compositedLayerMapping()->scrollingContentsLayer()) : m_layoutBox; 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.
This updated patch passes attached tests and fixes the previous invalidation issue we were discussing re: changing scroller background color after initial load. *Except*, it still fails to properly invalidate/repaint any of the included tests if you load and then remove/disable 'background' style entirely. The scroller retains a gray background. ToT does not exhibit this issue, so there's more to fix. Sending for thoughts while I continue looking in case another approach warranted. https://codereview.chromium.org/2254893005/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/repaint/overflow-scroll-local-background-text-color-change.html (right): https://codereview.chromium.org/2254893005/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/repaint/overflow-scroll-local-background-text-color-change.html:1: <!DOCTYPE html> This new test is a version of flackr@'s fast/scroll-behavior/overflow-scroll-with-local-background-and-text.html revised to be a repaint test, and removing border: 10px from #scroller since I didn't deem it relevant for this purpose. Perhaps this should be in paint/invalidation subdir, but I put it here alongside other similar overflow-scroll-* repaint tests. https://codereview.chromium.org/2254893005/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/scroll-behavior/overflow-scroll-with-local-background-and-text.html (right): https://codereview.chromium.org/2254893005/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/scroll-behavior/overflow-scroll-with-local-background-and-text.html:6: onload = function() { This is test case from flackr@ via http://crbug.com/638611 basically as-provided. I think the new-fangled thing to do is use runAfterLayoutAndPaint() below rather than double-rAF but either seems ok. The border in #scroller is also potentially extraneous. https://codereview.chromium.org/2254893005/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-local-background.html (right): https://codereview.chromium.org/2254893005/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/paint/invalidation/composited-overflow-with-local-background.html:7: onload = runRepaintAndPixelTest; Changed this to include pixels and --reset-results seems to capture the correctly recolored background (and fails without this patch). I know flackr@ said he'd had problems with this, maybe I am misunderstanding what he had issues with or something else has changed. https://codereview.chromium.org/2254893005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2254893005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:442: layer()->setNeedsRepaint(); We need this due to subsequent invalidateDisplayItemClient() DCHECK: DCHECK(!paintingLayer() || paintingLayer()->needsRepaint()); https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... I have concern this change may be introducing over-painting for this particular scroll/background situation, but note http://crrev.com/2235873002 already introduced line 440 which invalidates a rect sized to the entire scrolling contents. But perhaps calling PaintLayer::setNeedsRepaint() ends up acting as an overly large hammer and I'm unaware? I'll see if this produces diff in repaint text expectations for other layout tests. https://codereview.chromium.org/2254893005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:443: invalidateDisplayItemClient(*layer()->compositedLayerMapping()->scrollingContentsLayer(), invalidationReason); We need this to pair with the use of the scrolling contents layer as the display item in the BoxPainter change below.
The CQ bit was checked by wkorman@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...
Good news! This bit: > *Except*, it still fails to properly invalidate/repaint any of the included tests if you load and then remove/disable 'background' style entirely. The scroller retains a gray background. is not correct, I found when I just went to reconfirm. ToT exhibits the same sticky background color that cannot be totally removed once set. To repro, open paint/invalidation/composited-overflow-with-local-background.html, inspect the scroller and disable all background styles. You'll see whatever the last 'local' background color painted is sticky. Perhaps this is actually expected behavior given 'local' attachment. So, if I'm not in the weeds approach-wise, this patch is feasible.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
wkorman@chromium.org changed reviewers: + schenney@chromium.org - wangxianzhu@chromium.org
+schenney for sanity check and OWNERS
On 2016/08/18 at 23:55:35, wkorman wrote: > Good news! This bit: > > > *Except*, it still fails to properly invalidate/repaint any of the included tests if you load and then remove/disable 'background' style entirely. The scroller retains a gray background. > > is not correct, I found when I just went to reconfirm. ToT exhibits the same sticky background color that cannot be totally removed once set. > > To repro, open paint/invalidation/composited-overflow-with-local-background.html, inspect the scroller and disable all background styles. You'll see whatever the last 'local' background color painted is sticky. Perhaps this is actually expected behavior given 'local' attachment. > It sounds like we may need to update the scrolling contents layer to now be transparent in this case. I can look into this. > So, if I'm not in the weeds approach-wise, this patch is feasible. The approach looks good to me.
Seems sane. fast/scroll-behavior/overflow-scroll-with-local-background-and-text.html is failing though. You can take or leave the rebaselining changes. I don't want to derail a patch ready to land in progress just because of that. https://codereview.chromium.org/2254893005/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2254893005/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:813: crbug.com/638611 paint/invalidation/composited-overflow-with-local-background.html [ NeedsRebaseline ] Time to explore the new rebaselining method? Basically you leave these lines out then pull baselines from the try run to submit with the patch. I would also be fine adding failure expectations here and doing the rebaseline process as a separate patch. https://codereview.chromium.org/2254893005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2254893005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:442: layer()->setNeedsRepaint(); On 2016/08/18 23:48:11, wkorman wrote: > We need this due to subsequent invalidateDisplayItemClient() DCHECK: > > DCHECK(!paintingLayer() || paintingLayer()->needsRepaint()); > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > I have concern this change may be introducing over-painting for this particular > scroll/background situation, but note http://crrev.com/2235873002 already > introduced line 440 which invalidates a rect sized to the entire scrolling > contents. But perhaps calling PaintLayer::setNeedsRepaint() ends up acting as an > overly large hammer and I'm unaware? I'll see if this produces diff in repaint > text expectations for other layout tests. I can't answer that. Not sure who can.
Thanks, are you good w/ LGTM'ing? On 2016/08/19 at 18:10:43, schenney wrote: > Seems sane. > > fast/scroll-behavior/overflow-scroll-with-local-background-and-text.html is failing though. Ah, it looks like a reference mismatch with the text, I will find a way to resolve. https://codereview.chromium.org/2254893005/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2254893005/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:813: crbug.com/638611 paint/invalidation/composited-overflow-with-local-background.html [ NeedsRebaseline ] On 2016/08/19 at 18:10:43, Stephen Chennney wrote: > Time to explore the new rebaselining method? Basically you leave these lines out then pull baselines from the try run to submit with the patch. > > I would also be fine adding failure expectations here and doing the rebaseline process as a separate patch. Sure, I'll try the new process.
The CQ bit was checked by wkorman@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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 wkorman@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wkorman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org, schenney@chromium.org Link to the patchset: https://codereview.chromium.org/2254893005/#ps100001 (title: "Deal with delayed-invalidation object.")
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wkorman@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wkorman@chromium.org
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 #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fix visual rect for background box painting in composited scrollers. Use the scrolling contents GraphicsLayer as the DisplayItemClient for the background drawing when we're painting the full background of a composited scroller. BUG=638611 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix visual rect for background box painting in composited scrollers. Use the scrolling contents GraphicsLayer as the DisplayItemClient for the background drawing when we're painting the full background of a composited scroller. BUG=638611 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/ff27367de0553a7aa38f078e528d19024f2d3668 Cr-Commit-Position: refs/heads/master@{#413340} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ff27367de0553a7aa38f078e528d19024f2d3668 Cr-Commit-Position: refs/heads/master@{#413340}
Message was sent while issue was closed.
chrishtr@chromium.org changed reviewers: + chrishtr@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2254893005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2254893005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:442: layer()->setNeedsRepaint(); Why is this line needed? https://codereview.chromium.org/2254893005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right): https://codereview.chromium.org/2254893005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/BoxPainter.cpp:110: // We may paint a delayed-invalidation object before it's actually invalidated. Note this would be handled for Really? Which test failed without line 113?
Message was sent while issue was closed.
https://codereview.chromium.org/2254893005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2254893005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:442: layer()->setNeedsRepaint(); On 2016/08/22 17:34:54, chrishtr wrote: > Why is this line needed? Copying my comment from https://codereview.chromium.org/2254893005/#msg15 We need this due to subsequent invalidateDisplayItemClient() DCHECK: DCHECK(!paintingLayer() || paintingLayer()->needsRepaint()); https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... I have concern this change may be introducing over-painting for this particular scroll/background situation, but note http://crrev.com/2235873002 already introduced line 440 which invalidates a rect sized to the entire scrolling contents. But perhaps calling PaintLayer::setNeedsRepaint() ends up acting as an overly large hammer and I'm unaware? I'll see if this produces diff in repaint text expectations for other layout tests. https://codereview.chromium.org/2254893005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right): https://codereview.chromium.org/2254893005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/BoxPainter.cpp:110: // We may paint a delayed-invalidation object before it's actually invalidated. Note this would be handled for On 2016/08/22 17:34:54, chrishtr wrote: > Really? Which test failed without line 113? paint/invalidation/animated-gif-background-offscreen.html
Message was sent while issue was closed.
https://codereview.chromium.org/2254893005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2254893005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:442: layer()->setNeedsRepaint(); On 2016/08/22 at 17:42:27, wkorman wrote: > On 2016/08/22 17:34:54, chrishtr wrote: > > Why is this line needed? > > Copying my comment from https://codereview.chromium.org/2254893005/#msg15 > > We need this due to subsequent invalidateDisplayItemClient() DCHECK: > > DCHECK(!paintingLayer() || paintingLayer()->needsRepaint()); > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > I have concern this change may be introducing over-painting for this particular > scroll/background situation, but note http://crrev.com/2235873002 already > introduced line 440 which invalidates a rect sized to the entire scrolling > contents. But perhaps calling PaintLayer::setNeedsRepaint() ends up acting as an > overly large hammer and I'm unaware? I'll see if this produces diff in repaint > text expectations for other layout tests. No it's ok I think. Thanks for the context. https://codereview.chromium.org/2254893005/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/BoxPainter.cpp (right): https://codereview.chromium.org/2254893005/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/BoxPainter.cpp:110: // We may paint a delayed-invalidation object before it's actually invalidated. Note this would be handled for On 2016/08/22 at 17:42:27, wkorman wrote: > On 2016/08/22 17:34:54, chrishtr wrote: > > Really? Which test failed without line 113? > > paint/invalidation/animated-gif-background-offscreen.html ok. |
