Add network-efficiency-guardrails asset size checks [chromium/src : main]
Luis Flores (Gerrit)
Luis Flores added 1 comment![]( "Open in Gerrit")
Adding basic checks for resource sizes, would appreciate your feedback!
Related details
- Liang Zhao
- Viktoriya Bryhider
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao added 2 comments![]( "Open in Gerrit")
1024 * 1024) {Can we define these limit numbers in a more visible place? We might actually need it for tests.
Might want to also include this size limit in CL description.
virtual void CheckGuardrailsPolicyForLargeAsset() {}Please add comments to describe what the function does. The name of the function doesn't really tell me what it does. Is there a better name?
Related details
- Luis Flores
- Viktoriya Bryhider
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. |
Viktoriya Bryhider (Gerrit)
Viktoriya Bryhider added 1 comment![]( "Open in Gerrit")
mojom::blink::DocumentPolicyFeature::kNetworkEfficiencyGuardrails,CheckGuardrailsPolicyForRequest is called before check for response.WasFetchedViaServiceWorker() - do we still want/need to report policy violation if it was SW/cache(?) or some other way to get image without going to the network?
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 3 comments![]( "Open in Gerrit")
1024 * 1024) {Can we define these limit numbers in a more visible place? We might actually need it for tests.
Might want to also include this size limit in CL description.
Moved them to frame_fetch_context.h
mojom::blink::DocumentPolicyFeature::kNetworkEfficiencyGuardrails,CheckGuardrailsPolicyForRequest is called before check for response.WasFetchedViaServiceWorker() - do we still want/need to report policy violation if it was SW/cache(?) or some other way to get image without going to the network?
This isn't in the doc currently, but given the purpose of the policy (to flag perf costs), I think it should only be reported when we actually hit the network. I've looked at ResourceLoader and my understanding is we'll currently intercept all responses so I'll add checks for network cases only.
virtual void CheckGuardrailsPolicyForLargeAsset() {}Please add comments to describe what the function does. The name of the function doesn't really tell me what it does. Is there a better name?
Renamed to CheckGuardrailsPolicyForAssetSize and added a comment in fetch_context.h
Related details
- Liang Zhao
- Viktoriya Bryhider
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. |
Viktoriya Bryhider (Gerrit)
Viktoriya Bryhider added 3 comments![]( "Open in Gerrit")
static constexpr size_t kGuardrailsLargeDataThreshold = 100 * 1024; // 100kBnit(?) I have seen Units added in the name of the const(kSlowDeserializationSizeThresholdBytes for example) - may consider adding it here, like kGuardrailsLargeDataThresholdBytes.
!response.NetworkAccessed()) {May be add comment, why `!response.NetworkAccessed()` check is not enough. Can it be that NetworkAccessed and WasCached will be both true?
bool expect_violation;add default value here
Related details
- Liang Zhao
- 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")
Addressing feedback and adding last impl pieces.
static constexpr size_t kGuardrailsLargeDataThreshold = 100 * 1024; // 100kBnit(?) I have seen Units added in the name of the const(kSlowDeserializationSizeThresholdBytes for example) - may consider adding it here, like kGuardrailsLargeDataThresholdBytes.
Updated.
!response.NetworkAccessed()) {May be add comment, why `!response.NetworkAccessed()` check is not enough. Can it be that NetworkAccessed and WasCached will be both true?
Updated.
bool expect_violation;add default value here
- Liang Zhao
- Viktoriya Bryhider
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. |
Liang Zhao (Gerrit)
Liang Zhao added 1 comment![]( "Open in Gerrit")
bool is_enforced_policy =would has_enforced_policy a better name?
Related details
- Luis Flores
- Viktoriya Bryhider
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 8 comments![]( "Open in Gerrit")
Adding resource size check to network-efficiency-guardrails Document Policy, per https://docs.google.com/document/d/1gS1p2poGx_zvUKeYg-py_fHGU-GVHmm75cQSZNGglig
masonf@ for execution_context/local_dom_window
toyoshim@ for loader
would has_enforced_policy a better name?
Renamed.
static constexpr size_t kGuardrailsLargeDataThreshold = 100 * 1024; // 100kBLuis Floresnit(?) I have seen Units added in the name of the const(kSlowDeserializationSizeThresholdBytes for example) - may consider adding it here, like kGuardrailsLargeDataThresholdBytes.
Updated.
Marked as resolved.
!response.NetworkAccessed()) {Luis FloresMay be add comment, why `!response.NetworkAccessed()` check is not enough. Can it be that NetworkAccessed and WasCached will be both true?
Updated.
Marked as resolved.
1024 * 1024) {Luis FloresCan we define these limit numbers in a more visible place? We might actually need it for tests.
Might want to also include this size limit in CL description.
Moved them to frame_fetch_context.h
Moved to local_dom_window.h, where they're used.
mojom::blink::DocumentPolicyFeature::kNetworkEfficiencyGuardrails,Luis FloresCheckGuardrailsPolicyForRequest is called before check for response.WasFetchedViaServiceWorker() - do we still want/need to report policy violation if it was SW/cache(?) or some other way to get image without going to the network?
This isn't in the doc currently, but given the purpose of the policy (to flag perf costs), I think it should only be reported when we actually hit the network. I've looked at ResourceLoader and my understanding is we'll currently intercept all responses so I'll add checks for network cases only.
Marked as resolved.
bool expect_violation;Luis Floresadd default value here
Added.
Marked as resolved.
virtual void CheckGuardrailsPolicyForLargeAsset() {}Luis FloresPlease add comments to describe what the function does. The name of the function doesn't really tell me what it does. Is there a better name?
Renamed to CheckGuardrailsPolicyForAssetSize and added a comment in fetch_context.h
- Liang Zhao
- Mason Freed
- Takashi Toyoshima
- Viktoriya Bryhider
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
3 comments
execution_context/local_dom_window LGTM
// Make those const public for testing purpose.nit: `These are` rather than `Make those`
if (bytes < kGuardrailsLargeDataThresholdBytes) {`<=` ?
Related details
- Liang Zhao
- Luis Flores
- Takashi Toyoshima
- Viktoriya Bryhider
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 3 comments![]( "Open in Gerrit")
// Make those const public for testing purpose.nit: `These are` rather than `Make those`
Updated.
if (bytes < kGuardrailsLargeDataThresholdBytes) {`<=` ?
- Liang Zhao
- Mason Freed
- Takashi Toyoshima
- Viktoriya Bryhider
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")
Updated image threshold per discussion in https://docs.google.com/document/d/1RGxvtkoQbvApLVdipiASoUQjC0Jf-IKD9qV1EfUJHAQ/edit?disco=AAABxj9MH24
Related details
- Liang Zhao
- Mason Freed
- Takashi Toyoshima
- Viktoriya Bryhider
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. |
Mason Freed (Gerrit)
Mason Freed voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
3 comments
execution_context/local_dom_window still LGTM
// Make those const public for testing purpose.Luis Floresnit: `These are` rather than `Make those`
Updated.
Done
if (bytes < kGuardrailsLargeDataThresholdBytes) {Luis Flores`<=` ?
Updated and added tests at the boundary. Thanks!
- Liang Zhao
- Luis Flores
- Takashi Toyoshima
- Viktoriya Bryhider
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Liang Zhao (Gerrit)
Liang Zhao voted Code-Review+1![]( "Open in Gerrit")
- Luis Flores
- Takashi Toyoshima
- Viktoriya Bryhider
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |