ElementTiming/ContainerTiming: report paints of images in pseudo element contents [chromium/src : main]
José Dapena Paz (Gerrit)
José Dapena Paz has uploaded the change for review![]( "Open in Gerrit")
Commit message
ElementTiming/ContainerTiming: report paints of images in pseudo element contents
Added support for reporting the paints of images that are declared in the
content property of pseudo elements.
To implement it, we find the generating node:
- First, for anonymous nodes, we find the enclosing node that is not anonymous.
- Then, if it is a pseudo element, then we fetch the ultimate originating
element.
This is the element that is reported as lastPaintedElement in container timing
and as element in element timing.
Ticket for the discussion of what should happen with pseudo elements:
https://github.com/w3c/element-timing/issues/74
Change diff
Change information
- M third_party/blink/renderer/core/paint/image_painter.cc
- M third_party/blink/renderer/core/paint/timing/image_element_timing.cc
- A third_party/blink/web_tests/external/wpt/container-timing/tentative/pseudo-image-before-in-container-timing.html
- A third_party/blink/web_tests/external/wpt/container-timing/tentative/pseudo-with-image-content-in-container-timing.html
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
José Dapena Paz (Gerrit)
Message from José Dapena Paz![]( "Open in Gerrit")
Set Ready For Review
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
José Dapena Paz (Gerrit)
José Dapena Paz voted and added 1 comment![]( "Open in Gerrit")
Votes added by José Dapena Paz
| Commit-Queue | +1 |
1 comment
This is an early review request, to check if I am in the right path for supporting the failing pseudos.
Related details
- David Baron
- José Dapena Paz
- Michal Mocny
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Baron (Gerrit)
David Baron added 7 comments![]( "Open in Gerrit")
This is an early review request, to check if I am in the right path for supporting the failing pseudos.
I think if it works then it seems fine to me.
if (image_content &&
(IsA<HTMLImageElement>(node) || IsA<HTMLVideoElement>(node) ||
(!node && layout_image_.IsAnonymous())) &&I'm wondering whether this condition should be here at all. If I'm skimming the code correctly it looks like the remaining things that are excluded here are:
- the `<object>` element
- the `<input type=image>` element
- the `<picture>` element (maybe)
- images from the `list-style-image` property
It's not clear to me why any of these should be skipped given the rationale for this change. Would it make sense to drop this condition entirely (so the code is just `if (image_content && image_content->IsLoaded())`, and add some brief tests for the above?
Maybe this filename should have `background` in it somewhere, since from the filename I'd have expected this test be related to `content` rather than `background`.
<body>Please drop this `<body>` and let the `<style>` and `<script>` elements be in the `head`.
(Same for the other tests.)
background: url('/container-timing/resources/square100.png');
width: 100px;
height: 100px;This seems like it's a slightly more interesting test if the width and height are bigger than 100 and the image gets tiled. (Right now, it doesn't test *which* size is reported.)
Same for the element timing test.
let beforeRender;I think this `let` could, and probably should, be one scope further in (that is, inside the anonymous function passed to `async_test`).
content: url('/container-timing/resources/square100.png');
width: 100px;
height: 100px;This seems like it's a slightly more interesting test if the width and height are bigger than 100 and the image doesn't fill the element. (Right now, it doesn't test *which* size is reported.)
Same for the element timing test.
Related details
- José Dapena Paz
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 2 comments![]( "Open in Gerrit")
This is excellent! Thanks for actually getting to do it.
(I still haven't reviewed tests, and, I trust David's review of the implementation because for GeneratingNode etc, since I don't understand this area)
One note: the fix is for Element/Container timings, but for images we have a separate detector for LCP. I think its fine to land here first, but I want to try to consolidate these two image paint timing detectors together, same as for text.
if (image_content &&
(IsA<HTMLImageElement>(node) || IsA<HTMLVideoElement>(node) ||
(!node && layout_image_.IsAnonymous())) &&I'm wondering whether this condition should be here at all. If I'm skimming the code correctly it looks like the remaining things that are excluded here are:
- the `<object>` element
- the `<input type=image>` element
- the `<picture>` element (maybe)
- images from the `list-style-image` property
It's not clear to me why any of these should be skipped given the rationale for this change. Would it make sense to drop this condition entirely (so the code is just `if (image_content && image_content->IsLoaded())`, and add some brief tests for the above?
I am okay with this change for Element timing, though I'll try to dig into the original motivations. Perhaps it was as simple as the original patch for HTML Video support got this wrong, but maybe we had some specific use cases to restrict for images...
Related details
- José Dapena Paz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
if (image_content &&
(IsA<HTMLImageElement>(node) || IsA<HTMLVideoElement>(node) ||
(!node && layout_image_.IsAnonymous())) &&Michal MocnyI'm wondering whether this condition should be here at all. If I'm skimming the code correctly it looks like the remaining things that are excluded here are:
- the `<object>` element
- the `<input type=image>` element
- the `<picture>` element (maybe)
- images from the `list-style-image` property
It's not clear to me why any of these should be skipped given the rationale for this change. Would it make sense to drop this condition entirely (so the code is just `if (image_content && image_content->IsLoaded())`, and add some brief tests for the above?
I am okay with this change for Element timing, though I'll try to dig into the original motivations. Perhaps it was as simple as the original patch for HTML Video support got this wrong, but maybe we had some specific use cases to restrict for images...
Picture was apparently already working. It will report the img node inside (and that's where we should add the elementtiming. `containertiming` is going to allow getting that from `picture` node.
`<input type=image>`, `<object>` and `list-style-image` work with this change (last one also requiring pseudo support).
I will provide additional tests, and do the suggested clean ups. Finally, thanks to reducing this condition, the results are going to be more generic and we will get element timing support for more scenarios.
Related details
- David Baron
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
José Dapena Paz (Gerrit)
José Dapena Paz added 5 comments![]( "Open in Gerrit")
Maybe this filename should have `background` in it somewhere, since from the filename I'd have expected this test be related to `content` rather than `background`.
Done
Please drop this `<body>` and let the `<style>` and `<script>` elements be in the `head`.
(Same for the other tests.)
Done
background: url('/container-timing/resources/square100.png');
width: 100px;
height: 100px;This seems like it's a slightly more interesting test if the width and height are bigger than 100 and the image gets tiled. (Right now, it doesn't test *which* size is reported.)
Same for the element timing test.
Done
I think this `let` could, and probably should, be one scope further in (that is, inside the anonymous function passed to `async_test`).
Done
content: url('/container-timing/resources/square100.png');
width: 100px;
height: 100px;This seems like it's a slightly more interesting test if the width and height are bigger than 100 and the image doesn't fill the element. (Right now, it doesn't test *which* size is reported.)
Same for the element timing test.
Done
José Dapena Paz (Gerrit)
José Dapena Paz added 1 comment![]( "Open in Gerrit")
if (image_content &&
(IsA<HTMLImageElement>(node) || IsA<HTMLVideoElement>(node) ||
(!node && layout_image_.IsAnonymous())) &&I'm wondering whether this condition should be here at all. If I'm skimming the code correctly it looks like the remaining things that are excluded here are:
- the `<object>` element
- the `<input type=image>` element
- the `<picture>` element (maybe)
- images from the `list-style-image` property
It's not clear to me why any of these should be skipped given the rationale for this change. Would it make sense to drop this condition entirely (so the code is just `if (image_content && image_content->IsLoaded())`, and add some brief tests for the above?
José Dapena PazI am okay with this change for Element timing, though I'll try to dig into the original motivations. Perhaps it was as simple as the original patch for HTML Video support got this wrong, but maybe we had some specific use cases to restrict for images...
Picture was apparently already working. It will report the img node inside (and that's where we should add the elementtiming. `containertiming` is going to allow getting that from `picture` node.
`<input type=image>`, `<object>` and `list-style-image` work with this change (last one also requiring pseudo support).
I will provide additional tests, and do the suggested clean ups. Finally, thanks to reducing this condition, the results are going to be more generic and we will get element timing support for more scenarios.
Finally took the approach of removing all the checks so it includes more cases. So far it works.
Related details
- David Baron
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 4 comments![]( "Open in Gerrit")
This is great! And appreciate the new tests.
Do you think you might be interested in updating the LCP image paint timing detector to match, after this?
if (image_content &&
(IsA<HTMLImageElement>(node) || IsA<HTMLVideoElement>(node) ||
(!node && layout_image_.IsAnonymous())) &&Michal MocnyI'm wondering whether this condition should be here at all. If I'm skimming the code correctly it looks like the remaining things that are excluded here are:
- the `<object>` element
- the `<input type=image>` element
- the `<picture>` element (maybe)
- images from the `list-style-image` property
It's not clear to me why any of these should be skipped given the rationale for this change. Would it make sense to drop this condition entirely (so the code is just `if (image_content && image_content->IsLoaded())`, and add some brief tests for the above?
José Dapena PazI am okay with this change for Element timing, though I'll try to dig into the original motivations. Perhaps it was as simple as the original patch for HTML Video support got this wrong, but maybe we had some specific use cases to restrict for images...
José Dapena PazPicture was apparently already working. It will report the img node inside (and that's where we should add the elementtiming. `containertiming` is going to allow getting that from `picture` node.
`<input type=image>`, `<object>` and `list-style-image` work with this change (last one also requiring pseudo support).
I will provide additional tests, and do the suggested clean ups. Finally, thanks to reducing this condition, the results are going to be more generic and we will get element timing support for more scenarios.
Finally took the approach of removing all the checks so it includes more cases. So far it works.
I know this is a small change-- but I wonder if this is one that could be carved out into a finch experiment, to assess how many new candidates we get and what timing shift it causes.
Or, at least add a UseCounter for how often we arrive inside this branch, but None of the old conditions are met, so its a new source.
// Do not use LayoutObject::EnclosingNode as it does not check properly ifQ: should EnclosingNode be fixed, instead, or is this on purpose?
enclosing_node = e;Nit: you could just check if `IsPseudoElement()` here and return `UltimateOriginatingElement()` or `e`, then outside of the loop you `return nullptr`
Then you don't need `enclosing_node` or `break`
Related details
- David Baron
- José Dapena Paz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- David Baron
- José Dapena Paz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
if (image_content &&
(IsA<HTMLImageElement>(node) || IsA<HTMLVideoElement>(node) ||
(!node && layout_image_.IsAnonymous())) &&Michal MocnyI'm wondering whether this condition should be here at all. If I'm skimming the code correctly it looks like the remaining things that are excluded here are:
- the `<object>` element
- the `<input type=image>` element
- the `<picture>` element (maybe)
- images from the `list-style-image` property
It's not clear to me why any of these should be skipped given the rationale for this change. Would it make sense to drop this condition entirely (so the code is just `if (image_content && image_content->IsLoaded())`, and add some brief tests for the above?
José Dapena PazI am okay with this change for Element timing, though I'll try to dig into the original motivations. Perhaps it was as simple as the original patch for HTML Video support got this wrong, but maybe we had some specific use cases to restrict for images...
José Dapena PazPicture was apparently already working. It will report the img node inside (and that's where we should add the elementtiming. `containertiming` is going to allow getting that from `picture` node.
`<input type=image>`, `<object>` and `list-style-image` work with this change (last one also requiring pseudo support).
I will provide additional tests, and do the suggested clean ups. Finally, thanks to reducing this condition, the results are going to be more generic and we will get element timing support for more scenarios.
Michal MocnyFinally took the approach of removing all the checks so it includes more cases. So far it works.
I know this is a small change-- but I wonder if this is one that could be carved out into a finch experiment, to assess how many new candidates we get and what timing shift it causes.
Or, at least add a UseCounter for how often we arrive inside this branch, but None of the old conditions are met, so its a new source.
Marked as unresolved.
José Dapena Paz (Gerrit)
José Dapena Paz added 1 comment![]( "Open in Gerrit")
// Do not use LayoutObject::EnclosingNode as it does not check properly ifQ: should EnclosingNode be fixed, instead, or is this on purpose?
Yes, I tried to avoid a working function that assumes it will not get nullptr to be changed. Though, now you say this, I have doubts this could be really a good idea.
I checked, and the crash happens, as an example, when we get image finished event for a layout object that is not inserted yet in the layout tree and has no DOM node. I.e. the list markers, when added, and the image is cached and ready to paint, get the image finished event, and there is no node and we cannot resolve the pseudo either.
Maybe, question could be: would that mean we are getting, for some cases, the image finished event too much early, so we don't get then the timing event? I need to figure out the case for testing that reliably, but I suspect we may be missing events in that situation.
The solution would be checking when we add a layout object to a container if it may be interesting for element or container timing, and send the finished event at that point. Usually it will be accurate as when we get that scenario, we will complete the layout and paint later.
Related details
- David Baron
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
José Dapena Paz (Gerrit)
José Dapena Paz added 3 comments![]( "Open in Gerrit")
This is great! And appreciate the new tests.
Do you think you might be interested in updating the LCP image paint timing detector to match, after this?
Not a priority here, though I could try. Though, I'd love to get some guidance on where in LCP we are processing the layout images that could get discarded?
if (image_content &&
(IsA<HTMLImageElement>(node) || IsA<HTMLVideoElement>(node) ||
(!node && layout_image_.IsAnonymous())) &&Michal MocnyI'm wondering whether this condition should be here at all. If I'm skimming the code correctly it looks like the remaining things that are excluded here are:
- the `<object>` element
- the `<input type=image>` element
- the `<picture>` element (maybe)
- images from the `list-style-image` property
It's not clear to me why any of these should be skipped given the rationale for this change. Would it make sense to drop this condition entirely (so the code is just `if (image_content && image_content->IsLoaded())`, and add some brief tests for the above?
José Dapena PazI am okay with this change for Element timing, though I'll try to dig into the original motivations. Perhaps it was as simple as the original patch for HTML Video support got this wrong, but maybe we had some specific use cases to restrict for images...
José Dapena PazPicture was apparently already working. It will report the img node inside (and that's where we should add the elementtiming. `containertiming` is going to allow getting that from `picture` node.
`<input type=image>`, `<object>` and `list-style-image` work with this change (last one also requiring pseudo support).
I will provide additional tests, and do the suggested clean ups. Finally, thanks to reducing this condition, the results are going to be more generic and we will get element timing support for more scenarios.
Michal MocnyFinally took the approach of removing all the checks so it includes more cases. So far it works.
Michal MocnyI know this is a small change-- but I wonder if this is one that could be carved out into a finch experiment, to assess how many new candidates we get and what timing shift it causes.
Or, at least add a UseCounter for how often we arrive inside this branch, but None of the old conditions are met, so its a new source.
Marked as unresolved.
I guess UseCounter would be simpler, and it looks interesting to add it anyway. I guess it would be adding to ImageElementTiming::NotifyImageFinished, after checking it is needed for timing, so it counts only the elements that are matched because of the new conditions?
Finch seems more interesting though. What's the process to create a finch trial? Would it be just adding a runtime blink feature?
enclosing_node = e;Nit: you could just check if `IsPseudoElement()` here and return `UltimateOriginatingElement()` or `e`, then outside of the loop you `return nullptr`
Then you don't need `enclosing_node` or `break`
I will finally still reuse EnclosingNode(), but avoid the failing condition. We should not even try to get it if there is no node and no parent layout.
Related details
- David Baron
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
enclosing_node = e;José Dapena PazNit: you could just check if `IsPseudoElement()` here and return `UltimateOriginatingElement()` or `e`, then outside of the loop you `return nullptr`
Then you don't need `enclosing_node` or `break`
I will finally still reuse EnclosingNode(), but avoid the failing condition. We should not even try to get it if there is no node and no parent layout.
So, after trying again, another test is crashing. I will finally move to modify `LayoutObject::EnclosingNode` to return `nullptr` instead of crashing. I don't expect big performance problems as `LayoutObject::EnclosingNode` is not used in many places (and traversals are not expected to be long).
I guess the right solution would be actually knowing a condition we could check that will tell us that a DOM tree is detached by reading its child. But this solution is simpler for now.
Related details
- David Baron
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 3 comments![]( "Open in Gerrit")
José Dapena PazThis is great! And appreciate the new tests.
Do you think you might be interested in updating the LCP image paint timing detector to match, after this?
Not a priority here, though I could try. Though, I'd love to get some guidance on where in LCP we are processing the layout images that could get discarded?
Oh, it looks like we already don't constrain LCP candidates on these types, this is just an extra constraint added for Element Timing only.
Even more-so LGTM to remove to make consistent.
runtime enabled feature is all you need, yes. Then there is an internal system for configuring experiments which I could help set up. I think you might also not have access to chrome histogram data, but I could help verify any metric shifts.
I think a runtime flag would help qualify how much metrics shifted if we expect a significant effect of this change, while a UseCounter would just help confirm that this change rarely triggers and overall behaviour is similar to baseline.
---
IMHO for Element Timing, where we expect explicit opt-in for measures, and especially because LCP already doesn't constrain on these and is the more popular and default mechanism for Element Timing -- a single UseCounter is sufficient and should rarely trigger.
enclosing_node = e;José Dapena PazNit: you could just check if `IsPseudoElement()` here and return `UltimateOriginatingElement()` or `e`, then outside of the loop you `return nullptr`
Then you don't need `enclosing_node` or `break`
José Dapena PazI will finally still reuse EnclosingNode(), but avoid the failing condition. We should not even try to get it if there is no node and no parent layout.
So, after trying again, another test is crashing. I will finally move to modify `LayoutObject::EnclosingNode` to return `nullptr` instead of crashing. I don't expect big performance problems as `LayoutObject::EnclosingNode` is not used in many places (and traversals are not expected to be long).
I guess the right solution would be actually knowing a condition we could check that will tell us that a DOM tree is detached by reading its child. But this solution is simpler for now.
I'm not familiar enough to offer suggestions in this area-- defer to other reviewers
Related details
- David Baron
- José Dapena Paz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
I added both. Specially to have a kill switch... Maybe it would be better to set it already to enabled by default?
The problem: now there are several tests that fail with the default configuration. I need to find how to link tests with specific configuration flags.
Related details
- David Baron
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
José Dapena Paz (Gerrit)
Maybe adding a blink feature with the same name, and making it "experimental"?
José Dapena Paz (Gerrit)
José Dapena Paz added 3 comments![]( "Open in Gerrit")
base::FEATURE_DISABLED_BY_DEFAULT);I will move to use a runtime enabled feature for this. This way it will be enabled by default.
I will do this. Runtime feature set as experimetnal.
if (is_image_or_video_element) {Typo, should be `!image_or_video_element`.
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
(Patch is coming along great-- I didnt find anything beyond your own self-review comments)
Related details
- David Baron
- José Dapena Paz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
void LayoutImage::InsertedIntoTree() {LayoutImage also is a <video> and other things, is this desired for these objects as well?
Related details
- José Dapena Paz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
void LayoutImage::InsertedIntoTree() {LayoutImage also is a <video> and other things, is this desired for these objects as well?
To me, ideally yes. Though, I would actually like to verify how it works. Any example I could use to see how it works? Is it just adding a video node? Do I need to make it autoplay?
Related details
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
José Dapena Paz (Gerrit)
José Dapena Paz added 3 comments![]( "Open in Gerrit")
I will move to use a runtime enabled feature for this. This way it will be enabled by default.
Done
if (image_content &&
(IsA<HTMLImageElement>(node) || IsA<HTMLVideoElement>(node) ||
(!node && layout_image_.IsAnonymous())) &&Done
Typo, should be `!image_or_video_element`.
- David Baron
- Ian Kilpatrick
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
void LayoutImage::InsertedIntoTree() {José Dapena PazLayoutImage also is a <video> and other things, is this desired for these objects as well?
To me, ideally yes. Though, I would actually like to verify how it works. Any example I could use to see how it works? Is it just adding a video node? Do I need to make it autoplay?
After testing with <video>, I see elementtiming and containertiming both report if there is a `poster`. But not for the video contents themselves.
I think this is correct, and fits well with the purpose of showing any kind of image content.
Ideally in the future, we could add support for video contents in both. But for now it is not implemented (and should likely be another task and commit).
I will add tests for both container timing and element timing getting information from a video poster.
enclosing_node = e;Nit: you could just check if `IsPseudoElement()` here and return `UltimateOriginatingElement()` or `e`, then outside of the loop you `return nullptr`
Then you don't need `enclosing_node` or `break`
I will finally still reuse EnclosingNode(), but avoid the failing condition. We should not even try to get it if there is no node and no parent layout.
Michal MocnySo, after trying again, another test is crashing. I will finally move to modify `LayoutObject::EnclosingNode` to return `nullptr` instead of crashing. I don't expect big performance problems as `LayoutObject::EnclosingNode` is not used in many places (and traversals are not expected to be long).
I guess the right solution would be actually knowing a condition we could check that will tell us that a DOM tree is detached by reading its child. But this solution is simpler for now.
I'm not familiar enough to offer suggestions in this area-- defer to other reviewers
I finally adjusted the LayoutObject method to return nullptr in that case.
Related details
- Ian Kilpatrick
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
void LayoutImage::InsertedIntoTree() {José Dapena PazLayoutImage also is a <video> and other things, is this desired for these objects as well?
José Dapena PazTo me, ideally yes. Though, I would actually like to verify how it works. Any example I could use to see how it works? Is it just adding a video node? Do I need to make it autoplay?
After testing with <video>, I see elementtiming and containertiming both report if there is a `poster`. But not for the video contents themselves.
I think this is correct, and fits well with the purpose of showing any kind of image content.
Ideally in the future, we could add support for video contents in both. But for now it is not implemented (and should likely be another task and commit).
I will add tests for both container timing and element timing getting information from a video poster.
My mistake here: <video> poster is already supported without this patch. And this patch makes no difference with it. So I will not introduce a test for that here.
Related details
- Ian Kilpatrick
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
void LayoutImage::InsertedIntoTree() {José Dapena PazLayoutImage also is a <video> and other things, is this desired for these objects as well?
José Dapena PazTo me, ideally yes. Though, I would actually like to verify how it works. Any example I could use to see how it works? Is it just adding a video node? Do I need to make it autoplay?
José Dapena PazAfter testing with <video>, I see elementtiming and containertiming both report if there is a `poster`. But not for the video contents themselves.
I think this is correct, and fits well with the purpose of showing any kind of image content.
Ideally in the future, we could add support for video contents in both. But for now it is not implemented (and should likely be another task and commit).
I will add tests for both container timing and element timing getting information from a video poster.
My mistake here: <video> poster is already supported without this patch. And this patch makes no difference with it. So I will not introduce a test for that here.
This is I think a gap between element timing and LCP. LCP does support video, beyond poster images.
There are still some issues, and I've just updated Issue 372929290.
Perhaps this is yet another reason to merge image element and lcp timing detectors...
Related details
- Ian Kilpatrick
- José Dapena Paz
Code-Coverage
Code-Owners
Code-Review
| 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")
To the attention of the reviewers... Any other further action required? Maybe adding extra owners for specific reviews?
Related details
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
1 comment
This looks good to me, but patch needs to resolve merge conflict.
I still think the next followup is to see if the image paint timing detector used for LCP needs to be updated to match this new behaviour to reduce the differences (and ideally we just merge detectors completely)
Related details
- Ian Kilpatrick
- José Dapena Paz
Code-Coverage
Code-Owners
Code-Review
| 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")
This looks good to me, but patch needs to resolve merge conflict.
I still think the next followup is to see if the image paint timing detector used for LCP needs to be updated to match this new behaviour to reduce the differences (and ideally we just merge detectors completely)
Updated. Usual conflict with the metrics/web feature pair.
Related details
- Ian Kilpatrick
Code-Coverage
Code-Owners
Code-Review
| 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")
Adding reviewers for specific paths:
- Koji Ishii for core/layout
- Philip Rogers for core/paint
Related details
- Ian Kilpatrick
- Koji Ishii
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 2 comments![]( "Open in Gerrit")
void LayoutImage::InsertedIntoTree() {Is this a bugfix which is somewhat separate from the overall patch? If so, can you break this out into its own change and tests?
inline Node* GetGeneratingNode(const LayoutObject& layout_object) {Does this differ from `LayoutObject::GeneratingNode`? It seems like we should try to unify the logic, or document how it is different. I think it's possible that the existing function of the same name is missing some cases you are covering here.
Related details
- Ian Kilpatrick
- José Dapena Paz
- Koji Ishii
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
void LayoutImage::InsertedIntoTree() {Is this a bugfix which is somewhat separate from the overall patch? If so, can you break this out into its own change and tests?
From the patchset (6) that added this change:
Yes, I tried to avoid a working function that assumes it will not get nullptr to be changed. Though, now you say this, I have doubts this could be really a good idea.
>I checked, and the crash happens, as an example, when we get image finished event for a layout object that is not inserted yet in the layout tree and has no DOM node. I.e. the list markers, when added, and the image is cached and ready to paint, get the image finished event, and there is no node and we cannot resolve the pseudo either.
> Maybe, question could be: would that mean we are getting, for some cases, the image finished event too much early, so we don't get then the timing event? I need to figure out the case for testing that reliably, but I suspect we may be missing events in that situation.
> The solution would be checking when we add a layout object to a container if it may be interesting for element or container timing, and send the finished event at that point. Usually it will be accurate as when we get that scenario, we will complete the layout and paint later.
The solution for this problem was in two parts:
- One was making sure we do not crash if there is still no element
- Then considering the timing once we have the element information, and we know it should generate a paint.
The problem is that the case I found was very specific about being a pseudo element (list markers). But I could try to find a similar case, checking if containertiming is obtained. This would be elements that would not be considered for timing events (they do not have elementtiming or containertiming), but when added to the tree, are considered at that point.
So, I will try to find a reproducing case so I can split this change from the pseudo element support. If I cannot, I can still have this patch coming after landing pseudo element support, and add a specific case for it.
inline Node* GetGeneratingNode(const LayoutObject& layout_object) {Does this differ from `LayoutObject::GeneratingNode`? It seems like we should try to unify the logic, or document how it is different. I think it's possible that the existing function of the same name is missing some cases you are covering here.
Sure, there are a few differences:
- LayoutObject::GeneratingNode assumes GetNode() is not null. Here, as it does not act at the same points, GetNode() could be null. And this could be done in anonymous nodes also.
- For when we find a pseudo element, we use the specific element that originates it.
So, in the end, it is quite different. Originally this was a kind of safer GeneratingNode for this case, but it evolved a lot at later stages of the implementation.
What do you think if, instead, we rename this method to be more explicit about its use: `GetTimingNode()`?
We could also still add a comment explaining a bit the logic.
In the end this method does two different things:
- Finding an enclosing node (this is the part that `LayoutObject::GeneratingNode` is not doing).
- The part for pseudo elements about finding the ultimate originating element. This is a bit richer as it traverses more pseudo element ancestors, and also will check shadow host nodes, not only parent nodes. This part... may be interesting to add in `LayoutObject::GeneratingNode()`. In this case, we could just call `LayoutObjectGeneratingNode()` here. But, for that change, the risk is higher, as we would do a change that impacts all the callers of `GeneratingNode()`.
Related details
- Ian Kilpatrick
- Koji Ishii
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
inline Node* GetGeneratingNode(const LayoutObject& layout_object) {José Dapena PazDoes this differ from `LayoutObject::GeneratingNode`? It seems like we should try to unify the logic, or document how it is different. I think it's possible that the existing function of the same name is missing some cases you are covering here.
Sure, there are a few differences:
- LayoutObject::GeneratingNode assumes GetNode() is not null. Here, as it does not act at the same points, GetNode() could be null. And this could be done in anonymous nodes also.
- For when we find a pseudo element, we use the specific element that originates it.
So, in the end, it is quite different. Originally this was a kind of safer GeneratingNode for this case, but it evolved a lot at later stages of the implementation.
What do you think if, instead, we rename this method to be more explicit about its use: `GetTimingNode()`?
We could also still add a comment explaining a bit the logic.
In the end this method does two different things:
- Finding an enclosing node (this is the part that `LayoutObject::GeneratingNode` is not doing).
- The part for pseudo elements about finding the ultimate originating element. This is a bit richer as it traverses more pseudo element ancestors, and also will check shadow host nodes, not only parent nodes. This part... may be interesting to add in `LayoutObject::GeneratingNode()`. In this case, we could just call `LayoutObjectGeneratingNode()` here. But, for that change, the risk is higher, as we would do a change that impacts all the callers of `GeneratingNode()`.
I made `LayoutObject::EnclosingNode` return the same as the second part of the method, in https://chromium-review.googlesource.com/c/chromium/src/+/6898595
Running CQ does not throw any regression.
José Dapena Paz (Gerrit)
José Dapena Paz added 1 comment![]( "Open in Gerrit")
inline Node* GetGeneratingNode(const LayoutObject& layout_object) {José Dapena PazDoes this differ from `LayoutObject::GeneratingNode`? It seems like we should try to unify the logic, or document how it is different. I think it's possible that the existing function of the same name is missing some cases you are covering here.
José Dapena PazSure, there are a few differences:
- LayoutObject::GeneratingNode assumes GetNode() is not null. Here, as it does not act at the same points, GetNode() could be null. And this could be done in anonymous nodes also.
- For when we find a pseudo element, we use the specific element that originates it.
So, in the end, it is quite different. Originally this was a kind of safer GeneratingNode for this case, but it evolved a lot at later stages of the implementation.
What do you think if, instead, we rename this method to be more explicit about its use: `GetTimingNode()`?
We could also still add a comment explaining a bit the logic.
In the end this method does two different things:
- Finding an enclosing node (this is the part that `LayoutObject::GeneratingNode` is not doing).
- The part for pseudo elements about finding the ultimate originating element. This is a bit richer as it traverses more pseudo element ancestors, and also will check shadow host nodes, not only parent nodes. This part... may be interesting to add in `LayoutObject::GeneratingNode()`. In this case, we could just call `LayoutObjectGeneratingNode()` here. But, for that change, the risk is higher, as we would do a change that impacts all the callers of `GeneratingNode()`.
I made `LayoutObject::EnclosingNode` return the same as the second part of the method, in https://chromium-review.googlesource.com/c/chromium/src/+/6898595
Running CQ does not throw any regression.
Finally I did not modify LayoutObject::EnclosingNode, and I only update LayoutObject::GetGeneratingNode.
Related details
- Ian Kilpatrick
- Koji Ishii
- 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. |
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
Are you still hoping to update this patch?
Related details
- Ian Kilpatrick
- José Dapena Paz
- Koji Ishii
- Philip Rogers
José Dapena Paz (Gerrit)
José Dapena Paz added 2 comments![]( "Open in Gerrit")
Are you still hoping to update this patch?
I am retaking this this week. The pending issue is finding if I can split the patch in two, being one the image finished case.
But in december I spent some time trying to find a case where the issue happened without pseudos. And I could not find any.
So I will finally answer that I could not find a fix, so we would not split the patch in two.
It would be interesting to actually have some data to know if that case is really possible. WDYT?
void LayoutImage::InsertedIntoTree() {José Dapena PazIs this a bugfix which is somewhat separate from the overall patch? If so, can you break this out into its own change and tests?
From the patchset (6) that added this change:
Yes, I tried to avoid a working function that assumes it will not get nullptr to be changed. Though, now you say this, I have doubts this could be really a good idea.
>I checked, and the crash happens, as an example, when we get image finished event for a layout object that is not inserted yet in the layout tree and has no DOM node. I.e. the list markers, when added, and the image is cached and ready to paint, get the image finished event, and there is no node and we cannot resolve the pseudo either.
> Maybe, question could be: would that mean we are getting, for some cases, the image finished event too much early, so we don't get then the timing event? I need to figure out the case for testing that reliably, but I suspect we may be missing events in that situation.
> The solution would be checking when we add a layout object to a container if it may be interesting for element or container timing, and send the finished event at that point. Usually it will be accurate as when we get that scenario, we will complete the layout and paint later.The solution for this problem was in two parts:
- One was making sure we do not crash if there is still no element
- Then considering the timing once we have the element information, and we know it should generate a paint.
The problem is that the case I found was very specific about being a pseudo element (list markers). But I could try to find a similar case, checking if containertiming is obtained. This would be elements that would not be considered for timing events (they do not have elementtiming or containertiming), but when added to the tree, are considered at that point.
So, I will try to find a reproducing case so I can split this change from the pseudo element support. If I cannot, I can still have this patch coming after landing pseudo element support, and add a specific case for it.
I have been trying to find a case out of the pseudo elements that would reproduce this issue, and couldn't. So it may really be specific to pseudos.
In that condition, I think we don't want to split the patch in two.
Related details
- Ian Kilpatrick
- Koji Ishii
- 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 3 comments![]( "Open in Gerrit")
Votes added by Philip Rogers
| Code-Review | +1 |
3 comments
Node* node = GetNode();While I think this is correct, I'm worried about causing regressions because of the various callsites of this.
Can you put this change behind an enabled-by-default flag?
Can you add a test of this function? Something like:
```
TEST_F(LayoutObjectTest, GeneratingNode) {
SetBodyInnerHTML(R"HTML(
<style>
#pseudo::before { content: "before"; display: block; }
</style>
<div id="normal">Normal</div>
<div id="pseudo"></div>
<div id="table" style="display: table">
<div id="row" style="display: table-row">
Text
</div>
</div>
<div id="table2" style="display: table">
Text2
</div>
)HTML");
// 1. Normal element
LayoutObject* normal = GetLayoutObjectByElementId("normal");
EXPECT_EQ(GetElementById("normal"), normal->GeneratingNode());
// 2. Pseudo element ::before
Element* pseudo = GetElementById("pseudo");
LayoutObject* pseudo_layout = pseudo->GetLayoutObject();
LayoutObject* before = pseudo_layout->SlowFirstChild();
ASSERT_TRUE(before);
ASSERT_TRUE(before->IsPseudoElement());
EXPECT_EQ(pseudo, before->GeneratingNode());
// 3. Anonymous object
// The text node "Text" is inside "row". A table row expects cells.
// So an anonymous table cell should be created around the text.
// The hierarchy: LayoutTable -> LayoutTableRow -> LayoutTableCell (Anonymous) -> LayoutText
LayoutObject* row = GetLayoutObjectByElementId("row");
LayoutObject* anonymous_cell = row->SlowFirstChild();
ASSERT_TRUE(anonymous_cell);
EXPECT_TRUE(anonymous_cell->IsAnonymous());
EXPECT_TRUE(anonymous_cell->IsTableCell());
// GetNode() is null for anonymous objects, so it recurses up to Parent().
// Parent is "row", so it should return the row element.
EXPECT_EQ(GetElementById("row"), anonymous_cell->GeneratingNode());
// 4. Nested Anonymous objects
// Hierarchy: LayoutTable -> LayoutTableSection(Anon) -> LayoutTableRow(Anon) -> LayoutTableCell(Anon) -> LayoutText
LayoutObject* table2 = GetLayoutObjectByElementId("table2");
LayoutObject* section = table2->SlowFirstChild();
ASSERT_TRUE(section);
EXPECT_TRUE(section->IsAnonymous());
EXPECT_EQ(GetElementById("table2"), section->GeneratingNode());
LayoutObject* row2 = section->SlowFirstChild();
ASSERT_TRUE(row2);
EXPECT_TRUE(row2->IsAnonymous());
EXPECT_EQ(GetElementById("table2"), row2->GeneratingNode());
LayoutObject* cell2 = row2->SlowFirstChild();
ASSERT_TRUE(cell2);
EXPECT_TRUE(cell2->IsAnonymous());
EXPECT_EQ(GetElementById("table2"), cell2->GeneratingNode());
}
```
// Returns the styled node that caused the generation of this layoutObject.Can you update this comment?
Related details
- Ian Kilpatrick
- José Dapena Paz
- Koji Ishii
- 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. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
void LayoutImage::InsertedIntoTree() {José Dapena PazIs this a bugfix which is somewhat separate from the overall patch? If so, can you break this out into its own change and tests?
José Dapena PazFrom the patchset (6) that added this change:
Yes, I tried to avoid a working function that assumes it will not get nullptr to be changed. Though, now you say this, I have doubts this could be really a good idea.
>I checked, and the crash happens, as an example, when we get image finished event for a layout object that is not inserted yet in the layout tree and has no DOM node. I.e. the list markers, when added, and the image is cached and ready to paint, get the image finished event, and there is no node and we cannot resolve the pseudo either.
> Maybe, question could be: would that mean we are getting, for some cases, the image finished event too much early, so we don't get then the timing event? I need to figure out the case for testing that reliably, but I suspect we may be missing events in that situation.
> The solution would be checking when we add a layout object to a container if it may be interesting for element or container timing, and send the finished event at that point. Usually it will be accurate as when we get that scenario, we will complete the layout and paint later.The solution for this problem was in two parts:
- One was making sure we do not crash if there is still no element
- Then considering the timing once we have the element information, and we know it should generate a paint.
The problem is that the case I found was very specific about being a pseudo element (list markers). But I could try to find a similar case, checking if containertiming is obtained. This would be elements that would not be considered for timing events (they do not have elementtiming or containertiming), but when added to the tree, are considered at that point.
So, I will try to find a reproducing case so I can split this change from the pseudo element support. If I cannot, I can still have this patch coming after landing pseudo element support, and add a specific case for it.
I have been trying to find a case out of the pseudo elements that would reproduce this issue, and couldn't. So it may really be specific to pseudos.
In that condition, I think we don't want to split the patch in two.
Acknowledged