refactor: third_party/blink/renderer/core/html/parser/literal_buffer.h [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 4 comments![]( "Open in Gerrit")
size_t size_ = 0;Blink Style Guide: Prefer blink:: types over STL and base types. Consider using 'wtf_size_t' instead of 'size_t' for the member variable 'size_' to match the return type of 'size()' and reduce memory usage.
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)_
for (char c : std::string_view(expected_characters)) {Blink Style Guide: Prefer blink:: types over STL and base types. Please use 'StringView' (WTF::StringView) instead of 'std::string_view'.
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)_
for (char c : std::string_view(expected_characters)) {Blink Style Guide: Prefer blink:: types over STL and base types. Please use 'StringView' (WTF::StringView) instead of 'std::string_view'.
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)_
int /*nbDefaulted*/,Blink Style Guide: Prefer blink:: types over STL and base types. Please use 'StringView' (WTF::StringView) instead of 'std::string_view'.
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
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. |
Steinar H Gunderson (Gerrit)
Steinar H Gunderson added 1 comment![]( "Open in Gerrit")
Preemptively: Please run relevant benchmarks before sending this for review. This is the fifth AI-generated spanification CL we've seen only this week (!) that is a probable performance regression, and it's starting to be disruptive for team load. Thank you :-)
Related details
- Sergio Solano
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job linux-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1023ec81b10000
Related details
- Sergio Solano
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. |
Sergio Solano (Gerrit)
Sergio Solano added 4 comments![]( "Open in Gerrit")
Blink Style Guide: Prefer blink:: types over STL and base types. Consider using 'wtf_size_t' instead of 'size_t' for the member variable 'size_' to match the return type of 'size()' and reduce memory usage.
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)_
Done
for (char c : std::string_view(expected_characters)) {Blink Style Guide: Prefer blink:: types over STL and base types. Please use 'StringView' (WTF::StringView) instead of 'std::string_view'.
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)_
Done
for (char c : std::string_view(expected_characters)) {Blink Style Guide: Prefer blink:: types over STL and base types. Please use 'StringView' (WTF::StringView) instead of 'std::string_view'.
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)_
Done
Blink Style Guide: Prefer blink:: types over STL and base types. Please use 'StringView' (WTF::StringView) instead of 'std::string_view'.
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)_
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko added 8 comments![]( "Open in Gerrit")
for (unsigned i = 0; i < expected_characters.length(); ++i) {
source.AdvanceAndASSERT(expected_characters[i]);
}Same here?
for (unsigned i = 0; i < expected_characters.length(); ++i) {
source.AdvanceAndASSERTIgnoringCase(expected_characters[i]);
}This could just be a for each loop right?
```
for (const auto& expected : expected_characters) {
source.AdvanceAndASSERTIgnoringCase(expected);
}
```
// It is legal to copy Member<> with memcpy() here, since we are in a
// constructor. (We could use an initializer, but Clang doesn't
// manage to combine the loads and stores into larger parts.)
UNSAFE_TODO(memcpy(&values_, &other.values_, sizeof(values_)));
UNSAFE_TODO(memcpy(&children_, &other.children_, sizeof(children_)));This comment is very clear that this is heavily optimized code.
blink code should be spanified with extreme care.
Can you do some due diligence here and
1) check that speedometer3/jetstream2 doesn't experience a regression
2) Look for any micro benchmarks around this css parser?
3) That if you look for the assembly code in particular here it generates similar optimized code similar or better than before?
If we can't do 3) we should perhaps leave this until an expert with the skill to do so has a chance to look.
// SAFETY: The loop iterates over a WTF::Vector within the bounds of the
// container, from a valid iterator to the end.a WTF::Vector is not hardened I don't think we should promote this to UNSAFE_BUFFERS leave as UNSAFE_TODO, we can't meaningfully verify the bounds here by any API promise.
search_start != begin_times_.end(); UNSAFE_BUFFERS(++search_start)) {Same here (SMILInstanceTimeList is just a blink::Vector)
char buffer[1024];
// SAFETY: vsnprintf ensures not to write more than 1024 bytes.
UNSAFE_BUFFERS(vsnprintf(buffer, sizeof(buffer), message, args));Can we use `base::SafeSPrintf`?
// `GetError` is safe as it immediately converts the arguments into a
// `std::string` using `base::StringPrintV`.
UNSAFE_BUFFERS(GetParser(closure)->GetError(XMLErrors::kErrorTypeWarning,
message, args));I don't understand what is the buffers usage here we are suppressing? This just looks like we are reinterpreting casting and calling a function on the result? Can you let me know the error? looks like we shouldn't need a UNSAFE_BUFFERS?
UNSAFE_BUFFERS(GetParser(closure)->GetError(XMLErrors::kErrorTypeNonFatal,Same here?
Related details
- Sergio Solano
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. |
Stephen Nusko (Gerrit)
Stephen Nusko added 1 comment![]( "Open in Gerrit")
// It is legal to copy Member<> with memcpy() here, since we are in a
// constructor. (We could use an initializer, but Clang doesn't
// manage to combine the loads and stores into larger parts.)
UNSAFE_TODO(memcpy(&values_, &other.values_, sizeof(values_)));
UNSAFE_TODO(memcpy(&children_, &other.children_, sizeof(children_)));This comment is very clear that this is heavily optimized code.
blink code should be spanified with extreme care.
Can you do some due diligence here and
1) check that speedometer3/jetstream2 doesn't experience a regression
2) Look for any micro benchmarks around this css parser?
3) That if you look for the assembly code in particular here it generates similar optimized code similar or better than before?If we can't do 3) we should perhaps leave this until an expert with the skill to do so has a chance to look.
And I see you already did speedometer 😄
https://pinpoint-dot-chromeperf.appspot.com/job/1023ec81b10000
But can you run it on mac-m1_mini_2020-perf with 150 tries (it should default) as described here: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/benchmark_performance_regressions.md
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/14ee0c64f10000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/jetstream2 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17ddebc4f10000