[ElementTiming] Report text paints of size 0 #19525
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wpt-pr-bot
approved these changes
Oct 4, 2019
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-1838260
branch
2 times, most recently
from
9efcd43
to
d21f6ea
Compare
Currently, TextPaintTimingDetector uses |invisible_objects_| to keep track of text paints which occupy a size of 0. It is an optimization: LCP does not care about these paints, so it does not need to queue them to obtain paint times nor store a full TextRecord for them. However ElementTiming wants to expose all of the text paints for elements that are annotated with the elementtiming attribute. It is confusing for an element that is painted when outside the viewport to never be reported. To solve this problem, there are two alternatives. One is to use a single |painted_objects_| map which would contain both the visible and the invisible objects, and hence no extra logic would be needed for ElementTimng to receive these objects (but some care might be needed so LCP does not report objects of size 0). This is simpler to code but is less efficient as it adds extra work and memory for the invisible objects. The second alternative is to use a new data structure for text nodes that are of size 0 but need to be reported to ElementTiming. Paint times for these are assigned in AssignPaintTimeToQueuedRecords() and then the TextRecords are deleted (the fact that the element has been painted is still being handled by |invisible_objects_| so there is no need to keep the records for longer). This is the alternative implemented in this CL. Bug: 1011009 Change-Id: I2d15393fc61134b08a5c15bd81d062d42dfb29e4 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1838260 Reviewed-by: Steve Kobes <skobes@chromium.org> Commit-Queue: Nicolás Peña Moreno <npm@chromium.org> Cr-Commit-Position: refs/heads/master@{#704002}
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-1838260
branch
from
d21f6ea
to
07ed08a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, TextPaintTimingDetector uses |invisible_objects_| to keep
track of text paints which occupy a size of 0. It is an optimization:
LCP does not care about these paints, so it does not need to queue
them to obtain paint times nor store a full TextRecord for them. However
ElementTiming wants to expose all of the text paints for elements that
are annotated with the elementtiming attribute. It is confusing for an
element that is painted when outside the viewport to never be reported.
To solve this problem, there are two alternatives. One is to use a
single |painted_objects_| map which would contain both the visible and
the invisible objects, and hence no extra logic would be needed for
ElementTimng to receive these objects (but some care might be needed so
LCP does not report objects of size 0). This is simpler to code but is
less efficient as it adds extra work and memory for the invisible
objects.
The second alternative is to use a new data structure for text nodes
that are of size 0 but need to be reported to ElementTiming. Paint times
for these are assigned in AssignPaintTimeToQueuedRecords() and then the
TextRecords are deleted (the fact that the element has been painted is
still being handled by |invisible_objects_| so there is no need to keep
the records for longer). This is the alternative implemented in this CL.
Bug: 1011009
Change-Id: I2d15393fc61134b08a5c15bd81d062d42dfb29e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1838260
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704002}