Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2537653002:
Invalidate previousLocationInBacking in LayoutObject::clearPreviousVisualRects() (Closed)
|
Created:
4 years ago by Xianzhu Modified:
4 years ago Reviewers:
chrishtr 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInvalidate previousLocationInBacking in LayoutObject::clearPreviousVisualRects()
clearPreviousVisualRects() is called when the object's previous visual
rects become invalid (e.g. when the object becomes composited). We
should call setMayNeedPaintInvalidationSubtree to ensure paint
invalidation checking of subtree which would be triggered by changed
location if we had valid previous location.
BUG=666267
TEST=paint/invalidation/compositing/become-composited-zero-offset.html
Committed: https://crrev.com/b4a81bdbf064377ffee45f75b8ee79eb260d9942
Cr-Commit-Position: refs/heads/master@{#434894}
Patch Set 1 #
Total comments: 4
Patch Set 2 : - #Patch Set 3 : setMayNeedPaintInvalidationSubtree #
Messages
Total messages: 22 (13 generated)
The CQ bit was checked by wangxianzhu@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...
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org
Actually this is not a new bug, but happened to regress for the bug. Given an object whose locationInBacking == visualRect.location(), we don't save previousLocationInBacking in locationInBackingMap, but will use previousVisualRect().location() when previousLocationInBacking is requested. If this happens after LayoutObject::clearPreviousVisualRects(), previousLocationInBacking() will return LayoutPoint(). If the object's location happens to be LayoutPoint() we may miss the required paint invalidation checking on location change. Before https://codereview.chromium.org/2476813002 previousLocationInBacking() just returned the previous saved value (which may be invalid) but happened to correctly trigger subtree paint invalidation checking when the new location happened to be different from the saved value.
https://codereview.chromium.org/2537653002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2537653002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1266: // After clearing ("invalidating" the visual rects, mark this object as Noticed a typo here with mismatched parens. Might as well fix in this CL. https://codereview.chromium.org/2537653002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1268: setShouldDoFullPaintInvalidation(); This call here on line 1268 should cause full paint invalidation to be computed for the object, which will re-create the paint invalidation rect. I added it in this CL, for a very similar-sounding reason: https://chromium.googlesource.com/chromium/src/+/2234389b8ef6a9204d9a37bb1781... Why wouldn't the same process also lead to previousLocationInBacking being the correct value after paint invalidation of the same lifecycle update?
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
https://codereview.chromium.org/2537653002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2537653002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1266: // After clearing ("invalidating" the visual rects, mark this object as On 2016/11/29 00:57:59, chrishtr wrote: > Noticed a typo here with mismatched parens. Might as well fix in this CL. Done. https://codereview.chromium.org/2537653002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1268: setShouldDoFullPaintInvalidation(); On 2016/11/29 00:57:59, chrishtr wrote: > This call here on line 1268 should cause full paint invalidation to be computed > for the object, which will > re-create the paint invalidation rect. I added it in this CL, for a very > similar-sounding reason: > > https://chromium.googlesource.com/chromium/src/+/2234389b8ef6a9204d9a37bb1781... > > Why wouldn't the same process also lead to previousLocationInBacking being the > correct value after paint > invalidation of the same lifecycle update? We'll get the location recomputed, but may miss setForceSubtreeInvalidationCheckingWithinContainer ( https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...): if (previousLocation != paintInvalidator.previousLocationInBacking()) { newPaintInvalidationState .setForceSubtreeInvalidationCheckingWithinContainer(); } if the new location happens to be LayoutPoint().
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/29 at 01:43:27, wangxianzhu wrote: > https://codereview.chromium.org/2537653002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): > > https://codereview.chromium.org/2537653002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutObject.cpp:1266: // After clearing ("invalidating" the visual rects, mark this object as > On 2016/11/29 00:57:59, chrishtr wrote: > > Noticed a typo here with mismatched parens. Might as well fix in this CL. > > Done. > > https://codereview.chromium.org/2537653002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutObject.cpp:1268: setShouldDoFullPaintInvalidation(); > On 2016/11/29 00:57:59, chrishtr wrote: > > This call here on line 1268 should cause full paint invalidation to be computed > > for the object, which will > > re-create the paint invalidation rect. I added it in this CL, for a very > > similar-sounding reason: > > > > https://chromium.googlesource.com/chromium/src/+/2234389b8ef6a9204d9a37bb1781... > > > > Why wouldn't the same process also lead to previousLocationInBacking being the > > correct value after paint > > invalidation of the same lifecycle update? > > We'll get the location recomputed, but may miss setForceSubtreeInvalidationCheckingWithinContainer ( https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...): > > if (previousLocation != paintInvalidator.previousLocationInBacking()) { > newPaintInvalidationState > .setForceSubtreeInvalidationCheckingWithinContainer(); > } > > if the new location happens to be LayoutPoint(). I see. Can you just call setMayNeedPaintInvalidationSubtree()?
Description was changed from ========== Invalidate previousLocationInBacking in LayoutObject::clearPreviousVisualRects() clearPreviousVisualRects() is called when the object's previous visual rects become invalid (e.g. when the object becomes composited). We should also invalidate previousLocationInBacking with an invalidate value as the default LayoutPoint() will match a real zero location. BUG=666267 TEST=paint/invalidation/compositing/become-composited-zero-offset.html ========== to ========== Invalidate previousLocationInBacking in LayoutObject::clearPreviousVisualRects() clearPreviousVisualRects() is called when the object's previous visual rects become invalid (e.g. when the object becomes composited). We should call setMayNeedPaintInvalidationSubtree to ensure paint invalidation checking of subtree which would be triggered by changed location if we had valid previous location. BUG=666267 TEST=paint/invalidation/compositing/become-composited-zero-offset.html ==========
On 2016/11/29 01:51:44, chrishtr wrote: > > I see. Can you just call setMayNeedPaintInvalidationSubtree()? Good idea. Done.
The CQ bit was checked by wangxianzhu@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: This issue passed the CQ dry run.
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480394181159730,
"parent_rev": "6e67182a8cc0f781546ec3523f8168502477e122", "commit_rev":
"f180629fbf00296dd87d3c9f7c78f2db89be6d35"}
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Invalidate previousLocationInBacking in LayoutObject::clearPreviousVisualRects() clearPreviousVisualRects() is called when the object's previous visual rects become invalid (e.g. when the object becomes composited). We should call setMayNeedPaintInvalidationSubtree to ensure paint invalidation checking of subtree which would be triggered by changed location if we had valid previous location. BUG=666267 TEST=paint/invalidation/compositing/become-composited-zero-offset.html ========== to ========== Invalidate previousLocationInBacking in LayoutObject::clearPreviousVisualRects() clearPreviousVisualRects() is called when the object's previous visual rects become invalid (e.g. when the object becomes composited). We should call setMayNeedPaintInvalidationSubtree to ensure paint invalidation checking of subtree which would be triggered by changed location if we had valid previous location. BUG=666267 TEST=paint/invalidation/compositing/become-composited-zero-offset.html Committed: https://crrev.com/b4a81bdbf064377ffee45f75b8ee79eb260d9942 Cr-Commit-Position: refs/heads/master@{#434894} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b4a81bdbf064377ffee45f75b8ee79eb260d9942 Cr-Commit-Position: refs/heads/master@{#434894} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
