[DOMStorage] Add LevelDB-to-SQLite migration feature. [chromium/src : main]

0 views
Skip to first unread message

Xiaohan Zhao (Gerrit)

unread,
5:44 AM (2 hours ago) 5:44 AM
to Rahul Singh, Chromium LUCI CQ, chromium...@chromium.org, dmurph+watching...@chromium.org, dmurph+watch...@chromium.org, edgesto...@microsoft.com, storage...@chromium.org
Attention needed from Rahul Singh

Xiaohan Zhao voted and added 4 comments

Votes added by Xiaohan Zhao

Commit-Queue+1

4 comments

File components/services/storage/dom_storage/async_dom_storage_database.cc
Line 338, Patchset 9: migration_paths_ = std::move(result.migration_paths);
Rahul Singh . resolved

Instead of creating these path in DomStorageDatabase, we could just forward the storage_type and storage_partition_dir to this cb from Open() and build the paths here if (!is_sqlite_).

Xiaohan Zhao

Done

File components/services/storage/dom_storage/dom_storage_database.cc
Line 500, Patchset 9: base::FilePath leveldb_path =
DomStorageDatabase::GetLevelDbPath(storage_type, storage_partition_dir);
base::FilePath sqlite_path =
DomStorageDatabase::GetSqlitePath(storage_type, storage_partition_dir);
OpenResult::MigrationPaths migration_paths{
.leveldb_path = leveldb_path,
.sqlite_path = sqlite_path,
.sqlite_staging_path = sqlite_path.AddExtensionASCII("migrating"),
Rahul Singh . resolved

These don't need to be populated here, do they? We have access to these helper functions in AsyncDomStorageDatabase. I think the only thing we are integrating into the Open and initialize flow for is to CheckLevelDBStatus. So maybe all you really need is a `bool is_level_db_present`. And even that we can deduce from OpenResult::is_sqlite, no?

Because `bool is_level_db_present = !is_sqlite;`


So can we forego any changes to OpenResult and in the code leading up to InitializeDatabase?

Xiaohan Zhao

Done

File components/services/storage/dom_storage/features.cc
Line 26, Patchset 9: {DomStorageSqliteRolloutStage::kMigrateLevelDbToSqlite,
"MigrateLevelDbToSqlite"},
Rahul Singh . resolved

It feels odd to have this be a param for the NewDatabases feature. Migration will move *existing* databases from LevelDB to SQLite based backend. Should this be its own feature like `kDomStorageSqliteMigration` or do we need this approach for some reason. If so, should we add a comment to that effect?

Xiaohan Zhao

Done. Added the new feature `kDomStorageSqliteMigration` and removed this `DomStorageSqliteRolloutStage::kMigrateLevelDbToSqlite`. Thanks!

Line 66, Patchset 9: // Migration uses a dedicated open path and never consults this helper.
Rahul Singh . resolved

Should this be a NOTREACHED then?

Xiaohan Zhao

Done

Open in Gerrit

Related details

Attention is currently required from:
  • Rahul Singh
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: Iebd6bc758da9622dcfb03dd4882cbc3ccdccf543
Gerrit-Change-Number: 7858502
Gerrit-PatchSet: 11
Gerrit-Owner: Xiaohan Zhao <xiaoh...@microsoft.com>
Gerrit-Reviewer: Rahul Singh <rah...@microsoft.com>
Gerrit-Reviewer: Xiaohan Zhao <xiaoh...@microsoft.com>
Gerrit-Attention: Rahul Singh <rah...@microsoft.com>
Gerrit-Comment-Date: Fri, 10 Jul 2026 09:43:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Rahul Singh <rah...@microsoft.com>
satisfied_requirement
unsatisfied_requirement
open
diffy
Reply all
Reply to author
Forward
0 new messages