Move GetSiteURL to SecurityPrincipal interface [chromium/src : main]
Viktoriya Bryhider (Gerrit)
Viktoriya Bryhider added 17 comments![]( "Open in Gerrit")
Publishing removing GetSiteURL from SiteInstance.
I have moved all calls as is to `GetSecurityPrincipal().GetDeprecatedSiteURL()`. It should be green now. Checked all callsites from outside of /content and left unresolved comments where we can get rid of GetSecurityPrincipal().GetDeprecatedSiteURL() - can create separate Cls if you could confirm that those are acceptable replacements.
Please let me know how to proceed or if there a way to make it easier...
Thanks!
->GetSecurityPrincipal()`net::SchemefulSite(frame_host->GetLastCommittedURL()).GetURL()` should work here.
owner_site_instance->GetSecurityPrincipal().GetDeprecatedSiteURL());Can pass `owner_rfh` instead and use `owner_rfh->GetLastCommittedOrigin().GetURL()` in place of `owner_site_instance->GetSecurityPrincipal().GetDeprecatedSiteURL()`. I have tried this locally - and tests seems to pass... Can create CL
const GURL& site_url = web_contents->GetSiteInstance()Intentionally using SiteInstance::GetSiteURL - https://chromium-review.googlesource.com/c/chromium/src/+/7426181
const GURL& site_url =In theory can try to replace with `net::SchemefulSite(render_frame_host->GetLastCommittedURL())` to keep eTLD+1, but it will not work for hosted apps(?).
Another option is to keep it and wait for crbug.com/40775860 mentioned above to replace URL with title.
site_instance_->GetSecurityPrincipal().GetDeprecatedSiteURL().spec());I have tried to replace this with `net::SchemefulSite(rfh_->GetLastCommittedURL());` and run tests `browser_tests --gtest_filter="*FencedFrameTaskBrowserTest*" ` - the only difference is in last slash: `https://b.test/` vs `https://b.test` - looks like safe to replace to me.
site_instance_->GetSecurityPrincipal().GetDeprecatedSiteURL();Need effective URL for hosted apps - otherwise can use SchemefulSite(GetLastCommittedURL())
GURL site_url = contents->GetPrimaryMainFrame()Can potentially use `contents->GetLastCommittedURL()` here. BrowserInstantControllerTest.DefaultSearchProviderChanged passes with that change - not sure if there any corner cases.
webui::LogWebUIShown(web_contents_ret->GetSiteInstance()Test `LogWebUIUrlTest.ShownWebUIForPreloadedPage` is hitting this codepath: I have checked:
1. web_contents_ret->GetVisibleURL();
2. webui_url;
3. web_contents_ret->GetURL(); all of them are the same - can try to substitute `GetDeprecatedSiteURL()` with one of them.
const GURL& GuestViewBase::GetOwnerSiteURL() const {Tried to use `owner_rfh()->GetLastCommittedOrigin().GetURL()` here - tests which are hitting that codepath (*WebViewContentScript*) are passing, not sure about corner cases if any - can put CL.
owner_rfh()owner_rfh should have committed navigation from what I'm reading. Can try to use `owner_rfh()->GetLastCommittedOrigin().host()`. Have not tested.
render_frame_host()Have WIP to replace SiteURL with SecurityPrincipal in Services - https://chromium-review.googlesource.com/c/chromium/src/+/7712794
->GetSecurityPrincipal()Considering replacing it with `embedder_rfh->GetMainFrame()->GetLastCommittedOrigin()` similar to line 119 in this file. Have not found cases when it will not work.
->GetSiteInstance()Considering replacing it with `render_frame_host()->GetLastCommittedURL().withEmptyPath()`
.GetDeprecatedSiteURL()Seems safe to use frame.GetLastCommittedURL() or frame.GetLastCommittedOrigin() here.
const GURL& owner_site_url =Same as https://chromium-review.googlesource.com/c/chromium/src/+/7614138/2/chrome/browser/extensions/chrome_extensions_browser_client_non_android.cc - can use `owner_rfh->GetLastCommittedOrigin().GetURL()`
Related details
- Alex Moshchuk
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. |
Alex Moshchuk (Gerrit)
Alex Moshchuk added 17 comments![]( "Open in Gerrit")
Thanks, seems mostly reasonable to me following our offline discussion today, with some comments below (most responses would be for followup CLs).
Let's add a note that this is a mechanical CL and there are no behavior changes.
->GetSecurityPrincipal()`net::SchemefulSite(frame_host->GetLastCommittedURL()).GetURL()` should work here.
Ack, though I'm curious why this code specifically needs a site, rather than the last committed origin, for example. Might be worth discussing that with owners of this code in a followup CL.
owner_site_instance->GetSecurityPrincipal().GetDeprecatedSiteURL());Can pass `owner_rfh` instead and use `owner_rfh->GetLastCommittedOrigin().GetURL()` in place of `owner_site_instance->GetSecurityPrincipal().GetDeprecatedSiteURL()`. I have tried this locally - and tests seems to pass... Can create CL
Sounds good. The owner here should be at a chrome-extension: or WebUI URL, and I'd expect that this needs that extension ID (or WebUI host). It might be slightly tricky in that GetLastCommittedOrigin().GetURL() would be correct if the owner embeds an about:blank subframe in which it then embeds the <webview> tag, but may not work if the embedder had a sandboxed subframe (which would have an opaque origin), which then embedded the <webview>. So maybe the right thing to use might actually be `GetLastCommittedOrigin().GetTupleOrPrecursorTupleIfOpaque().GetURL()`.
Or maybe there's some other mechanism we can design for SecurityPrincipal to be able to tell whether it's for an extension, and if so, what the extension ID is (without violating content layering).
site_instance_->GetSecurityPrincipal().GetDeprecatedSiteURL().spec());I have tried to replace this with `net::SchemefulSite(rfh_->GetLastCommittedURL());` and run tests `browser_tests --gtest_filter="*FencedFrameTaskBrowserTest*" ` - the only difference is in last slash: `https://b.test/` vs `https://b.test` - looks like safe to replace to me.
Acknowledged.
However, note that in general, the task manager groups tasks by process and probably cares about effective URLs. So tasks that are part of hosted apps might still need a chrome-extension: site URL that actually conveys their process label.
GURL site_url = contents->GetPrimaryMainFrame()Can potentially use `contents->GetLastCommittedURL()` here. BrowserInstantControllerTest.DefaultSearchProviderChanged passes with that change - not sure if there any corner cases.
I don't know for sure, but these seem like places where we potentially might need effective URLs (e.g., there's a comparison involving 3P NTP below), so maybe they will still need effective URLs and use whatever new API we introduce for them on SecurityPrincipal.
webui::LogWebUIShown(web_contents_ret->GetSiteInstance()Test `LogWebUIUrlTest.ShownWebUIForPreloadedPage` is hitting this codepath: I have checked:
1. web_contents_ret->GetVisibleURL();
2. webui_url;
3. web_contents_ret->GetURL(); all of them are the same - can try to substitute `GetDeprecatedSiteURL()` with one of them.
I wouldn't recommend WebContents::GetVisibleURL or GetURL, as they have some problems. Seems like `webui_url` would contain the most relevant URL here. Looks like LogWebUIShown just needs it to be an origin.
const GURL& GuestViewBase::GetOwnerSiteURL() const {Tried to use `owner_rfh()->GetLastCommittedOrigin().GetURL()` here - tests which are hitting that codepath (*WebViewContentScript*) are passing, not sure about corner cases if any - can put CL.
Should be similar to the other <webview>-related case that I commented on above.
owner_rfh()owner_rfh should have committed navigation from what I'm reading. Can try to use `owner_rfh()->GetLastCommittedOrigin().host()`. Have not tested.
Should be similar to other webview cases above - let's make sure it works for cases where a sandboxed frame (with an opaque origin) embeds a <webview>, where we still need to be able to resolve the owner's host. (FWIW, SlimWebView will probably only be used on WebUI pages on Android.)
const GURL& site_url() const { return site_url_; }We could consider still keeping this as an additional non-virtual accessor on SiteInfo, just as a shorthand to use for code inside content. I'm not sure it's worth it, but something to consider. WDYT?
// for callers that have not yet been migrated away from the site URL.Some ideas for additional text to cover a couple of other points here:
- Cases like hosted apps should keep using the site URL for now for resolving effective URLs. We expect no new use cases of effective URLs to arise, and for most use cases of effective URLs to go away over time (for example, hosted apps are deprecated and will be removed).
- More SecurityPrincipal properties will be added over time, which should cover more Site URL use cases. If there's a property that's missing on SecurityPrincipal and that's needed for a new feature, reach out to content/OWNERS to discuss how to add it.
// DEPRECATED: Prefer more specific SecurityPrincipal methods (e.g.,Maybe also mention "GetLastCommittedOrigin() or" once again here?
virtual const GURL& GetSiteURL() const = 0;One note here is that this might be a popular API used by other content/ embedders, so we might be forcing them to do a bunch of work to use GetSecurityPrincipal()->GetDeprecatedSiteURL() instead. But there's probably no way around it.
->GetSecurityPrincipal()Considering replacing it with `embedder_rfh->GetMainFrame()->GetLastCommittedOrigin()` similar to line 119 in this file. Have not found cases when it will not work.
Should be similar to other webview-related cases, let's just be consistent in what we use in all of them, and do them all in one followup CL.
->GetSiteInstance()Considering replacing it with `render_frame_host()->GetLastCommittedURL().withEmptyPath()`
(same here)
.GetDeprecatedSiteURL()Seems safe to use frame.GetLastCommittedURL() or frame.GetLastCommittedOrigin() here.
Acknowledged - GetLastCommittedOrigin is probably better, as this is for crash keys, and we try to avoid exposing the full URL there for privacy reasons.
const GURL& owner_site_url =Same as https://chromium-review.googlesource.com/c/chromium/src/+/7614138/2/chrome/browser/extensions/chrome_extensions_browser_client_non_android.cc - can use `owner_rfh->GetLastCommittedOrigin().GetURL()`
- Viktoriya Bryhider
Viktoriya Bryhider (Gerrit)
Viktoriya Bryhider added 8 comments![]( "Open in Gerrit")
I have addressed review comments.
Added easier follow-ups for now - https://crrev.com/c/7796943 and https://crrev.com/c/7796820 and GetHost API - https://crrev.com/c/7794385.
There are 3 open questions with the main one if we want GetHost API and if it should be used for webview/extensions cases - could you please check when there is a time?
(this cl keep getting merge conflicts because of the files which it touches - I will rebase it again after all approvals...)
Let's add a note that this is a mechanical CL and there are no behavior changes.
Done
owner_site_instance->GetSecurityPrincipal().GetDeprecatedSiteURL());Alex MoshchukCan pass `owner_rfh` instead and use `owner_rfh->GetLastCommittedOrigin().GetURL()` in place of `owner_site_instance->GetSecurityPrincipal().GetDeprecatedSiteURL()`. I have tried this locally - and tests seems to pass... Can create CL
Sounds good. The owner here should be at a chrome-extension: or WebUI URL, and I'd expect that this needs that extension ID (or WebUI host). It might be slightly tricky in that GetLastCommittedOrigin().GetURL() would be correct if the owner embeds an about:blank subframe in which it then embeds the <webview> tag, but may not work if the embedder had a sandboxed subframe (which would have an opaque origin), which then embedded the <webview>. So maybe the right thing to use might actually be `GetLastCommittedOrigin().GetTupleOrPrecursorTupleIfOpaque().GetURL()`.
Or maybe there's some other mechanism we can design for SecurityPrincipal to be able to tell whether it's for an extension, and if so, what the extension ID is (without violating content layering).
I have added GetHost API - https://crrev.com/c/7794385/2/content/public/browser/security_principal.h - and I think I should be able to use it here. Do you have opinion which would be preffered between `GetLastCommittedOrigin().GetTupleOrPrecursorTupleIfOpaque().GetURL()` vs `SP.GetHost()` if we will agree to continue with GetHost?
GURL site_url = contents->GetPrimaryMainFrame()Alex MoshchukCan potentially use `contents->GetLastCommittedURL()` here. BrowserInstantControllerTest.DefaultSearchProviderChanged passes with that change - not sure if there any corner cases.
I don't know for sure, but these seem like places where we potentially might need effective URLs (e.g., there's a comparison involving 3P NTP below), so maybe they will still need effective URLs and use whatever new API we introduce for them on SecurityPrincipal.
const GURL& site_url() const { return site_url_; }We could consider still keeping this as an additional non-virtual accessor on SiteInfo, just as a shorthand to use for code inside content. I'm not sure it's worth it, but something to consider. WDYT?
Added back to decrease the diff - but there is no much usage of it. No strong preference.
// Compared to the AgentClusterKey, this URL might have been overridden from@ale...@chromium.org There is no other mentions about AgentClusterKey in this header and it may be confusing(?) - should I keep this comment which was copied from SiteInfo?
// for callers that have not yet been migrated away from the site URL.Some ideas for additional text to cover a couple of other points here:
- Cases like hosted apps should keep using the site URL for now for resolving effective URLs. We expect no new use cases of effective URLs to arise, and for most use cases of effective URLs to go away over time (for example, hosted apps are deprecated and will be removed).
- More SecurityPrincipal properties will be added over time, which should cover more Site URL use cases. If there's a property that's missing on SecurityPrincipal and that's needed for a new feature, reach out to content/OWNERS to discuss how to add it.
Done
// DEPRECATED: Prefer more specific SecurityPrincipal methods (e.g.,Maybe also mention "GetLastCommittedOrigin() or" once again here?
- Alex Moshchuk
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. |