Move GetStoragePartitionConfig to SecurityPrincipal interface [chromium/src : main]

0 views

Skip to first unread message

Viktoriya Bryhider (Gerrit)

unread,

Jan 28, 2026, 4:45:24 PM (2 days ago) Jan 28

to Alex Moshchuk, Liang Zhao, James Su, James Maclean, Hiroki Nakagawa, Kevin McNee, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, shimazu+se...@chromium.org, prerenderi...@chromium.org, blink-work...@chromium.org, gavin...@chromium.org, dtraino...@chromium.org, keithle...@chromium.org, dmurph+wat...@chromium.org, nona+...@chromium.org, extension...@chromium.org, chromium-a...@chromium.org, servicewor...@chromium.org, tranbaod...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, druber...@chromium.org, kinuko+ser...@chromium.org, shuche...@chromium.org, msrame...@chromium.org, yhanad...@chromium.org, navigation...@chromium.org, tburkar...@chromium.org, webap...@microsoft.com, dullweb...@chromium.org, ajwong...@chromium.org, alexmo...@chromium.org, creis...@chromium.org

Attention needed from Alex Moshchuk and Liang Zhao

Viktoriya Bryhider added 1 comment

Patchset-level comments

File-level comment, Patchset 14 (Latest):

Viktoriya Bryhider . resolved

This is first method for SecurityPrincipal interface - could you please review it at your convenience.
Commit message is altered from the generated one by AI prompt.

Need suggestion if to try to remove stuff from this file https://chromium-review.googlesource.com/c/chromium/src/+/7231185/14/chrome/browser/android/content/web_contents_factory.cc

Thanks.

Open in Gerrit

Related details

Attention is currently required from:

Alex Moshchuk
Liang 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

Charlie Reis (Gerrit)

unread,

Jan 30, 2026, 4:03:36 PM (9 hours ago) Jan 30

to Viktoriya Bryhider, Alex Moshchuk, Liang Zhao, James Su, James Maclean, Hiroki Nakagawa, Kevin McNee, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, shimazu+se...@chromium.org, prerenderi...@chromium.org, blink-work...@chromium.org, gavin...@chromium.org, dtraino...@chromium.org, keithle...@chromium.org, dmurph+wat...@chromium.org, nona+...@chromium.org, extension...@chromium.org, chromium-a...@chromium.org, servicewor...@chromium.org, tranbaod...@chromium.org, horo+...@chromium.org, kinuko...@chromium.org, druber...@chromium.org, kinuko+ser...@chromium.org, shuche...@chromium.org, msrame...@chromium.org, yhanad...@chromium.org, navigation...@chromium.org, tburkar...@chromium.org, webap...@microsoft.com, dullweb...@chromium.org, ajwong...@chromium.org, alexmo...@chromium.org, creis...@chromium.org

Attention needed from Alex Moshchuk, Liang Zhao and Viktoriya Bryhider

Charlie Reis added 4 comments

Patchset-level comments

File-level comment, Patchset 14 (Latest):

Charlie Reis . resolved

Thanks! This one seems mostly ok, but the verification logic might have an issue.

Commit Message

Line 13, Patchset 14 (Latest):content isolation.

Charlie Reis . unresolved

Once we settle on how to handle the verification logic, it's worth mentioning something about how it needed to be moved to make this possible.

File chrome/browser/android/content/web_contents_factory.cc

Line 75, Patchset 7: new WebContentsFactoryDataDeleter(web_contents.get(),

Viktoriya Bryhider . unresolved

This code was part of android experiment kMayLaunchUrlUsesSeparateStoragePartition https://chromium-review.googlesource.com/c/chromium/src/+/5462668 - the feature looks like gone - checking if we want to try to remove this code. Also it looks like the only feature where SiteInstance::CreateForFixedStoragePartition is used beside tests.

Charlie Reis

If it's not going to launch, then it sounds like it would be a good candidate for cleaning up unused code, possibly including CreateForFixedStoragePartition. That CreateForFixedStoragePartition API was added in https://chromium-review.googlesource.com/c/chromium/src/+/5012192, which apparently was used for Isolated PWAs at first, but it looks like it was removed as part of Lacros in https://chromium-review.googlesource.com/c/chromium/src/+/5947808.

I do see the [MayLaunchUrlUsesSeparateStoragePartition feature flag](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/flags/android/chrome_feature_list.cc;drc=618b6f8f616ad8eb1824b4030862039589de8178;l=694) still in the codebase, but the author doesn't seem to be on the team anymore. Maybe we can find someone on that team to confirm if the code can be removed?

Seems fine to do that separately, though I agree it would be nice to remove those.

File content/browser/site_info.cc

Line 446, Patchset 14 (Latest): verify_storage_partition_config_ = true;

Charlie Reis . unresolved

This doesn't look equivalent to the old SiteInstanceImpl::verify_storage_partition_info_ to me at first glance. That one appears to only be set to true if the StoragePartitionConfig is accessed before the SiteInstance has a site, but we can't check a similar condition from here.

I haven't paged in all the reasoning behind the validation, which was added back in https://chromium-review.googlesource.com/c/chromium/src/+/2745227, but I'm guessing we might want to preserve more about that behavior?