Xiaohan Zhao (Gerrit)

unread,

Jun 11, 2026, 3:58:54 PM (9 days ago) Jun 11

to Rahul Singh, Steve Becker, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, dmurph+watching...@chromium.org, dmurph+watch...@chromium.org, edgesto...@microsoft.com, storage...@chromium.org

Attention needed from Rahul Singh and Steve Becker

Xiaohan Zhao added 1 comment

File components/services/storage/dom_storage/async_dom_storage_database.cc

Line 308, Patchset 5 (Latest): base::BindOnce(&RecordLocalStorageOnDiskSizeKB,

Xiaohan Zhao . unresolved

Do we want the `RecordLocalStorageOnDiskSizeKB` to record even when `open_status` is not OK? The histogram "LocalStorage.DatabaseOnDiskSizeKB" summary says it is recorded when the database is opened. which sounds like a successful open.

Open in Gerrit

Related details

Attention is currently required from:

Rahul Singh
Steve Becker

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Rahul Singh (Gerrit)

unread,

Jun 11, 2026, 11:30:15 PM (8 days ago) Jun 11

to Steve Becker, Xiaohan Zhao, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, dmurph+watching...@chromium.org, dmurph+watch...@chromium.org, edgesto...@microsoft.com, storage...@chromium.org

Attention needed from Steve Becker and Xiaohan Zhao

Rahul Singh added 1 comment

File components/services/storage/dom_storage/async_dom_storage_database.cc

Line 308, Patchset 5: base::BindOnce(&RecordLocalStorageOnDiskSizeKB,

Xiaohan Zhao . resolved

Do we want the `RecordLocalStorageOnDiskSizeKB` to record even when `open_status` is not OK? The histogram "LocalStorage.DatabaseOnDiskSizeKB" summary says it is recorded when the database is opened. which sounds like a successful open.

Rahul Singh

Good question! In this histogram's previous emission location in dom_storage_context_wrapper.cc, we didn't have visibility into whether the DB opened successfully or not. To not bias data going forward, I decided to always fire this telemetry. I think it's worth adjusting the verbiage in the histograms.xml to reflect this. So I've updated it. Thanks!

Open in Gerrit

Related details

Attention is currently required from:

Steve Becker
Xiaohan Zhao

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Rahul Singh (Gerrit)

unread,

Jun 15, 2026, 3:33:22 PM (5 days ago) Jun 15

to Mingyu Lei, Steve Becker, Xiaohan Zhao, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, dmurph+watching...@chromium.org, dmurph+watch...@chromium.org, edgesto...@microsoft.com, storage...@chromium.org

Attention needed from Mingyu Lei, Steve Becker and Xiaohan Zhao

Rahul Singh added 1 comment

Patchset-level comments

File-level comment, Patchset 10 (Latest):

Rahul Singh . resolved

Adding leimy@ for OWNERS review of storage/histograms.xml.

Open in Gerrit

Related details

Attention is currently required from:

Mingyu Lei
Steve Becker
Xiaohan Zhao

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Steve Becker (Gerrit)

unread,

Jun 15, 2026, 5:45:31 PM (5 days ago) Jun 15

to Rahul Singh, Mingyu Lei, Xiaohan Zhao, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, dmurph+watching...@chromium.org, dmurph+watch...@chromium.org, edgesto...@microsoft.com, storage...@chromium.org

Attention needed from Mingyu Lei, Rahul Singh and Xiaohan Zhao

Steve Becker added 3 comments

File components/services/storage/dom_storage/dom_storage_database.cc

Line 45, Patchset 10 (Latest): base::saturated_cast<int>(base::ComputeDirectorySize(db_path) / 1024);

Steve Becker . unresolved

Not a problem from your change, but maybe we need a better way to measure SQLite diskspace usage. This computation omits the WAL file, which for DOMStorage might be significant since we're currently using the default checkpointing strategy.

Line 45, Patchset 10 (Latest): base::saturated_cast<int>(base::ComputeDirectorySize(db_path) / 1024);

Steve Becker . unresolved

nit: Can we use the ByteSize helper here?

Line 76, Patchset 10 (Latest): db_runner->PostTask(FROM_HERE,

Steve Becker . unresolved

Since this calculation does not depend on any database state, should we post to a random background sequence like the deleted code? That way, the operation won't block any DOMStorage database operations.

Open in Gerrit

Related details

Attention is currently required from:

Mingyu Lei
Rahul Singh
Xiaohan Zhao

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Rahul Singh (Gerrit)

unread,

Jun 16, 2026, 7:26:42 PM (4 days ago) Jun 16

to Mingyu Lei, Steve Becker, Xiaohan Zhao, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, dmurph+watching...@chromium.org, dmurph+watch...@chromium.org, edgesto...@microsoft.com, storage...@chromium.org

Attention needed from Mingyu Lei, Steve Becker and Xiaohan Zhao

Rahul Singh voted and added 3 comments

Votes added by Rahul Singh

Commit-Queue

+1

3 comments

File components/services/storage/dom_storage/dom_storage_database.cc

Line 45, Patchset 10: base::saturated_cast<int>(base::ComputeDirectorySize(db_path) / 1024);

Steve Becker . resolved

nit: Can we use the ByteSize helper here?

Rahul Singh

Done

Line 45, Patchset 10: base::saturated_cast<int>(base::ComputeDirectorySize(db_path) / 1024);

Steve Becker . resolved

Not a problem from your change, but maybe we need a better way to measure SQLite diskspace usage. This computation omits the WAL file, which for DOMStorage might be significant since we're currently using the default checkpointing strategy.

Rahul Singh

Done! ComputeDirectorySize would return 0 when not passed a directory path. So we just sum together the sqlite + sqlite-wal files for that backend.

Line 76, Patchset 10: db_runner->PostTask(FROM_HERE,

Steve Becker . resolved

Since this calculation does not depend on any database state, should we post to a random background sequence like the deleted code? That way, the operation won't block any DOMStorage database operations.

Rahul Singh

Done

Open in Gerrit

Related details

Attention is currently required from:

Mingyu Lei
Steve Becker
Xiaohan Zhao

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Steve Becker (Gerrit)

unread,

Jun 17, 2026, 6:40:46 PM (3 days ago) Jun 17

to Rahul Singh, Mingyu Lei, Xiaohan Zhao, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, dmurph+watching...@chromium.org, dmurph+watch...@chromium.org, edgesto...@microsoft.com, storage...@chromium.org

Attention needed from Mingyu Lei, Rahul Singh and Xiaohan Zhao

Steve Becker added 2 comments

File components/services/storage/dom_storage/dom_storage_database.cc

Line 56, Patchset 11 (Latest): base::StrCat({"LocalStorage.DatabaseOnDiskSizeKB",

Steve Becker . unresolved

Should we also record disk space usage for Session Storage, maybe in a future change?

Line 60, Patchset 11 (Latest): base::ByteSize(base::checked_cast<uint64_t>(size_bytes)));

Steve Becker . unresolved

Do we need to convert bytes to kilobytes maybe using base::ByteSize?

Open in Gerrit

Related details

Attention is currently required from:

Mingyu Lei
Rahul Singh
Xiaohan Zhao

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Rahul Singh (Gerrit)

unread,

Jun 18, 2026, 2:24:41 AM (2 days ago) Jun 18

to Mingyu Lei, Steve Becker, Xiaohan Zhao, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, dmurph+watching...@chromium.org, dmurph+watch...@chromium.org, edgesto...@microsoft.com, storage...@chromium.org

Attention needed from Mingyu Lei, Steve Becker and Xiaohan Zhao

Rahul Singh voted and added 2 comments

Votes added by Rahul Singh

Commit-Queue

+1

2 comments

File components/services/storage/dom_storage/dom_storage_database.cc

Line 56, Patchset 11: base::StrCat({"LocalStorage.DatabaseOnDiskSizeKB",

Steve Becker . resolved

Should we also record disk space usage for Session Storage, maybe in a future change?

Rahul Singh

Added a TODO

Line 60, Patchset 11: base::ByteSize(base::checked_cast<uint64_t>(size_bytes)));

Steve Becker . resolved

Do we need to convert bytes to kilobytes maybe using base::ByteSize?

Rahul Singh

UmaHistogramMemoryKB already takes care of calling the ByteSize
helper `InKiB()` for us.
https://source.chromium.org/chromium/chromium/src/+/main:base/metrics/histogram_functions.cc;drc=f929a0c3e751200212538123bfb58c0b491c3f62;l=326

Open in Gerrit

Related details

Attention is currently required from:

Mingyu Lei
Steve Becker
Xiaohan Zhao

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Steve Becker (Gerrit)

unread,

Jun 18, 2026, 1:03:10 PM (2 days ago) Jun 18

to Rahul Singh, Mingyu Lei, Xiaohan Zhao, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, dmurph+watching...@chromium.org, dmurph+watch...@chromium.org, edgesto...@microsoft.com, storage...@chromium.org

Attention needed from Mingyu Lei, Rahul Singh and Xiaohan Zhao

Steve Becker voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Mingyu Lei
Rahul Singh
Xiaohan Zhao

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Rahul Singh (Gerrit)

unread,

Jun 18, 2026, 2:37:25 PM (2 days ago) Jun 18

to Steve Becker, Mingyu Lei, Xiaohan Zhao, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, dmurph+watching...@chromium.org, dmurph+watch...@chromium.org, edgesto...@microsoft.com, storage...@chromium.org

Attention needed from Mingyu Lei and Xiaohan Zhao

Rahul Singh added 1 comment

Patchset-level comments

File-level comment, Patchset 12 (Latest):

Rahul Singh . resolved

Thanks for the review Steve!

@le...@chromium.org this is now ready for your OWNERS review of storage/histograms.xml. I updated the Experimental version of the histogram to use the OnDiskExperimental suffix like you'd suggested on the other CL!

Open in Gerrit

Related details

Attention is currently required from:

Mingyu Lei
Xiaohan Zhao

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Mingyu Lei (Gerrit)

unread,

Jun 19, 2026, 5:29:02 AM (yesterday) Jun 19

to Rahul Singh, Steve Becker, Xiaohan Zhao, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, dmurph+watching...@chromium.org, dmurph+watch...@chromium.org, edgesto...@microsoft.com, storage...@chromium.org

Attention needed from Rahul Singh and Xiaohan Zhao

Mingyu Lei voted and added 1 comment

Votes added by Mingyu Lei

Code-Review

+1

1 comment

File tools/metrics/histograms/metadata/storage/histograms.xml

Line 1036, Patchset 13 (Latest):<histogram name="LocalStorage.DatabaseOnDiskSizeKB{Suffix}" units="KB"

Mingyu Lei . unresolved

I feel that it might be better to give the token a more meaning name, like SqliteRolloutExperimentStatus instead of a general "Suffix".

Open in Gerrit

Related details

Attention is currently required from:

Rahul Singh
Xiaohan Zhao

Submit Requirements:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Rahul Singh (Gerrit)

unread,

Jun 19, 2026, 9:06:22 PM (10 hours ago) Jun 19

to Mingyu Lei, Steve Becker, Xiaohan Zhao, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, dmurph+watching...@chromium.org, dmurph+watch...@chromium.org, edgesto...@microsoft.com, storage...@chromium.org

Attention needed from Xiaohan Zhao

Rahul Singh voted and added 1 comment

Votes added by Rahul Singh

Commit-Queue

+1

1 comment

File tools/metrics/histograms/metadata/storage/histograms.xml

Line 1036, Patchset 13:<histogram name="LocalStorage.DatabaseOnDiskSizeKB{Suffix}" units="KB"

Mingyu Lei . resolved

I feel that it might be better to give the token a more meaning name, like SqliteRolloutExperimentStatus instead of a general "Suffix".

Rahul Singh

Good idea. Updated it to DomStorageSqliteRolloutStage. Thanks!

Open in Gerrit

Related details

Attention is currently required from:

Xiaohan Zhao

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

Review-Enforcement

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

Gerrit

satisfied_requirement

open

diffy

Rahul Singh (Gerrit)

unread,

12:26 AM (7 hours ago) 12:26 AM

to Mingyu Lei, Steve Becker, Xiaohan Zhao, Chromium LUCI CQ, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, dmurph+watching...@chromium.org, dmurph+watch...@chromium.org, edgesto...@microsoft.com, storage...@chromium.org

Attention needed from Xiaohan Zhao

Rahul Singh voted Commit-Queue+2

Commit-Queue

+2

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

12:39 AM (7 hours ago) 12:39 AM

to Rahul Singh, Mingyu Lei, Steve Becker, Xiaohan Zhao, Chromium Metrics Reviews, chromium...@chromium.org, asvitkine...@chromium.org, dmurph+watching...@chromium.org, dmurph+watch...@chromium.org, edgesto...@microsoft.com, storage...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

13 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: tools/metrics/histograms/metadata/storage/histograms.xml
Insertions: 4, Deletions: 3.

The diff is too large to show. Please review the diff.
```

Change information

Commit message:

DomStorage: Move LocalStorage.DatabaseOnDiskSizeKB into storage service

Moves emission of LocalStorage.DatabaseOnDiskSizeKB from
DomStorageContextWrapper into DomStorageDatabaseFactory's Open-time
histograms helper. It now picks the right backend path (LevelDB or
SQLite) and the right DatabaseMetricsType suffix during the SQLite
on-disk rollout. The unsuffixed name keeps firing for non-experimental
on-disk databases for backwards compatibility. A ".Experimental" variant
is added for the rollout cohort.

With DomStorageContextWrapper no longer needing to resolve a database
path, DomStorageDatabase::GetPath() has no remaining production callers.
So, this deletes it and migrates the remaining test callers to
GetLevelDbPath/GetSqlitePath.

Bug: 377242771

Change-Id: I4a35eee4f76bd9fed2139d29db25667c2dbbea41

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

Commit-Queue: Rahul Singh <rah...@microsoft.com>

Reviewed-by: Steve Becker <ste...@microsoft.com>

Reviewed-by: Mingyu Lei <le...@chromium.org>

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

Files:

M components/services/storage/dom_storage/async_dom_storage_database.cc
M components/services/storage/dom_storage/dom_storage_database.cc
M components/services/storage/dom_storage/dom_storage_database.h
M components/services/storage/dom_storage/local_storage_impl_unittest.cc
M components/services/storage/dom_storage/session_storage_impl_unittest.cc
M components/services/storage/dom_storage/sqlite/local_storage_sqlite_unittest.cc
M components/services/storage/dom_storage/sqlite/session_storage_sqlite_unittest.cc
M components/services/storage/dom_storage/test_support/fake_dom_storage_database_factory.cc
M content/browser/dom_storage/dom_storage_context_wrapper.cc
M tools/metrics/histograms/metadata/storage/histograms.xml

Change size: M

Delta: 10 files changed, 104 insertions(+), 80 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Steve Becker, +1 by Mingyu Lei

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

DomStorage: Move LocalStorage.DatabaseOnDiskSizeKB into storage service [chromium/src : main]

Xiaohan Zhao (Gerrit)

Xiaohan Zhao added 1 comment

Related details

Rahul Singh (Gerrit)

Rahul Singh added 1 comment

Related details

Rahul Singh (Gerrit)

Rahul Singh added 1 comment

Related details

Steve Becker (Gerrit)

Steve Becker added 3 comments

Related details

Rahul Singh (Gerrit)

Rahul Singh voted and added 3 comments

Votes added by Rahul Singh

3 comments

Related details

Steve Becker (Gerrit)

Steve Becker added 2 comments

Related details

Rahul Singh (Gerrit)

Rahul Singh voted and added 2 comments

Votes added by Rahul Singh

2 comments

Related details

Steve Becker (Gerrit)

Steve Becker voted Code-Review+1

Related details

Rahul Singh (Gerrit)

Rahul Singh added 1 comment

Related details

Mingyu Lei (Gerrit)

Mingyu Lei voted and added 1 comment

Votes added by Mingyu Lei

1 comment

Related details

Rahul Singh (Gerrit)

Rahul Singh voted and added 1 comment

Votes added by Rahul Singh

1 comment

Related details

Rahul Singh (Gerrit)

Rahul Singh voted Commit-Queue+2

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information