Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ElementTiming] Report text paints of size 0 #19525

Merged
merged 1 commit into from Oct 9, 2019

Conversation

@chromium-wpt-export-bot
Copy link
Collaborator

chromium-wpt-export-bot commented Oct 4, 2019

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}

Copy link
Collaborator

wpt-pr-bot left a comment

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1838260 branch 2 times, most recently from 9efcd43 to d21f6ea Oct 7, 2019
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 chromium-wpt-export-bot force-pushed the chromium-export-cl-1838260 branch from d21f6ea to 07ed08a Oct 9, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 9931f3b into master Oct 9, 2019
11 checks passed
11 checks passed
Azure Pipelines Build #20191009.7 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Taskcluster (pull_request) TaskGroup: success
Details
staging.wpt.fyi - safari[experimental] Safari results
Details
wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-1838260 branch Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.