Pass the AgentClusterKey from the browser process [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 3 comments![]( "Open in Gerrit")
AgentClusterKey* agent_cluster_key);Blink Style Guide: Naming - May leave obvious parameter names out of function declarations. The parameter name `agent_cluster_key` can be omitted from the function declaration in the header file as the type `AgentClusterKey*` is self-descriptive.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
WindowAgent(AgentGroupScheduler& agent_group_scheduler,Blink Style Guide: Naming - May leave obvious parameter names out of function declarations. The parameter name `agent_cluster_key` can be omitted from the function declaration in the header file as the type `AgentClusterKey*` is self-descriptive.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
WindowAgent* GetAgentForAgentClusterKey(Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. Consider using an enum for `has_potential_universal_access_privilege` to improve clarity at call sites, for example: `enum class UniversalAccessPrivilege { kNo, kYes };`.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
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. |
Camille Lamy (Gerrit)
Camille Lamy added 3 comments![]( "Open in Gerrit")
Blink Style Guide: Naming - May leave obvious parameter names out of function declarations. The parameter name `agent_cluster_key` can be omitted from the function declaration in the header file as the type `AgentClusterKey*` is self-descriptive.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
WindowAgent(AgentGroupScheduler& agent_group_scheduler,Blink Style Guide: Naming - May leave obvious parameter names out of function declarations. The parameter name `agent_cluster_key` can be omitted from the function declaration in the header file as the type `AgentClusterKey*` is self-descriptive.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
Blink Style Guide: Prefer enums or StrongAliases to bare bools for function parameters. Consider using an enum for `has_potential_universal_access_privilege` to improve clarity at call sites, for example: `enum class UniversalAccessPrivilege { kNo, kYes };`.
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Acknowledged
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camille Lamy (Gerrit)
Camille Lamy added 4 comments![]( "Open in Gerrit")
@Alex: PTAL. This is moving the agent cluster decision in the browser process and passing it to the renderer process. I have taken the option to try to use the AgentClusterKey from SiteInfo as much as possible, as I think we want to move to using it most of the time eventually. However, if we want to keep closer to the current code, we could also just check whether we have an origin-keyed/site-keyed/COI key and always update the origin/site of the key.
// Now build the appropriate AgentClusterKey for commit, starting with theI have been considering moving all the code below to SiteInfo, as it essentially undoes all the special cases of GetAgentClusterKeyForURL, so maybe having the two in the same place to compare would be nice.
bool is_navigation_to_existing_entry =I still have one test failing for this specific case. I think it's a case of a subframe navigation to an existing subframe NavigationEntry. I'm not sure how to write the condition for it.
mojom::CrossOriginIsolationMode cross_origin_isolation_mode)The presubmit checks complain about non-Blink mojom usage, but since this is the public interface that seems to be the correct thing to do to eventually translate into Blink mojom types. I'm not sure where I need to authorize this as I would have imagined that third_party/blink/public/web would not be covered by the check.
Related details
- Alex Moshchuk
- Daniel Cheng
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. |
Daniel Cheng (Gerrit)
Daniel Cheng added 10 comments![]( "Open in Gerrit")
commit_params->agent_cluster_key = blink::mojom::AgentClusterKey::New();Should `blink::CreateCommitNavigationParams()` also just set this? Seems like it's not really optional anyway.
(Though I guess it becomes messy if we make the agent cluster key a union as I suggested below–does the agent cluster key actually have three distinct states? Site, origin, and neither? Though based off the WebAgentClusterKey class... I'm not actually sure)
commit_params->agent_cluster_key = blink::mojom::AgentClusterKey::New();And that would help with some of the boilerplate elsewhere I think
// Indicates if cross-origin isolation gives access to cross-origin isolatedNit: extra space
// when cross-origin sioaltion is enabled by COOP + COEP. When cross-originNit: isolation.
// Additionally, the AgentClusterKey might have a CrossOriginIsolationKey isif
// The AgentClusterKey has either a site URL or an origin. In the first case,
// the AgentClusterKey is site keyed, in the second case, it is origin keyed.
url.mojom.Url? url;
url.mojom.Origin? origin;
// Additionally, the AgentClusterKey might have a CrossOriginIsolationKey is
// the context is cross-origin isolated. In this case, the AgentClusterKey
// must be origin-keyed.
// https://wicg.github.io/document-isolation-policy/#coi-agent-cluster-key
CrossOriginIsolationKey? isolation_key;It seems like maybe this should be a union consisting of either a `url.mojom.Url` or a `OriginKeyedAgentClusterKey` struct containing a `url.mojom.Origin` and a `CrossOriginIsolationKey` (not sure if the COI key is required in the case of origin-keyed origins)
std::optional<WebCrossOriginIsolationKey> isolation_key;As noted above, can we restructure this so it's a variant of a WebURL and a struct containing a WebSecurityOrigin and an optional key?
mojom::CrossOriginIsolationMode cross_origin_isolation_mode)The presubmit checks complain about non-Blink mojom usage, but since this is the public interface that seems to be the correct thing to do to eventually translate into Blink mojom types. I'm not sure where I need to authorize this as I would have imagined that third_party/blink/public/web would not be covered by the check.
I agree this is fine; can you link me to an example of the presubmit warning?
AgentClusterKey();What does this create by default? Is it site keyed, origin keyed, or neither?
// the custom HashTraits defined at the end of this file.I don't think we should have both--or at the very least, one should delegate to the other.
Can you file a TODO to clean this up? Is there a failing test someone can run when they are trying to figure out what's going wrong here?
Related details
- Alex Moshchuk
- Camille Lamy
Alex Moshchuk (Gerrit)
Alex Moshchuk added 11 comments![]( "Open in Gerrit")
Thanks! I'll defer to Daniel on the blink changes; a few comments on the content side.
class CONTENT_EXPORT AgentClusterKey {I guess I'm a bit worried that we now have 3-4 places that define AgentClusterKeys (here, mojo, WebAgentClusterKey, and blink::AgentClusterKey), and they might get out of sync. Maybe it's intentional that the browser-side and renderer-side keys aren't going to be identical (e.g., the renderer doesn't need oac_status_), though we could at least add comments reminding what else must be updated when someone wants to add something to the AgentClusterKey? And maybe we could use LINT.IfChange to enforce that things stay updated together where it makes sense?
// SiteInfo, as it will only duplicate the information in AgentClusterKey.Might be worth updating this documentation to mention the other definitions of agent cluster key used in the renderer (and for sending this key to the renderer), and how they differ practically from this key.
// Now build the appropriate AgentClusterKey for commit, starting with theI have been considering moving all the code below to SiteInfo, as it essentially undoes all the special cases of GetAgentClusterKeyForURL, so maybe having the two in the same place to compare would be nice.
Yes, it might be worth moving this code into a separate helper, and maybe SiteInfo is a better place for it, as this function is getting long. It might be worth keeping the focus of this function on the OAC stuff as before, and then computing the key_to_commit as a separate step.
One high-level concern I have with the content/browser side of this CL is indeed about introducing another place for handling these special cases, and moreover trying to reconstruct how we computed a SiteInfo based on the AgentClusterKey after the fact, which seems fragile (and potentially hard to maintain for future changes to the process model). Do you think it could be better to compute both the agent cluster key for SiteInfo and the agent cluster key to send to the renderer in the same place in GetAgentClusterKeyForURL? Basically, have all the special cases in one place, but computing two keys? Or at least compute the should_update_origin_or_site bit there, so that we know that the site URL we ended up with in SiteInfo is a Chrome-process-model specific URL and not what the actual agent cluster key should be per spec.
(I'm not sure this is better or even feasible, just curious if it's worth thinking about at all.)
// isolation key. Currently, pages cross-origin isolated through COOP and COEP
// are not given a cross-origin isolated AgentClusterKey. Override theIt seems that this will be addressed by your https://chromium-review.googlesource.com/c/chromium/src/+/6516176? Will you be landing this CL before or after that?
// the document already as a cross-origin isolated AgentClusterKey, whichhas
origin.scheme())) {Is this to undo the case [here](https://source.chromium.org/chromium/chromium/src/+/main:content/browser/site_info.cc;l=791;drc=7ce25d3420a0eb77c46618536b3434f7b9ceef66)? Does it matter that this only applies to top-chrome WebUIs and not all WebUIs? Should it be more restricted to IsWebUIAndUsesTLDForProcessLockURL cases?
should_update_origin_or_site = true;I guess we don't have this problem with features::kDefaultSiteInstanceGroups?
// redirect causes the response URL to no longer match the original
// SiteInstance. In that case, create a new AgentClusterKey.Is this actually intended behavior and not a bug? I think we try to disqualify the original SiteInstance in the NavigationEntry from being reused if it's no longer suitable for the redirected URL, with things like RenderFrameHostManager::CanUseDestinationInstance(). I'm surprised it would need its own condition here, as it seems like it might be violating our SiteInstance assignment rules.
if (key_to_commit.IsSiteKeyed()) {Can there be some sort of conversion function to populate a mojo AgentClusterKey from a normal one, ideally in agent_cluster_key.*?
process_lock.agent_cluster_key().IsCrossOriginIsolated();Can this be cleaned up independently, or does it depend on something in this CL?
Related details
- Camille Lamy
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. |