Refactor CanvasResource::GetSyncToken() [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
virtual void GetSyncToken() = 0;nit: Since this function no longer returns a value, its name `GetSyncToken` can be misleading. Consider renaming it to something like `UpdateSyncToken` or `EnsureSyncToken` to better reflect its action of updating internal state. (Blink Style Guide: Naming - Precede setters with the word “Set”; use bare words for getters)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
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. |
Mingjing Zhang (Gerrit)
Mingjing Zhang added 1 comment![]( "Open in Gerrit")
virtual void GetSyncToken() = 0;nit: Since this function no longer returns a value, its name `GetSyncToken` can be misleading. Consider renaming it to something like `UpdateSyncToken` or `EnsureSyncToken` to better reflect its action of updating internal state. (Blink Style Guide: Naming - Precede setters with the word “Set”; use bare words for getters)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Won't fix: `GetSyncToken()` will be renamed to e.g. `EndScopedAccess()` in follow-up CLs soon.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mingjing Zhang (Gerrit)
Mingjing Zhang added 3 comments![]( "Open in Gerrit")
friend class CanvasResourceProviderSharedImage;Note: this `friend` declaration is to allow the use of `sync_token()` in `CanvasResourceProviderSharedImage`.
friend class WebGPUMailboxTexture;Note: this `friend` declaration is to allow the use of `sync_token()` in `WebGPUMailboxTexture`.
void EnsureResourceRecycled(CanvasResourceProvider* provider,Note: this function is moved here so that it can call the non-public `resource->sync_token()`.
Related details
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Vasiliy Telezhnikov
| Code-Review | +1 |
2 comments
canvas_resource->GetSyncToken();(out of the scope here): It's quite unfortunate that we missed this one, before we removed the bool from CR it should have been no-op, because it's not matched with `WillDrawInternal`, so it would never generate one.
We probably need to remove it (in a follow-up), otherwise we don't have ScopedAccess to pass in here.
Related details
- Mingjing Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mingjing Zhang (Gerrit)
Mingjing Zhang added 1 comment![]( "Open in Gerrit")
canvas_resource->GetSyncToken();(out of the scope here): It's quite unfortunate that we missed this one, before we removed the bool from CR it should have been no-op, because it's not matched with `WillDrawInternal`, so it would never generate one.
We probably need to remove it (in a follow-up), otherwise we don't have ScopedAccess to pass in here.
Acknowledged. I'll remove it in a follow-up CL.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mingjing Zhang (Gerrit)
Mingjing Zhang voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
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")
Unreviewed changes
1 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Refactor CanvasResource::GetSyncToken()
This CL removes the return value of CanvasResource::GetSyncToken()
and lets its callers use CanvasResource::sync_token() instead to get
the sync token. This change is a prerequisite of follow-up changes of
turning GetSyncToken() into EndScopedAccess().
- M third_party/blink/renderer/platform/graphics/canvas_resource.cc
- M third_party/blink/renderer/platform/graphics/canvas_resource.h
- M third_party/blink/renderer/platform/graphics/canvas_resource_provider.cc
- M third_party/blink/renderer/platform/graphics/canvas_resource_provider_test.cc
- M third_party/blink/renderer/platform/graphics/gpu/webgpu_mailbox_texture.cc
Code-Review: +1 by Vasiliy Telezhnikov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |