-
Notifications
You must be signed in to change notification settings - Fork 5.8k
feat(web): add performance.clearResourceTimings() and setResourceTimingBufferSize() #31603
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
base: main
Are you sure you want to change the base?
feat(web): add performance.clearResourceTimings() and setResourceTimingBufferSize() #31603
Conversation
…ngBufferSize() Implements the missing Performance API methods as specified in the W3C Performance Timeline spec: - clearResourceTimings(): Removes all "resource" type entries from the performance timeline - setResourceTimingBufferSize(maxSize): Sets the buffer size for resource timing entries Both methods are implemented as no-ops since Deno doesn't currently track resource timing entries, but they are provided for web compatibility. Closes denoland#13605
WalkthroughAdded two methods to the Performance API: Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ext/web/15_performance.js (1)
429-437: Add argument type validation for consistency.While this is a no-op, the method should still validate the argument type to match the spec and maintain consistency with other methods in this file (e.g.,
clearMarks,getEntriesByName).Apply this diff:
setResourceTimingBufferSize(_maxSize) { webidl.assertBranded(this, PerformancePrototype); + const prefix = "Failed to execute 'setResourceTimingBufferSize' on 'Performance'"; webidl.requiredArguments( arguments.length, 1, - "Failed to execute 'setResourceTimingBufferSize' on 'Performance'", + prefix, ); + _maxSize = webidl.converters["unsigned long"]( + _maxSize, + prefix, + "Argument 1", + ); // This is a noop in Deno as we don't have resource timing entries }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cli/tsc/dts/lib.deno.shared_globals.d.ts(1 hunks)ext/web/15_performance.js(1 hunks)tests/unit/performance_test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js}: For JavaScript runtime debugging, enable V8 inspector with--inspect-brkflag and connect Chrome DevTools tochrome://inspect
Useconsole.log()for debug prints in JavaScript runtime code
Files:
tests/unit/performance_test.tsext/web/15_performance.jscli/tsc/dts/lib.deno.shared_globals.d.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug macos-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug linux-x86_64
🔇 Additional comments (3)
cli/tsc/dts/lib.deno.shared_globals.d.ts (1)
743-758: LGTM!The type definitions are correct and clearly document that these methods are no-ops in Deno. The signatures match the Performance API spec.
ext/web/15_performance.js (1)
421-427: LGTM!The implementation correctly follows the same pattern as
clearMarks()andclearMeasures(), filtering out entries by type.tests/unit/performance_test.ts (1)
75-96: LGTM!The tests verify the methods exist, don't throw with valid inputs, and enforce argument requirements. This is adequate coverage for no-op compatibility methods.
Now that performance.clearResourceTimings is implemented in workers, remove it from the expected failures list.
Summary
This PR implements the missing Performance API methods as specified in the W3C Performance Timeline spec:
clearResourceTimings(): Removes all performance entries with anentryTypeof "resource" from the performance timelinesetResourceTimingBufferSize(maxSize): Sets the desired size of the browser's resource timing bufferBoth methods are implemented as no-ops since Deno doesn't currently track resource timing entries, but they are provided for web compatibility.
Changes
clearResourceTimings()andsetResourceTimingBufferSize()methods to the Performance classTest plan
clearResourceTimings()- verifies method exists and doesn't throwsetResourceTimingBufferSize()- verifies method exists, accepts a size parameter, and throws when called without argumentsCloses #13605