WTF: Represent encoding aliases as StringView [chromium/src : main]
Kent Tamura (Gerrit)
Kent Tamura voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kent Tamura
| Auto-Submit | +1 |
1 comment
Related details
- Fredrik Söderquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
if (alias.Find([](UChar ch) { return ch == ','; }) != kNotFound) {nit: Use 'alias.Find(',')' instead of a lambda for simplicity. (Blink Style Guide: Clarity and Conciseness)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: 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)_
if (name.Find([](UChar ch) { return ch == '\0'; }) != kNotFound) {nit: Use 'name.Find('\0')' instead of a lambda for simplicity. (Blink Style Guide: Clarity and Conciseness)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: 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
- Fredrik Söderquist
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. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist voted and added 6 comments![]( "Open in Gerrit")
Votes added by Fredrik Söderquist
| Code-Review | +1 |
6 comments
LGTM % a few nits
return TextEncoding(StringView(base::as_bytes(base::span(encoding_name))));```suggestion
return TextEncoding(StringView(base::as_byte_span(encoding_name)));
```
StringView(base::as_bytes(base::span(response_head->charset)))),```suggestion
StringView(base::as_byte_span(response_head->charset))),
```
if (alias.Find([](UChar ch) { return ch == ','; }) != kNotFound) {nit: Use 'alias.Find(',')' instead of a lambda for simplicity. (Blink Style Guide: Clarity and Conciseness)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: 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)_
+1
if (const auto* impl = name.SharedImpl()) {
if (!impl->ContainsOnlyASCIIOrEmpty()) {
return g_null_atom;
}
}So, we're making the assumption here that if the StringView was not sourced from a String, it will already follow this restriction (since that how it used to work)? Shouldn't the same apply to the check for a `\0` then in that case? Anyway, might make sense to make a note about that.
if (name.Find([](UChar ch) { return ch == '\0'; }) != kNotFound) {nit: Use 'name.Find('\0')' instead of a lambda for simplicity. (Blink Style Guide: Clarity and Conciseness)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: 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)_
- Kent Tamura
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. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 2 comments![]( "Open in Gerrit")
if (alias.Find([](UChar ch) { return ch == ','; }) != kNotFound) {Fredrik Söderquistnit: Use 'alias.Find(',')' instead of a lambda for simplicity. (Blink Style Guide: Clarity and Conciseness)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: 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)_
+1
I realized that these were bogus (`StringView` doesn't have any other variants of `Find()`)... (Maybe it would make sense to add though... we have two users now at least...)
if (name.Find([](UChar ch) { return ch == '\0'; }) != kNotFound) {Fredrik Söderquistnit: Use 'name.Find('\0')' instead of a lambda for simplicity. (Blink Style Guide: Clarity and Conciseness)
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: 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)_
+1
Ditto here.
Kent Tamura (Gerrit)
Kent Tamura voted and added 3 comments![]( "Open in Gerrit")
Votes added by Kent Tamura
| Auto-Submit | +1 |
| Commit-Queue | +2 |
3 comments
return TextEncoding(StringView(base::as_bytes(base::span(encoding_name))));Kent Tamura```suggestion
return TextEncoding(StringView(base::as_byte_span(encoding_name)));
```
Done
StringView(base::as_bytes(base::span(response_head->charset)))),Kent Tamura```suggestion
StringView(base::as_byte_span(response_head->charset))),
```
Done
if (const auto* impl = name.SharedImpl()) {
if (!impl->ContainsOnlyASCIIOrEmpty()) {
return g_null_atom;
}
}So, we're making the assumption here that if the StringView was not sourced from a String, it will already follow this restriction (since that how it used to work)? Shouldn't the same apply to the check for a `\0` then in that case? Anyway, might make sense to make a note about that.
I added a comment about this optimization.
Shouldn't the same apply to the check for a \0 then in that case? Anyway, might make sense to make a note about that.
I removed the `\0` check.
I think it made sense while the HashMap key was 0-terminated string, but now it's a String and it's not worth to pay O(N) cost.
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/platform/wtf/text/text_encoding_registry.cc
Insertions: 4, Deletions: 3.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/core/html/parser/text_resource_decoder.cc
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/modules/shared_storage/shared_storage_worklet_global_scope.cc
Insertions: 1, Deletions: 2.
The diff is too large to show. Please review the diff.
```
Change information
WTF: Represent encoding aliases as StringView
* Change the key type of TextEncodingNameMap from `const char*` to
`String` because we don't use 0-terminated strings for aliases.
* Unify two TextEncoding constructors into one, which takes a
StringView
* Unify two AtomicCanonicalTextEncodingName() overloads into one,
which takes a StringView
This CL removes 3 UNSAFE_TODO()s, and has no behavior changes.
- M third_party/blink/renderer/core/html/parser/text_resource_decoder.cc
- M third_party/blink/renderer/modules/shared_storage/shared_storage_worklet_global_scope.cc
- M third_party/blink/renderer/platform/wtf/text/text_encoding.cc
- M third_party/blink/renderer/platform/wtf/text/text_encoding.h
- M third_party/blink/renderer/platform/wtf/text/text_encoding_registry.cc
- M third_party/blink/renderer/platform/wtf/text/text_encoding_registry.h
Code-Review: +1 by Fredrik Söderquist
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
if (const auto* impl = name.SharedImpl()) {
if (!impl->ContainsOnlyASCIIOrEmpty()) {
return g_null_atom;
}
}Kent TamuraSo, we're making the assumption here that if the StringView was not sourced from a String, it will already follow this restriction (since that how it used to work)? Shouldn't the same apply to the check for a `\0` then in that case? Anyway, might make sense to make a note about that.
I added a comment about this optimization.
Shouldn't the same apply to the check for a \0 then in that case? Anyway, might make sense to make a note about that.
I removed the `\0` check.
I think it made sense while the HashMap key was 0-terminated string, but now it's a String and it's not worth to pay O(N) cost.
👍
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |