[dns] Expose resolution details for successful requests [chromium/src : main]
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Commit-Queue | +1 |
1 comment
ricea@: PTAL, I want to submit this and (maybe two) subsequent CLs before branch cut (Monday). Thanks!
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. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 1 comment![]( "Open in Gerrit")
awillia@: Would you be able to take a look, if ricea@ couldn't take a look? I'd like to submit this and following CLs by the next branch cut (Monday).
Related details
- Adam Rice
- Andrew Williams
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 7 comments![]( "Open in Gerrit")
LGTM overall but leaving some questions and optional comments
NOTREACHED();optional: maybe stream `type` to the `NOTREACHED()` so if it ever does get reached we'll know which case it is
if (stale_info.has_value()) {Would it be better to have this logic in ResolveLocally? I'm wondering whether it will always be the case that stale_info.has_value means the source was kCache vs. kLocal, of if a change in ResolveLocally to the semantics could break this logic.
One benefit of moving it into ResolveLocally would be we wouldn't need this if statement in both RequestImpl::DoResolveLocally and ServiceEndpointRequestImpl::DoResolveLocally
If we went with this change, adding another out parameter to ResolveLocally seems not ideal. Maybe a follow-on refactor could make all of the passed back state be combined into a new struct that ResolveLocally returns (combining the results, tasks, and stale_info)? I also noticed HostCache::Entry already has "Source" that seems similar to ResolutionDetails. Should ResolutionDetails be added to HostCache::Entry? Could it replace Source?
Just thinking out loud, feel free to ignore
resolution_details_ = details;Just to check, should we set resolution_details_ to something in the `is_stale && stale_allowed_while_refreshing` case as well?
ResolutionSource::kCache);Just to check, it's OK that results from the secure cache and from the insecure cache share a ResolutionSource?
kNat64 = 8,I didn't see a test cases demonstrating kLocal or kNat64 as the ResolutionSource. Is there an existing test we can add those to? Same with kPlatform, but I'm assuming we haven't implemented those types of host resolutions yet.
struct NET_EXPORT ResolutionDetails {Is this in a struct because we plan to add stuff to it? If so and it will eventually have more than two primitive types in it, we may want to proactively std::move it when we pass it around since we may not think to add std::move to uses after adding members to this.
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. |