Implement performance.markConditional(...) [chromium/src : main]
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
Related details
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 7 comments![]( "Open in Gerrit")
Thanks for working on this! Mostly looks good.
A few nits about how to check for flag, and one substantive comment:
https://chromium-review.git.corp.google.com/c/chromium/src/+/7965263/comment/3fd6dc37_89cc7c78/
if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?
if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?
if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?
if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?
PerformanceEntryVector conditional_user_timing_;I don't think this is the right approach. We should retain a vector of structs (containing string+time), kind of like ScriptTimingInfo, Convert to PerformanceEntry only when reporting to the actual DOM timeline.
if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?
Related details
- Guohui Deng
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. |
Guohui Deng (Gerrit)
Guohui Deng added 6 comments![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?
I used DCHECK instead of CHECK. Is it O.K.? (This question also applies to the rest.)
if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?
Ditto
if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?
Ditto
if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?
Ditto.
PerformanceEntryVector conditional_user_timing_;I don't think this is the right approach. We should retain a vector of structs (containing string+time), kind of like ScriptTimingInfo, Convert to PerformanceEntry only when reporting to the actual DOM timeline.
Done. I also added security check (following the "scripts" example) -- I realized this is different from "standard user timing". The top frame can get the "conditional marks" from an Iframe.
if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Would this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?
- Noam Rosenthal
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. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 5 comments![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Guohui DengWould this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?
I used DCHECK instead of CHECK. Is it O.K.? (This question also applies to the rest.)
Acknowledged
if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Guohui DengWould this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?
Ditto
Done
if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Guohui DengWould this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?
Ditto
Acknowledged
if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Guohui DengWould this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?
Ditto.
Acknowledged
if (RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Guohui DengWould this be a noop if the feature is disabled? If so, perhaps make this non-conditional and CHECK instead of guarding by condition?
Ditto.
- Guohui Deng
Noam Rosenthal (Gerrit)
Noam Rosenthal added 5 comments![]( "Open in Gerrit")
DCHECK(conditional_marks_.empty());I think just CHECK(RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) in the condition below?
(CHECK and not DCHECK plz)
if (!RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Ditto
if (!RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Ditto
timing_info->SetConditionalMarks(conditional_marks);Another flag check?
<!DOCTYPE HTML>Is there a test for the security origin checks?
Guohui Deng (Gerrit)
Guohui Deng added 6 comments![]( "Open in Gerrit")
I think just CHECK(RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) in the condition below?
(CHECK and not DCHECK plz)
Done
if (!RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Guohui DengDitto
Done
if (!RuntimeEnabledFeatures::ConditionalTracingLoAFEnabled()) {Guohui DengDitto
Done
timing_info->SetConditionalMarks(conditional_marks);Guohui DengAnother flag check?
Done
PerformanceEntryVector conditional_user_timing_;Guohui DengI don't think this is the right approach. We should retain a vector of structs (containing string+time), kind of like ScriptTimingInfo, Convert to PerformanceEntry only when reporting to the actual DOM timeline.
Done. I also added security check (following the "scripts" example) -- I realized this is different from "standard user timing". The top frame can get the "conditional marks" from an Iframe.
Done
Is there a test for the security origin checks?
Thanks for the guidance! Now the superfluous security origin checks are removed.
Related details
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal added 2 comments![]( "Open in Gerrit")
See one substantive comment. Almost there!
double start_time = now();It needs to be base::TimeTicks::Now(), and pass TimeTicks around rather than double.
Only clamp to a double when you create the PerformanceMarkConditional object.
Related details
- Guohui Deng
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. |
Guohui Deng (Gerrit)
Guohui Deng added 2 comments![]( "Open in Gerrit")
It needs to be base::TimeTicks::Now(), and pass TimeTicks around rather than double.
Only clamp to a double when you create the PerformanceMarkConditional object.
Done.
I see, it must be converted in the correct window. (because of the |time_origin|)
Related details
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Code-Review+1![]( "Open in Gerrit")
- Guohui Deng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Russell (Gerrit)
Alex Russell added 2 comments![]( "Open in Gerrit")
constexpr size_t kConditionalUserTimingBufferSize = 200;Can you add a comment documenting why this is the right size, or link to a design doc that outlines the rationale? Thanks.
Related details
- Guohui Deng
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. |
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
constexpr size_t kConditionalUserTimingBufferSize = 200;Can you add a comment documenting why this is the right size, or link to a design doc that outlines the rationale? Thanks.
This is in line with many other buffer size:
None of these has a comment explaining why it is the right size. I wonder if you can show me an example. I don't think it's needed.
In addition, I need "owner" approval. You are the owner of "global-interface-listing-expected.txt" so I added you as reviewer.
Cheers,
Guohui
Related details
- Alex Russell
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
constexpr size_t kConditionalUserTimingBufferSize = 200;Guohui DengCan you add a comment documenting why this is the right size, or link to a design doc that outlines the rationale? Thanks.
This is in line with many other buffer size:
None of these has a comment explaining why it is the right size. I wonder if you can show me an example. I don't think it's needed.
In addition, I need "owner" approval. You are the owner of "global-interface-listing-expected.txt" so I added you as reviewer.
Cheers,
Guohui
This would eventually need to be in the registry (https://w3c.github.io/timing-entrytypes-registry/) or in the LoAF spec.
Related details
- Alex Russell
- Guohui Deng
Guohui Deng (Gerrit)
Guohui Deng added 1 comment![]( "Open in Gerrit")
constexpr size_t kConditionalUserTimingBufferSize = 200;Guohui DengCan you add a comment documenting why this is the right size, or link to a design doc that outlines the rationale? Thanks.
Noam RosenthalThis is in line with many other buffer size:
None of these has a comment explaining why it is the right size. I wonder if you can show me an example. I don't think it's needed.
In addition, I need "owner" approval. You are the owner of "global-interface-listing-expected.txt" so I added you as reviewer.
Cheers,
Guohui
This would eventually need to be in the registry (https://w3c.github.io/timing-entrytypes-registry/) or in the LoAF spec.
I added a TODO saying this needs to be in that registry.
In addition, I added a WPT test to verify the entries are capped at this limit.
Related details
- Alex Russell
- Noam Rosenthal
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. |
Alex Russell (Gerrit)
Alex Russell added 2 comments![]( "Open in Gerrit")
Resolved with out of band conversation.
constexpr size_t kConditionalUserTimingBufferSize = 200;Guohui DengCan you add a comment documenting why this is the right size, or link to a design doc that outlines the rationale? Thanks.
Noam RosenthalThis is in line with many other buffer size:
None of these has a comment explaining why it is the right size. I wonder if you can show me an example. I don't think it's needed.
In addition, I need "owner" approval. You are the owner of "global-interface-listing-expected.txt" so I added you as reviewer.
Cheers,
Guohui
Guohui DengThis would eventually need to be in the registry (https://w3c.github.io/timing-entrytypes-registry/) or in the LoAF spec.
I added a TODO saying this needs to be in that registry.
In addition, I added a WPT test to verify the entries are capped at this limit.
- Guohui Deng
- Noam Rosenthal
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Noam Rosenthal (Gerrit)
Noam Rosenthal voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Guohui Deng
Noam Rosenthal (Gerrit)
Noam Rosenthal added 1 comment![]( "Open in Gerrit")
- Chris Harrelson
- Guohui Deng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Harrelson (Gerrit)
Chris Harrelson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Chris Harrelson
| Code-Review | +1 |
1 comment
- Guohui Deng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guohui Deng (Gerrit)
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/61167.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Implement performance.markConditional(...)
performance.markConditional(...) creates lightweight tracing points that
only occur in PerformanceLongAnimationFrameTiming.
A follow-up CL will implement performance.measureConditional(...).
Explainer:
https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/ConditionalTracing/explainer-for-loaf.md
I2P:
https://groups.google.com/a/chromium.org/g/blink-dev/c/Ur_FiEgzLvk/m/NOCKYJs1BwAJ
- M android_webview/test/data/web_tests/virtual/stable/webexposed/global-interface-listing-expected.txt
- M android_webview/test/data/web_tests/webexposed/global-interface-listing-expected.txt
- M third_party/blink/renderer/bindings/generated_in_core.gni
- M third_party/blink/renderer/bindings/idl_in_core.gni
- M third_party/blink/renderer/core/frame/animation_frame_timing_monitor.cc
- M third_party/blink/renderer/core/frame/animation_frame_timing_monitor.h
- M third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
- M third_party/blink/renderer/core/frame/web_frame_widget_impl.h
- M third_party/blink/renderer/core/timing/animation_frame_timing_info.cc
- M third_party/blink/renderer/core/timing/animation_frame_timing_info.h
- M third_party/blink/renderer/core/timing/build.gni
- 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.idl
- M third_party/blink/renderer/core/timing/performance_entry.h
- M third_party/blink/renderer/core/timing/performance_entry_names.json5
- M third_party/blink/renderer/core/timing/performance_long_animation_frame_timing.cc
- M third_party/blink/renderer/core/timing/performance_long_animation_frame_timing.h
- M third_party/blink/renderer/core/timing/performance_long_animation_frame_timing.idl
- A third_party/blink/renderer/core/timing/performance_mark_conditional.cc
- A third_party/blink/renderer/core/timing/performance_mark_conditional.h
- A third_party/blink/renderer/core/timing/performance_mark_conditional.idl
- M third_party/blink/renderer/core/timing/window_performance.cc
- M third_party/blink/renderer/core/timing/window_performance.h
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- M third_party/blink/renderer/platform/widget/frame_widget.h
- A third_party/blink/web_tests/external/wpt/long-animation-frame/conditional-tracing-buffer-limit.html
- A third_party/blink/web_tests/external/wpt/long-animation-frame/conditional-tracing.html
- M third_party/blink/web_tests/virtual/stable/webexposed/global-interface-listing-expected.txt
- M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
Code-Review: +1 by Noam Rosenthal, +1 by Chris Harrelson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |