Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2787203002:
Fix GraphicsLayerUpdater::UpdateContext::compositingContainer() for corner cases (Closed)
|
Created:
3 years, 8 months ago by Xianzhu Modified:
3 years, 8 months ago Reviewers:
chrishtr CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix GraphicsLayerUpdater::UpdateContext::compositingContainer() for corner cases
For the following corner cases:
- parent layer is not a LayoutBlock (in case the layer or an ancestor
LayoutObject is a floating object under the parent layer),
- the current layer is a column spanner,
the current layer's compositingContainer may escape the normal layer hierarchy.
Fallback to slow path in the cases.
Also cleanup PaintLayer::containingLayer(), and added tests.
BUG=705073
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2787203002
Cr-Commit-Position: refs/heads/master@{#461221}
Committed: https://chromium.googlesource.com/chromium/src/+/7cd12d58b84042724fb696ba4980884cbad30e31
Patch Set 1 #
Total comments: 4
Patch Set 2 : - #Patch Set 3 : Fix #
Messages
Total messages: 20 (12 generated)
Description was changed from ========== Add a test about compositing container for layer under float under composited inline BUG=705732 ========== to ========== Add a test about compositing container for layer under float under composited inline BUG=705732 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org
https://codereview.chromium.org/2787203002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/GraphicsLayerUpdater.cpp (left): https://codereview.chromium.org/2787203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/GraphicsLayerUpdater.cpp:65: return layer.enclosingLayerWithCompositedLayerMapping(ExcludeSelf); The test (PaintLayerTest.CompositingContainerUnderFloat[UnderInline]) for the above code still passes with the above code removed. Do you remember why we added it? Perhaps the bug situation is also fixed by some other change after the above code was added. https://codereview.chromium.org/2787203002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerTest.cpp (right): https://codereview.chromium.org/2787203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerTest.cpp:550: TEST_P(PaintLayerTest, CompositingContainerUnderFloatUnderInline) { This test also passes without any real code change.
https://codereview.chromium.org/2787203002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/GraphicsLayerUpdater.cpp (left): https://codereview.chromium.org/2787203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/GraphicsLayerUpdater.cpp:65: return layer.enclosingLayerWithCompositedLayerMapping(ExcludeSelf); On 2017/03/30 at 22:23:12, Xianzhu wrote: > The test (PaintLayerTest.CompositingContainerUnderFloat[UnderInline]) for the above code still passes with the above code removed. Do you remember why we added it? Perhaps the bug situation is also fixed by some other change after the above code was added. The sequence of patches here was: https://codereview.chromium.org/2611233003 then https://codereview.chromium.org/2624843002 (fixes a bug introduced in the above) Maybe a test modeled on the situation in the tests in those CLs will expose the condition? It was a very obscure situation involving non-self- painting layers IIRC.
Description was changed from ========== Add a test about compositing container for layer under float under composited inline BUG=705732 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Cleanup and add a test about compositing for float under inline BUG=705732 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by wangxianzhu@chromium.org to run a CQ dry run
https://codereview.chromium.org/2787203002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/GraphicsLayerUpdater.cpp (left): https://codereview.chromium.org/2787203002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/GraphicsLayerUpdater.cpp:65: return layer.enclosingLayerWithCompositedLayerMapping(ExcludeSelf); On 2017/03/31 00:12:28, chrishtr wrote: > On 2017/03/30 at 22:23:12, Xianzhu wrote: > > The test (PaintLayerTest.CompositingContainerUnderFloat[UnderInline]) for the > above code still passes with the above code removed. Do you remember why we > added it? Perhaps the bug situation is also fixed by some other change after the > above code was added. > > The sequence of patches here was: > > https://codereview.chromium.org/2611233003 > > then > > https://codereview.chromium.org/2624843002 (fixes a bug introduced in > the above) > > Maybe a test modeled on the situation in the tests in those CLs will > expose the condition? It was a very obscure situation involving non-self- > painting layers IIRC. I just review the above CLs and my later related changes https://codereview.chromium.org/2650873002/ and https://codereview.chromium.org/2682933002/. It seems that we don't need the condition above because it seems to just return m_compositingAncestor. I added DCHECK_EQ(m_compositingAncestor, layer.enclosingLayerWithCompositedLayerMapping(ExcludeSelf)) here in the latest patch set, and all unit tests and paint/, compositing/ layout tests passed locally.
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: This issue passed the CQ dry run.
On 2017/03/31 at 04:14:43, wangxianzhu wrote: > https://codereview.chromium.org/2787203002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/compositing/GraphicsLayerUpdater.cpp (left): > > https://codereview.chromium.org/2787203002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/compositing/GraphicsLayerUpdater.cpp:65: return layer.enclosingLayerWithCompositedLayerMapping(ExcludeSelf); > On 2017/03/31 00:12:28, chrishtr wrote: > > On 2017/03/30 at 22:23:12, Xianzhu wrote: > > > The test (PaintLayerTest.CompositingContainerUnderFloat[UnderInline]) for the > > above code still passes with the above code removed. Do you remember why we > > added it? Perhaps the bug situation is also fixed by some other change after the > > above code was added. > > > > The sequence of patches here was: > > > > https://codereview.chromium.org/2611233003 > > > > then > > > > https://codereview.chromium.org/2624843002 (fixes a bug introduced in > > the above) > > > > Maybe a test modeled on the situation in the tests in those CLs will > > expose the condition? It was a very obscure situation involving non-self- > > painting layers IIRC. > > I just review the above CLs and my later related changes https://codereview.chromium.org/2650873002/ and https://codereview.chromium.org/2682933002/. > > It seems that we don't need the condition above because it seems to just return m_compositingAncestor. I added DCHECK_EQ(m_compositingAncestor, layer.enclosingLayerWithCompositedLayerMapping(ExcludeSelf)) here in the latest patch set, and all unit tests and paint/, compositing/ layout tests passed locally. Weird. Does the clusterfuzz bug in crbug.com/675205 no longer repro even with this code removed?
Description was changed from ========== Cleanup and add a test about compositing for float under inline BUG=705732 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix GraphicsLayerUpdater::UpdateContext::compositingContainer() for corner cases For the following corner cases: - parent layer is not a LayoutBlock (in case the layer or an ancestor LayoutObject is a floating object under the parent layer), - the current layer is a column spanner, the current layer's compositingContainer may escape the normal layer hierarchy. Fallback to slow path in the cases. Also cleanup PaintLayer::containingLayer(), and added tests. BUG=705732 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
On 2017/03/31 16:06:17, chrishtr wrote: > On 2017/03/31 at 04:14:43, wangxianzhu wrote: > > > https://codereview.chromium.org/2787203002/diff/1/third_party/WebKit/Source/c... > > File > third_party/WebKit/Source/core/layout/compositing/GraphicsLayerUpdater.cpp > (left): > > > > > https://codereview.chromium.org/2787203002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/layout/compositing/GraphicsLayerUpdater.cpp:65: > return layer.enclosingLayerWithCompositedLayerMapping(ExcludeSelf); > > On 2017/03/31 00:12:28, chrishtr wrote: > > > On 2017/03/30 at 22:23:12, Xianzhu wrote: > > > > The test (PaintLayerTest.CompositingContainerUnderFloat[UnderInline]) for > the > > > above code still passes with the above code removed. Do you remember why we > > > added it? Perhaps the bug situation is also fixed by some other change after > the > > > above code was added. > > > > > > The sequence of patches here was: > > > > > > https://codereview.chromium.org/2611233003 > > > > > > then > > > > > > https://codereview.chromium.org/2624843002 (fixes a bug introduced in > > > the above) > > > > > > Maybe a test modeled on the situation in the tests in those CLs will > > > expose the condition? It was a very obscure situation involving non-self- > > > painting layers IIRC. > > > > I just review the above CLs and my later related changes > https://codereview.chromium.org/2650873002/ and > https://codereview.chromium.org/2682933002/. > > > > It seems that we don't need the condition above because it seems to just > return m_compositingAncestor. I added DCHECK_EQ(m_compositingAncestor, > layer.enclosingLayerWithCompositedLayerMapping(ExcludeSelf)) here in the latest > patch set, and all unit tests and paint/, compositing/ layout tests passed > locally. > > Weird. Does the clusterfuzz bug in crbug.com/675205 no longer repro even with > this code removed? I didn't reproduce the bug, but reproduced with new tests. Now the CL cleans up and fixes the issues.
The CQ bit was checked by chrishtr@chromium.org
lgtm
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": 1490988092478780,
"parent_rev": "a459a6e71008ba58aaec1a9a95ac20b5864d5e82", "commit_rev":
"7cd12d58b84042724fb696ba4980884cbad30e31"}
Message was sent while issue was closed.
Description was changed from ========== Fix GraphicsLayerUpdater::UpdateContext::compositingContainer() for corner cases For the following corner cases: - parent layer is not a LayoutBlock (in case the layer or an ancestor LayoutObject is a floating object under the parent layer), - the current layer is a column spanner, the current layer's compositingContainer may escape the normal layer hierarchy. Fallback to slow path in the cases. Also cleanup PaintLayer::containingLayer(), and added tests. BUG=705732 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Fix GraphicsLayerUpdater::UpdateContext::compositingContainer() for corner cases For the following corner cases: - parent layer is not a LayoutBlock (in case the layer or an ancestor LayoutObject is a floating object under the parent layer), - the current layer is a column spanner, the current layer's compositingContainer may escape the normal layer hierarchy. Fallback to slow path in the cases. Also cleanup PaintLayer::containingLayer(), and added tests. BUG=705732 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2787203002 Cr-Commit-Position: refs/heads/master@{#461221} Committed: https://chromium.googlesource.com/chromium/src/+/7cd12d58b84042724fb696ba4980... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/7cd12d58b84042724fb696ba4980...
Message was sent while issue was closed.
Description was changed from ========== Fix GraphicsLayerUpdater::UpdateContext::compositingContainer() for corner cases For the following corner cases: - parent layer is not a LayoutBlock (in case the layer or an ancestor LayoutObject is a floating object under the parent layer), - the current layer is a column spanner, the current layer's compositingContainer may escape the normal layer hierarchy. Fallback to slow path in the cases. Also cleanup PaintLayer::containingLayer(), and added tests. BUG=705732 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2787203002 Cr-Commit-Position: refs/heads/master@{#461221} Committed: https://chromium.googlesource.com/chromium/src/+/7cd12d58b84042724fb696ba4980... ========== to ========== Fix GraphicsLayerUpdater::UpdateContext::compositingContainer() for corner cases For the following corner cases: - parent layer is not a LayoutBlock (in case the layer or an ancestor LayoutObject is a floating object under the parent layer), - the current layer is a column spanner, the current layer's compositingContainer may escape the normal layer hierarchy. Fallback to slow path in the cases. Also cleanup PaintLayer::containingLayer(), and added tests. BUG=705073 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2787203002 Cr-Commit-Position: refs/heads/master@{#461221} Committed: https://chromium.googlesource.com/chromium/src/+/7cd12d58b84042724fb696ba4980... ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
