[soft navs] Replace the need to "reset" LCP just to measure soft-LCP [chromium/src : main]
Michal Mocny (Gerrit)
Michal Mocny added 1 comment![]( "Open in Gerrit")
This is a bit of an early review request. The mostly just removes code and adds back the minimum needed to still measure soft-nav-entry, without issuing LCP entry.
There are two LCP WPT that (expectedly) fail, and one test for Image based "distant-leaf". I tested the distant-leaf use case locally and I do see a soft-nav entry emitted for it, but this CL changes the timing for soft-nav paint to come after Images are fully loaded, so maybe just needs more timeout? Or maybe its related to buffered:true not working?
Still digging.
Related details
- Johannes Henkel
- Scott Haseley
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 1 comment![]( "Open in Gerrit")
This patch is currently crashing renderer due to text paint aggregation. Happens more often with devtools. Feels likely related to Context* leaks between windows, now that we are storing it on the Record.
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
This is a bit of an early review request. The mostly just removes code and adds back the minimum needed to still measure soft-nav-entry, without issuing LCP entry.
There are two LCP WPT that (expectedly) fail, and one test for Image based "distant-leaf". I tested the distant-leaf use case locally and I do see a soft-nav entry emitted for it, but this CL changes the timing for soft-nav paint to come after Images are fully loaded, so maybe just needs more timeout? Or maybe its related to buffered:true not working?
Still digging.
Is the new test passing for you? It did for me yesterday, but may be worth confirming in your context.
lcp-unbuffered.html from https://chromium-review.googlesource.com/c/chromium/src/+/6634990
Related details
- Michal Mocny
- Scott Haseley
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 1 comment![]( "Open in Gerrit")
Johannes HenkelThis is a bit of an early review request. The mostly just removes code and adds back the minimum needed to still measure soft-nav-entry, without issuing LCP entry.
There are two LCP WPT that (expectedly) fail, and one test for Image based "distant-leaf". I tested the distant-leaf use case locally and I do see a soft-nav entry emitted for it, but this CL changes the timing for soft-nav paint to come after Images are fully loaded, so maybe just needs more timeout? Or maybe its related to buffered:true not working?
Still digging.
Is the new test passing for you? It did for me yesterday, but may be worth confirming in your context.
lcp-unbuffered.html from https://chromium-review.googlesource.com/c/chromium/src/+/6634990
That one also isnt-- but I am breaking LCP reporting with this patch. (I mean, I don't even try to emit any soft-LCP candidates now).
The next patch in this series will re-do the mechanism for reporting soft-LCP and that test should start to pass again.
We will have a temporary moment w/o soft-LCP. We might not want to have that state on ToT and so we might want to merge both patches, but at least for review and early testing, I think it makes sense to keep separate.
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel added 8 comments![]( "Open in Gerrit")
Read through this, mostly focusing on the first parts with the detectors and sprinkled some nitpicks consider or not as you see fit. Epic change! :-)
SoftNavigationContext* context = nullptr;
if (LocalDOMWindow* window = frame_view_->GetFrame().DomWindow()) {
if (SoftNavigationHeuristics* heuristics =
window->GetSoftNavigationHeuristics()) {
context = heuristics->IsNeededForTiming(node);
}
}could / should this be in a helper method, and only get invoked once you have a recorded image?
e.g. down just before l. 365, maybe it could say something along the lines of
SoftNavigationContext* context = GetContextIfNeededForTiming(node);
(or could pass the local dom window into there as well if you want to make it a helper in an anonymous namespace)
// above, since they both checkit feels like you were about to say something more here, perhaps?
if (record->soft_navigation_context_) {Could this one effectively be an older context, since the context only gets updated if records_manager_ found the image in l. 363? Is that OK? (I think maybe yes - just means that an earlier interaction causes this paint to continue to trickle?)
// If we are recording LCP, take the timing unless the currect LCP is alreadyPlease fix this WARNING reported by Spellchecker: "currect" is a possible misspelling of "correct" or "current".
Analyzer Description: Checks for common typos.
Owner: chops-so...@google.com
"currect" is a possible misspelling of "correct" or "current".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
if (bpp < kMinimumEntropyForLCP) {So this entropy thing will now apply to both cases above, is that right?
Perhaps could move it earlier in the method / somedhow merge it with the other early exit?
text_update_result.first, image_update_result.first, false);nitpick: is it easy-ish to put a name for the bool here still? is the AI right? :-)
// be way more overhead (untill paint scoping). Consider maybe justPlease fix this WARNING reported by Spellchecker: "untill" is a possible misspelling of "until".
Analyzer Description: Checks for common typos.
Owner: chops-so...@google.com
"untill" is a possible misspelling of "until".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Related details
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 14 comments![]( "Open in Gerrit")
if (record->soft_navigation_context_) {Could this one effectively be an older context, since the context only gets updated if records_manager_ found the image in l. 363? Is that OK? (I think maybe yes - just means that an earlier interaction causes this paint to continue to trickle?)
Also, what happens if the image record's element's context changes? (I think that's a different question).
frame_visual_rect, root_visual_rect, record_id.GetHash(), nullptr);maybe `/*soft_navigation_context=/*nullptr` for readability?
bool timing_needed_for_lcp =Any problems not returning if `timing_needed_for_lcp` is false? Wondering since you convinced yourself in the last patch to add this here, so wondering if we need a further check somewhere else after calling this.
if (!timing_needed_for_lcp && !timing_needed_for_soft_nav) {nit: consider just checking `soft_navigation_context` here and avoid the temporary. It's probably clear enough from the variable name and comment.
if (bpp < kMinimumEntropyForLCP) {So this entropy thing will now apply to both cases above, is that right?
Perhaps could move it earlier in the method / somedhow merge it with the other early exit?
FWIW I think having a separate check is nice here, to differentiate the reasons we early-out and keep the conditions fairly simple.
frame->View()->OnFirstContentfulPaint();The indent is off here - might need `git cl format`?
first_contentful_paint_presentation_ignoring_soft_navigations_.is_null());Can this be renamed?
return !recorded_set_.Contains(&object);FYI: This was the line that was causing innerText to fail. Note that the TextRecord is associated with the aggregating node.
if (record->soft_navigation_context_) {I think this needs a null guard (MaybeRecordTextRecord can return nullptr)
root_visual_rect, 0u, false /* is_needed_for_timing */, nullptr);style nit: perhaps `/*soft_navigation_context=*/nullptr` (I think the one for false before it is on the wrong side, so maybe fix that too).
if (LocalDOMWindow* window = frame_view_->GetFrame().DomWindow()) {Same as image -- can this be null?
DOMWindow* sourcen);```suggestion
DOMWindow* source);
```
SoftNavigationContext* IsNeededForTiming(Node* node);Maybe`GetSoftNavigationContextForTiming()`? I've added a few (to be reviewed) `GetSoftNavigationContextFor_X()` in the new class for pre-paint attribution.
Related details
- 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 19 comments![]( "Open in Gerrit")
SoftNavigationContext* context = nullptr;
if (LocalDOMWindow* window = frame_view_->GetFrame().DomWindow()) {
if (SoftNavigationHeuristics* heuristics =
window->GetSoftNavigationHeuristics()) {
context = heuristics->IsNeededForTiming(node);
}
}could / should this be in a helper method, and only get invoked once you have a recorded image?
e.g. down just before l. 365, maybe it could say something along the lines of
SoftNavigationContext* context = GetContextIfNeededForTiming(node);
(or could pass the local dom window into there as well if you want to make it a helper in an anonymous namespace)
In all paths we need to check for context, so I thought doing it here was best.
If its an old record, we need to see if context changed.
If its a new record, we need to save the first value.
If anything needs a helper, its just making it easier to the the SNH instance. But I think we should discuss how we could re-organize the class relationships in a future patch.
// above, since they both checkit feels like you were about to say something more here, perhaps?
I no longe recall :P I think something along the lines of "check if first video frame was shown" but I think its a redundant comment. I'll remove this.
if (record->soft_navigation_context_) {Scott HaseleyCould this one effectively be an older context, since the context only gets updated if records_manager_ found the image in l. 363? Is that OK? (I think maybe yes - just means that an earlier interaction causes this paint to continue to trickle?)
Also, what happens if the image record's element's context changes? (I think that's a different question).
Nit: Both branches assign context (Line 375 makes a new record with initial context)
Whenever we paint, we check if we need to "update" to a newer context, and attribute that paint to this latest context. We will always do a lookup (currently via tree walk) for every paint of every node.
(Matter of fact, I could add a CHECK here that this `== context` from above.)
This is also why the Pre-Paint walk will be quite useful... we will "just know" the context automatically.
---
However, there are some risks in other places which might need careful audit:
- After paint we still have to measure presentation time, and we'll update the record timestamps, and then call update LCP candidate... We won't re-check context if dom changed in the mean time... (small risk)
- After a Context "finds" a largest image/text candidate it caches a reference to it. If the context "for that record" changes, we won't "remove" that record from any existing Contexts. In theory, that means that Context A could paint media and then Context B could paint over that same media, and we risk Context A "seeing it". (More serious risk, but perhaps we are fine in current patch given other constraints).
- Others?
frame_visual_rect, root_visual_rect, record_id.GetHash(), nullptr);Michal Mocnymaybe `/*soft_navigation_context=/*nullptr` for readability?
Done (I have a vscode plugin that does this automatically and the extra comments actually make this less readable-- but I agree for code search etc it is useful)
// If we are recording LCP, take the timing unless the currect LCP is alreadyPlease fix this WARNING reported by Spellchecker: "currect" is a possible misspelling of "correct" or "current".
Analyzer Description: Checks for common typos.
Owner: chops-so...@google.com"currect" is a possible misspelling of "correct" or "current".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Done
// largerMichal Mocny```suggestion
// larger.
```
Done
bool timing_needed_for_lcp =Any problems not returning if `timing_needed_for_lcp` is false? Wondering since you convinced yourself in the last patch to add this here, so wondering if we need a further check somewhere else after calling this.
soft-LCP is a type of LCP.
Although there shouldn't be any far downstream side effects since LCP calculator checks if LCP is stopped-- I do think we could move this check down just a but further to limit how much needless bookkeeping we do for hard-LCP after its been stopped.
I'll work on that next.
if (!timing_needed_for_lcp && !timing_needed_for_soft_nav) {nit: consider just checking `soft_navigation_context` here and avoid the temporary. It's probably clear enough from the variable name and comment.
I... disagree. But only because I've been trying to reverse engineer all the places where we have various checks which seems obvious but are subtle. I'm trying to unify to similar names and conditions as needed.
(I agree this one is pretty simple)
if (bpp < kMinimumEntropyForLCP) {Scott HaseleySo this entropy thing will now apply to both cases above, is that right?
Perhaps could move it earlier in the method / somedhow merge it with the other early exit?
FWIW I think having a separate check is nice here, to differentiate the reasons we early-out and keep the conditions fairly simple.
This early exit always applied to soft-LCP, but, it now also applies to soft-nav-entry paint area requirement since now we only record if we create a record (and after its loaded).
I will leave it as-is.
frame->View()->OnFirstContentfulPaint();The indent is off here - might need `git cl format`?
Git cl format actually isn't fixing this! (I get warnings when I upload otherwise).
I actually manually fixed it once... and I think cl format actually broke it back ><. I'll try again.
first_contentful_paint_presentation_ignoring_soft_navigations_.is_null());Michal MocnyCan this be renamed?
Done
text_update_result.first, image_update_result.first, false);nitpick: is it easy-ish to put a name for the bool here still? is the AI right? :-)
Done
return !recorded_set_.Contains(&object);FYI: This was the line that was causing innerText to fail. Note that the TextRecord is associated with the aggregating node.
Agreed. I will try to do something similar as I did for Images, where I can check if the Node's Context was "changed" since last paint, and re-record paint if so.
However:
(a) But this requires keeping TextRecords around (I wrote this up in a comment or crbug, somewhere...)
(b) it requires a context dom tree walk here... and we would do that for a lot of nodes. Pre-Paint would fix that
(I might even remove the Image re-recording part from this patch, since its fairly new and unneeded.)
// be way more overhead (untill paint scoping). Consider maybe justPlease fix this WARNING reported by Spellchecker: "untill" is a possible misspelling of "until".
Analyzer Description: Checks for common typos.
Owner: chops-so...@google.com"untill" is a possible misspelling of "until".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Acknowledged
if (record->soft_navigation_context_) {I think this needs a null guard (MaybeRecordTextRecord can return nullptr)
Ah-- I think you found my bug!
I switched to using `context` directly since it should always be the same, but, we really do only want to do this when record is actually created.
root_visual_rect, 0u, false /* is_needed_for_timing */, nullptr);style nit: perhaps `/*soft_navigation_context=*/nullptr` (I think the one for false before it is on the wrong side, so maybe fix that too).
Done
if (LocalDOMWindow* window = frame_view_->GetFrame().DomWindow()) {Same as image -- can this be null?
Do you mean to suggest that we don't need to check if its null?
DOMWindow* sourcen);Michal Mocny```suggestion
DOMWindow* source);
```
Done
SoftNavigationContext* IsNeededForTiming(Node* node);Maybe`GetSoftNavigationContextForTiming()`? I've added a few (to be reviewed) `GetSoftNavigationContextFor_X()` in the new class for pre-paint attribution.
No joke, I had this same thought but didn't have time to come up with a new name..
This was my CL review bait :P Keen eyes!
I was thinking something like "TryGetParentContext", but I think the input Node* output SNC* makes this fairly self documenting.
Related details
- Johannes Henkel
- Scott Haseley
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 1 comment![]( "Open in Gerrit")
bool TextPaintTimingDetector::ShouldWalkObject(Note to reviewers:
Until we have "pre-paint" Context* caching, trying to ask the question "ShouldWalkObject" requires a dom walk. This might happen for a lot of nodes.
This feels like it could be more expensive than just saying "yes", and allowing the .Contains check below, which will usually still return false, but I'm not sure.
It does currently mean that some text nodes which were never previously recorded, might start to become recorded.
--
Some ideas to make this better:
- Until we have Pre-Paint walk to simplify getting context, maybe I could at least just ask SNH if there is any context/soft-nav at all going on.
- Maybe we should still be adding nodes which are skipped to recorded_set. Then, we could check recorded_set first, and only then check if is_needed_for_soft_nav... If not, still add to recorded_set (after all, Image paint timing does this-- it adds to recorded set even if decides not to measure paint or create a Record)
Related details
- Johannes Henkel
- Scott Haseley
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")
records_manager_.RecordFirstPaintAndMaybeCreateImageRecord(Woops-- there was a bug here I forgot to assign `record =` so we wouldnt "see" images the first time they are painted.
This was a bug and likely cause of some WPT fails.
bool timing_needed_for_lcp =Michal MocnyAny problems not returning if `timing_needed_for_lcp` is false? Wondering since you convinced yourself in the last patch to add this here, so wondering if we need a further check somewhere else after calling this.
soft-LCP is a type of LCP.
Although there shouldn't be any far downstream side effects since LCP calculator checks if LCP is stopped-- I do think we could move this check down just a but further to limit how much needless bookkeeping we do for hard-LCP after its been stopped.
I'll work on that next.
Alright, there are three special `ImageRecords` that are kept around outside of just the record-keeping set:
`largest_painted_image_`, `largest_pending_image_`, both used to implement `ImageRecordsManager::LargestImage()` , and that is only accessed via `ImagePaintTimingDetector::UpdateMetricsCandidate()`.
However:
- We delete the LCP calculator when LCP stops, anyway (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/paint/timing/paint_timing_detector.cc;l=275;drc=997022c9de8c1e4ed9081b8789c1057d0fce0e28;bpv=1;bpt=1)
- And even if we don't delete it, we guard updating image candidates if we aren't recording LCP (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/paint/timing/paint_timing_detector.cc;l=477-480;drc=997022c9de8c1e4ed9081b8789c1057d0fce0e28;bpv=1;bpt=1)
Still-- I changed to guard updating these values for all pathways when LCP is no longer recording. Just in case any accessors do other things (even in the future).
---
The other value is: `largest_ignored_image_`.
Ultimately, this is used to track "ignored LCP canddiate" when opacity is 0, but only for a very special case of content directly on `documentElement`? and toggled when opacity goes from 0 to non-0 here: (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/layout_object.cc;l=3141;drc=997022c9de8c1e4ed9081b8789c1057d0fce0e28;bpv=1;bpt=1)
When toggled, this pathway just reports/updates through the normal path (the one above), so any restrictions we put there should be enough... Still, I also guarded this one.
---
There might be other implications I'm not finding, but I think this is fine: we are updating the background bookkeeping but none of the "LCP-effects now" and we always guarded the "Emit" side from doing anything.
Perf timeline seems okay, I might do some double checking on Tracing/metrics in case we over-report while bookkeeping (but I dont see anything atm)
Related details
- Johannes Henkel
- Scott Haseley
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 3 comments![]( "Open in Gerrit")
Johannes HenkelThis is a bit of an early review request. The mostly just removes code and adds back the minimum needed to still measure soft-nav-entry, without issuing LCP entry.
There are two LCP WPT that (expectedly) fail, and one test for Image based "distant-leaf". I tested the distant-leaf use case locally and I do see a soft-nav entry emitted for it, but this CL changes the timing for soft-nav paint to come after Images are fully loaded, so maybe just needs more timeout? Or maybe its related to buffered:true not working?
Still digging.
Michal MocnyIs the new test passing for you? It did for me yesterday, but may be worth confirming in your context.
lcp-unbuffered.html from https://chromium-review.googlesource.com/c/chromium/src/+/6634990
That one also isnt-- but I am breaking LCP reporting with this patch. (I mean, I don't even try to emit any soft-LCP candidates now).
The next patch in this series will re-do the mechanism for reporting soft-LCP and that test should start to pass again.
We will have a temporary moment w/o soft-LCP. We might not want to have that state on ToT and so we might want to merge both patches, but at least for review and early testing, I think it makes sense to keep separate.
It now works! So do lots of other tests now, except one known expected failure.
I will run a dry-run to see what else still breaks.
records_manager_.RecordFirstPaintAndMaybeCreateImageRecord(Woops-- there was a bug here I forgot to assign `record =` so we wouldnt "see" images the first time they are painted.
This was a bug and likely cause of some WPT fails.
Done
bool TextPaintTimingDetector::ShouldWalkObject(Note to reviewers:
Until we have "pre-paint" Context* caching, trying to ask the question "ShouldWalkObject" requires a dom walk. This might happen for a lot of nodes.
This feels like it could be more expensive than just saying "yes", and allowing the .Contains check below, which will usually still return false, but I'm not sure.
It does currently mean that some text nodes which were never previously recorded, might start to become recorded.
--
Some ideas to make this better:
- Until we have Pre-Paint walk to simplify getting context, maybe I could at least just ask SNH if there is any context/soft-nav at all going on.
- Maybe we should still be adding nodes which are skipped to recorded_set. Then, we could check recorded_set first, and only then check if is_needed_for_soft_nav... If not, still add to recorded_set (after all, Image paint timing does this-- it adds to recorded set even if decides not to measure paint or create a Record)
I changed this as discussed above.
Leaving comment open so you see it during next review.
Related details
- Johannes Henkel
- Scott Haseley
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 1 comment![]( "Open in Gerrit")
Excellent, CQ found only 2 tests fail, one which is expected and I'll update test expectations now.
The other is the new devtools protocol test-- I don't know if I changed the Tracing data or if the test itself is needing an update.
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley added 6 comments![]( "Open in Gerrit")
Didn't get through it all, but spent some time poking at the `ShouldWalkObject()` difference (see comment). I'll review the rest tomorrow.
frame_visual_rect, root_visual_rect, record_id.GetHash(), nullptr);Michal Mocnymaybe `/*soft_navigation_context=/*nullptr` for readability?
Done (I have a vscode plugin that does this automatically and the extra comments actually make this less readable-- but I agree for code search etc it is useful)
FYI there is a style guide rule for this, which is why I mentioned it. https://google.github.io/styleguide/cppguide.html#Function_Argument_Comments. And yeah, it's helpful for us curmudgeons using a vim + codesearch workflow :)
// TODO(mmocny): Create a new entry type, specific to soft-navsDo we want/have a bug for this?
// TODO(mmocny):Is there more to this, or is the TODO the comment below? (here and below). I can't tell if these are placeholders/things that need to happen this CL or not.
recorded_set_.insert(&aggregator);I think change this is observable through elementtiming. There's an assumption that this cannot go from false to true for the same node, but I think it's actually possible.
This is probably silly, but I constructed an example where I think the behavior is different (confirmed on ToT with just adding this line, so I think it's valid).
Example: https://paste.googleplex.com/6045216999735296.
The textarea should autofocus. If you type abc (or any three separate keystrokes) in the text area (it should autofocus), you'll get a PerformanceElementTiming entry without the change, but not with it. This happens because:
1. Keystroke 1 stops lcp
2. Keystroke 2 adds a div w/ background color, which gets painted. There's no elementtiming on the div, LCP has stopped bc of keystroke 1, and the keystroke was not targeted at <body> (so no SNC), so we mark it recorded. Without this line, we might possibly revisit the node
3. Keystroke 3 adds elementtiming to the div and appends a text node, which causes us to hit this again. We get different results because of the recorded set logic because there was no text the first time it painted (so RecordAggregatedText didn't get called).
if (LocalDOMWindow* window = frame_view_->GetFrame().DomWindow()) {Michal MocnySame as image -- can this be null?
Do you mean to suggest that we don't need to check if its null?
Yeah wondering if we need the check, since I wouldn't think we could paint with a null window. Fine to keep it though. I'll resolve.
Related details
- Johannes Henkel
- 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 5 comments![]( "Open in Gerrit")
frame_visual_rect, root_visual_rect, record_id.GetHash(), nullptr);Michal Mocnymaybe `/*soft_navigation_context=/*nullptr` for readability?
Scott HaseleyDone (I have a vscode plugin that does this automatically and the extra comments actually make this less readable-- but I agree for code search etc it is useful)
FYI there is a style guide rule for this, which is why I mentioned it. https://google.github.io/styleguide/cppguide.html#Function_Argument_Comments. And yeah, it's helpful for us curmudgeons using a vim + codesearch workflow :)
I'm also a vim + codesearch curmudgeon... I just switched to vscode + neovim because...chromium. Its a bit buggy and slow sometimes which is annoying but some of the features are worth it to me.
// TODO(mmocny): Create a new entry type, specific to soft-navsDo we want/have a bug for this?
We do, Issue 424433918, I will update comment, thanks.
// TODO(mmocny):Is there more to this, or is the TODO the comment below? (here and below). I can't tell if these are placeholders/things that need to happen this CL or not.
This was a note to self and should have been removed.
(Some of these members could be removed, but not this one, but the comment made me unsure)
recorded_set_.insert(&aggregator);I think change this is observable through elementtiming. There's an assumption that this cannot go from false to true for the same node, but I think it's actually possible.
This is probably silly, but I constructed an example where I think the behavior is different (confirmed on ToT with just adding this line, so I think it's valid).
Example: https://paste.googleplex.com/6045216999735296.
The textarea should autofocus. If you type abc (or any three separate keystrokes) in the text area (it should autofocus), you'll get a PerformanceElementTiming entry without the change, but not with it. This happens because:
1. Keystroke 1 stops lcp
2. Keystroke 2 adds a div w/ background color, which gets painted. There's no elementtiming on the div, LCP has stopped bc of keystroke 1, and the keystroke was not targeted at <body> (so no SNC), so we mark it recorded. Without this line, we might possibly revisit the node
3. Keystroke 3 adds elementtiming to the div and appends a text node, which causes us to hit this again. We get different results because of the recorded set logic because there was no text the first time it painted (so RecordAggregatedText didn't get called).
Good catch.
However, I think this is acceptable.
Late application of `elementtiming` after first paint of the element is already very random and unpredictable. In the test above, you are making it work by:
If any of those criteria you mention above changed, elementtiming wouldn't work (and folks already know to work around this but ensuring attribute set before content added to dom).
---
Additionally, I think we can "fix it even better", soon:
- We already plan to recognize updates to text nodes and "reset" the recorded bit (or some other mechanism) esp if we have a Context change
- We could just extend this to do it regardless of context change if we want (this would support text "growing" for LCP candidate-- risky), or
- Just specifically look for first application of elementtiming attribute and re-record the paint timing if so.
---
Alternatively, once we make Node->Context cheaper via PrePaint walk, then we don't need this cache, because we won't mind checking this "every single paint" quite as much.
I still think its more consistent to do it.
void SoftNavigationContext::UpdateSoftLcpCandidate() {Right now this only gets called after we measure paint timing for an animation frame.
But what happens if:
- URL updates
- We start measuring paints, and tracking largest content
- We emit soft-nav entry
- No more paints
I wonder if soft-nav entry emitting should also "flush" existing paint timings. We only Emit after OnPaintFinished so it should be safe to do so, but, the issue with Paint->Presentation time starts to become relevant...
Related details
- Johannes Henkel
- Scott Haseley
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 1 comment![]( "Open in Gerrit")
recorded_set_.insert(&aggregator);Michal MocnyI think change this is observable through elementtiming. There's an assumption that this cannot go from false to true for the same node, but I think it's actually possible.
This is probably silly, but I constructed an example where I think the behavior is different (confirmed on ToT with just adding this line, so I think it's valid).
Example: https://paste.googleplex.com/6045216999735296.
The textarea should autofocus. If you type abc (or any three separate keystrokes) in the text area (it should autofocus), you'll get a PerformanceElementTiming entry without the change, but not with it. This happens because:
1. Keystroke 1 stops lcp
2. Keystroke 2 adds a div w/ background color, which gets painted. There's no elementtiming on the div, LCP has stopped bc of keystroke 1, and the keystroke was not targeted at <body> (so no SNC), so we mark it recorded. Without this line, we might possibly revisit the node
3. Keystroke 3 adds elementtiming to the div and appends a text node, which causes us to hit this again. We get different results because of the recorded set logic because there was no text the first time it painted (so RecordAggregatedText didn't get called).
Good catch.
However, I think this is acceptable.
Late application of `elementtiming` after first paint of the element is already very random and unpredictable. In the test above, you are making it work by:
If any of those criteria you mention above changed, elementtiming wouldn't work (and folks already know to work around this but ensuring attribute set before content added to dom).
---
Additionally, I think we can "fix it even better", soon:
- We already plan to recognize updates to text nodes and "reset" the recorded bit (or some other mechanism) esp if we have a Context change
- We could just extend this to do it regardless of context change if we want (this would support text "growing" for LCP candidate-- risky), or
- Just specifically look for first application of elementtiming attribute and re-record the paint timing if so.
---
Alternatively, once we make Node->Context cheaper via PrePaint walk, then we don't need this cache, because we won't mind checking this "every single paint" quite as much.
I still think its more consistent to do it.
And to add-- on current Chrome release we will clear this recorded set of nodes as soon as user Interacts (for soft-nav measurement v1).
I think that probably also broke `elementtiming` in even worse ways?
If thats right, then this new regression is to a new behaviour that hasn't branched yet :P
Related details
- Johannes Henkel
- Scott Haseley
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 3 comments![]( "Open in Gerrit")
Johannes HenkelThis is a bit of an early review request. The mostly just removes code and adds back the minimum needed to still measure soft-nav-entry, without issuing LCP entry.
There are two LCP WPT that (expectedly) fail, and one test for Image based "distant-leaf". I tested the distant-leaf use case locally and I do see a soft-nav entry emitted for it, but this CL changes the timing for soft-nav paint to come after Images are fully loaded, so maybe just needs more timeout? Or maybe its related to buffered:true not working?
Still digging.
Michal MocnyIs the new test passing for you? It did for me yesterday, but may be worth confirming in your context.
lcp-unbuffered.html from https://chromium-review.googlesource.com/c/chromium/src/+/6634990
Michal MocnyThat one also isnt-- but I am breaking LCP reporting with this patch. (I mean, I don't even try to emit any soft-LCP candidates now).
The next patch in this series will re-do the mechanism for reporting soft-LCP and that test should start to pass again.
We will have a temporary moment w/o soft-LCP. We might not want to have that state on ToT and so we might want to merge both patches, but at least for review and early testing, I think it makes sense to keep separate.
It now works! So do lots of other tests now, except one known expected failure.
I will run a dry-run to see what else still breaks.
Done
bool timing_needed_for_lcp =Michal MocnyAny problems not returning if `timing_needed_for_lcp` is false? Wondering since you convinced yourself in the last patch to add this here, so wondering if we need a further check somewhere else after calling this.
Michal Mocnysoft-LCP is a type of LCP.
Although there shouldn't be any far downstream side effects since LCP calculator checks if LCP is stopped-- I do think we could move this check down just a but further to limit how much needless bookkeeping we do for hard-LCP after its been stopped.
I'll work on that next.
Alright, there are three special `ImageRecords` that are kept around outside of just the record-keeping set:
`largest_painted_image_`, `largest_pending_image_`, both used to implement `ImageRecordsManager::LargestImage()` , and that is only accessed via `ImagePaintTimingDetector::UpdateMetricsCandidate()`.
However:
- We delete the LCP calculator when LCP stops, anyway (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/paint/timing/paint_timing_detector.cc;l=275;drc=997022c9de8c1e4ed9081b8789c1057d0fce0e28;bpv=1;bpt=1)
- And even if we don't delete it, we guard updating image candidates if we aren't recording LCP (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/paint/timing/paint_timing_detector.cc;l=477-480;drc=997022c9de8c1e4ed9081b8789c1057d0fce0e28;bpv=1;bpt=1)
Still-- I changed to guard updating these values for all pathways when LCP is no longer recording. Just in case any accessors do other things (even in the future).
---
The other value is: `largest_ignored_image_`.
Ultimately, this is used to track "ignored LCP canddiate" when opacity is 0, but only for a very special case of content directly on `documentElement`? and toggled when opacity goes from 0 to non-0 here: (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/layout_object.cc;l=3141;drc=997022c9de8c1e4ed9081b8789c1057d0fce0e28;bpv=1;bpt=1)
When toggled, this pathway just reports/updates through the normal path (the one above), so any restrictions we put there should be enough... Still, I also guarded this one.
---
There might be other implications I'm not finding, but I think this is fine: we are updating the background bookkeeping but none of the "LCP-effects now" and we always guarded the "Emit" side from doing anything.
Perf timeline seems okay, I might do some double checking on Tracing/metrics in case we over-report while bookkeeping (but I dont see anything atm)
Done
bool TextPaintTimingDetector::ShouldWalkObject(Michal MocnyNote to reviewers:
Until we have "pre-paint" Context* caching, trying to ask the question "ShouldWalkObject" requires a dom walk. This might happen for a lot of nodes.
This feels like it could be more expensive than just saying "yes", and allowing the .Contains check below, which will usually still return false, but I'm not sure.
It does currently mean that some text nodes which were never previously recorded, might start to become recorded.
--
Some ideas to make this better:
- Until we have Pre-Paint walk to simplify getting context, maybe I could at least just ask SNH if there is any context/soft-nav at all going on.
- Maybe we should still be adding nodes which are skipped to recorded_set. Then, we could check recorded_set first, and only then check if is_needed_for_soft_nav... If not, still add to recorded_set (after all, Image paint timing does this-- it adds to recorded set even if decides not to measure paint or create a Record)
I changed this as discussed above.
Leaving comment open so you see it during next review.
Done
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel added 1 comment![]( "Open in Gerrit")
if (!IsRecordingLargestTextPaint() &&
!TextElementTiming::NeededForTiming(*node)) {nitpick: maybe this could say
if (IsRecordingLargestTextPaint() || TextElementTiming::NeededForTiming(*node))
return true;
and perhaps similar to some more of the stuff below, to reduce nesting (and double negatives) a bit?
Related details
- Michal Mocny
- Scott Haseley
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 1 comment![]( "Open in Gerrit")
if (!IsRecordingLargestTextPaint() &&
!TextElementTiming::NeededForTiming(*node)) {nitpick: maybe this could say
if (IsRecordingLargestTextPaint() || TextElementTiming::NeededForTiming(*node))
return true;
and perhaps similar to some more of the stuff below, to reduce nesting (and double negatives) a bit?
- Johannes Henkel
- Scott Haseley
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 1 comment![]( "Open in Gerrit")
if (!IsRecordingLargestTextPaint() &&
!TextElementTiming::NeededForTiming(*node)) {Michal Mocnynitpick: maybe this could say
if (IsRecordingLargestTextPaint() || TextElementTiming::NeededForTiming(*node))
return true;
and perhaps similar to some more of the stuff below, to reduce nesting (and double negatives) a bit?
Oh yeah, now that this has swapped order, we can do this.
Done
Related details
- Johannes Henkel
- Scott Haseley
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michal Mocny
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
recorded_set_.insert(&aggregator);Michal MocnyI think change this is observable through elementtiming. There's an assumption that this cannot go from false to true for the same node, but I think it's actually possible.
This is probably silly, but I constructed an example where I think the behavior is different (confirmed on ToT with just adding this line, so I think it's valid).
Example: https://paste.googleplex.com/6045216999735296.
The textarea should autofocus. If you type abc (or any three separate keystrokes) in the text area (it should autofocus), you'll get a PerformanceElementTiming entry without the change, but not with it. This happens because:
1. Keystroke 1 stops lcp
2. Keystroke 2 adds a div w/ background color, which gets painted. There's no elementtiming on the div, LCP has stopped bc of keystroke 1, and the keystroke was not targeted at <body> (so no SNC), so we mark it recorded. Without this line, we might possibly revisit the node
3. Keystroke 3 adds elementtiming to the div and appends a text node, which causes us to hit this again. We get different results because of the recorded set logic because there was no text the first time it painted (so RecordAggregatedText didn't get called).
Michal MocnyGood catch.
However, I think this is acceptable.
Late application of `elementtiming` after first paint of the element is already very random and unpredictable. In the test above, you are making it work by:
If any of those criteria you mention above changed, elementtiming wouldn't work (and folks already know to work around this but ensuring attribute set before content added to dom).
---
Additionally, I think we can "fix it even better", soon:
- We already plan to recognize updates to text nodes and "reset" the recorded bit (or some other mechanism) esp if we have a Context change
- We could just extend this to do it regardless of context change if we want (this would support text "growing" for LCP candidate-- risky), or
- Just specifically look for first application of elementtiming attribute and re-record the paint timing if so.
---
Alternatively, once we make Node->Context cheaper via PrePaint walk, then we don't need this cache, because we won't mind checking this "every single paint" quite as much.
I still think its more consistent to do it.
And to add-- on current Chrome release we will clear this recorded set of nodes as soon as user Interacts (for soft-nav measurement v1).
I think that probably also broke `elementtiming` in even worse ways?
If thats right, then this new regression is to a new behaviour that hasn't branched yet :P
Resolving this, after conversation with Scott.
(I somewhat think this should likely have always worked this way)
Related details
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Annie Sullivan (Gerrit)
Annie Sullivan voted![]( "Open in Gerrit")
- Johannes Henkel
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted Auto-Submit+0![]( "Open in Gerrit")
- Johannes Henkel
- Scott Haseley
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 8 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
8 comments
IN_PROC_BROWSER_TEST_P(SoftNavigationTest, NoSoftNavigation) {If this is being re-enabled intentionally, can you remove the lines above?
WeakMember<SoftNavigationContext> soft_navigation_context_;As a follow-up, we should fix the style in this franken-class-struct:
- `lcp_rect_info_` and this variable are named with class style
- everything else is named like it's a struct
- All members are public
Given it's kind of a mess, no this name seems fine for now
class SoftNavigationContext;nit: Remove? Alternatively remove the include and don't inline the c'tor.
// calculator), or, just skip this next step if the candidates arent done.Please fix this WARNING reported by Spellchecker: "arent" is a possible misspelling of "aren't".
Analyzer Description: Checks for common typos.
Owner: chops-so...@google.com
"arent" is a possible misspelling of "aren't".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
if (!WasEmitted() || !lcp_calculator_) {nit: Can we remove this check? This can never be null since it's created in the constructor and never cleared.
I was wondering, however, if there's any advantage to waiting until something paints to create it? I guess only if we end up getting a lot of SNCs without paint, so probably not worth it.
// TODO:
// if (RuntimeEnabledFeatures::SoftNavigationHeuristicsEnabled(window_)) {Remove? Or is something needed here?
Related details
- Johannes Henkel
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel added 9 comments![]( "Open in Gerrit")
Epic cL! I read to soft_navigation_context.cc, will continue but figure I should send my scribbles.
frame_visual_rect, gfx::ToRoundedRect(root_visual_rect));So there's now potentially lcp_rect_info_ with a rounded root_visual_rect and also root_visual_rect saved without rounding into a field.
frame_visual_rect, gfx::ToRoundedRect(root_visual_rect));now stored both as a rounded thingy when tracing is enabled, and in the field above, root_visual_rect
class CORE_EXPORT ImageRecord : public GarbageCollected<ImageRecord> {curiosity: why is it needed, this cl only touches stuff in blink/renderer/core, is that right? do we need it between the two subdirectories as well?
IsRecordingLargestImagePaint(), record_id, rect_size, image_border,this went from !IsRecording to IsRecording - intentionally?
if (bpp < kMinimumEntropyForLCP) {i may have asked this before (it's blurry) -
this line means we're also imposing the minimum lcp entropy for soft navs, is that right? since even if l. 620 makes timing_needed_for_soft_nav true, we'll actually return nullptr in l. 627?
gfx::RectF root_visual_rect_;
std::unique_ptr<LCPRectInfo> lcp_rect_info_;also potentially redundant storage of the root_visual_rect
here, it's actually slightly easier to see since you put the two fields next to each other? might do that as well in the other header file (image_paint_timing_detector.h)?
dict.Add("unattributedPaintedArea", unattributed_area_);// calculator), or, just skip this next step if the candidates arent done.Please fix this WARNING reported by Spellchecker: "arent" is a possible misspelling of "aren't".
Analyzer Description: Checks for common typos.
Owner: chops-so...@google.com
"arent" is a possible misspelling of "aren't".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
- Annie Sullivan
- Michal Mocny
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Michal Mocny (Gerrit)
Michal Mocny added 16 comments![]( "Open in Gerrit")
Epic cL! I read to soft_navigation_context.cc, will continue but figure I should send my scribbles.
Ty!
IN_PROC_BROWSER_TEST_P(SoftNavigationTest, NoSoftNavigation) {If this is being re-enabled intentionally, can you remove the lines above?
It wasn't, It should remain MAYBE_.
I had DISABLED_ it then realize we pass this one because.... still no soft nav :P
frame_visual_rect, gfx::ToRoundedRect(root_visual_rect));now stored both as a rounded thingy when tracing is enabled, and in the field above, root_visual_rect
Good observation. I'll punt because the tracking enabled track doesn't matter and I dont understand this use case-- but the whole shape of Image/Text Records could do with a nice audit!
frame_visual_rect, gfx::ToRoundedRect(root_visual_rect));So there's now potentially lcp_rect_info_ with a rounded root_visual_rect and also root_visual_rect saved without rounding into a field.
Acknowledged
class CORE_EXPORT ImageRecord : public GarbageCollected<ImageRecord> {curiosity: why is it needed, this cl only touches stuff in blink/renderer/core, is that right? do we need it between the two subdirectories as well?
I needed it or else linking w/ component build would fail.
I assume because ImageRecord is in paint/timing and SNH/SNC are in core/timing, but I didn't dig deeper than that.
(Maybe CORE_EXPORT is a bit of a misnomer?)
WeakMember<SoftNavigationContext> soft_navigation_context_;As a follow-up, we should fix the style in this franken-class-struct:
- `lcp_rect_info_` and this variable are named with class style
- everything else is named like it's a struct
- All members are public
Given it's kind of a mess, no this name seems fine for now
Agree! I think I added `_` by accident because the TextRecords use that style and I pasted to make consistent between them... so that needs fixing too.
class SoftNavigationContext;nit: Remove? Alternatively remove the include and don't inline the c'tor.
Done
IsRecordingLargestImagePaint(), record_id, rect_size, image_border,this went from !IsRecording to IsRecording - intentionally?
Correct! It used to be used in a different way so instead of "ignore unless.." it now just passes "is_recording_lcp"
if (bpp < kMinimumEntropyForLCP) {i may have asked this before (it's blurry) -
this line means we're also imposing the minimum lcp entropy for soft navs, is that right? since even if l. 620 makes timing_needed_for_soft_nav true, we'll actually return nullptr in l. 627?
Correct.
We now enforce ALL "contentful" criteria before "observing" the paint.
There is a distinction in semantics for FCP and LCP, and I may want to poke SNH:: to inform of FCP in a future patch-- but in terms of Area tracking and LCP candidates, we want to unify the conditions here.
gfx::RectF root_visual_rect_;
std::unique_ptr<LCPRectInfo> lcp_rect_info_;also potentially redundant storage of the root_visual_rect
here, it's actually slightly easier to see since you put the two fields next to each other? might do that as well in the other header file (image_paint_timing_detector.h)?
Totally agree, but punting since I dont want to audit the tracing use cases right now.
dict.Add("unattributedPaintedArea", unattributed_area_);TY!
// calculator), or, just skip this next step if the candidates arent done.Please fix this WARNING reported by Spellchecker: "arent" is a possible misspelling of "aren't".
Analyzer Description: Checks for common typos.
Owner: chops-so...@google.com"arent" is a possible misspelling of "aren't".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Done
// calculator), or, just skip this next step if the candidates arent done.Please fix this WARNING reported by Spellchecker: "arent" is a possible misspelling of "aren't".
Analyzer Description: Checks for common typos.
Owner: chops-so...@google.com"arent" is a possible misspelling of "aren't".
To bypass Spellchecker, add a footer with DISABLE_SPELLCHECKER
Done
if (!WasEmitted() || !lcp_calculator_) {nit: Can we remove this check? This can never be null since it's created in the constructor and never cleared.
I was wondering, however, if there's any advantage to waiting until something paints to create it? I guess only if we end up getting a lot of SNCs without paint, so probably not worth it.
Done
// Rename thisMichal MocnyRemove?
Done
// TODO:
// if (RuntimeEnabledFeatures::SoftNavigationHeuristicsEnabled(window_)) {Remove? Or is something needed here?
Good eye. I think we might still want this, and I left the comment to not forget (then forgot).
We need this to ensure we don't Emit LCP when the feature flag is off. However, the risk is mostly quite limited in that the entries are guarded by the PerformanceObserver flag, and the PerforamnceObserver won't expose the entry type to JS.
But emitting entries for no reason risks issuing traces or something, so I wrapped that with a guard. This might change as we add-back UKM.
Related details
- Annie Sullivan
- Johannes Henkel
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Michal Mocny (Gerrit)
Michal Mocny voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Annie Sullivan
- Johannes Henkel
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johannes Henkel (Gerrit)
Johannes Henkel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Johannes Henkel
| Code-Review | +1 |
1 comment
Related details
- Annie Sullivan
- Michal Mocny
- Scott Haseley
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
1 comment
Still LGTM
Related details
- Annie Sullivan
- Michal Mocny
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Annie Sullivan
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted Commit-Queue+0![]( "Open in Gerrit")
| Commit-Queue | +0 |
Related details
- Annie Sullivan
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Annie Sullivan
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
33 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/timing/window_performance.cc
Insertions: 5, Deletions: 0.
@@ -1485,6 +1485,11 @@
const String& url,
Element* element,
bool is_triggered_by_soft_navigation) {
+ if (is_triggered_by_soft_navigation &&
+ !RuntimeEnabledFeatures::SoftNavigationHeuristicsEnabled(
+ GetExecutionContext())) {
+ return;
+ }
DOMHighResTimeStamp load_timestamp =
MonotonicTimeToDOMHighResTimeStamp(load_time);
```
```
The name of the file: third_party/blink/renderer/core/timing/soft_navigation_heuristics.cc
Insertions: 2, Deletions: 1.
@@ -399,7 +399,6 @@
auto* performance = DOMWindowPerformance::performance(*window_.Get());
performance->AddSoftNavigationEntry(AtomicString(context->InitialUrl()),
context->UserInteractionTimestamp());
-
ReportSoftNavigationToMetrics(frame, context);
TRACE_EVENT_INSTANT("scheduler,devtools.timeline,loading",
@@ -431,6 +430,8 @@
if (!context_for_current_url_) {
return;
}
+ // Performance timeline won't allow emitting LCP entries without this flag,
+ // but we can save a lot of needless work by also just not even trying.
if (RuntimeEnabledFeatures::SoftNavigationHeuristicsEnabled(window_)) {
context_for_current_url_->UpdateSoftLcpCandidate();
}
```
```
The name of the file: third_party/blink/web_tests/http/tests/inspector-protocol/tracing/soft-navigations-expected.txt
Insertions: 1, Deletions: 0.
@@ -73,3 +73,4 @@
tid: number
ts: number
}
+
```
```
The name of the file: third_party/blink/renderer/core/paint/timing/paint_timing_detector.cc
Insertions: 4, Deletions: 6.
@@ -245,7 +245,7 @@
void PaintTimingDetector::OnInputOrScroll() {
// If we have already stopped and we're no longer recording the largest image
// paint, then abort.
- if (!image_paint_timing_detector_->IsRecordingLargestImagePaint()) {
+ if (!record_lcp_to_metrics_) {
return;
}
@@ -319,11 +319,9 @@
return;
}
- if (record_lcp_to_metrics_) {
- auto latest_lcp_details =
- GetLargestContentfulPaintCalculator()->LatestLcpDetails();
- lcp_details_for_metrics_ = latest_lcp_details;
- }
+ auto latest_lcp_details =
+ GetLargestContentfulPaintCalculator()->LatestLcpDetails();
+ lcp_details_for_metrics_ = latest_lcp_details;
DidChangePerformanceTiming();
}
```
```
The name of the file: third_party/blink/renderer/core/paint/timing/text_paint_timing_detector.cc
Insertions: 2, Deletions: 1.
@@ -306,6 +306,7 @@
}
}
+ bool is_needed_for_lcp = IsRecordingLargestTextPaint();
bool can_report_timing =
text_element_timing_ ? text_element_timing_->CanReportElements() : false;
HeapVector<Member<const LayoutObject>> keys_to_be_removed;
@@ -319,7 +320,7 @@
text_element_timing_->OnTextObjectPainted(*record, paint_timing_info);
}
- if (ltp_manager_ && (record->recorded_size > 0u)) {
+ if (is_needed_for_lcp && ltp_manager_ && (record->recorded_size > 0u)) {
ltp_manager_->MaybeUpdateLargestText(record);
}
keys_to_be_removed.push_back(key);
```
Change information
[soft navs] Replace the need to "reset" LCP just to measure soft-LCP
Previously, we required clearing text and image paint records, as well
as managing a "restarted" state for image LCP, just to record paint
timings for soft navigation detection.
Now, we change the paint timing detectors to continue to record even
after hard-LCP stops, because there are more clients for this
information (hard LCP and Interactions-->Paints, which power soft LCP).
We may also want to unify image element timing after this, similar to
text element timing.
With this change, we remove all the old mechanics for "LCP resetting"
used for soft-navs measurement since we can now just keep observing the
stream of new paints.
However, as a side effect of this, we also add a new implementation for
soft-LCP, powered by the Interaction Context and emitted directly from
PaintRecords that it observers, rather than the global LCP algorithm.
---
Some other changes/improvements:
- Change to only record loaded Images for soft-navs, rather than first paint of images.
- Change to only record Images which are "contentful", rather than all image paints even if it may be invisible content.
- Subtle changes to element-timing for text which has been previously painted and which has a late addition of elementtiming attribute.
- M chrome/browser/page_load_metrics/integration_tests/soft_navigation_metrics_browsertest.cc
- M third_party/blink/renderer/core/paint/timing/image_paint_timing_detector.cc
- M third_party/blink/renderer/core/paint/timing/image_paint_timing_detector.h
- M third_party/blink/renderer/core/paint/timing/largest_contentful_paint_calculator.cc
- M third_party/blink/renderer/core/paint/timing/largest_contentful_paint_calculator.h
- M third_party/blink/renderer/core/paint/timing/paint_timing.cc
- M third_party/blink/renderer/core/paint/timing/paint_timing.h
- M third_party/blink/renderer/core/paint/timing/paint_timing_detector.cc
- M third_party/blink/renderer/core/paint/timing/paint_timing_detector.h
- M third_party/blink/renderer/core/paint/timing/text_paint_timing_detector.cc
- M third_party/blink/renderer/core/paint/timing/text_paint_timing_detector.h
- M third_party/blink/renderer/core/timing/performance.cc
- M third_party/blink/renderer/core/timing/performance.h
- M third_party/blink/renderer/core/timing/performance_paint_timing.cc
- M third_party/blink/renderer/core/timing/performance_paint_timing.h
- M third_party/blink/renderer/core/timing/soft_navigation_context.cc
- M third_party/blink/renderer/core/timing/soft_navigation_context.h
- M third_party/blink/renderer/core/timing/soft_navigation_heuristics.cc
- M third_party/blink/renderer/core/timing/soft_navigation_heuristics.h
- M third_party/blink/renderer/core/timing/soft_navigation_heuristics_test.cc
- M third_party/blink/renderer/core/timing/window_performance.cc
- M third_party/blink/renderer/core/timing/window_performance.h
- A third_party/blink/web_tests/external/wpt/soft-navigation-heuristics/softnav-before-lcp-paint.tentative-expected.txt
- M third_party/blink/web_tests/http/tests/inspector-protocol/tracing/soft-navigations-expected.txt
Code-Review: +1 by Johannes Henkel, +1 by Scott Haseley
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |