Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1934833002:
Ensure filter and reflection outsets are applied on paint invalidation rect (Closed)
|
Created:
4 years, 7 months ago by Xianzhu Modified:
4 years, 7 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. |
DescriptionEnsure filter and reflection outsets are applied on paint invalidation rect
Previously filter and reflection outsets are applied in
mapToVisualRectInAncestorSpace along the container chain. This missed
filters/reflections between the object and its container.
BUG=607989
TEST=fast/repaint/filter-invalidation-positioned-child.html
TEST=fast/repaint/reflection-invalidation-positioned-child.html
Committed: https://crrev.com/05fb4239a14a319f2a76de26b1f5a5413bb71ed1
Cr-Commit-Position: refs/heads/master@{#391525}
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 12
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 1
Messages
Total messages: 28 (5 generated)
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org, jbroman@chromium.org
https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1992: toLayoutBox(parent)->inflateVisualRectForReflectionAndFilter(rect); Doesn't |rect| need to be in the space of parent| for this to be valid?
https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1992: toLayoutBox(parent)->inflateVisualRectForReflectionAndFilter(rect); On 2016/05/02 22:16:30, chrishtr wrote: > Doesn't |rect| need to be in the space of parent| for this to be valid? Not necessary. The function just inflate the rect by maximum outsets of the filter effect. The result rect covers all the affected area of this object with the ancestor filter applied.
https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1992: toLayoutBox(parent)->inflateVisualRectForReflectionAndFilter(rect); On 2016/05/02 at 22:21:48, Xianzhu wrote: > On 2016/05/02 22:16:30, chrishtr wrote: > > Doesn't |rect| need to be in the space of parent| for this to be valid? > > Not necessary. The function just inflate the rect by maximum outsets of the filter effect. The result rect covers all the affected area of this object with the ancestor filter applied. But if the rect is not originally in the space of the box, how can these outsets possibly be applied correctly?
On 2016/05/02 22:51:50, chrishtr wrote: > https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutBox.cpp:1992: > toLayoutBox(parent)->inflateVisualRectForReflectionAndFilter(rect); > On 2016/05/02 at 22:21:48, Xianzhu wrote: > > On 2016/05/02 22:16:30, chrishtr wrote: > > > Doesn't |rect| need to be in the space of parent| for this to be valid? > > > > Not necessary. The function just inflate the rect by maximum outsets of the > filter effect. The result rect covers all the affected area of this object with > the ancestor filter applied. > > But if the rect is not originally in the space of the box, how can these outsets > possibly be applied correctly? For example: <div id="inner" style="filter: ..blurry..; width: 1000px; height: 1000px"> <div id="inner" style="width: 100px; height: 100px; background-color: blue"> </div> Suppose the blurry outsets is 10px. When inner changes background-color, as its blurry, the affected area is (0,0 100x100) with outset 10px = (-10,-10 120x120) in its local space. Then this rect will be mapped into coordination space of paint invalidation container. The added code will not encounter any transform or flipping (which may cause the outsets not applicable), otherwise we'll not skip filter when finding the container of the current object.
On 2016/05/02 23:31:35, Xianzhu wrote: > On 2016/05/02 22:51:50, chrishtr wrote: > > > https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > > > > https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:1992: > > toLayoutBox(parent)->inflateVisualRectForReflectionAndFilter(rect); > > On 2016/05/02 at 22:21:48, Xianzhu wrote: > > > On 2016/05/02 22:16:30, chrishtr wrote: > > > > Doesn't |rect| need to be in the space of parent| for this to be valid? > > > > > > Not necessary. The function just inflate the rect by maximum outsets of the > > filter effect. The result rect covers all the affected area of this object > with > > the ancestor filter applied. > > > > But if the rect is not originally in the space of the box, how can these > outsets > > possibly be applied correctly? > > For example: > <div id="inner" style="filter: ..blurry..; width: 1000px; height: 1000px"> > <div id="inner" style="width: 100px; height: 100px; background-color: blue"> > </div> > > Suppose the blurry outsets is 10px. When inner changes background-color, as its > blurry, the affected area is (0,0 100x100) with outset 10px = (-10,-10 120x120) > in its local space. Then this rect will be mapped into coordination space of > paint invalidation container. > > The added code will not encounter any transform or flipping (which may cause the > outsets not applicable), otherwise we'll not skip filter when finding the > container of the current object. Correction: the example should be <div id="inner" style="filter: ..blurry..; width: 1000px; height: 1000px"> <div id="inner" style="width: 100px; height: 100px; background-color: blue"> </div>
On 2016/05/02 23:31:35, Xianzhu wrote: > On 2016/05/02 22:51:50, chrishtr wrote: > > > https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > > > > https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:1992: > > toLayoutBox(parent)->inflateVisualRectForReflectionAndFilter(rect); > > On 2016/05/02 at 22:21:48, Xianzhu wrote: > > > On 2016/05/02 22:16:30, chrishtr wrote: > > > > Doesn't |rect| need to be in the space of parent| for this to be valid? > > > > > > Not necessary. The function just inflate the rect by maximum outsets of the > > filter effect. The result rect covers all the affected area of this object > with > > the ancestor filter applied. > > > > But if the rect is not originally in the space of the box, how can these > outsets > > possibly be applied correctly? > > For example: > <div id="inner" style="filter: ..blurry..; width: 1000px; height: 1000px"> > <div id="inner" style="width: 100px; height: 100px; background-color: blue"> > </div> > > Suppose the blurry outsets is 10px. When inner changes background-color, as its > blurry, the affected area is (0,0 100x100) with outset 10px = (-10,-10 120x120) > in its local space. Then this rect will be mapped into coordination space of > paint invalidation container. > > The added code will not encounter any transform or flipping (which may cause the > outsets not applicable), otherwise we'll not skip filter when finding the > container of the current object. Correction: the example should be: <div id="outer" style="filter: ..blurry..; width: 1000px; height: 1000px"> <div id="inner" style="width: 100px; height: 100px; background-color: blue"> </div>
On 2016/05/02 at 23:35:19, wangxianzhu wrote: > On 2016/05/02 23:31:35, Xianzhu wrote: > > On 2016/05/02 22:51:50, chrishtr wrote: > > > > > https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... > > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > > > > > > > https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:1992: > > > toLayoutBox(parent)->inflateVisualRectForReflectionAndFilter(rect); > > > On 2016/05/02 at 22:21:48, Xianzhu wrote: > > > > On 2016/05/02 22:16:30, chrishtr wrote: > > > > > Doesn't |rect| need to be in the space of parent| for this to be valid? > > > > > > > > Not necessary. The function just inflate the rect by maximum outsets of the > > > filter effect. The result rect covers all the affected area of this object > > with > > > the ancestor filter applied. > > > > > > But if the rect is not originally in the space of the box, how can these > > outsets > > > possibly be applied correctly? > > > > For example: > > <div id="inner" style="filter: ..blurry..; width: 1000px; height: 1000px"> > > <div id="inner" style="width: 100px; height: 100px; background-color: blue"> > > </div> > > > > Suppose the blurry outsets is 10px. When inner changes background-color, as its > > blurry, the affected area is (0,0 100x100) with outset 10px = (-10,-10 120x120) > > in its local space. Then this rect will be mapped into coordination space of > > paint invalidation container. > > > > The added code will not encounter any transform or flipping (which may cause the > > outsets not applicable), otherwise we'll not skip filter when finding the > > container of the current object. > > Correction: the example should be: > <div id="outer" style="filter: ..blurry..; width: 1000px; height: 1000px"> > <div id="inner" style="width: 100px; height: 100px; background-color: blue"> > </div> Outsets like for blurs that happen to be symmetric are one thing. What about filters that transform? Or reflection? That depends on the rect being in the space of the box that has the filter or reflection, as far as I can see. The code seems to bear me out I think? e.g. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2016/05/02 23:48:15, chrishtr wrote: > On 2016/05/02 at 23:35:19, wangxianzhu wrote: > > On 2016/05/02 23:31:35, Xianzhu wrote: > > > On 2016/05/02 22:51:50, chrishtr wrote: > > > > > > > > https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... > > > > File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): > > > > > > > > > > > > https://codereview.chromium.org/1934833002/diff/1/third_party/WebKit/Source/c... > > > > third_party/WebKit/Source/core/layout/LayoutBox.cpp:1992: > > > > toLayoutBox(parent)->inflateVisualRectForReflectionAndFilter(rect); > > > > On 2016/05/02 at 22:21:48, Xianzhu wrote: > > > > > On 2016/05/02 22:16:30, chrishtr wrote: > > > > > > Doesn't |rect| need to be in the space of parent| for this to be > valid? > > > > > > > > > > Not necessary. The function just inflate the rect by maximum outsets of > the > > > > filter effect. The result rect covers all the affected area of this object > > > with > > > > the ancestor filter applied. > > > > > > > > But if the rect is not originally in the space of the box, how can these > > > outsets > > > > possibly be applied correctly? > > > > > > For example: > > > <div id="inner" style="filter: ..blurry..; width: 1000px; height: 1000px"> > > > <div id="inner" style="width: 100px; height: 100px; background-color: > blue"> > > > </div> > > > > > > Suppose the blurry outsets is 10px. When inner changes background-color, as > its > > > blurry, the affected area is (0,0 100x100) with outset 10px = (-10,-10 > 120x120) > > > in its local space. Then this rect will be mapped into coordination space of > > > paint invalidation container. > > > > > > The added code will not encounter any transform or flipping (which may cause > the > > > outsets not applicable), otherwise we'll not skip filter when finding the > > > container of the current object. > > > > Correction: the example should be: > > <div id="outer" style="filter: ..blurry..; width: 1000px; height: 1000px"> > > <div id="inner" style="width: 100px; height: 100px; background-color: > blue"> > > </div> > > Outsets like for blurs that happen to be symmetric are one thing. What about > filters that transform? Or reflection? That depends on the rect being in the > space of the box that has the filter or reflection, > as far as I can see. The code seems to bear me out I think? e.g. > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... You are right. I considered only cases like blurry filter. We need a logic like ancestorSkipped to apply the offset from the filter/reflection to the box.
Description was changed from ========== Ensure filter and reflection outsets are applied on paint invalidation rect Previously filter and reflection outsets are applied in mapToVisualRectInAncestorSpace along the container chain. This missed filters/reflections between the object and its container. BUG=607989 TEST=fast/repaint/filter-invalidation-positioned-child.html ========== to ========== Ensure filter and reflection outsets are applied on paint invalidation rect Previously filter and reflection outsets are applied in mapToVisualRectInAncestorSpace along the container chain. This missed filters/reflections between the object and its container. BUG=607989 TEST=fast/repaint/filter-invalidation-positioned-child.html TEST=fast/repaint/reflection-invalidation-positioned-child.html ==========
Fixed the coordinates issue. Ptal. BTW, moved the style diff check for reflection change from ComputedStyle::diffNeedsFullLayoutAndPaintInvalidation() into diffNeedsFullLayout() and diffNeedsPaintInvalidationLayer() to ensure positioned descendants are properly invalidated when reflection changes.
https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1989: // Apply filter and reflection outsets between this object and container or ancestor. Suggest not phrasing this as "outsets". Not all filters are blur-like. For instance, an offset filter shifts the image (while that _can_ be viewed as an outset, it's pretty awkward to as it isn't an expansion). And with reflections it's even more different (in particular, it's sensitive to the coordinate space being used). Characterizing this the way Blink is trending toward -- and the way Skia does -- is better: there's a function mapping rectangles through filters (forwards or backwards, depending on the case).
https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1988: if (filterOrReflectionSkipped) { Please factor the contents of this if into a helper method. https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1995: LayoutSize parentOffset = parent->offsetFromAncestorContainer(container); This is expensive, but I suppose this whole situation is quite rare, so ok? https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:699: if (!RuntimeEnabledFeatures::cssBoxReflectFilterEnabled() && !rareNonInheritedData->reflectionDataEquivalent(*other.rareNonInheritedData.get())) Why is it no longer needed when cssBoxReflectFilterEnabled() is true? Also, is the refactoring here required to fix a known bug?
https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1988: if (filterOrReflectionSkipped) { On 2016/05/03 17:31:38, chrishtr wrote: > Please factor the contents of this if into a helper method. Done. https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1989: // Apply filter and reflection outsets between this object and container or ancestor. On 2016/05/03 17:30:48, jbroman wrote: > Suggest not phrasing this as "outsets". Not all filters are blur-like. > > For instance, an offset filter shifts the image (while that _can_ be viewed as > an outset, it's pretty awkward to as it isn't an expansion). And with > reflections it's even more different (in particular, it's sensitive to the > coordinate space being used). > > Characterizing this the way Blink is trending toward -- and the way Skia does -- > is better: there's a function mapping rectangles through filters (forwards or > backwards, depending on the case). Thanks for the info. Done. https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1995: LayoutSize parentOffset = parent->offsetFromAncestorContainer(container); On 2016/05/03 17:31:38, chrishtr wrote: > This is expensive, but I suppose this whole situation is quite rare, so ok? I believe it's rare because no layout test covered this, and no bug report for it. https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:699: if (!RuntimeEnabledFeatures::cssBoxReflectFilterEnabled() && !rareNonInheritedData->reflectionDataEquivalent(*other.rareNonInheritedData.get())) On 2016/05/03 17:31:38, chrishtr wrote: > Why is it no longer needed when cssBoxReflectFilterEnabled() is true? > > Also, is the refactoring here required to fix a known bug? With cssBoxReflectFilterEnabled() we go through the filter path (new line 790). The bug had not been known until I found it with the new test case for this CL :)
https://codereview.chromium.org/1934833002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/repaint/filter-invalidation-positioned-child.html (right): https://codereview.chromium.org/1934833002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/repaint/filter-invalidation-positioned-child.html:19: Tests paint invalidation of positioned object when it's ancestor changes filter. Nit: s/it's/its/ https://codereview.chromium.org/1934833002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/repaint/reflection-invalidation-positioned-child.html (right): https://codereview.chromium.org/1934833002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/repaint/reflection-invalidation-positioned-child.html:37: Tests paint invalidation of positioned object when it's ancestor changes reflection. Nit: s/it's/its/
https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:699: if (!RuntimeEnabledFeatures::cssBoxReflectFilterEnabled() && !rareNonInheritedData->reflectionDataEquivalent(*other.rareNonInheritedData.get())) On 2016/05/03 at 18:00:24, Xianzhu wrote: > On 2016/05/03 17:31:38, chrishtr wrote: > > Why is it no longer needed when cssBoxReflectFilterEnabled() is true? > > > > Also, is the refactoring here required to fix a known bug? > > With cssBoxReflectFilterEnabled() we go through the filter path (new line 790). > > The bug had not been known until I found it with the new test case for this CL :) Ok. But I still don't understand what was wrong with the old ComputedStyle code that you are fixing. Can you give an example? I can see why the change on 790 would be needed, but what about the other parts? Which test fails without this change?
https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:699: if (!RuntimeEnabledFeatures::cssBoxReflectFilterEnabled() && !rareNonInheritedData->reflectionDataEquivalent(*other.rareNonInheritedData.get())) On 2016/05/03 19:19:57, chrishtr wrote: > On 2016/05/03 at 18:00:24, Xianzhu wrote: > > On 2016/05/03 17:31:38, chrishtr wrote: > > > Why is it no longer needed when cssBoxReflectFilterEnabled() is true? > > > > > > Also, is the refactoring here required to fix a known bug? > > > > With cssBoxReflectFilterEnabled() we go through the filter path (new line > 790). > > > > The bug had not been known until I found it with the new test case for this CL > :) > > Ok. But I still don't understand what was wrong with the old ComputedStyle code > that you are fixing. > Can you give an example? I can see why the change on 790 would be needed, but > what about the other parts? > Which test fails without this change? Previously when reflection changes, we mark the object needs repaint and needs full layout (in old diffNeedsFullLayoutAndPaintInvalidation()). This is not adequate because we also need to invalidate the non-composited sub-layers (which requires needsPaintInvalidationLayer()). The changes at 699 (in diffNeedsFullLayout()) and 720 (in diffNeedsPaintInvalidationLayer()) ensure that when reflection changes, we will do full layout and full paint invalidation of the sub-layers. The new test fast/repaint/reflection-invalidation-positioned-child.html would fail without these changes because of missing full paint invalidation of the positioned child (a wrong incremental invalidation would be issued instead). https://codereview.chromium.org/1934833002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/repaint/filter-invalidation-positioned-child.html (right): https://codereview.chromium.org/1934833002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/repaint/filter-invalidation-positioned-child.html:19: Tests paint invalidation of positioned object when it's ancestor changes filter. On 2016/05/03 19:17:57, chrishtr wrote: > Nit: s/it's/its/ Done. https://codereview.chromium.org/1934833002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/repaint/reflection-invalidation-positioned-child.html (right): https://codereview.chromium.org/1934833002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/repaint/reflection-invalidation-positioned-child.html:37: Tests paint invalidation of positioned object when it's ancestor changes reflection. On 2016/05/03 19:17:57, chrishtr wrote: > Nit: s/it's/its/ Done. https://codereview.chromium.org/1934833002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/repaint/reflection-invalidation-positioned-child-expected.txt (right): https://codereview.chromium.org/1934833002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/repaint/reflection-invalidation-positioned-child-expected.txt:22: "reason": "style change" Without the changes in ComputedStyle.cpp (for !cssBoxReflectFilterEnabled()), this invalidate would be incorrect: "rect": [400, 180, 50, 20], "reason": "incremental"
https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:699: if (!RuntimeEnabledFeatures::cssBoxReflectFilterEnabled() && !rareNonInheritedData->reflectionDataEquivalent(*other.rareNonInheritedData.get())) On 2016/05/03 at 19:53:39, Xianzhu wrote: > On 2016/05/03 19:19:57, chrishtr wrote: > > On 2016/05/03 at 18:00:24, Xianzhu wrote: > > > On 2016/05/03 17:31:38, chrishtr wrote: > > > > Why is it no longer needed when cssBoxReflectFilterEnabled() is true? > > > > > > > > Also, is the refactoring here required to fix a known bug? > > > > > > With cssBoxReflectFilterEnabled() we go through the filter path (new line > > 790). > > > > > > The bug had not been known until I found it with the new test case for this CL > > :) > > > > Ok. But I still don't understand what was wrong with the old ComputedStyle code > > that you are fixing. > > Can you give an example? I can see why the change on 790 would be needed, but > > what about the other parts? > > Which test fails without this change? > > Previously when reflection changes, we mark the object needs repaint and needs full layout (in old diffNeedsFullLayoutAndPaintInvalidation()). This is not adequate because we also need to invalidate the non-composited sub-layers (which requires needsPaintInvalidationLayer()). The changes at 699 (in diffNeedsFullLayout()) and 720 (in diffNeedsPaintInvalidationLayer()) ensure that when reflection changes, we will do full layout and full paint invalidation of the sub-layers. > > The new test fast/repaint/reflection-invalidation-positioned-child.html would fail without these changes because of missing full paint invalidation of the positioned child (a wrong incremental invalidation would be issued instead). Ah, ok. I see. I had overlooked the difference between needsPaintInvalidationLayer and needsPaintInvalidationObject. How about renaming the former to something like needsPaintInvalidationObjectAndSubtree?
https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right): https://codereview.chromium.org/1934833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/style/ComputedStyle.cpp:699: if (!RuntimeEnabledFeatures::cssBoxReflectFilterEnabled() && !rareNonInheritedData->reflectionDataEquivalent(*other.rareNonInheritedData.get())) On 2016/05/03 20:04:37, chrishtr wrote: > On 2016/05/03 at 19:53:39, Xianzhu wrote: > > On 2016/05/03 19:19:57, chrishtr wrote: > > > On 2016/05/03 at 18:00:24, Xianzhu wrote: > > > > On 2016/05/03 17:31:38, chrishtr wrote: > > > > > Why is it no longer needed when cssBoxReflectFilterEnabled() is true? > > > > > > > > > > Also, is the refactoring here required to fix a known bug? > > > > > > > > With cssBoxReflectFilterEnabled() we go through the filter path (new line > > > 790). > > > > > > > > The bug had not been known until I found it with the new test case for > this CL > > > :) > > > > > > Ok. But I still don't understand what was wrong with the old ComputedStyle > code > > > that you are fixing. > > > Can you give an example? I can see why the change on 790 would be needed, > but > > > what about the other parts? > > > Which test fails without this change? > > > > Previously when reflection changes, we mark the object needs repaint and needs > full layout (in old diffNeedsFullLayoutAndPaintInvalidation()). This is not > adequate because we also need to invalidate the non-composited sub-layers (which > requires needsPaintInvalidationLayer()). The changes at 699 (in > diffNeedsFullLayout()) and 720 (in diffNeedsPaintInvalidationLayer()) ensure > that when reflection changes, we will do full layout and full paint invalidation > of the sub-layers. > > > > The new test fast/repaint/reflection-invalidation-positioned-child.html would > fail without these changes because of missing full paint invalidation of the > positioned child (a wrong incremental invalidation would be issued instead). > > Ah, ok. I see. I had overlooked the difference between > needsPaintInvalidationLayer and needsPaintInvalidationObject. How about renaming > the former to something like > needsPaintInvalidationObjectAndSubtree? Good idea. Will create another CL for this.
lgtm
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1934833002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1934833002/60001
Message was sent while issue was closed.
Description was changed from ========== Ensure filter and reflection outsets are applied on paint invalidation rect Previously filter and reflection outsets are applied in mapToVisualRectInAncestorSpace along the container chain. This missed filters/reflections between the object and its container. BUG=607989 TEST=fast/repaint/filter-invalidation-positioned-child.html TEST=fast/repaint/reflection-invalidation-positioned-child.html ========== to ========== Ensure filter and reflection outsets are applied on paint invalidation rect Previously filter and reflection outsets are applied in mapToVisualRectInAncestorSpace along the container chain. This missed filters/reflections between the object and its container. BUG=607989 TEST=fast/repaint/filter-invalidation-positioned-child.html TEST=fast/repaint/reflection-invalidation-positioned-child.html ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Ensure filter and reflection outsets are applied on paint invalidation rect Previously filter and reflection outsets are applied in mapToVisualRectInAncestorSpace along the container chain. This missed filters/reflections between the object and its container. BUG=607989 TEST=fast/repaint/filter-invalidation-positioned-child.html TEST=fast/repaint/reflection-invalidation-positioned-child.html ========== to ========== Ensure filter and reflection outsets are applied on paint invalidation rect Previously filter and reflection outsets are applied in mapToVisualRectInAncestorSpace along the container chain. This missed filters/reflections between the object and its container. BUG=607989 TEST=fast/repaint/filter-invalidation-positioned-child.html TEST=fast/repaint/reflection-invalidation-positioned-child.html Committed: https://crrev.com/05fb4239a14a319f2a76de26b1f5a5413bb71ed1 Cr-Commit-Position: refs/heads/master@{#391525} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/05fb4239a14a319f2a76de26b1f5a5413bb71ed1 Cr-Commit-Position: refs/heads/master@{#391525} |
