New issue
Advanced search Search tips

Issue 864681 link

Starred by 1 user

Issue metadata

Status: Available
Owner: ----
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

[net] Add base::span support to //net/cert APIs

Project Member Reported by jdoerrie@chromium.org, Jul 17

Issue description

Similarly 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.
 
Labels: Hotlist-CodeHealth Hotlist-Refactoring
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.
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
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.
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