danakj (Gerrit)

unread,

Jun 4, 2024, 1:55:49 PMJun 4

to David Benjamin, Sunny Sachanandani, chromium...@chromium.org, Daniel Cheng, Peter Beverloo, Tom Sepez, alemat...@chromium.org, andysjl...@chromium.org, asvitki...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, browser-comp...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, core-web-vita...@chromium.org, croissant-...@chromium.org, cros-ed...@google.com, cros-print...@google.com, csharris...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org, gavin...@chromium.org, haoliu...@chromium.org, jdeblas...@chromium.org, loading-rev...@chromium.org, mac-r...@chromium.org, mfoltz...@chromium.org, net-r...@chromium.org, nwoked...@chromium.org, oshima...@chromium.org, pmonett...@chromium.org, print-rev...@chromium.org, rouslan+au...@chromium.org, rrsilva+wat...@google.com, spang...@chromium.org, speed-metr...@chromium.org, tbarzi...@chromium.org, tracing...@chromium.org, vakh+safe_br...@chromium.org, wfh+...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from David Benjamin and Sunny Sachanandani

danakj added 1 comment

Patchset-level comments

File-level comment, Patchset 2 (Latest):

danakj . resolved

davidben: overall
sunnyps: gpu (cuz I changed code more pervasively there)

Open in Gerrit

Related details

Attention is currently required from:

David Benjamin
Sunny Sachanandani

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

danakj (Gerrit)

unread,

Jun 4, 2024, 2:31:27 PMJun 4

to Tricium, Chromium LUCI CQ, David Benjamin, Sunny Sachanandani, chromium...@chromium.org, Daniel Cheng, Peter Beverloo, Tom Sepez, alemat...@chromium.org, andysjl...@chromium.org, asvitki...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, browser-comp...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, core-web-vita...@chromium.org, croissant-...@chromium.org, cros-ed...@google.com, cros-print...@google.com, csharris...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org, gavin...@chromium.org, haoliu...@chromium.org, jdeblas...@chromium.org, loading-rev...@chromium.org, mac-r...@chromium.org, mfoltz...@chromium.org, net-r...@chromium.org, nwoked...@chromium.org, oshima...@chromium.org, pmonett...@chromium.org, print-rev...@chromium.org, rouslan+au...@chromium.org, rrsilva+wat...@google.com, spang...@chromium.org, speed-metr...@chromium.org, tbarzi...@chromium.org, tracing...@chromium.org, vakh+safe_br...@chromium.org, wfh+...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from David Benjamin and Sunny Sachanandani

danakj added 1 comment

File gpu/command_buffer/service/memory_program_cache.cc

Line 99, Patchset 5 (Latest): for (AttributeMap::const_iterator iter = shader->attrib_map().begin();

danakj . resolved

use range-based for loop instead (https://clang.llvm.org/extra/clang-tidy/checks/modernize/loop-convert.html)
(Note: running clang-tidy on this file failed; this diagnostic might be incorrect as a result.)
(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)

Ok I rewrote a bunch of for loops in memory_program_cache.cc..

Open in Gerrit

Related details

Attention is currently required from:

David Benjamin
Sunny Sachanandani

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

David Benjamin (Gerrit)

unread,

Jun 4, 2024, 2:45:30 PMJun 4

to danakj, Tricium, Chromium LUCI CQ, Sunny Sachanandani, chromium...@chromium.org, Daniel Cheng, Peter Beverloo, Tom Sepez, alemat...@chromium.org, andysjl...@chromium.org, asvitki...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, browser-comp...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, core-web-vita...@chromium.org, croissant-...@chromium.org, cros-ed...@google.com, cros-print...@google.com, csharris...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org, gavin...@chromium.org, haoliu...@chromium.org, jdeblas...@chromium.org, loading-rev...@chromium.org, mac-r...@chromium.org, mfoltz...@chromium.org, net-r...@chromium.org, nwoked...@chromium.org, oshima...@chromium.org, pmonett...@chromium.org, print-rev...@chromium.org, rouslan+au...@chromium.org, rrsilva+wat...@google.com, spang...@chromium.org, speed-metr...@chromium.org, tbarzi...@chromium.org, tracing...@chromium.org, vakh+safe_br...@chromium.org, wfh+...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from Sunny Sachanandani and danakj

David Benjamin voted and added 7 comments

Votes added by David Benjamin

Code-Review

+1

7 comments

File chrome/browser/ui/ash/wallpaper_controller_client_impl.cc

Line 116, Patchset 4: // not.

David Benjamin . resolved

Hahaha.

File chrome/browser/win/conflicts/module_blocklist_cache_updater.cc

Line 103, Patchset 4: .copy_from(base::SHA1Hash(base::as_byte_span(module_basename)));

David Benjamin . resolved

SHA-1 hashes (20 bytes) are probably juuuust getting to the point that it's worth having an out param to avoid the extra copy, but I doubt it matters for this code. :-)

(cf the discussions we had way back about how U64ToBigEndian and friends should work. Or how Rust's `digest` crate has both `finalize` and `finalize_into`.)

File gpu/command_buffer/service/memory_program_cache.cc

Line 317, Patchset 4: ProgramLRUCache::iterator found = store_.Get(sha_string);

David Benjamin . resolved

Suspect we'd need to teach base::LRUCache to support heterogenous lookup if we want to string_view this.

Line 367, Patchset 4: RunShaderCallback(client, proto.get(), sha_string);

David Benjamin . resolved

Hah. Not only is the `std::string` wasteful, but `RunShaderCallback` takes the string by value.

File gpu/command_buffer/service/program_cache.cc

Line 181, Patchset 6:

      CHECK(buffer.WriteU8LittleEndian(static_cast<uint8_t>(value >> 24)));
      CHECK(buffer.WriteU8LittleEndian(static_cast<uint8_t>(value >> 16)));
      CHECK(buffer.WriteU8LittleEndian(static_cast<uint8_t>(value >> 8)));
      CHECK(buffer.WriteU8LittleEndian(static_cast<uint8_t>(value)));

David Benjamin . unresolved

I believe this is simply:

```suggestion
      CHECK(buffer.WriteI32BigEndian(value));
```

OpenGL seems to promise that `GLint` is a 32-bit, signed integer. Also, man, I gotta say `WriteU8LittleEndian` continues to throw me off every time. Still think we should call it `WriteU8`, because it's very confusing.

File net/cert/x509_certificate.cc

Line 739, Patchset 6: base::span(CRYPTO_BUFFER_data(buffer), CRYPTO_BUFFER_len(buffer)));

David Benjamin . unresolved

There's already a `CryptoBufferAsSpan` in net/cert/x509_util.h. No sense in carrying around two of these, since each will need its own UNSAFE_BUFFERS in the long run.

File net/cert/x509_util.h

Line 162, Patchset 6:// `base::as_string_view(cert->cert_span())`.

David Benjamin . resolved

I mean, these wrappers atop the CRYPTO_BUFFER accessor is just as safe, but I don't mind having the extra wrapper. *shrug*

Open in Gerrit

Related details

Attention is currently required from:

Sunny Sachanandani
danakj

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sunny Sachanandani (Gerrit)

unread,

Jun 4, 2024, 2:53:46 PMJun 4

to danakj, David Benjamin, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, Peter Beverloo, Tom Sepez, alemat...@chromium.org, andysjl...@chromium.org, asvitki...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, browser-comp...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, core-web-vita...@chromium.org, croissant-...@chromium.org, cros-ed...@google.com, cros-print...@google.com, csharris...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org, gavin...@chromium.org, haoliu...@chromium.org, jdeblas...@chromium.org, loading-rev...@chromium.org, mac-r...@chromium.org, mfoltz...@chromium.org, net-r...@chromium.org, nwoked...@chromium.org, oshima...@chromium.org, pmonett...@chromium.org, print-rev...@chromium.org, rouslan+au...@chromium.org, rrsilva+wat...@google.com, spang...@chromium.org, speed-metr...@chromium.org, tbarzi...@chromium.org, tracing...@chromium.org, vakh+safe_br...@chromium.org, wfh+...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from danakj

Sunny Sachanandani voted and added 1 comment

Votes added by Sunny Sachanandani

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Sunny Sachanandani . resolved

gpu lgtm

Open in Gerrit

Related details

Attention is currently required from:

danakj

Submit Requirements:

Code-Coverage
Code-Review

No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Jun 4, 2024, 4:02:11 PMJun 4

to danakj, David Benjamin, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, Peter Beverloo, Tom Sepez, alemat...@chromium.org, andysjl...@chromium.org, asvitki...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, browser-comp...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, core-web-vita...@chromium.org, croissant-...@chromium.org, cros-ed...@google.com, cros-print...@google.com, csharris...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org, gavin...@chromium.org, haoliu...@chromium.org, jdeblas...@chromium.org, loading-rev...@chromium.org, mac-r...@chromium.org, mfoltz...@chromium.org, net-r...@chromium.org, nwoked...@chromium.org, oshima...@chromium.org, pmonett...@chromium.org, print-rev...@chromium.org, rouslan+au...@chromium.org, rrsilva+wat...@google.com, spang...@chromium.org, speed-metr...@chromium.org, tbarzi...@chromium.org, tracing...@chromium.org, vakh+safe_br...@chromium.org, wfh+...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from danakj

findit...@appspot.gserviceaccount.com voted Code-Coverage+1

This change meets the code coverage requirements.

Code-Coverage

+1

Open in Gerrit

Related details

Attention is currently required from:

danakj

Submit Requirements:

Code-Coverage
Code-Review
No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Tom Sepez (Gerrit)

unread,

Jun 4, 2024, 5:22:12 PMJun 4

to danakj, findit...@appspot.gserviceaccount.com, David Benjamin, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, Peter Beverloo, alemat...@chromium.org, andysjl...@chromium.org, asvitki...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, browser-comp...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, core-web-vita...@chromium.org, croissant-...@chromium.org, cros-ed...@google.com, cros-print...@google.com, csharris...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org, gavin...@chromium.org, haoliu...@chromium.org, jdeblas...@chromium.org, loading-rev...@chromium.org, mac-r...@chromium.org, mfoltz...@chromium.org, net-r...@chromium.org, nwoked...@chromium.org, oshima...@chromium.org, pmonett...@chromium.org, print-rev...@chromium.org, rouslan+au...@chromium.org, rrsilva+wat...@google.com, spang...@chromium.org, speed-metr...@chromium.org, tbarzi...@chromium.org, tracing...@chromium.org, vakh+safe_br...@chromium.org, wfh+...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from danakj

Tom Sepez added 1 comment

File base/containers/span_unittest.cc

Line 1491, Patchset 7 (Latest): EXPECT_EQ(byte_span.size(), kVec.size() * sizeof(int));

Tom Sepez . unresolved

Maybe an EXPECT_DEATH() if we try to make a mismatches span.

satisfied_requirement

unsatisfied_requirement

open

diffy

Tom Sepez (Gerrit)

unread,

Jun 4, 2024, 5:25:57 PMJun 4

to danakj, findit...@appspot.gserviceaccount.com, David Benjamin, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, Peter Beverloo, alemat...@chromium.org, andysjl...@chromium.org, asvitki...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, browser-comp...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, core-web-vita...@chromium.org, croissant-...@chromium.org, cros-ed...@google.com, cros-print...@google.com, csharris...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org, gavin...@chromium.org, haoliu...@chromium.org, jdeblas...@chromium.org, loading-rev...@chromium.org, mac-r...@chromium.org, mfoltz...@chromium.org, net-r...@chromium.org, nwoked...@chromium.org, oshima...@chromium.org, pmonett...@chromium.org, print-rev...@chromium.org, rouslan+au...@chromium.org, rrsilva+wat...@google.com, spang...@chromium.org, speed-metr...@chromium.org, tbarzi...@chromium.org, tracing...@chromium.org, vakh+safe_br...@chromium.org, wfh+...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from danakj

Tom Sepez added 4 comments

File components/sync/base/unique_position_unittest.cc

Line 398, Patchset 7 (Latest): base::SHA1Hash(base::as_bytes(base::make_span(input))));

Tom Sepez . unresolved

nit: should be as_byte_span() ?

File components/sync/engine/bookmark_update_preprocessing.cc

Line 93, Patchset 7 (Latest): base::SHA1Hash(base::as_bytes(base::make_span(unique_tag)));

Tom Sepez . unresolved

nit: should be as_byte_span() ?

File components/sync_bookmarks/bookmark_specifics_conversions.cc

Line 189, Patchset 7 (Latest): base::SHA1Hash(base::as_bytes(base::make_span(unique_tag)));

Tom Sepez . unresolved

nit: should be as_byte_span() ? Numerous usages throughout.

File content/browser/cache_storage/cache_storage_manager.cc

Line 128, Patchset 7 (Latest): base::SHA1Hash(base::as_bytes(base::make_span(identifier)))));

Tom Sepez . unresolved

nit: I'm going to stop flagging these now ...

satisfied_requirement

unsatisfied_requirement

open

diffy

danakj (Gerrit)

unread,

Jun 5, 2024, 11:44:08 AMJun 5

to findit...@appspot.gserviceaccount.com, David Benjamin, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, Peter Beverloo, Tom Sepez, alemat...@chromium.org, andysjl...@chromium.org, asvitki...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, browser-comp...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, core-web-vita...@chromium.org, croissant-...@chromium.org, cros-ed...@google.com, cros-print...@google.com, csharris...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org, gavin...@chromium.org, haoliu...@chromium.org, jdeblas...@chromium.org, loading-rev...@chromium.org, mac-r...@chromium.org, mfoltz...@chromium.org, net-r...@chromium.org, nwoked...@chromium.org, oshima...@chromium.org, pmonett...@chromium.org, print-rev...@chromium.org, rouslan+au...@chromium.org, rrsilva+wat...@google.com, spang...@chromium.org, speed-metr...@chromium.org, tbarzi...@chromium.org, tracing...@chromium.org, vakh+safe_br...@chromium.org, wfh+...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from David Benjamin

danakj added 8 comments

Patchset-level comments

File-level comment, Patchset 7 (Latest):

danakj . resolved

PTAL

File base/containers/span_unittest.cc

Line 1491, Patchset 7 (Latest): EXPECT_EQ(byte_span.size(), kVec.size() * sizeof(int));

Tom Sepez . resolved

Maybe an EXPECT_DEATH() if we try to make a mismatches span.

danakj

Done

File components/sync/base/unique_position_unittest.cc

Line 398, Patchset 7 (Latest): base::SHA1Hash(base::as_bytes(base::make_span(input))));

Tom Sepez . resolved

nit: should be as_byte_span() ?

danakj

Done

File components/sync/engine/bookmark_update_preprocessing.cc

Line 93, Patchset 7 (Latest): base::SHA1Hash(base::as_bytes(base::make_span(unique_tag)));

Tom Sepez . resolved

nit: should be as_byte_span() ?

danakj

Done

File components/sync_bookmarks/bookmark_specifics_conversions.cc

Line 189, Patchset 7 (Latest): base::SHA1Hash(base::as_bytes(base::make_span(unique_tag)));

Tom Sepez . resolved

nit: should be as_byte_span() ? Numerous usages throughout.

danakj

Done

File content/browser/cache_storage/cache_storage_manager.cc

Line 128, Patchset 7 (Latest): base::SHA1Hash(base::as_bytes(base::make_span(identifier)))));

Tom Sepez . resolved

nit: I'm going to stop flagging these now ...

danakj

Done

File gpu/command_buffer/service/program_cache.cc

Line 181, Patchset 6:

      CHECK(buffer.WriteU8LittleEndian(static_cast<uint8_t>(value >> 24)));
      CHECK(buffer.WriteU8LittleEndian(static_cast<uint8_t>(value >> 16)));
      CHECK(buffer.WriteU8LittleEndian(static_cast<uint8_t>(value >> 8)));
      CHECK(buffer.WriteU8LittleEndian(static_cast<uint8_t>(value)));

David Benjamin . resolved

I believe this is simply:
```suggestion
      CHECK(buffer.WriteI32BigEndian(value));
```
OpenGL seems to promise that `GLint` is a 32-bit, signed integer. Also, man, I gotta say `WriteU8LittleEndian` continues to throw me off every time. Still think we should call it `WriteU8`, because it's very confusing.

danakj

Done

File net/cert/x509_certificate.cc

Line 739, Patchset 6: base::span(CRYPTO_BUFFER_data(buffer), CRYPTO_BUFFER_len(buffer)));

David Benjamin . resolved

There's already a `CryptoBufferAsSpan` in net/cert/x509_util.h. No sense in carrying around two of these, since each will need its own UNSAFE_BUFFERS in the long run.

danakj

ok i removed cert_span() and references to it. i added a comment instead that points to CryptoBufferAsSpan() so that folks can find that.

Open in Gerrit

Related details

Attention is currently required from:

David Benjamin

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

danakj (Gerrit)

unread,

Jun 5, 2024, 12:56:54 PMJun 5

to findit...@appspot.gserviceaccount.com, David Benjamin, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, Peter Beverloo, Tom Sepez, alemat...@chromium.org, andysjl...@chromium.org, asvitki...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, browser-comp...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, core-web-vita...@chromium.org, croissant-...@chromium.org, cros-ed...@google.com, cros-print...@google.com, csharris...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org, gavin...@chromium.org, haoliu...@chromium.org, jdeblas...@chromium.org, loading-rev...@chromium.org, mac-r...@chromium.org, mfoltz...@chromium.org, net-r...@chromium.org, nwoked...@chromium.org, oshima...@chromium.org, pmonett...@chromium.org, print-rev...@chromium.org, rouslan+au...@chromium.org, rrsilva+wat...@google.com, spang...@chromium.org, speed-metr...@chromium.org, tbarzi...@chromium.org, tracing...@chromium.org, vakh+safe_br...@chromium.org, wfh+...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from David Benjamin and danakj

danakj voted and added 1 comment

Votes added by danakj

Commit-Queue

+1

1 comment

File net/cert/x509_certificate.cc

Line 739, Patchset 6: base::span(CRYPTO_BUFFER_data(buffer), CRYPTO_BUFFER_len(buffer)));

David Benjamin . resolved

There's already a `CryptoBufferAsSpan` in net/cert/x509_util.h. No sense in carrying around two of these, since each will need its own UNSAFE_BUFFERS in the long run.

danakj

ok i removed cert_span() and references to it. i added a comment instead that points to CryptoBufferAsSpan() so that folks can find that.

danakj

I have put cert_span() back and it's implemented by CryptoBufferAsSpan().

Open in Gerrit

Related details

Attention is currently required from:

David Benjamin
danakj

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

David Benjamin (Gerrit)

unread,

Jun 5, 2024, 1:11:37 PMJun 5

to danakj, findit...@appspot.gserviceaccount.com, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, Peter Beverloo, Tom Sepez, alemat...@chromium.org, andysjl...@chromium.org, asvitki...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, browser-comp...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, core-web-vita...@chromium.org, croissant-...@chromium.org, cros-ed...@google.com, cros-print...@google.com, csharris...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org, gavin...@chromium.org, haoliu...@chromium.org, jdeblas...@chromium.org, loading-rev...@chromium.org, mac-r...@chromium.org, mfoltz...@chromium.org, net-r...@chromium.org, nwoked...@chromium.org, oshima...@chromium.org, pmonett...@chromium.org, print-rev...@chromium.org, rouslan+au...@chromium.org, rrsilva+wat...@google.com, spang...@chromium.org, speed-metr...@chromium.org, tbarzi...@chromium.org, tracing...@chromium.org, vakh+safe_br...@chromium.org, wfh+...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from danakj

David Benjamin voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

danakj

Submit Requirements:

Code-Coverage
Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

danakj (Gerrit)

unread,

Jun 5, 2024, 1:38:52 PMJun 5

to David Benjamin, findit...@appspot.gserviceaccount.com, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, Peter Beverloo, Tom Sepez, alemat...@chromium.org, andysjl...@chromium.org, asvitki...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, browser-comp...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, core-web-vita...@chromium.org, croissant-...@chromium.org, cros-ed...@google.com, cros-print...@google.com, csharris...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org, gavin...@chromium.org, haoliu...@chromium.org, jdeblas...@chromium.org, loading-rev...@chromium.org, mac-r...@chromium.org, mfoltz...@chromium.org, net-r...@chromium.org, nwoked...@chromium.org, oshima...@chromium.org, pmonett...@chromium.org, print-rev...@chromium.org, rouslan+au...@chromium.org, rrsilva+wat...@google.com, spang...@chromium.org, speed-metr...@chromium.org, tbarzi...@chromium.org, tracing...@chromium.org, vakh+safe_br...@chromium.org, wfh+...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from danakj

danakj voted

Commit-Queue	+2
Owners-Override	+1

satisfied_requirement

open

diffy

findit-for-me@appspot.gserviceaccount.com (Gerrit)

unread,

Jun 5, 2024, 2:04:07 PMJun 5

to danakj, David Benjamin, Tricium, Chromium LUCI CQ, chromium...@chromium.org, Daniel Cheng, Peter Beverloo, Tom Sepez, alemat...@chromium.org, andysjl...@chromium.org, asvitki...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, browser-comp...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, core-web-vita...@chromium.org, croissant-...@chromium.org, cros-ed...@google.com, cros-print...@google.com, csharris...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org, gavin...@chromium.org, haoliu...@chromium.org, jdeblas...@chromium.org, loading-rev...@chromium.org, mac-r...@chromium.org, mfoltz...@chromium.org, net-r...@chromium.org, nwoked...@chromium.org, oshima...@chromium.org, pmonett...@chromium.org, print-rev...@chromium.org, rouslan+au...@chromium.org, rrsilva+wat...@google.com, spang...@chromium.org, speed-metr...@chromium.org, tbarzi...@chromium.org, tracing...@chromium.org, vakh+safe_br...@chromium.org, wfh+...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Attention needed from danakj

findit...@appspot.gserviceaccount.com voted Code-Coverage+1

This change meets the code coverage requirements.

Code-Coverage

+1

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jun 5, 2024, 3:14:15 PMJun 5

to danakj, findit...@appspot.gserviceaccount.com, David Benjamin, Sunny Sachanandani, Tricium, chromium...@chromium.org, Daniel Cheng, Peter Beverloo, Tom Sepez, alemat...@chromium.org, andysjl...@chromium.org, asvitki...@chromium.org, blundell+...@chromium.org, bmcquad...@chromium.org, browser-comp...@chromium.org, chromium-a...@chromium.org, chromiumme...@microsoft.com, core-web-vita...@chromium.org, croissant-...@chromium.org, cros-ed...@google.com, cros-print...@google.com, csharris...@chromium.org, druber...@chromium.org, extension...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org, gavin...@chromium.org, haoliu...@chromium.org, jdeblas...@chromium.org, loading-rev...@chromium.org, mac-r...@chromium.org, mfoltz...@chromium.org, net-r...@chromium.org, nwoked...@chromium.org, oshima...@chromium.org, pmonett...@chromium.org, print-rev...@chromium.org, rouslan+au...@chromium.org, rrsilva+wat...@google.com, spang...@chromium.org, speed-metr...@chromium.org, tbarzi...@chromium.org, tracing...@chromium.org, vakh+safe_br...@chromium.org, wfh+...@chromium.org, xinghui...@chromium.org, zackha...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

Remove base::SHA1HashBytes(), consolidate into base::SHA1Hash()

Now that there's no ambiguity we don't need the span suffix, just
as we don't have one in base::HexEncode().

Convert GPU Command Buffer code from using `char*` with a known constant
length to be `span<uint8_t, kHashLength>` for compile-time
bounds-safety.

Convert other uses of SHA1HashBytes() to SHA1Hash() and the use of
spans.

Add an overload of base::as_byte_span and base::as_writable_byte_span
that takes a compile-time size for the output span, and verifies at
runtime that it is correct. This allows the caller to skip calling the
span ctor or make_span<N> overload in order to move from a variable
sized container to a fixed-size byte span.

R=davi...@chromium.org, sun...@chromium.org

Bug: 40284755

Change-Id: I0e6b923303731e846655ee25ec59d78eccc62d00

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5597223

Code-Coverage: findit...@appspot.gserviceaccount.com <findit...@appspot.gserviceaccount.com>

Reviewed-by: David Benjamin <davi...@chromium.org>

Commit-Queue: danakj <dan...@chromium.org>

Owners-Override: danakj <dan...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1310804}

Files:

M ash/ambient/managed/screensaver_image_downloader.cc
M ash/ambient/managed/screensaver_image_downloader_unittest.cc
M ash/ambient/managed/screensaver_images_policy_handler_unittest.cc
M base/containers/span.h
M base/containers/span_unittest.cc
M base/hash/hash_perftest.cc
M base/hash/sha1.h
M base/hash/sha1_boringssl.cc
M base/hash/sha1_nacl.cc
M base/hash/sha1_unittest.cc
M base/metrics/metrics_hashes.cc
M base/trace_event/memory_allocator_dump_guid.cc
M chrome/browser/ash/login/session/user_session_manager.cc
M chrome/browser/component_updater/metadata_table_chromeos.cc
M chrome/browser/extensions/api/certificate_provider/certificate_provider_apitest.cc
M chrome/browser/favicon/favicon_utils.cc
M chrome/browser/media/router/discovery/discovery_network_monitor.cc
M chrome/browser/page_load_metrics/observers/core/ukm_page_load_metrics_observer.cc
M chrome/browser/printing/pdf_to_emf_converter_browsertest.cc
M chrome/browser/printing/pwg_raster_converter_browsertest.cc
M chrome/browser/safe_browsing/download_protection/download_protection_util.cc
M chrome/browser/safe_browsing/download_protection/download_protection_util_unittest.cc
M chrome/browser/sync/test/integration/search_engines_helper.cc
M chrome/browser/ui/ash/wallpaper_controller_client_impl.cc
M chrome/browser/win/conflicts/module_blocklist_cache_updater.cc
M chrome/browser/win/conflicts/module_blocklist_cache_updater_unittest.cc
M chrome/common/pepper_permission_util.cc
M chromeos/ash/components/login/auth/challenge_response/cert_utils_unittest.cc
M chromeos/components/kcer/helpers/key_helper.cc
M chromeos/components/kcer/kcer_token_utils.cc
M components/account_manager_core/chromeos/account_manager.cc
M components/account_manager_core/chromeos/account_manager_unittest.cc
M components/autofill/core/common/signatures.cc
M components/crx_file/id_util.cc
M components/image_fetcher/core/cache/image_cache.cc
M components/lookalikes/core/lookalike_url_util.cc
M components/metrics/environment_recorder.cc
M components/sync/base/client_tag_hash.cc
M components/sync/base/hash_util.cc
M components/sync/base/unique_position_unittest.cc
M components/sync/engine/bookmark_update_preprocessing.cc
M components/sync_bookmarks/bookmark_specifics_conversions.cc
M components/variations/entropy_provider.cc
M content/browser/cache_storage/cache_storage_manager.cc
M google_apis/gcm/engine/gservices_settings.cc
M gpu/command_buffer/service/memory_program_cache.cc
M gpu/command_buffer/service/memory_program_cache.h
M gpu/command_buffer/service/program_cache.cc
M gpu/command_buffer/service/program_cache.h
M gpu/command_buffer/service/program_cache_unittest.cc
M net/cert/internal/trust_store_nss.cc
M net/cert/internal/trust_store_win.cc
M net/cert/x509_certificate.cc
M net/cert/x509_certificate.h
M net/cert/x509_certificate_unittest.cc
M net/disk_cache/simple/simple_util.cc
M net/http/http_cache.cc
M net/test/test_certificate_data.h
M printing/pdf_metafile_cg_mac_unittest.cc
M testing/libfuzzer/fuzzers/sha1_fuzzer.cc

Change size: L

Delta: 60 files changed, 353 insertions(+), 349 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Coverage: +1 by findit...@appspot.gserviceaccount.com
Code-Review: +1 by David Benjamin

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

Remove base::SHA1HashBytes(), consolidate into base::SHA1Hash() [chromium/src : main]

danakj (Gerrit)

danakj added 1 comment

Related details

danakj (Gerrit)

danakj added 1 comment

Related details

David Benjamin (Gerrit)

David Benjamin voted and added 7 comments

Votes added by David Benjamin

7 comments

Related details

Sunny Sachanandani (Gerrit)

Sunny Sachanandani voted and added 1 comment

Votes added by Sunny Sachanandani

1 comment

Related details

findit-for-me@appspot.gserviceaccount.com (Gerrit)

findit...@appspot.gserviceaccount.com voted Code-Coverage+1

Related details

Tom Sepez (Gerrit)

Tom Sepez added 1 comment

Tom Sepez (Gerrit)

Tom Sepez added 4 comments

danakj (Gerrit)

danakj added 8 comments

Related details

danakj (Gerrit)

danakj voted and added 1 comment

Votes added by danakj

1 comment

Related details

David Benjamin (Gerrit)

David Benjamin voted Code-Review+1

Related details

danakj (Gerrit)

danakj voted

findit-for-me@appspot.gserviceaccount.com (Gerrit)

findit...@appspot.gserviceaccount.com voted Code-Coverage+1

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information