Use base::MakeFlatSet() where easy [chromium/src : main]
1 view
Skip to first unread message
Daniel Cheng (Gerrit)
Apr 24, 2026, 4:29:01 PM (5 days ago) Apr 24
to Adam Rice, Daniel Cheng, chromium...@chromium.org, Rijubrata Bhaumik, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chfreme...@chromium.org, jophba...@chromium.org, jimmyxgong+wat...@chromium.org, tbarzi...@chromium.org, chrome-intelligence-te...@google.com, mfoltz+wa...@chromium.org, feature-me...@chromium.org, crisrael+wa...@google.com, ozone-...@chromium.org, zelin+watch-we...@chromium.org, chrome-intell...@chromium.org, chromium-a...@chromium.org, japhet+...@chromium.org, derinel+wat...@google.com, jackshira+wa...@google.com, loyso...@chromium.org, mek+w...@chromium.org, michaelcheco+wa...@google.com, philli...@chromium.org, cambickel+watc...@google.com, penghu...@chromium.org, longbowei+watc...@google.com, dmurph+watc...@chromium.org, aixba+wat...@chromium.org, jonmann+wat...@chromium.org, dibyapal+wa...@chromium.org, webauthn...@chromium.org, extension...@chromium.org, pushi+watc...@google.com, hansberry+wa...@chromium.org, oshima...@chromium.org, cc-...@chromium.org, webap...@microsoft.com, mgiuca...@chromium.org, joeantonetti+...@google.com, kuragin+web-ap...@chromium.org, rrsilva+wat...@google.com, ajayramamurthy...@google.com
Attention needed from Adam Rice
Daniel Cheng added 5 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 8 (Latest):
Daniel Cheng . resolved
Happy to stamp with OO+1 once comments are addressed (or I might be wrong about some of them!)
Commit Message
Line 10, Patchset 8 (Latest):
emplace() in a loop where the change can be accomplished mechanically.Daniel Cheng . unresolved
Presumably `MakeFlatSet()` is also more efficient, right?
File chrome/browser/ash/app_list/search/ranking/best_match_ranker.cc
Line 117, Patchset 8 (Latest):
[&](const auto result) { return result->id(); });Daniel Cheng . unresolved
Does this copy something potentially big?
File chrome/browser/ash/login/challenge_response_auth_keys_loader.cc
Line 67, Patchset 8 (Latest):
[&](const auto item) { return item.first; });Daniel Cheng . unresolved
Same question here.
File components/webauthn/core/browser/passkey_sync_bridge.cc
Line 124, Patchset 8 (Latest):
[](const auto& it) { return std::string_view(it.first); });Daniel Cheng . unresolved
It'd be nice to add a comment here why string_view won't dangle (though I know the original didn't have it either).
Related details
Attention is currently required from:
- Adam Rice
Submit Requirements:
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. |
Jan Wilken Dörrie (Gerrit)
Apr 26, 2026, 5:12:04 AM (4 days ago) Apr 26
to Adam Rice, Daniel Cheng, chromium...@chromium.org, Rijubrata Bhaumik, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chfreme...@chromium.org, jophba...@chromium.org, jimmyxgong+wat...@chromium.org, tbarzi...@chromium.org, chrome-intelligence-te...@google.com, mfoltz+wa...@chromium.org, feature-me...@chromium.org, crisrael+wa...@google.com, ozone-...@chromium.org, zelin+watch-we...@chromium.org, chrome-intell...@chromium.org, chromium-a...@chromium.org, japhet+...@chromium.org, derinel+wat...@google.com, jackshira+wa...@google.com, loyso...@chromium.org, mek+w...@chromium.org, michaelcheco+wa...@google.com, philli...@chromium.org, cambickel+watc...@google.com, penghu...@chromium.org, longbowei+watc...@google.com, dmurph+watc...@chromium.org, aixba+wat...@chromium.org, jonmann+wat...@chromium.org, dibyapal+wa...@chromium.org, webauthn...@chromium.org, extension...@chromium.org, pushi+watc...@google.com, hansberry+wa...@chromium.org, oshima...@chromium.org, cc-...@chromium.org, webap...@microsoft.com, mgiuca...@chromium.org, joeantonetti+...@google.com, kuragin+web-ap...@chromium.org, rrsilva+wat...@google.com, ajayramamurthy...@google.com
Attention needed from Adam Rice
Jan Wilken Dörrie added 1 comment![]( "Open in Gerrit")
File ash/public/cpp/accelerator_actions.cc
Line 219, Patchset 8 (Latest):
auto all = base::MakeFlatSet<AcceleratorAction>(Jan Wilken Dörrie . unresolved
drive-by nit: just `return base::MakeFlatSet...`;
also in several other places below
Related details
Attention is currently required from:
- Adam Rice
Submit Requirements:
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. |
Adam Rice (Gerrit)
Apr 27, 2026, 9:18:13 PM (2 days ago) Apr 27
to Jan Wilken Dörrie, Daniel Cheng, chromium...@chromium.org, Rijubrata Bhaumik, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chfreme...@chromium.org, jophba...@chromium.org, jimmyxgong+wat...@chromium.org, tbarzi...@chromium.org, chrome-intelligence-te...@google.com, mfoltz+wa...@chromium.org, feature-me...@chromium.org, crisrael+wa...@google.com, ozone-...@chromium.org, zelin+watch-we...@chromium.org, chrome-intell...@chromium.org, chromium-a...@chromium.org, japhet+...@chromium.org, derinel+wat...@google.com, jackshira+wa...@google.com, loyso...@chromium.org, mek+w...@chromium.org, michaelcheco+wa...@google.com, philli...@chromium.org, cambickel+watc...@google.com, penghu...@chromium.org, longbowei+watc...@google.com, dmurph+watc...@chromium.org, aixba+wat...@chromium.org, jonmann+wat...@chromium.org, dibyapal+wa...@chromium.org, webauthn...@chromium.org, extension...@chromium.org, pushi+watc...@google.com, hansberry+wa...@chromium.org, oshima...@chromium.org, cc-...@chromium.org, webap...@microsoft.com, mgiuca...@chromium.org, joeantonetti+...@google.com, kuragin+web-ap...@chromium.org, rrsilva+wat...@google.com, ajayramamurthy...@google.com
Attention needed from Daniel Cheng and Jan Wilken Dörrie
Adam Rice added 5 comments![]( "Open in Gerrit")
Commit Message
Line 10, Patchset 8:
emplace() in a loop where the change can be accomplished mechanically.Daniel Cheng . resolved
Presumably `MakeFlatSet()` is also more efficient, right?
Adam Rice
Good point. I added an explanation of the reason for the change.
File ash/public/cpp/accelerator_actions.cc
drive-by nit: just `return base::MakeFlatSet...`;
also in several other places below
Adam Rice
Done
File chrome/browser/ash/app_list/search/ranking/best_match_ranker.cc
Does this copy something potentially big?
Adam Rice
The argument type was copied from the original loop, since there's no consistently right answer. In this case it's a `base::WeakPtr`, so it's not big, but it is wasteful to copy, so I changed it to a reference.
File chrome/browser/ash/login/challenge_response_auth_keys_loader.cc
Line 67, Patchset 8:
[&](const auto item) { return item.first; });Daniel Cheng . resolved
Adam RiceSame question here.
In this case it's a `value_type` for `base::DictValue`, ie. `std::pair<const std::string&, const Value&>`. It shouldn't matter if this is copied or not, but I changed it to a reference to make the code easier to read.
File components/webauthn/core/browser/passkey_sync_bridge.cc
Line 124, Patchset 8:
[](const auto& it) { return std::string_view(it.first); });Daniel Cheng . resolved
It'd be nice to add a comment here why string_view won't dangle (though I know the original didn't have it either).
Attention is currently required from:
- Daniel Cheng
- Jan Wilken Dörrie
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Apr 28, 2026, 12:30:58 AM (yesterday) Apr 28
to Adam Rice, Daniel Cheng, Jan Wilken Dörrie, chromium...@chromium.org, Rijubrata Bhaumik, android-bu...@system.gserviceaccount.com, Chromium LUCI CQ, chfreme...@chromium.org, jophba...@chromium.org, jimmyxgong+wat...@chromium.org, tbarzi...@chromium.org, chrome-intelligence-te...@google.com, mfoltz+wa...@chromium.org, feature-me...@chromium.org, crisrael+wa...@google.com, ozone-...@chromium.org, zelin+watch-we...@chromium.org, chrome-intell...@chromium.org, chromium-a...@chromium.org, japhet+...@chromium.org, derinel+wat...@google.com, jackshira+wa...@google.com, loyso...@chromium.org, mek+w...@chromium.org, michaelcheco+wa...@google.com, philli...@chromium.org, cambickel+watc...@google.com, penghu...@chromium.org, longbowei+watc...@google.com, dmurph+watc...@chromium.org, aixba+wat...@chromium.org, jonmann+wat...@chromium.org, dibyapal+wa...@chromium.org, webauthn...@chromium.org, extension...@chromium.org, pushi+watc...@google.com, hansberry+wa...@chromium.org, oshima...@chromium.org, cc-...@chromium.org, webap...@microsoft.com, mgiuca...@chromium.org, joeantonetti+...@google.com, kuragin+web-ap...@chromium.org, rrsilva+wat...@google.com, ajayramamurthy...@google.com
Attention needed from Adam Rice and Jan Wilken Dörrie
Daniel Cheng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
| Owners-Override | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Adam Rice
- Jan Wilken Dörrie
Submit Requirements:
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages