Initial implementation for network-efficiency-guardrails [chromium/src : main]
Luis Flores (Gerrit)
Message from Luis Flores![]( "Open in Gerrit")
Set Ready For Review
Related details
- Luis Flores
Code-Coverage
Code-Owners
Code-Review
| 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")
Starting prototype implementation for network-efficiency-guardrails Document Policy. This CL implements compression checks for the "basic" category in linked explainer: aka.ms/webembeddedperf and https://docs.google.com/document/d/1RGxvtkoQbvApLVdipiASoUQjC0Jf-IKD9qV1EfUJHAQ. Would appreciate your feedback on the design. Thanks!
network-efficiency-guardrails
* http compression (js, css, json): this CL
* http compression (html): upcoming CL
* resource size: upcoming CL
* compressed formats: upcoming CL
Related details
- Ari Chivukula
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula added 4 comments![]( "Open in Gerrit")
ResourceType resource_type,maybe it can be a TODO for now, but there probably needs to be a browsertest for this and not just a wpt
const DocumentPolicy* enforce_policy =the check above on 527 could be a report-only policy or the enforced policy? Might be worth leaving a comment
// todo: turn into enum for versioningwhy leave as TODO here?
Document-Policy: network-efficiency-guardrailsshould Document-Policy-Report-Only be tested too?
Related details
- Luis Flores
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 3 comments![]( "Open in Gerrit")
Can you add a comment that includes a link to a spec or an explainer that defines the implemented behavior below?
default:
return;Can you avoid using `default` but list up all ResourceTypes instead, so that we can find this by a compile error when people add a new ResourceType?
Can you have a method comment?
A link for a spec (or an equivalent) that I asked in another comment, might be placed here.
Related details
- Luis Flores
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
Thanks for the feedback. I have addressed actionable comments. ari...@chromium.org Would appreciate thoughts on enum and naming comments.
ResourceType resource_type,maybe it can be a TODO for now, but there probably needs to be a browsertest for this and not just a wpt
Thanks. I can add in a follow up patchset/CL, I'd like to nail down the overall design first.
Can you add a comment that includes a link to a spec or an explainer that defines the implemented behavior below?
Done
default:
return;Can you avoid using `default` but list up all ResourceTypes instead, so that we can find this by a compile error when people add a new ResourceType?
Done
const DocumentPolicy* enforce_policy =the check above on 527 could be a report-only policy or the enforced policy? Might be worth leaving a comment
I have added a comment.
// todo: turn into enum for versioningwhy leave as TODO here?
Haven't decided on the final shape for versioning yet. I'm trying to start with something simple and have apps try it first. There's no strictness comparison for enum either so unsure yet whether that's the right solution. I can remove the comment if that's better? Or would it be better to be decided now?
Can you have a method comment?
A link for a spec (or an equivalent) that I asked in another comment, might be placed here.
Done
Document-Policy: network-efficiency-guardrailsshould Document-Policy-Report-Only be tested too?
Thanks, this was actually great calling out as there was in fact a bug for reporting only. I had been struggling with IsFeatureEnabled but now reading through Document Policy implementation and spec draft again, I think the problem is that both spec and impl assume |false| to be a stricter value than |true| which doesn't hold for the way this feature is currently done.
I have changed the check in this CL as a middle step, but I'm considering to switch the configuration point from "network-efficiency-guardrails" to "no-network-efficiency-guardrails", as it would fit better with existing Document Policy infrastructure.
Thoughts?
Related details
- Ari Chivukula
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 3 comments![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +1 |
3 comments
Luis FloresCan you add a comment that includes a link to a spec or an explainer that defines the implemented behavior below?
Done
Acknowledged
default:
return;Luis FloresCan you avoid using `default` but list up all ResourceTypes instead, so that we can find this by a compile error when people add a new ResourceType?
Done
Acknowledged
Luis FloresCan you have a method comment?
A link for a spec (or an equivalent) that I asked in another comment, might be placed here.
Done
- Ari Chivukula
- Luis Flores
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula added 6 comments![]( "Open in Gerrit")
ResourceType resource_type,Luis Floresmaybe it can be a TODO for now, but there probably needs to be a browsertest for this and not just a wpt
Thanks. I can add in a follow up patchset/CL, I'd like to nail down the overall design first.
please add a TODO for that then
if (!security_context.GetDocumentPolicy()this is kinda awkward, maybe:
```
bool is_report_only_policy = ...
bool is_enforced_policy = ...
if (!is_report_only_policy && !is_enforced_policy) {
return;
}
mojom::blink::PolicyDisposition disposition = is_enforced_policy ? kEnforce : kReport;
```
const DocumentPolicy* enforce_policy =Luis Floresthe check above on 527 could be a report-only policy or the enforced policy? Might be worth leaving a comment
I have added a comment.
Done
response.HttpHeaderField(http_names::kContentEncoding).empty()) {check this early for an exit?
// todo: turn into enum for versioningLuis Floreswhy leave as TODO here?
Haven't decided on the final shape for versioning yet. I'm trying to start with something simple and have apps try it first. There's no strictness comparison for enum either so unsure yet whether that's the right solution. I can remove the comment if that's better? Or would it be better to be decided now?
I'm confused, is this being rolled out now or just in early prototyping? If it's the latter then it's okay to be unsure for now but you want a TODO like:
`// TODO(crbug.com/XXX): ...`
Document-Policy: network-efficiency-guardrailsLuis Floresshould Document-Policy-Report-Only be tested too?
Thanks, this was actually great calling out as there was in fact a bug for reporting only. I had been struggling with IsFeatureEnabled but now reading through Document Policy implementation and spec draft again, I think the problem is that both spec and impl assume |false| to be a stricter value than |true| which doesn't hold for the way this feature is currently done.
I have changed the check in this CL as a middle step, but I'm considering to switch the configuration point from "network-efficiency-guardrails" to "no-network-efficiency-guardrails", as it would fit better with existing Document Policy infrastructure.
Thoughts?
If you were using an enum instead of a bool would it be easier? The implementation here differs enough from the explainer I'm not quite sure how it's intended to evolve
Related details
- Luis Flores
Luis Flores (Gerrit)
Luis Flores added 6 comments![]( "Open in Gerrit")
Addressing more comments, sharing additional details on others.
ResourceType resource_type,Luis Floresmaybe it can be a TODO for now, but there probably needs to be a browsertest for this and not just a wpt
Ari ChivukulaThanks. I can add in a follow up patchset/CL, I'd like to nail down the overall design first.
please add a TODO for that then
Done
if (!security_context.GetDocumentPolicy()this is kinda awkward, maybe:
```
bool is_report_only_policy = ...
bool is_enforced_policy = ...
if (!is_report_only_policy && !is_enforced_policy) {
return;
}
mojom::blink::PolicyDisposition disposition = is_enforced_policy ? kEnforce : kReport;
```
Done
response.HttpHeaderField(http_names::kContentEncoding).empty()) {check this early for an exit?
Do you mean moving the compression check above? I was thinking that since most cases wouldn't have the policy, it'd be better to start checking for that. Are you suggesting this should be the first check instead?
// todo: turn into enum for versioningLuis Floreswhy leave as TODO here?
Ari ChivukulaHaven't decided on the final shape for versioning yet. I'm trying to start with something simple and have apps try it first. There's no strictness comparison for enum either so unsure yet whether that's the right solution. I can remove the comment if that's better? Or would it be better to be decided now?
I'm confused, is this being rolled out now or just in early prototyping? If it's the latter then it's okay to be unsure for now but you want a TODO like:
`// TODO(crbug.com/XXX): ...`
It's an early prototype, but we have partner app willing to try it out in this early stage. I have added as todo. Apologies for the confusion.
Document-Policy: network-efficiency-guardrailsLuis Floresshould Document-Policy-Report-Only be tested too?
Ari ChivukulaThanks, this was actually great calling out as there was in fact a bug for reporting only. I had been struggling with IsFeatureEnabled but now reading through Document Policy implementation and spec draft again, I think the problem is that both spec and impl assume |false| to be a stricter value than |true| which doesn't hold for the way this feature is currently done.
I have changed the check in this CL as a middle step, but I'm considering to switch the configuration point from "network-efficiency-guardrails" to "no-network-efficiency-guardrails", as it would fit better with existing Document Policy infrastructure.
Thoughts?
If you were using an enum instead of a bool would it be easier? The implementation here differs enough from the explainer I'm not quite sure how it's intended to evolve
The history is a bit messy. Apologies for that. I'm still working on this feature based on the explainer, but there are a few things not mapping 1:1 currently:
- Overall, we want a set of configuration points to control buckets of perf heuristics. This CL is for the first config point and we're giving it an actual name "NetworkEfficiencyGuardrails" beyond the "basic" moniker used elsewhere. I have a more scoped doc for this: ["basic" category overview](https://docs.google.com/document/d/1RGxvtkoQbvApLVdipiASoUQjC0Jf-IKD9qV1EfUJHAQ/edit?usp=sharing) and a queue of planned CLs as described in [Patchset2](https://chromium-review.googlesource.com/c/chromium/src/+/6844536/comments/e9066dee_cb4a539c).
- We've received feedback that blocking might not be good solution so we're starting with reporting and will evaluate blocking after feedback on the prototype.
- Versioning isn't decided yet, so I was thinking of sticking to bool for this CL.
The immediate plan was to have it as boolean for now and decide on versioning, potentially through enum, as it moves forward. I can explore using an enum now, but unsure whether that will be the solution. Alternatively, I was considering switching the name to "no network efficiency guardrails" to fit Document Policy infra better. I have realized in its current state, policy inheritance will likely break.
Related details
- Ari Chivukula
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
ResourceType resource_type,Luis Floresmaybe it can be a TODO for now, but there probably needs to be a browsertest for this and not just a wpt
Ari ChivukulaThanks. I can add in a follow up patchset/CL, I'd like to nail down the overall design first.
Luis Floresplease add a TODO for that then
Done
Added at top of method in third_party/blink/renderer/core/loader/frame_fetch_context.cc
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted and added 1 comment![]( "Open in Gerrit")
Votes added by Takashi Toyoshima
| Code-Review | +1 |
1 comment
I don't have any extra comment other than arichiv@ already pointed.
So, defer to arichiv@
Related details
- Ari Chivukula
- Luis Flores
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula voted and added 5 comments![]( "Open in Gerrit")
Votes added by Ari Chivukula
| Code-Review | +1 |
5 comments
LGTM third_party/blink/renderer/core/permissions_policy/document_policy_features.json5
ResourceType resource_type,Luis Floresmaybe it can be a TODO for now, but there probably needs to be a browsertest for this and not just a wpt
Ari ChivukulaThanks. I can add in a follow up patchset/CL, I'd like to nail down the overall design first.
Luis Floresplease add a TODO for that then
Luis FloresDone
Added at top of method in third_party/blink/renderer/core/loader/frame_fetch_context.cc
Done
response.HttpHeaderField(http_names::kContentEncoding).empty()) {Luis Florescheck this early for an exit?
Do you mean moving the compression check above? I was thinking that since most cases wouldn't have the policy, it'd be better to start checking for that. Are you suggesting this should be the first check instead?
I guess it's not really much of a difference
// todo: turn into enum for versioningLuis Floreswhy leave as TODO here?
Ari ChivukulaHaven't decided on the final shape for versioning yet. I'm trying to start with something simple and have apps try it first. There's no strictness comparison for enum either so unsure yet whether that's the right solution. I can remove the comment if that's better? Or would it be better to be decided now?
Luis FloresI'm confused, is this being rolled out now or just in early prototyping? If it's the latter then it's okay to be unsure for now but you want a TODO like:
`// TODO(crbug.com/XXX): ...`
It's an early prototype, but we have partner app willing to try it out in this early stage. I have added as todo. Apologies for the confusion.
Done
Document-Policy: network-efficiency-guardrailsLuis Floresshould Document-Policy-Report-Only be tested too?
Ari ChivukulaThanks, this was actually great calling out as there was in fact a bug for reporting only. I had been struggling with IsFeatureEnabled but now reading through Document Policy implementation and spec draft again, I think the problem is that both spec and impl assume |false| to be a stricter value than |true| which doesn't hold for the way this feature is currently done.
I have changed the check in this CL as a middle step, but I'm considering to switch the configuration point from "network-efficiency-guardrails" to "no-network-efficiency-guardrails", as it would fit better with existing Document Policy infrastructure.
Thoughts?
Luis FloresIf you were using an enum instead of a bool would it be easier? The implementation here differs enough from the explainer I'm not quite sure how it's intended to evolve
The history is a bit messy. Apologies for that. I'm still working on this feature based on the explainer, but there are a few things not mapping 1:1 currently:
- Overall, we want a set of configuration points to control buckets of perf heuristics. This CL is for the first config point and we're giving it an actual name "NetworkEfficiencyGuardrails" beyond the "basic" moniker used elsewhere. I have a more scoped doc for this: ["basic" category overview](https://docs.google.com/document/d/1RGxvtkoQbvApLVdipiASoUQjC0Jf-IKD9qV1EfUJHAQ/edit?usp=sharing) and a queue of planned CLs as described in [Patchset2](https://chromium-review.googlesource.com/c/chromium/src/+/6844536/comments/e9066dee_cb4a539c).
- We've received feedback that blocking might not be good solution so we're starting with reporting and will evaluate blocking after feedback on the prototype.
- Versioning isn't decided yet, so I was thinking of sticking to bool for this CL.
The immediate plan was to have it as boolean for now and decide on versioning, potentially through enum, as it moves forward. I can explore using an enum now, but unsure whether that will be the solution. Alternatively, I was considering switching the name to "no network efficiency guardrails" to fit Document Policy infra better. I have realized in its current state, policy inheritance will likely break.
as long as this is just early prototyping, I'm fine with some thrash on the design.
Related details
- Luis Flores
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula added 1 comment![]( "Open in Gerrit")
if (!security_context.GetDocumentPolicy()Luis Floresthis is kinda awkward, maybe:
```
bool is_report_only_policy = ...
bool is_enforced_policy = ...
if (!is_report_only_policy && !is_enforced_policy) {
return;
}
mojom::blink::PolicyDisposition disposition = is_enforced_policy ? kEnforce : kReport;
```
Done
Done
Related details
- Luis Flores
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/54573.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Luis Flores
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Flores (Gerrit)
Luis Flores voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| 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")
toyo...@chromium.org ari...@chromium.org I had not realized there was a crash on CQ. I've fixed it and rebased the CL. I'd appreciate your review on this latest patchset.
Related details
- Ari Chivukula
- Takashi Toyoshima
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima voted Code-Review+1![]( "Open in Gerrit")
- Ari Chivukula
- Luis Flores
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ari Chivukula (Gerrit)
Ari Chivukula voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ari Chivukula
| Code-Review | +1 |
1 comment
I don't think there were new changes for me to review
Related details
- Luis Flores
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Luis Flores (Gerrit)
Luis Flores voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Initial implementation for network-efficiency-guardrails
Adds Document Policy configuration point for
network-efficiency-guardrails and base implementation for HTTP resource
compression check.
- M third_party/blink/public/mojom/permissions_policy/document_policy_feature.mojom
- M third_party/blink/renderer/core/loader/frame_fetch_context.cc
- M third_party/blink/renderer/core/loader/frame_fetch_context.h
- M third_party/blink/renderer/core/permissions_policy/document_policy_features.json5
- M third_party/blink/renderer/platform/loader/fetch/fetch_context.h
- M third_party/blink/renderer/platform/loader/fetch/resource_loader.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- A third_party/blink/web_tests/external/wpt/document-policy/experimental-features/network-efficiency-guardrails-json.tentative.html
- A third_party/blink/web_tests/external/wpt/document-policy/experimental-features/network-efficiency-guardrails-json.tentative.html.headers
- A third_party/blink/web_tests/external/wpt/document-policy/experimental-features/network-efficiency-guardrails-report-only.tentative.html
- A third_party/blink/web_tests/external/wpt/document-policy/experimental-features/network-efficiency-guardrails-report-only.tentative.html.headers
- A third_party/blink/web_tests/external/wpt/document-policy/experimental-features/network-efficiency-guardrails.tentative.html
- A third_party/blink/web_tests/external/wpt/document-policy/experimental-features/network-efficiency-guardrails.tentative.html.headers
- A third_party/blink/web_tests/external/wpt/document-policy/experimental-features/resources/compressed.css
- A third_party/blink/web_tests/external/wpt/document-policy/experimental-features/resources/compressed.json
- M tools/metrics/histograms/metadata/blink/enums.xml
Code-Review: +1 by Ari Chivukula, +1 by Takashi Toyoshima
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/54573
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |