Introduce PreloadActivationReportManager [chromium/src : main]
Jiacheng Guo (Gerrit)
Jiacheng Guo added 4 comments![]( "Open in Gerrit")
// Manages sending activation beacons for prefetched or prerendered resourcesJiacheng GuoThe prefetch activation beacon are only sent for pages rather than all resources, update the comment to reflect this.
Done
// TODO(crbug.com/40227626): Add appropriate traffic annotation.Jiacheng GuoUse `MISSING_TRAFFIC_ANNOTATION` for the time being. We will add the traffic annotation separately.
Done
GetWebContents().GetPrimaryMainFrame()->GetStoragePartition();Jiacheng GuoIt is dangerous to call `GetWebContents()` here since the active WebContents might be different from the NavigationRequest trigerring the method. Instead we should pass in the WebContents bound to the prefetch/prerender.
Done
auto* loader_ptr = loader.get();Jiacheng GuoDoes this even work? `loader` will be destructed when the function returns and loader_ptr will be dangling. Maybe we should refer to the pattern in `chrome/browser/profile_resetter/reset_report_uploader.cc`
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas Ouellet-Payeur (Gerrit)
Nicolas Ouellet-Payeur voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nicolas Ouellet-Payeur
| Code-Review | +1 |
1 comment
cookies_allowed: NOit uses GetPrimaryMainFrame()->GetStoragePartition(), so it sounds like yes?
Related details
- Hiroki Nakagawa
- Jiacheng Guo
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. |
Jiacheng Guo (Gerrit)
Jiacheng Guo added 1 comment![]( "Open in Gerrit")
cookies_allowed: NOit uses GetPrimaryMainFrame()->GetStoragePartition(), so it sounds like yes?
We are always using credential mode `request->credentials_mode = network::mojom::CredentialsMode::kOmit` so cookies are not sent. `GetPrimaryMainFrame()->GetStoragePartition()` is accessed to share the URL loader when sending requests from the browser process as instructed in the [comments](https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/storage_partition.h;l=117;drc=ce3629f6f1cdbdb670dbf759e6b7c89c4a92a8fb).
Related details
- Hiroki Nakagawa
- Nicolas Ouellet-Payeur
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. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 1 comment![]( "Open in Gerrit")
: public WebContentsUserData<PreloadActivationReportManager> {I wonder if this should reuse `KeepAliveURLLoaderService` so that a request can outlive tab close. See our private chat for details.
Related details
- Jiacheng Guo
- Nicolas Ouellet-Payeur
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. |
Nicolas Ouellet-Payeur (Gerrit)
Nicolas Ouellet-Payeur voted and added 1 comment![]( "Open in Gerrit")
Votes added by Nicolas Ouellet-Payeur
| Code-Review | +1 |
1 comment
Jiacheng Guoit uses GetPrimaryMainFrame()->GetStoragePartition(), so it sounds like yes?
We are always using credential mode `request->credentials_mode = network::mojom::CredentialsMode::kOmit` so cookies are not sent. `GetPrimaryMainFrame()->GetStoragePartition()` is accessed to share the URL loader when sending requests from the browser process as instructed in the [comments](https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/storage_partition.h;l=117;drc=ce3629f6f1cdbdb670dbf759e6b7c89c4a92a8fb).
Right, gotcha
Related details
- Jiacheng Guo
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. |
Jiacheng Guo (Gerrit)
Jiacheng Guo added 1 comment![]( "Open in Gerrit")
: public WebContentsUserData<PreloadActivationReportManager> {I wonder if this should reuse `KeepAliveURLLoaderService` so that a request can outlive tab close. See our private chat for details.
I've uploaded the CL to make PreloadActivationReportManager lifetime bound to the Profile rather than the WebContents. The `KeepAliveURLLoaderService` is closely related to renderer process and blink features so I prefer to keep the service as a separate class.
Related details
- Hiroki Nakagawa
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. |
Hiroki Nakagawa (Gerrit)
Hiroki Nakagawa added 7 comments![]( "Open in Gerrit")
// that have been consumed.Can you leave comments about lifetime model of this manager? (e.g., This is bound to
`BrowserContext` so that beacon requests can outlive their initiator documents)
: public WebContentsUserData<PreloadActivationReportManager> {Jiacheng GuoI wonder if this should reuse `KeepAliveURLLoaderService` so that a request can outlive tab close. See our private chat for details.
I've uploaded the CL to make PreloadActivationReportManager lifetime bound to the Profile rather than the WebContents. The `KeepAliveURLLoaderService` is closely related to renderer process and blink features so I prefer to keep the service as a separate class.
Acked. Sounds reasonable.
DCHECK_CURRENTLY_ON(BrowserThread::UI);`CHECK(browser_context);`
WebContents* web_contents) {```
DCHECK_CURRENTLY_ON(BrowserThread::UI);
CHECK(web_contents);
```
request->credentials_mode = network::mojom::CredentialsMode::kOmit;Did you audit the other parameters on ResourceRequest? Is it okay to leave them unset (e.g., `request_initiator`, `load_flags`, `resource_type`, `keepalive`, `skip_service_worker`)?
I don't mean to block this CL. If they haven't been audited yet, we can handle it in a follow-up.
base::Unretained(this), std::move(it)));Is it guaranteed that this callback is never called after `this` (`PreloadActivationReportManager`) is destroyed?
I wonder if `loader` can be bound to the callback itself so that the callback doesn't need to touch `PreloadActivationReportManager`?
```
loader_ptr->DownloadHeadersOnly(
storage_partition->GetURLLoaderFactoryForBrowserProcess().get(),
base::DoNothingWithBoundArgs(std::move(loader));
```
scoped_refptr<net::HttpResponseHeaders> headers) {`DCHECK_CURRENTLY_ON(BrowserThread::UI);`
Related details
- Jiacheng Guo
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. |
Jiacheng Guo (Gerrit)
Jiacheng Guo added 6 comments![]( "Open in Gerrit")
Can you leave comments about lifetime model of this manager? (e.g., This is bound to
`BrowserContext` so that beacon requests can outlive their initiator documents)
Done
DCHECK_CURRENTLY_ON(BrowserThread::UI);Jiacheng Guo`CHECK(browser_context);`
Done
WebContents* web_contents) {Jiacheng Guo```
DCHECK_CURRENTLY_ON(BrowserThread::UI);
CHECK(web_contents);
```
Done
request->credentials_mode = network::mojom::CredentialsMode::kOmit;Did you audit the other parameters on ResourceRequest? Is it okay to leave them unset (e.g., `request_initiator`, `load_flags`, `resource_type`, `keepalive`, `skip_service_worker`)?
I don't mean to block this CL. If they haven't been audited yet, we can handle it in a follow-up.
Done
Is it guaranteed that this callback is never called after `this` (`PreloadActivationReportManager`) is destroyed?
I wonder if `loader` can be bound to the callback itself so that the callback doesn't need to touch `PreloadActivationReportManager`?
```
loader_ptr->DownloadHeadersOnly(
storage_partition->GetURLLoaderFactoryForBrowserProcess().get(),
base::DoNothingWithBoundArgs(std::move(loader));
```
Done
scoped_refptr<net::HttpResponseHeaders> headers) {Jiacheng Guo`DCHECK_CURRENTLY_ON(BrowserThread::UI);`
Done
Related details
- Hiroki Nakagawa
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |