[net] Populate DNS resolution details to internal load info [chromium/src : main]
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 1 comment![]( "Open in Gerrit")
ricea@: PTAL, 2nd CL to populate DNS resolution details, which I want to submit before branch cut.
Related details
- Adam Rice
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Williams (Gerrit)
Andrew Williams added 16 comments![]( "Open in Gerrit")
Leaving some feedback and questions
// this request.nit: mention when this can be std::nullopt
// this session.nit: mention when this can be std::nullopt here as well
resolution_details_ = resolution_details;nit: should we just initialize this in the initializer list? Also, we can std::move the std::optional
std::optional<ResolutionDetails> resolution_details = std::nullopt) {optional: it seems like the more idiomatic way to set resolution_detail for the tests in this file is to have a member variable that either gets assigned to before the call to Initialize or gets set via a setter like SetRequest / SetResponse
/*resolution_details=*/resolution_details,nit: no need for the comment here since it's not a literal value
EXPECT_EQ(OK, stream_->InitializeStream(true, DEFAULT_PRIORITY,nit: looks like it's not done elsewhere but for these new tests it'd be helpful to add `/*can_send_early=*/` for the first parameter
TEST_P(QuicHttpStreamTest, PopulateLoadTimingInternalInfo_ReusedStream) {Instead of ReusedStream should we say ReusedConnection or ReusedSession or something?
{} /* dns_aliases */);nit: `/*dns_aliases=*/`
callback2.callback()));optional: It seems a lot of existing tests also use the pattern of passing a TestCompletionCallback::callback() and not relying on it being called, but it seems like we could more simply use `base::NullCallback()`
EXPECT_EQ(ResolutionSource::kSecure,Assuming a second stream corresponds to a second request, if that request is made after the DNS resolution for the first request finishes (but before the DNS record expires), wouldn't we expect the ResolutionSource for the second stream to point to the DNS cache?
resolution_details_(resolution_details),nit: can std::move here
std::move(proxy_stream_), std::move(user_agent), net_log(), network_);just to double-check, we don't want to pass resolution_details_ for proxied sessions?
dns_resolution_details, use_dns_aliases, std::move(dns_aliases),nit: can std::move here and below
dns_resolution_end_time, dns_resolution_details, use_dns_aliases,nit: can std::move here as well
dns_resolution_end_time, dns_resolution_details,nit: can std::move here and several places below as well
Related details
- Adam Rice
- Kenichi Ishibashi
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. |