Add test for Network Efficiency Guardrails chunked responses [chromium/src : main]
Luis Flores (Gerrit)
Luis Flores added 1 comment![]( "Open in Gerrit")
Adding test for chunked responses as discussed in previous CL 7173352. PTAL
Related details
- Daniel Cheng
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 3 comments![]( "Open in Gerrit")
Do we have a way to verify that the resulting response is actually chunked, i.e. we don't accidentally flatten the shared buffer somewhere?
CheckImageLoadViolation(1024 * 1024, true, /*enable_policy=*/true,It would be helpful to have some explanatory comments here. I suppose it's self-documenting to people who are more familiar, but it's not self-documenting to me.
e.g.
// expected violation because ...
// Registers with a custom response.Document the new parameter. What happens if chunk size is 0, for instance? Does that mean unchunked, i.e. the same as nullopt? Or is that not allowed?
Related details
- Luis Flores
- Takashi Toyoshima
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. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 2 comments![]( "Open in Gerrit")
Thank you for adding this.
Overall, lgtm, but we may have some test variants (with comments)
CheckImageLoadViolation(1024 * 1024, true, /*enable_policy=*/true,It would be helpful to have some explanatory comments here. I suppose it's self-documenting to people who are more familiar, but it's not self-documenting to me.
e.g.
// expected violation because ...
+1.
Also good to keep 2 types of tests, one runs with data within a single chunk, and the other runs with multiple chunk data.
Related details
- Luis Flores
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. |
Luis Flores (Gerrit)
Luis Flores added 4 comments![]( "Open in Gerrit")
Do we have a way to verify that the resulting response is actually chunked, i.e. we don't accidentally flatten the shared buffer somewhere?
I have added a test checking actual call count to `DidReceiveData`.
CheckImageLoadViolation(1024 * 1024, true, /*enable_policy=*/true,Takashi ToyoshimaIt would be helpful to have some explanatory comments here. I suppose it's self-documenting to people who are more familiar, but it's not self-documenting to me.
e.g.
// expected violation because ...
+1.
Also good to keep 2 types of tests, one runs with data within a single chunk, and the other runs with multiple chunk data.
Thanks, I've added comments for these chunking cases.
toyoshim@ for data in a single chunk, existing cases in `FrameFetchContextNetworkGuardrailsTest.LargeImageResourceSizeLimit` already use a single chunk as default. Is this what you are referring to? or are you suggesting to have additional tests beyond those?
// Registers with a custom response.Document the new parameter. What happens if chunk size is 0, for instance? Does that mean unchunked, i.e. the same as nullopt? Or is that not allowed?
Thanks, I've added comments throughout the functions that add the parameter.
Related details
- Daniel Cheng
- Takashi Toyoshima
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. |
Luis Flores (Gerrit)
Luis Flores added 1 comment![]( "Open in Gerrit")
// Registers with a custom response.Luis FloresDocument the new parameter. What happens if chunk size is 0, for instance? Does that mean unchunked, i.e. the same as nullopt? Or is that not allowed?
Thanks, I've added comments throughout the functions that add the parameter.
Also updated the parameter to simply take `size_t` in and use `0` as no chunking. Comments reflect the updated behavior.