[LNA] Remove URLRequest.target_ip_address_space [chromium/src : main]
Hubert Chao (Gerrit)
Hubert Chao voted and added 1 comment![]( "Open in Gerrit")
Votes added by Hubert Chao
| Commit-Queue | +1 |
1 comment
cthomp@ for initial review, mmenke@ for services/network/OWNERS
Related details
- Chris Thompson
- mmenke
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
mmenke (Gerrit)
mmenke added 5 comments![]( "Open in Gerrit")
// This is set if:"This is set if" implies this is an optional, but it is not. Should it be an optional?
// * The request had a required_address_space set but thenit: Here, and on the next two lines, use backticks around field names (`)
// resource_address_space did not match. In this case, this will be set otto
Related details
- Chris Thompson
- Hubert Chao
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. |
Chris Thompson (Gerrit)
Chris Thompson added 3 comments![]( "Open in Gerrit")
Overall LGTM, but let's consider the "default to kUnknown" vs. optional question below.
// Error here is that we initially saw the email in the```suggestion
// Error here is that we initially saw the response in the
```
// This is set if:"This is set if" implies this is an optional, but it is not. Should it be an optional?
Having this default to `unknown` is consistent with the other PNA/LNA address space fields. I don't have a strong preference on having this be an optional vs. adjusting the comments to be "defaults to kUnknown unless XYZ", although optional is maybe a bit more explicit
Related details
- Hubert Chao
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. |
mmenke (Gerrit)
mmenke added 1 comment![]( "Open in Gerrit")
// This is set if:Chris Thompson"This is set if" implies this is an optional, but it is not. Should it be an optional?
Having this default to `unknown` is consistent with the other PNA/LNA address space fields. I don't have a strong preference on having this be an optional vs. adjusting the comments to be "defaults to kUnknown unless XYZ", although optional is maybe a bit more explicit
I'm happy to defer to you two here, the name and value really confused me, at least, until I realized it was to have additional metadata in the case of specific errors (Admittedly, that may partly be because my context was full of the input param target_address_space, and I was trying to model `inconsistent_address_space` as an input param)