Remove unneeded string conversions in PaintWorklet [chromium/src : main]
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
I /think/ this is safe, but I'm not 100% sure because of https://chromium-review.googlesource.com/c/chromium/src/+/3900462.
That CL says:
```
// Caching of small strings below is not thread safe: newly constructed
// AtomicString are not safely published.
```
Is this something that code that doesn't interact with v8 directly is going to have to worry about? Or can we go ahead and simplify this?
(Note that I'm planning on deleting `CrossThreadCopier` altogether in the followup)
Related details
- Kentaro Hara
- Luis Pardo
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. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
bool alpha);Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. To improve readability at the call site, consider using an enum (e.g., `enum class AlphaMode { kOpaque, kHasAlpha }`) or a `base::StrongAlias<class HasAlphaTag, bool>` for the `alpha` parameter.
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
- Kentaro Hara
- Luis Pardo
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. |
Luis Pardo (Gerrit)
Luis Pardo voted and added 2 comments![]( "Open in Gerrit")
Votes added by Luis Pardo
| Code-Review | +1 |
2 comments
`
I /think/ this is safe, but I'm not 100% sure because of https://chromium-review.googlesource.com/c/chromium/src/+/3900462.
That CL says:
```
// Caching of small strings below is not thread safe: newly constructed
// AtomicString are not safely published.
```Is this something that code that doesn't interact with v8 directly is going to have to worry about? Or can we go ahead and simplify this?
(Note that I'm planning on deleting `CrossThreadCopier` altogether in the followup)
Change LGTM. I think the comment is wrong, seems like at the time I thought there was an issue with the thread-safety of the initialization of the vector, looking back, it just needs to be marked with `DEFINE_THREAD_SAFE_STATIC_LOCAL`.
Related details
- Daniel Cheng
- Kentaro Hara
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Commit-Queue | +2 |
1 comment
Luis PardoI /think/ this is safe, but I'm not 100% sure because of https://chromium-review.googlesource.com/c/chromium/src/+/3900462.
That CL says:
```
// Caching of small strings below is not thread safe: newly constructed
// AtomicString are not safely published.
```Is this something that code that doesn't interact with v8 directly is going to have to worry about? Or can we go ahead and simplify this?
(Note that I'm planning on deleting `CrossThreadCopier` altogether in the followup)
Change LGTM. I think the comment is wrong, seems like at the time I thought there was an issue with the thread-safety of the initialization of the vector, looking back, it just needs to be marked with `DEFINE_THREAD_SAFE_STATIC_LOCAL`.
- Kentaro Hara
Kentaro Hara (Gerrit)
Kentaro Hara voted Code-Review+1![]( "Open in Gerrit")
- Daniel Cheng
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Commit-Queue | +2 |
1 comment
bool alpha);Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. To improve readability at the call site, consider using an enum (e.g., `enum class AlphaMode { kOpaque, kHasAlpha }`) or a `base::StrongAlias<class HasAlphaTag, bool>` for the `alpha` parameter.
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)_
Won't fix: this is pre-existing code.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Remove unneeded string conversions in PaintWorklet
AtomicStrings have been thread-safe since https://crrev.com/1057879, so
there is no need to convert to String and then back to AtomicString just
to pass it across threads.
- M third_party/blink/renderer/modules/csspaint/document_paint_definition.cc
- M third_party/blink/renderer/modules/csspaint/document_paint_definition.h
- M third_party/blink/renderer/modules/csspaint/paint_worklet.cc
- M third_party/blink/renderer/modules/csspaint/paint_worklet.h
- M third_party/blink/renderer/modules/csspaint/paint_worklet_proxy_client.cc
- M third_party/blink/renderer/modules/csspaint/paint_worklet_test.cc
- M third_party/blink/renderer/platform/wtf/cross_thread_copier.h
Code-Review: +1 by Kentaro Hara, +1 by Luis Pardo
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |