Add origin trial support for Container Timing [chromium/src : main]
José Dapena Paz (Gerrit)
José Dapena Paz has uploaded the change for review![]( "Open in Gerrit")
Commit message
Add origin trial support for Container Timing
This enables Container Timing to be controlled via origin trials by
adding origin_trial_feature_name to runtime_enabled_features.json5.
This means the generated method ContainerTimingEnabled now receives the
ExecutionContext parameter, so updated all the checks to get
per-context feature enablement with origin trials.
Change diff
Change information
- M third_party/blink/renderer/core/dom/element.cc
- M third_party/blink/renderer/core/html/html_element.cc
- M third_party/blink/renderer/core/paint/timing/image_element_timing.cc
- M third_party/blink/renderer/core/paint/timing/text_element_timing.cc
- M third_party/blink/renderer/core/timing/window_performance.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 2 comments![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
2 comments
Change looks fine to me!
Question though, I saw Jason email blink-dev about ready for developer testing. Just confirming that you know this is not the same as origin trial and that you need to move to OT step next?
(also I'm glad to see Bas from Mozilla experimenting with this!!)
Cheers.
if (!RuntimeEnabledFeatures::ContainerTimingEnabled(execution_context)) {Drive by comment: It looks to me like this gets called quite often? With each image paint we check if it is needed for container timing and before checking attributes we check for this flag. Later, after measuring, we check again. Etc etc. This happens for all users, even when the feature is off, I think.
I wonder if you could instead have a ContainerTiming class that only initialized if the flag is enabled, and otherwise would always be nullptr and a simple check from all paint operations?
(Soft navigation heuristics follows that pattern)
---
I'll leave this resolved since its unrelated to this patch but consider investigating if this is needed. I don't know what the cost of frequent flag checks is...
Related details
- Dominic Farolino
- José Dapena Paz
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Chenney (Gerrit)
Stephen Chenney added 1 comment![]( "Open in Gerrit")
if (!RuntimeEnabledFeatures::ContainerTimingEnabled(execution_context)) {Drive by comment: It looks to me like this gets called quite often? With each image paint we check if it is needed for container timing and before checking attributes we check for this flag. Later, after measuring, we check again. Etc etc. This happens for all users, even when the feature is off, I think.
I wonder if you could instead have a ContainerTiming class that only initialized if the flag is enabled, and otherwise would always be nullptr and a simple check from all paint operations?
(Soft navigation heuristics follows that pattern)
---
I'll leave this resolved since its unrelated to this patch but consider investigating if this is needed. I don't know what the cost of frequent flag checks is...
Runtime flag checks are kinda expensive, but we do many of them in style/layout/paint and they tend to rotate in and out, so the net effect on performance is sort of a constant overhead justified by the benefits.
It is why feature flag removal is an important thing to stay on top of.
Related details
- Dominic Farolino
- José Dapena Paz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino voted Code-Review+1![]( "Open in Gerrit")
- José Dapena Paz
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
José Dapena Paz (Gerrit)
José Dapena Paz added 2 comments![]( "Open in Gerrit")
Change looks fine to me!
Question though, I saw Jason email blink-dev about ready for developer testing. Just confirming that you know this is not the same as origin trial and that you need to move to OT step next?
(also I'm glad to see Bas from Mozilla experimenting with this!!)
Cheers.
Yes, origin trial is something we want to do, and we are moving towards that. But dev trial was ready to announce. Thanks anyway for pointing out. The processes here are somehwo confusing
if (!RuntimeEnabledFeatures::ContainerTimingEnabled(execution_context)) {Stephen ChenneyDrive by comment: It looks to me like this gets called quite often? With each image paint we check if it is needed for container timing and before checking attributes we check for this flag. Later, after measuring, we check again. Etc etc. This happens for all users, even when the feature is off, I think.
I wonder if you could instead have a ContainerTiming class that only initialized if the flag is enabled, and otherwise would always be nullptr and a simple check from all paint operations?
(Soft navigation heuristics follows that pattern)
---
I'll leave this resolved since its unrelated to this patch but consider investigating if this is needed. I don't know what the cost of frequent flag checks is...
Runtime flag checks are kinda expensive, but we do many of them in style/layout/paint and they tend to rotate in and out, so the net effect on performance is sort of a constant overhead justified by the benefits.
It is why feature flag removal is an important thing to stay on top of.
I will cache the value in WindowPerformance (that can be accessed both from TextElementTiming and ImageElementTiming) to just calculate the value once. This should simplify the checks.
Related details
- Michal Mocny
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
José Dapena Paz (Gerrit)
José Dapena Paz added 1 comment![]( "Open in Gerrit")
if (!RuntimeEnabledFeatures::ContainerTimingEnabled(execution_context)) {Stephen ChenneyDrive by comment: It looks to me like this gets called quite often? With each image paint we check if it is needed for container timing and before checking attributes we check for this flag. Later, after measuring, we check again. Etc etc. This happens for all users, even when the feature is off, I think.
I wonder if you could instead have a ContainerTiming class that only initialized if the flag is enabled, and otherwise would always be nullptr and a simple check from all paint operations?
(Soft navigation heuristics follows that pattern)
---
I'll leave this resolved since its unrelated to this patch but consider investigating if this is needed. I don't know what the cost of frequent flag checks is...
José Dapena PazRuntime flag checks are kinda expensive, but we do many of them in style/layout/paint and they tend to rotate in and out, so the net effect on performance is sort of a constant overhead justified by the benefits.
It is why feature flag removal is an important thing to stay on top of.
I will cache the value in WindowPerformance (that can be accessed both from TextElementTiming and ImageElementTiming) to just calculate the value once. This should simplify the checks.
- Dominic Farolino
- Michal Mocny
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| 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
thanks for the change to add caching.
One followup you might want to test: Origin Trials can be dynamically enabled by injecting <meta> tags during load, and this is often the easiest way for 3p scripts to participate in an OT. If you check the flag too early, and cache it, this disables the ability to dynamically enable the feature in the middle of a load.
On the other hand, starting to track element timing with the feature disabled and then switching to enabled might lead to broken states...
In a perfect world you would cache the value like you do here, but ensure that you dont check the value of the element timing feature flag too early, giving scripts enough time to inject (at least all scripts in <head>).
In practice, I think as long as you dont check the flag value until the first performance observer or paint, I think thats fine-- but a common footgun people get into is there are a lot of scripts that automatically read the list of `PerformanceObserver.supportedEntryTypes` and getting that list will sometimes automatically read feature flags and leave them in a cached state!
Related details
- Dominic Farolino
- José Dapena Paz
- Stephen Chenney
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |