Add method to retrieve or compute SHA256 hash of blink::ParkableString [chromium/src : main]
Takashi Nakayama (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Benoit Lize
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 6 comments![]( "Open in Gerrit")
// The lifetime of a returning value is the same as that of `this`.nit: "The returned `DigestHolder` shouldn't outlive `this`." is clearer.
i.e. The lifetime of a returning value SHOULD BE the same as (OR LESS THAN) that of `this`.
std::variant<raw_ptr<const SecureStringDigest>,`raw_ref` if this is always non-null?
DCHECK(digest);nit: prefer `CHECK()` over `DCHECK()`.
// A reference wrapper to hold the digest of a string.Could you elaborate a little more why this is needed and how to use `DigestHolder`?
e.g.
```
`DigestHolder` is to (possibly) keep-alive its underlying temporary
`SecureStringDigest`, so use the result of `DigestHolder.Get()` while its
`DigestHolder` is alive.
Also `DigestHolder` shouldn't outlive its `ParableString`.
```
inline constexpr size_t kStringDigestSize = 32; // SHA256.Could you split the mechanical renaming of `ParkableStringImpl::SecureDigest` into a separate preparational CL? That will make the codereview much easier.
inline constexpr size_t kStringDigestSize = 32; // SHA256.I felt perhaps this name is too broad (especially because this is in the global `blink` namespace), but I'll defer to bindings owners.
If we want to have these in `blink::` namespace, it's helpful to have comments to explain what are these and when to use them.
I'm wondering if it's sufficient to move these to `ParkableString::kDigestSize` / `ParkableString::SecureDigest` (if the primary motivation is to avoid references to `ParkableStringImpl`).
Related details
- Benoit Lize
- Kouhei Ueno
- Leszek Swirski
- Takashi Nakayama
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. |
Greg Thompson (Gerrit)
Greg Thompson added 1 comment![]( "Open in Gerrit")
inline constexpr size_t kStringDigestSize = 32; // SHA256.Can we call this what it is -- `kSha256Size`? Or are you trying to leave room to change the algorithm without needing to rename this constant?
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
static std::unique_ptr<SecureStringDigest> HashString(StringImpl* string);This is pre-existing but it's kind of awkward to use std::unique_ptr<Vector<...>> here... seems like we'd be better off just returning a std::array<uint8_t, 32>.
The `std::array<...>` costs 128 bits and can probably be passed in two registers.
`std::unique_ptr` + `Vector` means two indirections through the heap and (presumably) two words of overhead for storing the BRP refcount... so at this point we're already strictly worse than passing the std::array directly...
Takashi Nakayama (Gerrit)
Takashi Nakayama added 7 comments![]( "Open in Gerrit")
// The lifetime of a returning value is the same as that of `this`.nit: "The returned `DigestHolder` shouldn't outlive `this`." is clearer.
i.e. The lifetime of a returning value SHOULD BE the same as (OR LESS THAN) that of `this`.
Done. Thanks!
std::variant<raw_ptr<const SecureStringDigest>,`raw_ref` if this is always non-null?
I want to keep this `raw_ptr` because using `raw_ref` here needs a lot of boilerplate code. Let's chat offline if you want further examples.
nit: prefer `CHECK()` over `DCHECK()`.
Done
// A reference wrapper to hold the digest of a string.Could you elaborate a little more why this is needed and how to use `DigestHolder`?
e.g.```
`DigestHolder` is to (possibly) keep-alive its underlying temporary
`SecureStringDigest`, so use the result of `DigestHolder.Get()` while its
`DigestHolder` is alive.Also `DigestHolder` shouldn't outlive its `ParableString`.
```
Updated. PTAL.
static std::unique_ptr<SecureStringDigest> HashString(StringImpl* string);This is pre-existing but it's kind of awkward to use std::unique_ptr<Vector<...>> here... seems like we'd be better off just returning a std::array<uint8_t, 32>.
The `std::array<...>` costs 128 bits and can probably be passed in two registers.
`std::unique_ptr` + `Vector` means two indirections through the heap and (presumably) two words of overhead for storing the BRP refcount... so at this point we're already strictly worse than passing the std::array directly...
Acknowledged, but let me do that in a separate CL.
inline constexpr size_t kStringDigestSize = 32; // SHA256.Can we call this what it is -- `kSha256Size`? Or are you trying to leave room to change the algorithm without needing to rename this constant?
Renamed to `kSha256Bytes`. Thanks!
inline constexpr size_t kStringDigestSize = 32; // SHA256.I felt perhaps this name is too broad (especially because this is in the global `blink` namespace), but I'll defer to bindings owners.
If we want to have these in `blink::` namespace, it's helpful to have comments to explain what are these and when to use them.I'm wondering if it's sufficient to move these to `ParkableString::kDigestSize` / `ParkableString::SecureDigest` (if the primary motivation is to avoid references to `ParkableStringImpl`).
Let me keep this as is because I feel like `SecureStringDigest` is a general concept unlimited to `ParkableString` while it is only used by `ParkableString` currently. I added a TODO comment about this. PTAL.
Related details
- Benoit Lize
- Daniel Cheng
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
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. |
Greg Thompson (Gerrit)
Greg Thompson added 1 comment![]( "Open in Gerrit")
// The lifetime of a returning value is the same as that of `this`.Takashi Nakayamanit: "The returned `DigestHolder` shouldn't outlive `this`." is clearer.
i.e. The lifetime of a returning value SHOULD BE the same as (OR LESS THAN) that of `this`.
Done. Thanks!
how about:
```
[[nodiscard]] DigestHolder Digest() const LIFETIME_BOUND;
```
(https://source.chromium.org/chromium/chromium/src/+/main:base/compiler_specific.h;drc=339cdcedfbf02c8178c399c13a9f1b4188a1164e;l=742)
Related details
- Benoit Lize
- Daniel Cheng
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
- Takashi Nakayama
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 2 comments![]( "Open in Gerrit")
// A reference wrapper to hold the digest of a string.Takashi NakayamaCould you elaborate a little more why this is needed and how to use `DigestHolder`?
e.g.```
`DigestHolder` is to (possibly) keep-alive its underlying temporary
`SecureStringDigest`, so use the result of `DigestHolder.Get()` while its
`DigestHolder` is alive.Also `DigestHolder` shouldn't outlive its `ParableString`.
```
Updated. PTAL.
Done
inline constexpr size_t kStringDigestSize = 32; // SHA256.Takashi NakayamaI felt perhaps this name is too broad (especially because this is in the global `blink` namespace), but I'll defer to bindings owners.
If we want to have these in `blink::` namespace, it's helpful to have comments to explain what are these and when to use them.I'm wondering if it's sufficient to move these to `ParkableString::kDigestSize` / `ParkableString::SecureDigest` (if the primary motivation is to avoid references to `ParkableStringImpl`).
Let me keep this as is because I feel like `SecureStringDigest` is a general concept unlimited to `ParkableString` while it is only used by `ParkableString` currently. I added a TODO comment about this. PTAL.
OK, then, could you add a comment to explain the "general concept"?
e.g. what this is, where we (should) use this, that we use SHA256 and why.
A brief/simple comment should be sufficient.
Related details
- Benoit Lize
Takashi Nakayama (Gerrit)
Takashi Nakayama added 3 comments![]( "Open in Gerrit")
// The lifetime of a returning value is the same as that of `this`.Takashi Nakayamanit: "The returned `DigestHolder` shouldn't outlive `this`." is clearer.
i.e. The lifetime of a returning value SHOULD BE the same as (OR LESS THAN) that of `this`.
Greg ThompsonDone. Thanks!
how about:
```
[[nodiscard]] DigestHolder Digest() const LIFETIME_BOUND;
```
(https://source.chromium.org/chromium/chromium/src/+/main:base/compiler_specific.h;drc=339cdcedfbf02c8178c399c13a9f1b4188a1164e;l=742)
I didn't know I could use the chromium version of `[[clang::lifetimebound]]`!! I added it to `ParkableString::Digest()` and `DigestHolder::Get()`.
inline constexpr size_t kStringDigestSize = 32; // SHA256.Takashi NakayamaI felt perhaps this name is too broad (especially because this is in the global `blink` namespace), but I'll defer to bindings owners.
If we want to have these in `blink::` namespace, it's helpful to have comments to explain what are these and when to use them.I'm wondering if it's sufficient to move these to `ParkableString::kDigestSize` / `ParkableString::SecureDigest` (if the primary motivation is to avoid references to `ParkableStringImpl`).
Hiroshige HayashizakiLet me keep this as is because I feel like `SecureStringDigest` is a general concept unlimited to `ParkableString` while it is only used by `ParkableString` currently. I added a TODO comment about this. PTAL.
OK, then, could you add a comment to explain the "general concept"?
e.g. what this is, where we (should) use this, that we use SHA256 and why.
A brief/simple comment should be sufficient.
Done
inline constexpr size_t kStringDigestSize = 32; // SHA256.Could you split the mechanical renaming of `ParkableStringImpl::SecureDigest` into a separate preparational CL? That will make the codereview much easier.
- Benoit Lize
- Greg Thompson
- Hiroshige Hayashizaki
- Kouhei Ueno
- Leszek Swirski
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |