[LNA] remove PNA preflight support [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@: general review
mmenke@: mainly services/network/*, rest is mostly boilerplate removal but still might be worth looking at since it derives from the services/network changes
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 6 comments![]( "Open in Gerrit")
Is there any way we could split this into multiple CLs? It's very easy to miss stuff in giant CLs, even removal CLs.
if (net_error == net::OK) {Seems like we should get rid of this block entirely, remove the status check on line 985, and copy the if(status) DCHECK on line 995, then remove the if on line 989. The real issue I want to address here is splitting the no-net-error return path into two different results.
status->target_address_space = request_.target_ip_address_space;Is this field still needed? It's not removed in this CL, but I think all lines that set it are removed in this CL (other than the one in the mojom traits file)? That also implies it should be removed from the CorsErrorStatus mojom type.
#include "base/component_export.h"
#include "base/containers/enum_set.h"No longer needed?
#include "base/containers/enum_set.h"No longer needed?
head.headers, header_names::kAccessControlAllowPrivateNetwork);This constant no longer needed?
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. |
mmenke (Gerrit)
mmenke added 1 comment![]( "Open in Gerrit")
if (net_error == net::OK) {Seems like we should get rid of this block entirely, remove the status check on line 985, and copy the if(status) DCHECK on line 995, then remove the if on line 989. The real issue I want to address here is splitting the no-net-error return path into two different results.
*two different blocks, rather.
Hubert Chao (Gerrit)
Hubert Chao added 6 comments![]( "Open in Gerrit")
Is there any way we could split this into multiple CLs? It's very easy to miss stuff in giant CLs, even removal CLs.
I couldn't spot an easy way to do this, no. This CL started with me removing the two preflight enum values from
services/network/public/mojom/client_security_state.mojom
and then cascaded from there. I could just delete the enums and then the tests in services/network/ccrs/* that depend on the enum values without deleting the code that the tests are exercising, but that didn't feel like a good option to me to delete the tests without deleting the code they tested.
I did stop and not remove the target_ip_address_space fields (which was actually the goal that started this series of CLs), as that was going make things a lot bigger.
happy to try something if you've got a suggestion on how to break this up.
mmenkeSeems like we should get rid of this block entirely, remove the status check on line 985, and copy the if(status) DCHECK on line 995, then remove the if on line 989. The real issue I want to address here is splitting the no-net-error return path into two different results.
*two different blocks, rather.
Done I think?
status->target_address_space = request_.target_ip_address_space;Is this field still needed? It's not removed in this CL, but I think all lines that set it are removed in this CL (other than the one in the mojom traits file)? That also implies it should be removed from the CorsErrorStatus mojom type.
that field is probably not needed anymore. But I didn't start that removal because that would've made the CL bigger.
See the note in my commit message about `target_ip_address_space`
#include "base/containers/enum_set.h"Hubert ChaoNo longer needed?
Done
#include "base/component_export.h"
#include "base/containers/enum_set.h"Hubert ChaoNo longer needed?
Done
head.headers, header_names::kAccessControlAllowPrivateNetwork);This constant no longer needed?
- 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 2 comments![]( "Open in Gerrit")
Hubert ChaoIs there any way we could split this into multiple CLs? It's very easy to miss stuff in giant CLs, even removal CLs.
I couldn't spot an easy way to do this, no. This CL started with me removing the two preflight enum values from
services/network/public/mojom/client_security_state.mojom
and then cascaded from there. I could just delete the enums and then the tests in services/network/ccrs/* that depend on the enum values without deleting the code that the tests are exercising, but that didn't feel like a good option to me to delete the tests without deleting the code they tested.
I did stop and not remove the target_ip_address_space fields (which was actually the goal that started this series of CLs), as that was going make things a lot bigger.
happy to try something if you've got a suggestion on how to break this up.
Hrm...I don't suppose we could remove the fields from URLResponseHead and PrivateNetworkAccessPreflightResult, removing the blink code that reads them, and the code that sets them (replacing it temporarily with CHECKs that we wouldn't set them to anything useful - branch is in ~1 hour, so that code will not make it to stable), tearing down tests in the process, and then tear down the rest of the stuff this removes in a followup? That's not exactly a lovely split, but with the CHECKs, I think it's acceptable. I think you wouldn't even necessarily need to land the CLs separately, could merge them and land together once reviewed. It's just very hard to catch stuff in this massive CL.
I agree this code is very heavily integrated, and it's difficult to figure out if things can be broken up.
status->target_address_space = request_.target_ip_address_space;Hubert ChaoIs this field still needed? It's not removed in this CL, but I think all lines that set it are removed in this CL (other than the one in the mojom traits file)? That also implies it should be removed from the CorsErrorStatus mojom type.
that field is probably not needed anymore. But I didn't start that removal because that would've made the CL bigger.
See the note in my commit message about `target_ip_address_space`
SGTM. As long as stuff is on your radar, I'm fine with (and actively support) multiple CLs. Sorry for missing the description - I need to learn to read those carefully before reviewing.
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. |
mmenke (Gerrit)
mmenke added 1 comment![]( "Open in Gerrit")
Hubert ChaoIs there any way we could split this into multiple CLs? It's very easy to miss stuff in giant CLs, even removal CLs.
mmenkeI couldn't spot an easy way to do this, no. This CL started with me removing the two preflight enum values from
services/network/public/mojom/client_security_state.mojom
and then cascaded from there. I could just delete the enums and then the tests in services/network/ccrs/* that depend on the enum values without deleting the code that the tests are exercising, but that didn't feel like a good option to me to delete the tests without deleting the code they tested.
I did stop and not remove the target_ip_address_space fields (which was actually the goal that started this series of CLs), as that was going make things a lot bigger.
happy to try something if you've got a suggestion on how to break this up.
Hrm...I don't suppose we could remove the fields from URLResponseHead and PrivateNetworkAccessPreflightResult, removing the blink code that reads them, and the code that sets them (replacing it temporarily with CHECKs that we wouldn't set them to anything useful - branch is in ~1 hour, so that code will not make it to stable), tearing down tests in the process, and then tear down the rest of the stuff this removes in a followup? That's not exactly a lovely split, but with the CHECKs, I think it's acceptable. I think you wouldn't even necessarily need to land the CLs separately, could merge them and land together once reviewed. It's just very hard to catch stuff in this massive CL.
I agree this code is very heavily integrated, and it's difficult to figure out if things can be broken up.
If this turns out not to be possible, or to require too much work, please don't hesitate to say so.
Hubert Chao (Gerrit)
Hubert Chao added 1 comment![]( "Open in Gerrit")
Hubert ChaoIs there any way we could split this into multiple CLs? It's very easy to miss stuff in giant CLs, even removal CLs.
mmenkeI couldn't spot an easy way to do this, no. This CL started with me removing the two preflight enum values from
services/network/public/mojom/client_security_state.mojom
and then cascaded from there. I could just delete the enums and then the tests in services/network/ccrs/* that depend on the enum values without deleting the code that the tests are exercising, but that didn't feel like a good option to me to delete the tests without deleting the code they tested.
I did stop and not remove the target_ip_address_space fields (which was actually the goal that started this series of CLs), as that was going make things a lot bigger.
happy to try something if you've got a suggestion on how to break this up.
mmenkeHrm...I don't suppose we could remove the fields from URLResponseHead and PrivateNetworkAccessPreflightResult, removing the blink code that reads them, and the code that sets them (replacing it temporarily with CHECKs that we wouldn't set them to anything useful - branch is in ~1 hour, so that code will not make it to stable), tearing down tests in the process, and then tear down the rest of the stuff this removes in a followup? That's not exactly a lovely split, but with the CHECKs, I think it's acceptable. I think you wouldn't even necessarily need to land the CLs separately, could merge them and land together once reviewed. It's just very hard to catch stuff in this massive CL.
I agree this code is very heavily integrated, and it's difficult to figure out if things can be broken up.
If this turns out not to be possible, or to require too much work, please don't hesitate to say so.
- Chris Thompson
- mmenke
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. |
Hubert Chao (Gerrit)
Hubert Chao abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |