Add Runtime Enabled Feature for Service Worker Subresource Filter [chromium/src : main]
Noah Lemen (Gerrit)
Attention is currently required from: Matt Falkenhagen, Kinuko Yasuda, Andrew Comminos.
Noah Lemen would like Matt Falkenhagen and Kinuko Yasuda to review this change.
Add Runtime Enabled Feature for Service Worker Subresource Filter
Initially added in https://crrev.com/c/2806051. Afterwards we realized
we were missing a Runtime Enabled Feature to allow us to conduct an
origin trial. This CL adds the Runtime Enabled Feature and switches
detection to use that.
Because origin trial status is not guaranteed to be known at startup,
it also removes feature detection from setting the feature, instead
opting to only enforce the feature later when the URL loader is created.
ChromeStatus entry: https://chromestatus.com/feature/6015753541124096
Bug: 1202160
Change-Id: I0af4aa81b84da18a822650548594dd72b5f6492c
---
M chrome/browser/about_flags.cc
M chrome/browser/flag_descriptions.cc
M chrome/browser/flag_descriptions.h
M content/browser/service_worker/service_worker_subresource_filter_browsertest.cc
M content/renderer/render_frame_impl.cc
M content/renderer/service_worker/service_worker_network_provider_for_frame.cc
M content/renderer/service_worker/service_worker_network_provider_for_frame.h
M content/renderer/service_worker/service_worker_provider_context.h
D content/test/data/service_worker/subresource_filter.html
D content/test/data/service_worker/subresource_filter.html.mock-http-headers
D content/test/data/service_worker/subresource_filter_empty.html
D content/test/data/service_worker/subresource_filter_empty.html.mock-http-headers
M third_party/blink/public/mojom/web_feature/web_feature.mojom
M third_party/blink/public/web/web_local_frame.h
M third_party/blink/renderer/core/frame/web_local_frame_impl.cc
M third_party/blink/renderer/core/frame/web_local_frame_impl.h
M third_party/blink/renderer/platform/runtime_enabled_features.json5
M tools/metrics/histograms/enums.xml
18 files changed, 123 insertions(+), 95 deletions(-)
To view, visit change 3061699. To unsubscribe, or for help writing mail filters, visit settings.
Noah Lemen (Gerrit)
Attention is currently required from: Matt Falkenhagen, Kinuko Yasuda, Andrew Comminos.
2 comments:
File content/child/runtime_features.cc:
Patch Set #1, Line 387:
{"ServiceWorkerSubresourceFilter",Is this needed anymore?
Done
File content/renderer/service_worker/service_worker_network_provider_for_frame.cc:
Patch Set #2, Line 151:
// need to move feature usage tracking here?I think it's worth moving it here -- perhaps call the UseCounter on the frame's document loader with […]
Done
To view, visit change 3061699. To unsubscribe, or for help writing mail filters, visit settings.
Matt Falkenhagen (Gerrit)
Attention is currently required from: Kinuko Yasuda, Noah Lemen.
3 comments:
Patchset:
looks reasonable
File content/browser/service_worker/service_worker_subresource_filter_browsertest.cc:
It looks like InterceptRequest() up to this point is trying to preserve the default behavior. I think we fallback to the default behavior by returning false in these cases. However, ideally we shouldn't need to use URLLoaderInterceptor as it's a last resort when we need a lot of control over the load. I think we can get the behavior we want using RegisterRequestHandler() with a callback that provides a custom response for certain URLs... just the /filter, /nofilter, and /emptyfilter cases?
File content/renderer/service_worker/service_worker_network_provider_for_frame.cc:
Patch Set #7, Line 154:
// need to move feature usage tracking here?Is the comment intentional?
To view, visit change 3061699. To unsubscribe, or for help writing mail filters, visit settings.
Matt Falkenhagen (Gerrit)
Attention is currently required from: Kinuko Yasuda, Noah Lemen.
1 comment:
Patchset:
And sorry for the delay.
To view, visit change 3061699. To unsubscribe, or for help writing mail filters, visit settings.
Noah Lemen (Gerrit)
Attention is currently required from: Matt Falkenhagen, Kinuko Yasuda.
2 comments:
File content/browser/service_worker/service_worker_subresource_filter_browsertest.cc:
It looks like InterceptRequest() up to this point is trying to preserve the default behavior. […]
I had the impression that URLLoaderInterceptor was needed to handle the origin trial token properly: https://source.chromium.org/chromium/chromium/src/+/main:content/browser/origin_trials/origin_trials_browsertest.cc;l=54-65?q=%22%2F%2F%20We%20use%20a%20URLLoaderInterceptor,%20rather%20than%20the%20EmbeddedTestServer%22&ss=chromium
Is there a way to handle that via EmbeddedTestServer?
I couldn't seem to get the default behavior working when returning false. I was able to clean this up somewhat though.
File content/renderer/service_worker/service_worker_network_provider_for_frame.cc:
Patch Set #7, Line 154:
// need to move feature usage tracking here?Is the comment intentional?
Whoops, no. Left in from WIP. Thanks for pointing out!
To view, visit change 3061699. To unsubscribe, or for help writing mail filters, visit settings.
Matt Falkenhagen (Gerrit)
Attention is currently required from: Kinuko Yasuda, Noah Lemen.
Patch set 9:Code-Review +1
1 comment:
Patchset:
Sorry for the delay. lgtm.
To view, visit change 3061699. To unsubscribe, or for help writing mail filters, visit settings.
Kinuko Yasuda (Gerrit)
Attention is currently required from: Noah Lemen.
To view, visit change 3061699. To unsubscribe, or for help writing mail filters, visit settings.
Andrew Comminos (Gerrit)
Attention is currently required from: Noah Lemen.
Patch set 10:Commit-Queue +2
Andrew Comminos (Gerrit)
Attention is currently required from: Noah Lemen.
Patch set 11:Commit-Queue +2
Andrew Comminos (Gerrit)
Attention is currently required from: Noah Lemen.
Patch set 12:Commit-Queue +2
Andrew Comminos (Gerrit)
Attention is currently required from: Noah Lemen.
Patch set 12:Commit-Queue +2
To view, visit change 3061699. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Add Runtime Enabled Feature for Service Worker Subresource Filter
Initially added in https://crrev.com/c/2806051. Afterwards we realized
we were missing a Runtime Enabled Feature to allow us to conduct an
origin trial. This CL adds the Runtime Enabled Feature and switches
detection to use that.
Because origin trial status is not guaranteed to be known at startup,
it also removes feature detection from setting the feature, instead
opting to only enforce the feature later when the URL loader is created.
ChromeStatus entry: https://chromestatus.com/feature/6015753541124096
Bug: 1202160
Change-Id: I0af4aa81b84da18a822650548594dd72b5f6492c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3061699
Commit-Queue: Andrew Comminos <acom...@fb.com>
Reviewed-by: Matt Falkenhagen <fal...@chromium.org>
Reviewed-by: Kinuko Yasuda <kin...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#910661}
---
M chrome/browser/about_flags.cc
M chrome/browser/flag-metadata.json
M chrome/browser/flag_descriptions.cc
M chrome/browser/flag_descriptions.h
M content/browser/service_worker/service_worker_subresource_filter_browsertest.cc
M content/renderer/render_frame_impl.cc
M content/renderer/service_worker/service_worker_network_provider_for_frame.cc
M content/renderer/service_worker/service_worker_network_provider_for_frame.h
M content/renderer/service_worker/service_worker_provider_context.h
D content/test/data/service_worker/subresource_filter.html
D content/test/data/service_worker/subresource_filter.html.mock-http-headers
D content/test/data/service_worker/subresource_filter_empty.html
D content/test/data/service_worker/subresource_filter_empty.html.mock-http-headers
M third_party/blink/public/mojom/web_feature/web_feature.mojom
M third_party/blink/public/web/web_local_frame.h
M third_party/blink/renderer/core/frame/web_local_frame_impl.cc
M third_party/blink/renderer/core/frame/web_local_frame_impl.h
M third_party/blink/renderer/platform/runtime_enabled_features.json5
M tools/metrics/histograms/enums.xml
19 files changed, 133 insertions(+), 101 deletions(-)
To view, visit change 3061699. To unsubscribe, or for help writing mail filters, visit settings.