Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AddWebResourceRequestedFilter for workers #2560

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vbryh-msft
Copy link
Contributor

  • AddWebResourceRequestedFilter for workers

* AddWebResourceRequestedFilter for workers
@david-risney david-risney changed the title AddWebResourceRequestedFilter for workers (#2499) AddWebResourceRequestedFilter for workers Jun 30, 2022
{
[flags]
enum CoreWebView2WebResourceRequestSourceKinds
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

[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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

RequestedSourceKind 

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Sample] ignored hresults

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

===

# 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compat both functionally and perf.

{
[flags]
enum CoreWebView2WebResourceRequestSourceKinds
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RequestedSourceKinds (added the "ed")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please do

RequestedSourceKind

/// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo please fix.


C++
```cpp
m_webviewEventSource->AddWebResourceRequestedFilterWithRequestSourceKinds(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is an overload

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

===

# 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compat both functionally and perf.


[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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// 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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo please fix.

{
[flags]
enum CoreWebView2WebResourceRequestSourceKinds
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// | `*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/` |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is a pre-existing error in the documentatoin. Should be

Suggested change
/// | `*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/` |

@david-risney 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Proposal Review WebView2 API Proposal for review. review completed WebView2 API Proposal that's been reviewed and now needs final update and push
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants