AddWebResourceRequestedFilter for workers by vbryh-msft · Pull Request #2560 · MicrosoftEdge/WebView2Feedback

vbryh-msft · 2022-06-29T21:42:56Z

AddWebResourceRequestedFilter for workers

* AddWebResourceRequestedFilter for workers

FrankC-msft · 2022-07-07T17:04:40Z

specs/WebResourceRequestedWorkers.md

+{
+    [flags]
+    enum CoreWebView2WebResourceRequestSourceKinds
+    {


Missing None=0x0. WinRT API guidelines are to be explicit about it. Any reason not to include that? (API comment says we already check for zero, named enum seems clearer)

None would mean that you don't want the event to ever be raised?

Please add None = 0x0

In this case a None is not valid to pass in to AddFilter (it will throw) but it still makes sense to have a None value so you can write code something like the following:

CoreWebView2WebResourceRequestSourceKinds kinds = None; if (blah) { kinds |= Document; } if (somethingelse) { kinds |= SharedWorker;

FrankC-msft · 2022-07-07T17:30:06Z

specs/WebResourceRequestedWorkers.md

+[uuid(1cc6a402-3724-4e4a-9099-8cf60f2f93a1), object, pointer_default(unique)]
+interface ICoreWebView2_16: ICoreWebView2_15 {
+  /// A web resource request with a resource context that matches this
+  /// filter's resource context, an URI that matches this filter's URI


"an URI", typo of "and a URI" ? I think you're mirroring the opening remark from https://docs.microsoft.com/en-us/dotnet/api/microsoft.web.webview2.core.corewebview2.addwebresourcerequestedfilter?view=webview2-dotnet-1.0.1264.42

Please fix.

Looks like a typo in the existing docs:

URI Filter String Request URI Match Notes

*example https://example No The URI is normalized before filter matching so the actual URI used for comparison is https://example.com/

*example/ https://example Yes Just like above, but this time the filter ends with a / just like the normalized URI

I think this should be

URI Filter String Request URI Match Notes

*example https://example No The URI is normalized before filter matching so the actual URI used for comparison is https://example/

*example/ https://example Yes Just like above, but this time the filter ends with a / just like the normalized URI

FrankC-msft · 2022-07-07T17:34:38Z

specs/WebResourceRequestedWorkers.md

+  /// matches no URIs.
+  ///
+  /// For more information about resource context filters, navigate to
+  /// [COREWEBVIEW2_WEB_RESOURCE_CONTEXT].


"navigate to", should this link or point to a specific url like https://docs.microsoft.com/en-us/dotnet/api/microsoft.web.webview2.core.corewebview2webresourcecontext?view=webview2-dotnet-1.0.1264.42 ?

Please open bug for this existing issue in our public docs.

FrankC-msft · 2022-07-07T17:46:15Z

specs/WebResourceRequestedWorkers.md

+# Background
+Currently [WebResourceRequested events](https://docs.microsoft.com/en-us/microsoft-edge/webview2/reference/win32/iwebview2webview5?view=webview2-0.8.355#add_webresourcerequested) are raised only for top level document HTTP requests. We are extending [AddWebResourceRequestedFilter](https://docs.microsoft.com/en-us/microsoft-edge/webview2/reference/win32/iwebview2webview5?view=webview2-0.8.355#addwebresourcerequestedfilter) API to allow you additionally to subscribe to events raised from Service or Shared workers and fixing an issue where out of process iframes did not raise WebResourceRequested events as part of the main page.
+
+# Examples


No samples for Removing a filter, or any error handling. Any interesting examples to provide for forgetting to set a filter?

About forgetting to set a filter we have in the doc for event subscription: "At least one filter must be added for the event to run."

Instead of adding a sample showing what not to do, let's add a comment to the sample code noting that a filter must be added.

Removing is much less common and we don't think sample is necessary.

There aren't really any interesting error cases for runtime sort of errors useful to demonstrate in a sample.

FrankC-msft · 2022-07-07T17:48:43Z

specs/WebResourceRequestedWorkers.md

+webView.CoreWebView2.AddWebResourceRequestedFilter("*.jpg", CoreWebView2WebResourceContext.All, CoreWebView2WebResourceRequestSourceKinds.ServiceWorker);
+webView.CoreWebView2.WebResourceRequested += (sender, args) =>
+{
+    if (args.RequestSourceKinds == CoreWebView2WebResourceRequestSourceKinds.ServiceWorker &&


Do I need to check the SourceKinds if I used a filter when I used a filter? Could this be an assertion?

Do I need to use a FlagSet check instead of == equality since the input param supports Flag operations? (Not sure if there's a best practice for assuming exactly one flag set in context)

Agreed the [flags] on this is odd here, I can't find any precedent for it.

The alternative would be to have two enums, one singular for the event args and one plural for the method.

Another alternative would be to give the property a singular name:

args.RequestSourceKind == CoreWebView2WebResourceRequestSourceKinds.ServiceWorker

Its a common pattern for this API to check the args because usually the end developer has more than one filter set. We should update the sample to have more than one filter set.

Re the [flags] enum, please rename the property as suggested to singular

RequestedSourceKind

FrankC-msft · 2022-07-07T17:50:38Z

specs/WebResourceRequestedWorkers.md

+```cpp
+m_webviewEventSource->AddWebResourceRequestedFilterWithRequestSourceKinds(
+            L"*.jpg", COREWEBVIEW2_WEB_RESOURCE_CONTEXT_ALL,
+            COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS_SERVICE_WORKER);


[Sample] ignored hresults

Please add code to check the HRESULT. Please also check for anywhere else in sample code.

MikeHillberg · 2022-07-07T18:50:57Z

specs/WebResourceRequestedWorkers.md

+===
+
+# Background
+Currently [WebResourceRequested events](https://docs.microsoft.com/en-us/microsoft-edge/webview2/reference/win32/iwebview2webview5?view=webview2-0.8.355#add_webresourcerequested) are raised only for top level document HTTP requests. We are extending [AddWebResourceRequestedFilter](https://docs.microsoft.com/en-us/microsoft-edge/webview2/reference/win32/iwebview2webview5?view=webview2-0.8.355#addwebresourcerequestedfilter) API to allow you additionally to subscribe to events raised from Service or Shared workers and fixing an issue where out of process iframes did not raise WebResourceRequested events as part of the main page.


Why add a new API to enable it rather than adding to the args? Compatibility?

Compat both functionally and perf.

MikeHillberg · 2022-07-07T18:53:10Z

specs/WebResourceRequestedWorkers.md

+{
+    [flags]
+    enum CoreWebView2WebResourceRequestSourceKinds
+    {


None would mean that you don't want the event to ever be raised?

MikeHillberg · 2022-07-07T19:38:01Z

specs/WebResourceRequestedWorkers.md

+webView.CoreWebView2.AddWebResourceRequestedFilter("*.jpg", CoreWebView2WebResourceContext.All, CoreWebView2WebResourceRequestSourceKinds.ServiceWorker);
+webView.CoreWebView2.WebResourceRequested += (sender, args) =>
+{
+    if (args.RequestSourceKinds == CoreWebView2WebResourceRequestSourceKinds.ServiceWorker &&


RequestedSourceKinds (added the "ed")

Yes please do

RequestedSourceKind

MikeHillberg · 2022-07-07T19:39:10Z

specs/WebResourceRequestedWorkers.md

+  /// normalized, any URI fragment has been removed, and non-ASCII hostnames
+  /// have been converted to punycode.
+  ///
+  /// Specifying a `nullptr` for the uri is equivalent to an empty string which


Is this a valid scenario or should it be invalid-arg?

Good point, but we're going to maintain consistency with existing public API and not change anything here.

MikeHillberg · 2022-07-07T19:39:32Z

specs/WebResourceRequestedWorkers.md

+  /// For more information about request source kinds, navigate to
+  /// [COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS].
+  ///
+  /// Because service workers and shared workers run separately from any one HTML document theirs


Suggested change

/// Because service workers and shared workers run separately from any one HTML document theirs

/// Because service workers and shared workers run separately from any one HTML document their

Typo please fix.

MikeHillberg · 2022-07-07T19:41:27Z

specs/WebResourceRequestedWorkers.md

+
+C++
+```cpp
+m_webviewEventSource->AddWebResourceRequestedFilterWithRequestSourceKinds(


Is this an overload? (Why the long name?)

Yes, it is an overload

MikeHillberg · 2022-07-07T19:42:31Z

specs/WebResourceRequestedWorkers.md

+        [interface_name("Microsoft.Web.WebView2.Core.ICoreWebView2_Manual")]
+        {
+            void AddWebResourceRequestedFilter(
+                    String uri,


Maybe uriPattern to indicate that it's not necessarily an actual URI (thus the string type)

Leave as 'uri'. It is a better name but we want to match the existing overload's parameter name.

MikeHillberg · 2022-07-07T19:44:19Z

specs/WebResourceRequestedWorkers.md

+webView.CoreWebView2.AddWebResourceRequestedFilter("*.jpg", CoreWebView2WebResourceContext.All, CoreWebView2WebResourceRequestSourceKinds.ServiceWorker);
+webView.CoreWebView2.WebResourceRequested += (sender, args) =>
+{
+    if (args.RequestSourceKinds == CoreWebView2WebResourceRequestSourceKinds.ServiceWorker &&


Agreed the [flags] on this is odd here, I can't find any precedent for it.

The alternative would be to have two enums, one singular for the event args and one plural for the method.

Another alternative would be to give the property a singular name:

args.RequestSourceKind == CoreWebView2WebResourceRequestSourceKinds.ServiceWorker

david-risney · 2022-07-07T20:07:38Z

specs/WebResourceRequestedWorkers.md

+===
+
+# Background
+Currently [WebResourceRequested events](https://docs.microsoft.com/en-us/microsoft-edge/webview2/reference/win32/iwebview2webview5?view=webview2-0.8.355#add_webresourcerequested) are raised only for top level document HTTP requests. We are extending [AddWebResourceRequestedFilter](https://docs.microsoft.com/en-us/microsoft-edge/webview2/reference/win32/iwebview2webview5?view=webview2-0.8.355#addwebresourcerequestedfilter) API to allow you additionally to subscribe to events raised from Service or Shared workers and fixing an issue where out of process iframes did not raise WebResourceRequested events as part of the main page.


Compat both functionally and perf.

david-risney · 2022-07-07T20:12:57Z

specs/WebResourceRequestedWorkers.md

+
+[uuid(1cc6a402-3724-4e4a-9099-8cf60f2f93a1), object, pointer_default(unique)]
+interface ICoreWebView2_16: ICoreWebView2_15 {
+  /// A web resource request with a resource context that matches this


Look into updating this ref doc to make it even more explicit and closer to the top of the docs that you must set a filter for this event to ever be raised.

david-risney · 2022-07-07T20:16:07Z

specs/WebResourceRequestedWorkers.md

+  /// times, then it must be removed as many times as it was added for the
+  /// removal to be effective.  Returns `E_INVALIDARG` for a filter that was
+  /// never added.
+  HRESULT RemoveWebResourceRequestedFilterWithRequestSourceKinds(


Should document that if you add for multiple requestSourceKinds in an Add call and remove just one of them in a Remove call, that the filter remains for the non-removed requestSourceKinds.

david-risney · 2022-07-07T20:17:27Z

specs/WebResourceRequestedWorkers.md

+# Background
+Currently [WebResourceRequested events](https://docs.microsoft.com/en-us/microsoft-edge/webview2/reference/win32/iwebview2webview5?view=webview2-0.8.355#add_webresourcerequested) are raised only for top level document HTTP requests. We are extending [AddWebResourceRequestedFilter](https://docs.microsoft.com/en-us/microsoft-edge/webview2/reference/win32/iwebview2webview5?view=webview2-0.8.355#addwebresourcerequestedfilter) API to allow you additionally to subscribe to events raised from Service or Shared workers and fixing an issue where out of process iframes did not raise WebResourceRequested events as part of the main page.
+
+# Examples


Instead of adding a sample showing what not to do, let's add a comment to the sample code noting that a filter must be added.

Removing is much less common and we don't think sample is necessary.

There aren't really any interesting error cases for runtime sort of errors useful to demonstrate in a sample.

david-risney · 2022-07-07T20:19:26Z

specs/WebResourceRequestedWorkers.md

+webView.CoreWebView2.AddWebResourceRequestedFilter("*.jpg", CoreWebView2WebResourceContext.All, CoreWebView2WebResourceRequestSourceKinds.ServiceWorker);
+webView.CoreWebView2.WebResourceRequested += (sender, args) =>
+{
+    if (args.RequestSourceKinds == CoreWebView2WebResourceRequestSourceKinds.ServiceWorker &&


Its a common pattern for this API to check the args because usually the end developer has more than one filter set. We should update the sample to have more than one filter set.

david-risney · 2022-07-07T20:27:21Z

specs/WebResourceRequestedWorkers.md

+  /// normalized, any URI fragment has been removed, and non-ASCII hostnames
+  /// have been converted to punycode.
+  ///
+  /// Specifying a `nullptr` for the uri is equivalent to an empty string which


Good point, but we're going to maintain consistency with existing public API and not change anything here.

david-risney · 2022-07-07T20:28:24Z

specs/WebResourceRequestedWorkers.md

+  /// matches no URIs.
+  ///
+  /// For more information about resource context filters, navigate to
+  /// [COREWEBVIEW2_WEB_RESOURCE_CONTEXT].


Please open bug for this existing issue in our public docs.

david-risney · 2022-07-07T20:28:34Z

specs/WebResourceRequestedWorkers.md

+  /// For more information about request source kinds, navigate to
+  /// [COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS].
+  ///
+  /// Because service workers and shared workers run separately from any one HTML document theirs


Typo please fix.

david-risney · 2022-07-07T20:33:11Z

specs/WebResourceRequestedWorkers.md

+{
+    [flags]
+    enum CoreWebView2WebResourceRequestSourceKinds
+    {


Please add None = 0x0

In this case a None is not valid to pass in to AddFilter (it will throw) but it still makes sense to have a None value so you can write code something like the following:

CoreWebView2WebResourceRequestSourceKinds kinds = None; if (blah) { kinds |= Document; } if (somethingelse) { kinds |= SharedWorker;

david-risney · 2022-07-07T20:35:51Z

specs/WebResourceRequestedWorkers.md

+        [interface_name("Microsoft.Web.WebView2.Core.ICoreWebView2_Manual")]
+        {
+            void AddWebResourceRequestedFilter(
+                    String uri,


Leave as 'uri'. It is a better name but we want to match the existing overload's parameter name.

oldnewthing · 2022-07-07T20:55:53Z

specs/WebResourceRequestedWorkers.md

+  /// | `*example` | `https://contoso.com/example` | Yes | The filter matches across URI parts  |
+  /// | `*example` | `https://contoso.com/path/?example` | Yes | The filter matches across URI parts |
+  /// | `*example` | `https://contoso.com/path/?query#example` | No | The filter is matched against the URI with no fragment |
+  /// | `*example` | `https://example` | No | The URI is normalized before filter matching so the actual URI used for comparison is `https://example.com/` |


AddWebResourceRequestedFilter for workers (#2499)

660b372

* AddWebResourceRequestedFilter for workers

david-risney changed the title ~~AddWebResourceRequestedFilter for workers (#2499)~~ AddWebResourceRequestedFilter for workers Jun 30, 2022

FrankC-msft reviewed Jul 7, 2022

View reviewed changes

MikeHillberg reviewed Jul 7, 2022

View reviewed changes

david-risney requested changes Jul 7, 2022

View reviewed changes

oldnewthing reviewed Jul 7, 2022

View reviewed changes

david-risney added API Proposal Review WebView2 API Proposal for review. review completed WebView2 API Proposal that's been reviewed and now needs final update and push labels Jul 13, 2022

URI Filter String	Request URI	Match	Notes
*example	https://example	No	The URI is normalized before filter matching so the actual URI used for comparison is https://example.com/
*example/	https://example	Yes	Just like above, but this time the filter ends with a / just like the normalized URI

	/// Because service workers and shared workers run separately from any one HTML document theirs
	/// Because service workers and shared workers run separately from any one HTML document their

	/// \| `*example` \| `https://example` \| No \| The URI is normalized before filter matching so the actual URI used for comparison is `https://example.com/` \|
	/// \| `*example` \| `https://example` \| No \| The URI is normalized before filter matching so the actual URI used for comparison is `https://example/` \|

Conversation

vbryh-msft commented Jun 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment