Paint timing: unify image generating-node resolution via LayoutObject [chromium/src : main]
José Dapena Paz (Gerrit)
José Dapena Paz has uploaded the change for review![]( "Open in Gerrit")
Commit message
Paint timing: unify image generating-node resolution via LayoutObject
The background-image timing path in box_painter_base.cc and the soft
navigation tracker used paint_timing::ImageGeneratingNode(Node*), a
less exhaustive helper for finding the generating node.
LayoutObject::GeneratingNode() already implements a more complete
algorithm (walks anonymous parents and recursively unwinds nested
pseudos to UltimateOriginatingElement).
Move other users to use this complete implementation (passing the
LayoutObject when needed).
Change diff
Change information
- M third_party/blink/renderer/core/paint/box_fragment_painter.h
- M third_party/blink/renderer/core/paint/box_model_object_painter.cc
- M third_party/blink/renderer/core/paint/box_painter_base.cc
- M third_party/blink/renderer/core/paint/box_painter_base.h
- M third_party/blink/renderer/core/paint/timing/container_timing_paint_attribution_tracker.cc
- M third_party/blink/renderer/core/paint/timing/container_timing_paint_attribution_tracker_test.cc
- M third_party/blink/renderer/core/paint/timing/paint_timing_utils.h
- M third_party/blink/renderer/core/timing/soft_navigation_paint_attribution_tracker.cc
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 2 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
2 comments
layout_object ? layout_object->GeneratingNode() : nullptr;Can this really be null if we're painting it?
Related details
- José Dapena Paz
- Michal Mocny
- Philip Rogers
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. |
José Dapena Paz (Gerrit)
José Dapena Paz added 1 comment![]( "Open in Gerrit")
layout_object ? layout_object->GeneratingNode() : nullptr;Can this really be null if we're painting it?
Right, effectively non-null when painting, as layout_object will always have a value in the paths this is being called.
But I have doubts of what to do: if I remove the guard, then we assume LayoutObject is always non null. I could even change the type to be const LayoutObject& to enforce this expects a valid layout object.
But then other callers will need some check just to protect from the fact that a caller my assume layout_object_ is not null here. We could keep this as defensive code (and add a comment) or enforce this (and other callers) to use T&. But then we may need checks or similar guards somewhere else.
What would be the preferred way in Chromium for this?
I Kept it as pre-existing defensive code. And it actually still fires if the GeneratingNode() kill-switch is off, and the consumers want a non-null Node&. The layout_object ? ternary is redundant here though, can drop it.
Related details
- Michal Mocny
- Philip Rogers
- Scott Haseley
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. |
José Dapena Paz (Gerrit)
José Dapena Paz added 1 comment![]( "Open in Gerrit")
layout_object ? layout_object->GeneratingNode() : nullptr;José Dapena PazCan this really be null if we're painting it?
Right, effectively non-null when painting, as layout_object will always have a value in the paths this is being called.
But I have doubts of what to do: if I remove the guard, then we assume LayoutObject is always non null. I could even change the type to be const LayoutObject& to enforce this expects a valid layout object.
But then other callers will need some check just to protect from the fact that a caller my assume layout_object_ is not null here. We could keep this as defensive code (and add a comment) or enforce this (and other callers) to use T&. But then we may need checks or similar guards somewhere else.
What would be the preferred way in Chromium for this?
I Kept it as pre-existing defensive code. And it actually still fires if the GeneratingNode() kill-switch is off, and the consumers want a non-null Node&. The layout_object ? ternary is redundant here though, can drop it.
The last comment paragraph is from a misunderstanding of the comment, youc an ignore it.
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
layout_object ? layout_object->GeneratingNode() : nullptr;José Dapena PazCan this really be null if we're painting it?
José Dapena PazRight, effectively non-null when painting, as layout_object will always have a value in the paths this is being called.
But I have doubts of what to do: if I remove the guard, then we assume LayoutObject is always non null. I could even change the type to be const LayoutObject& to enforce this expects a valid layout object.
But then other callers will need some check just to protect from the fact that a caller my assume layout_object_ is not null here. We could keep this as defensive code (and add a comment) or enforce this (and other callers) to use T&. But then we may need checks or similar guards somewhere else.
What would be the preferred way in Chromium for this?
I Kept it as pre-existing defensive code. And it actually still fires if the GeneratingNode() kill-switch is off, and the consumers want a non-null Node&. The layout_object ? ternary is redundant here though, can drop it.
The last comment paragraph is from a misunderstanding of the comment, youc an ignore it.
Typically I prefer a CHECK in these situations where it doesn't make sense for it to be null (for documentation, prevention of untested branches / unreachable branches, catch bugs, etc.).
@p...@chromium.org: this is in paint code, do you have a preference? I typically CHECK() layout objects during paint since it would be bad if we were painting null ones.
Related details
- José Dapena Paz
- Michal Mocny
- Philip Rogers
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. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
// Base class for box painting. Has no dependencies on the layout tree and thusThis comment describes why we don't want to add LayoutObject to this class. Basically, when there is fragmentation (e.g., multi-col), we don't want anyone to look up information (like bounds) from the layout object, which would be incorrect.
The BoxPainterBase::node_ can be null for anonymous boxes, such as the layout object created for LayoutTableSection which doesn't have a corresponding node, or for pseudo elements like the anonymous LayoutImage created from content: url('[img]'), or scrollbar parts, and maybe more.
I think we can probably just pass in the generating node instead of a node to BoxPainterBase, but that is a potentially large refactor with risks. Instead, what do you think about just adding Node* generating_node_ as a member of this class instead of a LayoutObject?
Related details
- José Dapena Paz
- Michal Mocny
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. |
José Dapena Paz (Gerrit)
José Dapena Paz added 2 comments![]( "Open in Gerrit")
// Base class for box painting. Has no dependencies on the layout tree and thusThis comment describes why we don't want to add LayoutObject to this class. Basically, when there is fragmentation (e.g., multi-col), we don't want anyone to look up information (like bounds) from the layout object, which would be incorrect.
The BoxPainterBase::node_ can be null for anonymous boxes, such as the layout object created for LayoutTableSection which doesn't have a corresponding node, or for pseudo elements like the anonymous LayoutImage created from content: url('[img]'), or scrollbar parts, and maybe more.
I think we can probably just pass in the generating node instead of a node to BoxPainterBase, but that is a potentially large refactor with risks. Instead, what do you think about just adding Node* generating_node_ as a member of this class instead of a LayoutObject?
Done: BoxPainterBase now holds `Node* generating_node_` instead of the
LayoutObject. Callers resolve it
via PhysicalBoxFragment::GeneratingNode() and box.GeneratingNode(); no behaviour
change.
It's now resolved at painter construction instead of on background image processing. Problem: instead of traversing only for background images, it does it always (but only on constructor). That may have a cost.
If we want to make that be fetched lazily, I guess we would need to go back to use layout object? Not sure if we have a way to know beforehand that we are going to use the image paths.
layout_object ? layout_object->GeneratingNode() : nullptr;José Dapena PazCan this really be null if we're painting it?
José Dapena PazRight, effectively non-null when painting, as layout_object will always have a value in the paths this is being called.
But I have doubts of what to do: if I remove the guard, then we assume LayoutObject is always non null. I could even change the type to be const LayoutObject& to enforce this expects a valid layout object.
But then other callers will need some check just to protect from the fact that a caller my assume layout_object_ is not null here. We could keep this as defensive code (and add a comment) or enforce this (and other callers) to use T&. But then we may need checks or similar guards somewhere else.
What would be the preferred way in Chromium for this?
I Kept it as pre-existing defensive code. And it actually still fires if the GeneratingNode() kill-switch is off, and the consumers want a non-null Node&. The layout_object ? ternary is redundant here though, can drop it.
Scott HaseleyThe last comment paragraph is from a misunderstanding of the comment, youc an ignore it.
Typically I prefer a CHECK in these situations where it doesn't make sense for it to be null (for documentation, prevention of untested branches / unreachable branches, catch bugs, etc.).
@p...@chromium.org: this is in paint code, do you have a preference? I typically CHECK() layout objects during paint since it would be bad if we were painting null ones.
This line is gone, as we pass directly the generating node. No need for the check here anymore.
Related details
- Michal Mocny
- Philip Rogers
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. |
Philip Rogers (Gerrit)
Philip Rogers voted and added 2 comments![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
2 comments
// Base class for box painting. Has no dependencies on the layout tree and thusJosé Dapena PazThis comment describes why we don't want to add LayoutObject to this class. Basically, when there is fragmentation (e.g., multi-col), we don't want anyone to look up information (like bounds) from the layout object, which would be incorrect.
The BoxPainterBase::node_ can be null for anonymous boxes, such as the layout object created for LayoutTableSection which doesn't have a corresponding node, or for pseudo elements like the anonymous LayoutImage created from content: url('[img]'), or scrollbar parts, and maybe more.
I think we can probably just pass in the generating node instead of a node to BoxPainterBase, but that is a potentially large refactor with risks. Instead, what do you think about just adding Node* generating_node_ as a member of this class instead of a LayoutObject?
Done: BoxPainterBase now holds `Node* generating_node_` instead of the
LayoutObject. Callers resolve it
via PhysicalBoxFragment::GeneratingNode() and box.GeneratingNode(); no behaviour
change.It's now resolved at painter construction instead of on background image processing. Problem: instead of traversing only for background images, it does it always (but only on constructor). That may have a cost.
If we want to make that be fetched lazily, I guess we would need to go back to use layout object? Not sure if we have a way to know beforehand that we are going to use the image paths.
- José Dapena Paz
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
José Dapena Paz (Gerrit)
José Dapena Paz voted Commit-Queue+2![]( "Open in Gerrit")
- Michal Mocny
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
Paint timing: unify image generating-node resolution via LayoutObject
The background-image timing path in box_painter_base.cc and the soft
navigation tracker used paint_timing::ImageGeneratingNode(Node*), a
less exhaustive helper for finding the generating node.
LayoutObject::GeneratingNode() already implements a more complete
algorithm (walks anonymous parents and recursively unwinds nested
pseudos to UltimateOriginatingElement).
Move both callers to that implementation and drop the helper.
The soft navigation tracker already has the LayoutObject at hand. In the
paint path the generating node is resolved by BoxPainterBase's callers and
passed in as a Node*, so BoxPainterBase keeps having no dependency on the
layout tree.
- M third_party/blink/renderer/core/paint/box_fragment_painter.h
- M third_party/blink/renderer/core/paint/box_model_object_painter.cc
- M third_party/blink/renderer/core/paint/box_painter_base.cc
- M third_party/blink/renderer/core/paint/box_painter_base.h
- M third_party/blink/renderer/core/paint/timing/paint_timing_utils.h
- M third_party/blink/renderer/core/timing/soft_navigation_paint_attribution_tracker.cc
Code-Review: +1 by Scott Haseley, +1 by Philip Rogers
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |