[net] Add base::span support to //net/cert APIs |
|
Issue descriptionSimilarly to https://crbug.com/864676 this bug tracks an effort to augment (pointer, length) based APIs in //net/cert with base::span overloads. For example, affected APIs include net::X509Certificate::CreateFromBytes, net::X509Certificate::CreateFromBytesUnsafeOptions and net::X509Certificate::CreateCertBufferFromBytes.
,
Jul 17
Can you please discuss the motivation for these bugs a bit more? "Augment" implies "Add more overloads", and I don't think that's a good or productive refactoring.
,
Jul 18
This came up during this CL [1]. The goal of these bugs is to replace the current (pointer, length) based API with base::span based APIs, that wrap this information already. This results in easier to use APIs (due to implicit conversions from most common containers), as well as more security, as it is harder to provide the wrong length. See the top level comment of base::span for further details [2]. Ideally all old callsites are migrated and use the new APIs. However, this might affect a large amount of code, and it will be easier to add new overloads first, deprecate the old API, and overtime migrate callsites. Another issue worth discussing is if binary data should be passed as uint8_t* instead of char*. There is an effort among //base OWNERs to do this for //base, and it could / should apply to other parts of Chromium as well. [1] https://chromium-review.googlesource.com/c/chromium/src/+/1137819/3/chrome/browser/extensions/api/certificate_provider/certificate_provider_api.cc#137 [2] https://codesearch.chromium.org/chromium/src/base/containers/span.h?l=84-105&rcl=f739ba61eedb2cc10d2c489e0974c2cd9af6e818
,
Jul 18
Right, I've been converting sites to span, but I don't agree with an approach of "overloads and migrate" - that ends up increasing code complexity unless someone is actively driving it down. Thus, the solution should not be "overload" but "replace" - and that means substantially more work. Internally, see the links at go/lsc to understand the philosophy of making large-scale refactoring changes like this - is the value a net-positive. Adding more ways to do something is not, generally speaking, a net-positive, and change for cosmetics sake (like NULL -> nullptr), is not generally a good idea either.
,
Jul 18
FWIW, I really like base::span, so I'm a fan in general. I don't have as strong of views on overload vs. replace. In general, I agree that replace is nicer as it avoids keeping extra cruft around, but a multi-step thing with overloads can be practical, as long as there's some commitment to not leave it half-migrated. |
|
►
Sign in to add a comment |
|
Comment 1 by jdoerrie@chromium.org
, Jul 17