Remove uses of BodyAsStringCallbackDeprecated (remaining) [chromium/src : main]
Andrew Williams (Gerrit)
Andrew Williams added 1 comment![]( "Open in Gerrit")
Hi mmenke@, would you mind reviewing this overall and then I'll have a base/ owner stamp Owner's Override on this? Thanks!
Related details
- mmenke
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke voted and added 2 comments![]( "Open in Gerrit")
Votes added by mmenke
| Code-Review | +1 |
2 comments
LGTM!
Related details
- Andrew Williams
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. |
Andrew Williams (Gerrit)
Andrew Williams added 1 comment![]( "Open in Gerrit")
? std::make_optional("")Andrew Williamsstd::string(), maybe?
With just `std::string()` (no std::make_optional) the compiler complains with:
```
../../content/services/auction_worklet/public/cpp/auction_downloader.cc:423:22: error: incompatible operand types ('std::string' (aka 'basic_string<char>') and 'const nullopt_t')
423 | ? std::string()
| ^ ~~~~~~~~~~~~~
424 | : std::nullopt);
| ~~~~~~~~~~~~
```
But were you suggesting `std::make_optional(std::string())`? They are equivalent AFAICT (https://godbolt.org/z/r3zxz9d3M) but I've updated to that
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke voted and added 1 comment![]( "Open in Gerrit")
Votes added by mmenke
| Code-Review | +1 |
1 comment
? std::make_optional("")Andrew Williamsstd::string(), maybe?
With just `std::string()` (no std::make_optional) the compiler complains with:
```
../../content/services/auction_worklet/public/cpp/auction_downloader.cc:423:22: error: incompatible operand types ('std::string' (aka 'basic_string<char>') and 'const nullopt_t')
423 | ? std::string()
| ^ ~~~~~~~~~~~~~
424 | : std::nullopt);
| ~~~~~~~~~~~~
```But were you suggesting `std::make_optional(std::string())`? They are equivalent AFAICT (https://godbolt.org/z/r3zxz9d3M) but I've updated to that
Yes, that's what I meant - that's why I highlighted only the `""`.
std::make_optional(char*) has to deduce through some magic it needs to create a string (and then does so from an empty string, calculating its length, determining it doesn't have to allocate memory, etc). Admittedly, that could theoretically be deduced at build time, but I'm not sure it is.
I'm a bit surprised `""` works, actually, as type deduction sometimes has issues with trinary statements.
Related details
- Andrew Williams
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Williams (Gerrit)
Andrew Williams added 2 comments![]( "Open in Gerrit")
Thanks mmenke@!
Nico, as a //base owner would you mind adding Owner's Override to this CL since we've gotten a +1 from a //services/network owner and since there aren't any //services/network owners with Owner's Override? We've done this with a few other CLs now, for example: https://chromium-review.googlesource.com/c/chromium/src/+/7148206/comments/3c530d02_481cbf27
? std::make_optional("")Andrew Williamsstd::string(), maybe?
mmenkeWith just `std::string()` (no std::make_optional) the compiler complains with:
```
../../content/services/auction_worklet/public/cpp/auction_downloader.cc:423:22: error: incompatible operand types ('std::string' (aka 'basic_string<char>') and 'const nullopt_t')
423 | ? std::string()
| ^ ~~~~~~~~~~~~~
424 | : std::nullopt);
| ~~~~~~~~~~~~
```But were you suggesting `std::make_optional(std::string())`? They are equivalent AFAICT (https://godbolt.org/z/r3zxz9d3M) but I've updated to that
Yes, that's what I meant - that's why I highlighted only the `""`.
std::make_optional(char*) has to deduce through some magic it needs to create a string (and then does so from an empty string, calculating its length, determining it doesn't have to allocate memory, etc). Admittedly, that could theoretically be deduced at build time, but I'm not sure it is.
I'm a bit surprised `""` works, actually, as type deduction sometimes has issues with trinary statements.
Yes, that's what I meant - that's why I highlighted only the "".
Ah I overlooked that!
And thanks for the explanation about make_optional(char *), that makes sense and does seem like a lot more work
Related details
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nico Weber
| Owners-Override | +1 |
1 comment
I'm assuming the bodies here are small enough that the additional copying caused by moving from unique_ptr to optional isn't a problem? (as in, if you don't move() it in one place, previously I think you'd get a compile error, while now you get a silent copy I think.)
Related details
- Andrew Williams
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke added 1 comment![]( "Open in Gerrit")
I'm assuming the bodies here are small enough that the additional copying caused by moving from unique_ptr to optional isn't a problem? (as in, if you don't move() it in one place, previously I think you'd get a compile error, while now you get a silent copy I think.)
I don't think we can rely on that.
Related details
- Andrew Williams
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. |
Andrew Williams (Gerrit)
Andrew Williams added 1 comment![]( "Open in Gerrit")
mmenkeI'm assuming the bodies here are small enough that the additional copying caused by moving from unique_ptr to optional isn't a problem? (as in, if you don't move() it in one place, previously I think you'd get a compile error, while now you get a silent copy I think.)
I don't think we can rely on that.
Of the callbacks that accepted the `std::unique_ptr<std::string>` parameter and then used the response data in other functions, very few passed the whole `std::unique_ptr` around, with most either passing a `std::string` or a `const std::string&` corresponding to the `std::unique_ptr` value. For those uses I think std::optional is a more accurate representation of how users are interacting with the response data.
If we did want to ensure that the response data was harder to silently copy, maybe we should use a custom class for it instead, with methods for accessing the underlying std::string that makes the caller explicitly decide whether to create a copy or move the underlying data?
Related details
- Nico Weber
- mmenke
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. |
Andrew Williams (Gerrit)
Andrew Williams voted and added 2 comments![]( "Open in Gerrit")
Votes added by Andrew Williams
| Commit-Queue | +2 |
2 comments
Thank you both!
mmenkeI'm assuming the bodies here are small enough that the additional copying caused by moving from unique_ptr to optional isn't a problem? (as in, if you don't move() it in one place, previously I think you'd get a compile error, while now you get a silent copy I think.)
Andrew WilliamsI don't think we can rely on that.
Of the callbacks that accepted the `std::unique_ptr<std::string>` parameter and then used the response data in other functions, very few passed the whole `std::unique_ptr` around, with most either passing a `std::string` or a `const std::string&` corresponding to the `std::unique_ptr` value. For those uses I think std::optional is a more accurate representation of how users are interacting with the response data.
If we did want to ensure that the response data was harder to silently copy, maybe we should use a custom class for it instead, with methods for accessing the underlying std::string that makes the caller explicitly decide whether to create a copy or move the underlying data?
Marking this as resolved and landing this, but we can continue this discussion offline if you all would like
Related details
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Williams (Gerrit)
Andrew Williams voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Remove uses of BodyAsStringCallbackDeprecated (remaining)
Updates multiple existing uses of the versions of
SimpleURLLoader::DownloadToStringOfUnboundedSizeUntilCrashAndDie that
take a BodyAsStringCallbackDeprecated to use the versions that takes a
BodyAsStringCallback. This primarily involves updating the callback
function to accept a std::optional<std::string> instead of a
std::unique_ptr<std::string>. This CL touches the remaining uses (other
than those in CLs currently under review)
- M chrome/test/chromedriver/net/net_util.cc
- M components/net_log/net_export_file_writer_unittest.cc
- M content/browser/interest_group/trusted_signals_fetcher.cc
- M content/browser/interest_group/trusted_signals_fetcher.h
- M content/browser/media/url_provision_fetcher.cc
- M content/browser/media/url_provision_fetcher.h
- M content/services/auction_worklet/direct_from_seller_signals_requester.cc
- M content/services/auction_worklet/direct_from_seller_signals_requester.h
- M content/services/auction_worklet/public/cpp/auction_downloader.cc
- M content/services/auction_worklet/public/cpp/auction_downloader.h
- M content/services/auction_worklet/public/cpp/auction_downloader_unittest.cc
- M content/services/auction_worklet/trusted_kvv2_signals.cc
- M content/services/auction_worklet/trusted_kvv2_signals.h
- M content/services/auction_worklet/trusted_signals.cc
- M content/services/auction_worklet/trusted_signals.h
- M content/services/auction_worklet/worklet_loader.cc
- M content/services/auction_worklet/worklet_loader.h
- M rlz/lib/financial_ping.cc
- M services/network/public/cpp/cross_thread_pending_shared_url_loader_factory_unittest.cc
- M services/network/transitional_url_loader_factory_owner_unittest.cc
- M storage/browser/blob/blob_url_store_impl_unittest.cc
- M third_party/libaddressinput/chromium/chrome_metadata_source.cc
- M third_party/libaddressinput/chromium/chrome_metadata_source.h
Code-Review: +1 by mmenke
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |