Reapply "[layout] Reinsert LayoutObjects which undergo a in-flow state change." [chromium/src : main]
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
I'll land this after the next branch point next week.
For the delta between the reapply and the update see:
https://chromium-review.googlesource.com/c/chromium/src/+/7427180/1..5
All a just nullptr checks so for. Clusterfuzz seemed to do a good job finding issues, so we'll see what else it finds in the reapply.
Related details
- Morten Stenshorne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick voted Code-Review-1![]( "Open in Gerrit")
| Code-Review | -1 |
Related details
- Morten Stenshorne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne added 2 comments![]( "Open in Gerrit")
All the new nullptr checks... are we sure we really should be inside these functions if we have a nullptr?
} else if (Layer() && (RuntimeEnabledFeatures::We'll now do this for the root layer. Any concerns?
Related details
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ian Kilpatrick
| Code-Review | +0 |
2 comments
All the new nullptr checks... are we sure we really should be inside these functions if we have a nullptr?
I think so - an arguable one is `DirtyVisibleContentStatus` - but it also calls `MarkAncestorChainForFlagsUpdate` which has a side-effect on the current layer which i think is needed.
} else if (Layer() && (RuntimeEnabledFeatures::We'll now do this for the root layer. Any concerns?
I don't think so - (can we even change the style for the root layer?).
This part of the change was reverted in:
https://chromiumdash.appspot.com/commit/d19611f290f1c793202dacb7ce790c86ed6db9c3
and clusterfuzz seemed ok with it for a few weeks (compared with a few hours for the rest of this change).
Related details
- Morten Stenshorne
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Morten Stenshorne (Gerrit)
Morten Stenshorne voted and added 1 comment![]( "Open in Gerrit")
Votes added by Morten Stenshorne
| Code-Review | +1 |
1 comment
Ian KilpatrickWe'll now do this for the root layer. Any concerns?
I don't think so - (can we even change the style for the root layer?).
This part of the change was reverted in:
https://chromiumdash.appspot.com/commit/d19611f290f1c793202dacb7ce790c86ed6db9c3and clusterfuzz seemed ok with it for a few weeks (compared with a few hours for the rest of this change).
I don't think so - (can we even change the style for the root layer?).
Yes, we can, via StyleResolver::PropagateStyleToViewport(), but in order to end up in this particular code path for LayoutView, LayerTypeRequired() would have to say that a layer is no longer required. That seems impossible.
This part of the change was reverted in:
https://chromiumdash.appspot.com/commit/d19611f290f1c793202dacb7ce790c86ed6db9c3and clusterfuzz seemed ok with it for a few weeks (compared with a few hours for the rest of this change).
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ian Kilpatrick
| Commit-Queue | +2 |
I'll land this after the next branch point next week.
For the delta between the reapply and the update see:
https://chromium-review.googlesource.com/c/chromium/src/+/7427180/1..5All a just nullptr checks so for. Clusterfuzz seemed to do a good job finding issues, so we'll see what else it finds in the reapply.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Reapply "[layout] Reinsert LayoutObjects which undergo a in-flow state change."
This reverts commit 07002c40c631b6ee35ee32d2bbccacc75e9052a0.
This patch reinserts layout-objects into the layout-tree when their
out-of-flow state changes. (E.g. a change from position:static to
position:absolute or float:left to float:none).
When their out-of-flow state changes we'll remove it from the
layout-object tree, update the style, then reinsert into the tree. This
is similar to layout-tree reattachment, except the layout-object (and
related objects/caches) are kept around.
This simplifies our logic when style changes in this way. Previously
we'd need to mutate the layout tree during LayoutObject::SetStyle which
was complex and error prone, and a source of bugs.
See removal patch for complexity we can remove:
https://chromium-review.googlesource.com/c/chromium/src/+/7263470
All behaviour changes should be behind the flag:
LayoutReinsertOnInFlowStateChange
- M third_party/blink/renderer/core/dom/element.cc
- M third_party/blink/renderer/core/layout/layout_block_flow.cc
- M third_party/blink/renderer/core/layout/layout_box.cc
- M third_party/blink/renderer/core/layout/layout_box_model_object.cc
- M third_party/blink/renderer/core/layout/layout_inline.cc
- M third_party/blink/renderer/core/layout/layout_object.cc
- M third_party/blink/renderer/core/layout/layout_object.h
- M third_party/blink/renderer/core/paint/paint_layer.cc
- M third_party/blink/renderer/core/style/computed_style.cc
- M third_party/blink/renderer/core/style/computed_style.h
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- A third_party/blink/web_tests/external/wpt/css/CSS2/floats/crashtests/float-dynamic-change-1.html
- A third_party/blink/web_tests/external/wpt/css/CSS2/floats/crashtests/float-dynamic-change-2.html
- A third_party/blink/web_tests/external/wpt/css/CSS2/floats/crashtests/float-dynamic-change-3.html
- A third_party/blink/web_tests/external/wpt/css/CSS2/floats/crashtests/float-dynamic-change-4.html
- M third_party/blink/web_tests/paint/invalidation/position/position-change-keeping-geometry-expected.txt
- M third_party/blink/web_tests/paint/invalidation/position/positioned-document-element-expected.txt
- M third_party/blink/web_tests/platform/mac/paint/invalidation/position/position-change-keeping-geometry-expected.txt
Code-Review: +1 by Morten Stenshorne
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/57186
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |