Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1900263002:
Update PaintLayer::needsPaintPhase flags when layer self-painting status changes (Closed)
|
Created:
4 years, 8 months ago by Xianzhu Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, slimming-paint-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate PaintLayer::needsPaintPhase flags when layer self-painting status changes
This had been included in https://codereview.chromium.org/1862313002/
before I removed it thinking that self-painting status should cause
layout and paint invalidation. However, bug 604351 is a case that
self-painting status change doesn't cause layout and paint invalidation.
BUG=604351
TEST=PaintLayerPainterTest.PaintPhasesUpdateOnBecomingSelfPainting
TEST=PaintLayerPainterTest.PaintPhasesUpdateOnBecomingNonSelfPainting
Committed: https://crrev.com/2d55d696deec11d34539475374366a63d1dfd082
Cr-Commit-Position: refs/heads/master@{#388880}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 4
Messages
Total messages: 18 (4 generated)
wangxianzhu@chromium.org changed reviewers: + pdr@chromium.org
https://codereview.chromium.org/1900263002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/1900263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2466: if (PaintLayer* parent = this->parent()) { The tests in this patch seem to pass without this code change.
https://codereview.chromium.org/1900263002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/1900263002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2466: if (PaintLayer* parent = this->parent()) { On 2016/04/19 21:17:57, pdr wrote: > The tests in this patch seem to pass without this code change. Thanks for verifying the tests. Sorry for not verifying by myself. Thought the tests work. Removed the floating object because we force layout when the floating object's ancestor changes self-painting status. Verified this time :)
On 2016/04/19 at 23:35:47, wangxianzhu wrote: > https://codereview.chromium.org/1900263002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): > > https://codereview.chromium.org/1900263002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/paint/PaintLayer.cpp:2466: if (PaintLayer* parent = this->parent()) { > On 2016/04/19 21:17:57, pdr wrote: > > The tests in this patch seem to pass without this code change. > > Thanks for verifying the tests. Sorry for not verifying by myself. Thought the tests work. > > Removed the floating object because we force layout when the floating object's ancestor changes self-painting status. > > Verified this time :) I think you forgot to upload the new patch.
On 2016/04/20 00:01:05, pdr wrote: > On 2016/04/19 at 23:35:47, wangxianzhu wrote: > > > https://codereview.chromium.org/1900263002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): > > > > > https://codereview.chromium.org/1900263002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/paint/PaintLayer.cpp:2466: if (PaintLayer* > parent = this->parent()) { > > On 2016/04/19 21:17:57, pdr wrote: > > > The tests in this patch seem to pass without this code change. > > > > Thanks for verifying the tests. Sorry for not verifying by myself. Thought the > tests work. > > > > Removed the floating object because we force layout when the floating object's > ancestor changes self-painting status. > > > > Verified this time :) > > I think you forgot to upload the new patch. Sorry. Uploaded.
ping...
Overall this looks good, just one question. https://codereview.chromium.org/1900263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp (right): https://codereview.chromium.org/1900263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp:427: " <div>" Why is this inner div required?
https://codereview.chromium.org/1900263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp (right): https://codereview.chromium.org/1900263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp:427: " <div>" On 2016/04/20 23:13:39, pdr wrote: > Why is this inner div required? Haven't looked into the root cause, but without the div the bug couldn't be reproduced. May debug to find out how.
https://codereview.chromium.org/1900263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp (right): https://codereview.chromium.org/1900263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp:427: " <div>" On 2016/04/21 00:42:35, Xianzhu wrote: > On 2016/04/20 23:13:39, pdr wrote: > > Why is this inner div required? > > Haven't looked into the root cause, but without the div the bug couldn't be > reproduced. May debug to find out how. Without the div, the div at line 428 will be marked mayNeedPaintInvalidation during full layout (caused by the style change) of the 'will-be-self-painting' div.
pdr@chromium.org changed reviewers: + chrishtr@chromium.org
https://codereview.chromium.org/1900263002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp (right): https://codereview.chromium.org/1900263002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp:427: " <div>" On 2016/04/21 at 01:03:41, Xianzhu wrote: > On 2016/04/21 00:42:35, Xianzhu wrote: > > On 2016/04/20 23:13:39, pdr wrote: > > > Why is this inner div required? > > > > Haven't looked into the root cause, but without the div the bug couldn't be > > reproduced. May debug to find out how. > > Without the div, the div at line 428 will be marked mayNeedPaintInvalidation during full layout (caused by the style change) of the 'will-be-self-painting' div. This doesn't seem obviously wrong to me, but I think it's a sign that we don't fully understand this change. If this works without the div, what mechanism is merging the paint phase flags?
The CQ bit was checked by pdr@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1900263002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1900263002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Update PaintLayer::needsPaintPhase flags when layer self-painting status changes This had been included in https://codereview.chromium.org/1862313002/ before I removed it thinking that self-painting status should cause layout and paint invalidation. However, bug 604351 is a case that self-painting status change doesn't cause layout and paint invalidation. BUG=604351 TEST=PaintLayerPainterTest.PaintPhasesUpdateOnBecomingSelfPainting TEST=PaintLayerPainterTest.PaintPhasesUpdateOnBecomingNonSelfPainting ========== to ========== Update PaintLayer::needsPaintPhase flags when layer self-painting status changes This had been included in https://codereview.chromium.org/1862313002/ before I removed it thinking that self-painting status should cause layout and paint invalidation. However, bug 604351 is a case that self-painting status change doesn't cause layout and paint invalidation. BUG=604351 TEST=PaintLayerPainterTest.PaintPhasesUpdateOnBecomingSelfPainting TEST=PaintLayerPainterTest.PaintPhasesUpdateOnBecomingNonSelfPainting Committed: https://crrev.com/2d55d696deec11d34539475374366a63d1dfd082 Cr-Commit-Position: refs/heads/master@{#388880} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/2d55d696deec11d34539475374366a63d1dfd082 Cr-Commit-Position: refs/heads/master@{#388880} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
