[WIP] OMTPrefetch: Introduce PrePrefetchContainer [chromium/src : main]
Taiyo Mizuhashi (Gerrit)
Taiyo Mizuhashi added 2 comments![]( "Open in Gerrit")
Early return. I think this will be reviewable assuming "PrefetchContainer is created on a specific sequence", once test flakiness is resolved
resource_request_ = CreateResourceRequestForNavigation(Taiyo MizuhashiAdd a comment to `CreateResourceRequestForNavigation()` that `CreateResourceRequestForNavigation()` can be called from non-UI thread for preprefetch.
Done
Related details
- Hiroshige Hayashizaki
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Taiyo Mizuhashi (Gerrit)
Taiyo Mizuhashi added 1 comment![]( "Open in Gerrit")
Although crrev.com/c/7544249 is not fully ready (sorry), I would like to start this review because I think is reviewable assuming "PrefetchContainer is created on a specific sequence (which will be ensured by `PrePrefetchService`, and necessary object (PrefetchRequest, URLLoaderFactory, etc is appropriately provided by them",
PTAL, thanks!
Related details
- Hiroki Nakagawa
- Ken Okada
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ken Okada (Gerrit)
Ken Okada added 7 comments![]( "Open in Gerrit")
This CL introduces a class `PrePrefetchContainer` that represents aan
// - This is always created from static function `CreateAndStart()`, whichvia?
return container;`return std::move(container)` (Or, it is using NVRO?)
DCHECK(!BrowserThread::CurrentlyOn(content::BrowserThread::UI));Why `DCEHCK`? I believe that it should be `CHECK`.
net::IsolationInfo::RequestType::kMainFrame, origin, origin,Could you add comment `/*arg=*/`?
/*devtools_observer=*/mojo::NullRemote(), net::RequestPriority::HIGHEST,Could you add a comment why we use HIGHEST here, while ordinal prefetch uses `PrefetchPriority`. (Or, we'll loose the context in the futurue.)
Related details
- Hiroki Nakagawa
- Hiroshige Hayashizaki
- Taiyo Mizuhashi
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. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 3 comments![]( "Open in Gerrit")
class CONTENT_EXPORT PrePrefetchContainer {nit: final
// dedicated `SequencedTaskRunner.nit:
```
`SequencedTaskRunner`
```
(missing closing quote).
// ResourceRequest copy is needed here. Please see crbug.com/444482515 andProbably we don't need copy here, especially because preprefetch container doesn't handle redirects for now.
Related details
- Hiroki Nakagawa
- Taiyo Mizuhashi
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 2 comments![]( "Open in Gerrit")
// combines ctor and constructor and Start(). This is always called fromctor and constructor
This repeats the same thing.
return container;`return std::move(container)` (Or, it is using NVRO?)
Yes, this is valid and more preferable than explicit `std::move`.
Related details
- Taiyo Mizuhashi
Taiyo Mizuhashi (Gerrit)
Taiyo Mizuhashi voted and added 10 comments![]( "Open in Gerrit")
Votes added by Taiyo Mizuhashi
| Commit-Queue | +1 |
10 comments
class CONTENT_EXPORT PrePrefetchContainer {Taiyo Mizuhashinit: final
Done
// combines ctor and constructor and Start(). This is always called fromctor and constructor
This repeats the same thing.
Done
// - This is always created from static function `CreateAndStart()`, whichTaiyo Mizuhashivia?
Done
nit:
```
`SequencedTaskRunner`
```
(missing closing quote).
Done
return container;Hiroki Nakagawa`return std::move(container)` (Or, it is using NVRO?)
Yes, this is valid and more preferable than explicit `std::move`.
yes, thanks!
DCHECK(!BrowserThread::CurrentlyOn(content::BrowserThread::UI));Why `DCEHCK`? I believe that it should be `CHECK`.
Actually I thought a bit. maybe as a common practice, DCHECK is heavily used for thread checks e.g. the call site number of `CHECK_CURRENTLY_ON` vs `DCHECK_CURRENTLY_ON` is "68 vs 32441, which I just followed this first.
anyway CHECK is always preferred (as we chatted before IIUC) so change this to CHECK here. lmk if you have thoughts.
One thing in my mind is performance concerns so maybe I can replace it all at once and do a local test after all CLs are settled.
net::IsolationInfo::RequestType::kMainFrame, origin, origin,Could you add comment `/*arg=*/`?
Done
/*devtools_observer=*/mojo::NullRemote(), net::RequestPriority::HIGHEST,Could you add a comment why we use HIGHEST here, while ordinal prefetch uses `PrefetchPriority`. (Or, we'll loose the context in the futurue.)
I believe this is covered by the above comment stating " Construct a proper PrePrefetch, using the prefetch common utility." After we can use the common `ResourceRequest` utility, it should be calculated from `PrefetchPriority` given by `PrefetchRequest`.
// ResourceRequest copy is needed here. Please see crbug.com/444482515 andProbably we don't need copy here, especially because preprefetch container doesn't handle redirects for now.
- Hiroki Nakagawa
- Hiroshige Hayashizaki
- Ken Okada
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. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 5 comments![]( "Open in Gerrit")
// shouldn't be acceeed except for 1) dtor of `PrePrefetchHandle` or 2)nit: typo (accessed)
// `PrePrefetchServiceImpl`, which is always on its sequence.I think it's better to explicitly name the sequence, e.g.
the `PrePrefetchServiceCore` sequence or
the `PrePrefetchServiceImpl` sequence.
ditto elsewhere in this CL (and other CLs).
Threading is very complicated in general, and thus let's count, name and mention the threads/sequences explicitly and concisely. IIUC there are three things (am I understanding correctly?):
- The UI thread,
- The `PrePrefetchServiceCore` sequence, and
- Any thread/sequence that calls `StartPrePrefetch`.
How about organizing the cc file based on threading? e.g.
```
// ----------------------------------------------------------------
// Methods to be called from the `PrePrefetchServiceCore` sequence.
(all methods except for dtor)
// ----------------------------------------------------------------
// Methods that can be called from any thread.
(dtor)
```
mojo::PendingRemote<network::mojom::URLLoaderFactory> url_loader_factory) {optional: perhaps it's better to explicitly CHECK the sequence affinity, i.e.
passing `const PrePrefetchServiceImpl&` here and all other methods and introduce/call `PrePrefetchServiceImpl::CheckCalledOnValidSequence()` or so.
(but for now it's also OK to land this and iterate refining later)
Related details
- Hiroki Nakagawa
- Ken Okada
- Taiyo Mizuhashi
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 voted and added 9 comments![]( "Open in Gerrit")
Votes added by Hiroki Nakagawa
| Code-Review | +1 |
9 comments
LGTM, thanks!
combines ctor and constructor and Start(). This is always called fromctor and constructor (repeating)
- After being created, this should currently be owned by `PrePrefetchHandle`, and shouldn't be accessed except for 1) dtor of `PrePrefetchHandle` or 2) PrePrefetch consumption happening on the UI thread, which both terminates the lifetime of `this`.These may be too detailed for the CL description. Instead, we could simply say:
Regarding the thread/lifetime model of the PrePrefetchContainer, see the class comment.
Bonus: This saves us from having to keep the CL description in sync with the class comment after typo fixes
- We should construct a proper `ResourceRequest` for `PrePrefetch`, using the prefetch common utility (This will be done at least after crbug.com/483079815 is resolved). In addition to that, we also take care of some modification done on proxied `URLLoaderFactory` [2]. To do these, we should take care of those properties that are UI-thread dependent / that are tied to the embedder (See the comment of `PrePrefetchServiceImpl::ctor`).Can you run the "format"?
// from non-UI thread when preprefetching. (Please seepre-prefetching
auto receiver = url_loader_.InitWithNewPipeAndPassReceiver();
auto client_remote =
url_loader_client_receiver_.InitWithNewPipeAndPassRemote();Can we inline these two lines into the `CreateLoaderAndStart` call?
TEST_F(PrePrefetchContainerTest, StartPrePrefetch) {Can you briefly explain what the purpose of this test? IIUC,
// This test confirm that `PrePrefetchContainer` starts and sends a network request when `PrePrefetchContainer::CreateAndStart(ForTesting)` is called.
// Wait the request is received and the container is created.Wait until
CHECK(container);`ASSERT` (`CHECK` is not recommended in tests)
Related details
- Ken Okada
- Taiyo Mizuhashi
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. |
Ken Okada (Gerrit)
Ken Okada voted and added 2 comments![]( "Open in Gerrit")
Votes added by Ken Okada
| Code-Review | +1 |
2 comments
LGTM % `s/a/an/`
/*devtools_observer=*/mojo::NullRemote(), net::RequestPriority::HIGHEST,Taiyo MizuhashiCould you add a comment why we use HIGHEST here, while ordinal prefetch uses `PrefetchPriority`. (Or, we'll loose the context in the futurue.)
I believe this is covered by the above comment stating " Construct a proper PrePrefetch, using the prefetch common utility." After we can use the common `ResourceRequest` utility, it should be calculated from `PrefetchPriority` given by `PrefetchRequest`.
- Taiyo Mizuhashi