Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2276783002:
Fix incorrect invalidation of foreign objects under window zoom. (Closed)
|
Created:
4 years, 4 months ago by wkorman Modified:
4 years, 2 months ago 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 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix incorrect invalidation of foreign objects under window zoom.
Add an over-invalidation hack along the lines of what already exists
for dealing with ancestor clip changes.
On window zoom we see a call to invalidate where the foreign object's
previous paint invalidation rect matches the new paint invalidation rect, but we
still have to force subtree invalidation in order to allow subtree to resize for
the new zoom level.
BUG=640265
Patch Set 1 #
Total comments: 5
Messages
Total messages: 10 (4 generated)
Description was changed from ========== Fix incorrect invalidation of foreign objects under window zoom. When the window resizes we see a call to invalidate where the foreign object's previous paint invalidation rect matches the new paint invalidation rect, but we still have to force subtree invalidation in order to allow subtree to resize for the new zoom level. BUG=640265 ========== to ========== Fix incorrect invalidation of foreign objects under window zoom. Add an over-invalidation hack along the lines of what already exists for dealing with ancestor clip changes. On window zoom we see a call to invalidate where the foreign object's previous paint invalidation rect matches the new paint invalidation rect, but we still have to force subtree invalidation in order to allow subtree to resize for the new zoom level. BUG=640265 ==========
wkorman@chromium.org changed reviewers: + pdr@chromium.org
https://codereview.chromium.org/2276783002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2276783002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:419: if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled() This is just a minor adaptation of your patch: https://codereview.chromium.org/1835843002/patch/1/10036 Per my change description however, I find that on window zoom we see this called with the new and previous paint invalidation rect matching, thus we won't force subtree invalidation, and it appears that we do need to do so in order for all contents of the foreign object to be invalidated. So, I've removed: && previousPaintInvalidationRect != this->previousPaintInvalidationRect() which could mean we'll be over-invalidating more frequently than we would, ideally. Do you have alternate suggestions or insight?
pdr@chromium.org changed reviewers: + fs@opera.com
pdr@chromium.org changed reviewers: + wangxianzhu@chromium.org
https://codereview.chromium.org/2276783002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2276783002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:419: if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled() On 2016/08/23 at 23:13:10, wkorman wrote: > This is just a minor adaptation of your patch: > https://codereview.chromium.org/1835843002/patch/1/10036 > > Per my change description however, I find that on window zoom we see this called with the new and previous paint invalidation rect matching, thus we won't force subtree invalidation, and it appears that we do need to do so in order for all contents of the foreign object to be invalidated. > > So, I've removed: > > && previousPaintInvalidationRect != this->previousPaintInvalidationRect() > > which could mean we'll be over-invalidating more frequently than we would, ideally. Do you have alternate suggestions or insight? I am not sure if my original patch was on the right track. Looking back at it, I think I was off in the weeds. Do you know how paint invalidation works for zoom changes in svg in general? The root SVG layout object should apply zoom globally (see LayoutSVGRoot::buildLocalToBorderBoxTransform and SVGRootPainter::transformToPixelSnappedBorderBox). When the root's zoom changes, how do regular SVG objects get invalidated? I've added WangXianzhu and Fs in case they spot anything obvious here.
https://codereview.chromium.org/2276783002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2276783002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:419: if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled() On 2016/08/24 18:12:19, pdr. wrote: > On 2016/08/23 at 23:13:10, wkorman wrote: > > This is just a minor adaptation of your patch: > > https://codereview.chromium.org/1835843002/patch/1/10036 > > > > Per my change description however, I find that on window zoom we see this > called with the new and previous paint invalidation rect matching, thus we won't > force subtree invalidation, and it appears that we do need to do so in order for > all contents of the foreign object to be invalidated. > > > > So, I've removed: > > > > && previousPaintInvalidationRect != this->previousPaintInvalidationRect() > > > > which could mean we'll be over-invalidating more frequently than we would, > ideally. Do you have alternate suggestions or insight? > > I am not sure if my original patch was on the right track. Looking back at it, I > think I was off in the weeds. > > Do you know how paint invalidation works for zoom changes in svg in general? The > root SVG layout object should apply zoom globally (see > LayoutSVGRoot::buildLocalToBorderBoxTransform and > SVGRootPainter::transformToPixelSnappedBorderBox). When the root's zoom changes, > how do regular SVG objects get invalidated? > > I've added WangXianzhu and Fs in case they spot anything obvious here. I think the zoom case works because we layout the whole subtree and then invalidate the objects which changed layout. Clip is different because it doesn't affect subtree layout.
https://codereview.chromium.org/2276783002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2276783002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:419: if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled() On 2016/08/24 at 18:12:19, pdr. wrote: > On 2016/08/23 at 23:13:10, wkorman wrote: > > This is just a minor adaptation of your patch: > > https://codereview.chromium.org/1835843002/patch/1/10036 > > > > Per my change description however, I find that on window zoom we see this called with the new and previous paint invalidation rect matching, thus we won't force subtree invalidation, and it appears that we do need to do so in order for all contents of the foreign object to be invalidated. > > > > So, I've removed: > > > > && previousPaintInvalidationRect != this->previousPaintInvalidationRect() > > > > which could mean we'll be over-invalidating more frequently than we would, ideally. Do you have alternate suggestions or insight? > > I am not sure if my original patch was on the right track. Looking back at it, I think I was off in the weeds. > > Do you know how paint invalidation works for zoom changes in svg in general? The root SVG layout object should apply zoom globally (see LayoutSVGRoot::buildLocalToBorderBoxTransform and SVGRootPainter::transformToPixelSnappedBorderBox). When the root's zoom changes, how do regular SVG objects get invalidated? Like Xianzhu say, zoom ought to trigger layout for the whole subtree (the fancy screen-scale-changed flag doohickey), which then in turn will invalidate the objects. Maybe that breaks down at the fO boundary. Can think of obvious stuff wrt clipping (although I rather suspect we don't apply it at all in these cases...)
https://codereview.chromium.org/2276783002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2276783002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:419: if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled() On 2016/08/24 at 19:11:44, fs wrote: > On 2016/08/24 at 18:12:19, pdr. wrote: > > On 2016/08/23 at 23:13:10, wkorman wrote: > > > This is just a minor adaptation of your patch: > > > https://codereview.chromium.org/1835843002/patch/1/10036 > > > > > > Per my change description however, I find that on window zoom we see this called with the new and previous paint invalidation rect matching, thus we won't force subtree invalidation, and it appears that we do need to do so in order for all contents of the foreign object to be invalidated. > > > > > > So, I've removed: > > > > > > && previousPaintInvalidationRect != this->previousPaintInvalidationRect() > > > > > > which could mean we'll be over-invalidating more frequently than we would, ideally. Do you have alternate suggestions or insight? > > > > I am not sure if my original patch was on the right track. Looking back at it, I think I was off in the weeds. > > > > Do you know how paint invalidation works for zoom changes in svg in general? The root SVG layout object should apply zoom globally (see LayoutSVGRoot::buildLocalToBorderBoxTransform and SVGRootPainter::transformToPixelSnappedBorderBox). When the root's zoom changes, how do regular SVG objects get invalidated? > > Like Xianzhu say, zoom ought to trigger layout for the whole subtree (the fancy screen-scale-changed flag doohickey), which then in turn will invalidate the objects. Maybe that breaks down at the fO boundary. Can think of obvious stuff wrt clipping (although I rather suspect we don't apply it at all in these cases...) I see, that makes sense. Would something like the following work? if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled() && previousPaintInvalidationRect != this->previousPaintInvalidationRect() && !usesCompositedScrolling() // Note that isLayoutView() below becomes unnecessary after the launch of root layer scrolling. && (hasOverflowClip() || isLayoutView() || isSVGRoot())) newPaintInvalidationState.setForceSubtreeInvalidationRectUpdateWithinContainer();
On 2016/08/24 at 19:55:23, pdr wrote: > https://codereview.chromium.org/2276783002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): > > https://codereview.chromium.org/2276783002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:419: if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled() > On 2016/08/24 at 19:11:44, fs wrote: > > On 2016/08/24 at 18:12:19, pdr. wrote: > > > On 2016/08/23 at 23:13:10, wkorman wrote: > > > > This is just a minor adaptation of your patch: > > > > https://codereview.chromium.org/1835843002/patch/1/10036 > > > > > > > > Per my change description however, I find that on window zoom we see this called with the new and previous paint invalidation rect matching, thus we won't force subtree invalidation, and it appears that we do need to do so in order for all contents of the foreign object to be invalidated. > > > > > > > > So, I've removed: > > > > > > > > && previousPaintInvalidationRect != this->previousPaintInvalidationRect() > > > > > > > > which could mean we'll be over-invalidating more frequently than we would, ideally. Do you have alternate suggestions or insight? > > > > > > I am not sure if my original patch was on the right track. Looking back at it, I think I was off in the weeds. > > > > > > Do you know how paint invalidation works for zoom changes in svg in general? The root SVG layout object should apply zoom globally (see LayoutSVGRoot::buildLocalToBorderBoxTransform and SVGRootPainter::transformToPixelSnappedBorderBox). When the root's zoom changes, how do regular SVG objects get invalidated? > > > > Like Xianzhu say, zoom ought to trigger layout for the whole subtree (the fancy screen-scale-changed flag doohickey), which then in turn will invalidate the objects. Maybe that breaks down at the fO boundary. Can think of obvious stuff wrt clipping (although I rather suspect we don't apply it at all in these cases...) Uhm, s/Can/Can't/ there if that wasn't clear from context... > I see, that makes sense. > > Would something like the following work? > if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled() > && previousPaintInvalidationRect != this->previousPaintInvalidationRect() > && !usesCompositedScrolling() > // Note that isLayoutView() below becomes unnecessary after the launch of root layer scrolling. > && (hasOverflowClip() || isLayoutView() || isSVGRoot())) > newPaintInvalidationState.setForceSubtreeInvalidationRectUpdateWithinContainer(); Are there indications that this is a problem in other cases than the "fO boundary"? (The above seems to cover quite a bit more.) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
