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
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
210 changes: 210 additions & 0 deletions specs/WebResourceRequestedWorkers.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,210 @@
WebResourceRequested events for workers
===

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


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

Subscribe to WebResourceRequested event for service workers and override the image in request with the local one

.Net
```c#
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 

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

args.ResourceContext == CoreWebView2WebResourceContext.Image)
{
Stream fileStream = new FileStream("new_image.jpg", FileMode.Open, FileAccess.Read, FileShare.Read);
CoreWebView2WebResourceResponse overriddenResponse =
webView.CoreWebView2.Environment.CreateWebResourceResponse(fileStream, 200, "OK", "Content-Type: image/jpeg");
args.Response = overriddenResponse;
}
};
webView.CoreWebView2.Navigate("https://www.url.com/");
```

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

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.

m_webviewEventSource->add_WebResourceRequested(
Callback<ICoreWebView2WebResourceRequestedEventHandler>(
[this, file_path](ICoreWebView2* webview, ICoreWebView2WebResourceRequestedEventArgs* args)
-> HRESULT {
wil::com_ptr<ICoreWebView2WebResourceRequestedEventArgs2>
webResourceRequestArgs;
if (SUCCEEDED(args->QueryInterface(IID_PPV_ARGS(&webResourceRequestArgs))))
{
COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS requestSourceKinds =
COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS_ALL;
webResourceRequestArgs->get_RequestSourceKinds(&requestSourceKinds);
COREWEBVIEW2_WEB_RESOURCE_CONTEXT context;
args->get_ResourceContext(&context);
if (requestSourceKinds ==
COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS_SERVICE_WORKER &&
context == COREWEBVIEW2_WEB_RESOURCE_CONTEXT_IMAGE)
{
Microsoft::WRL::ComPtr<IStream> response_stream;
Microsoft::WRL::ComPtr<ICoreWebView2_2> webview2;
Microsoft::WRL::ComPtr<ICoreWebView2Environment> environment;
CHECK_FAILURE(m_webviewEventSource->QueryInterface(IID_PPV_ARGS(&webview2)));
CHECK_FAILURE(webview2->get_Environment(&environment));
CHECK_FAILURE(SHCreateStreamOnFileEx(
file_path, STGM_READ, FILE_ATTRIBUTE_NORMAL,
FALSE, nullptr, &response_stream));
Microsoft::WRL::ComPtr<ICoreWebView2WebResourceResponse> response;
CHECK_FAILURE(environment->CreateWebResourceResponse(
response_stream.Get(), 200, L"OK", L"Content-Type: image/jpeg",
&response));
args->put_Response(response.Get());
}
}
return S_OK;
})
.Get(),
&m_webResourceRequestedToken);
```

# API Details
## IDL
```c#
/// Specifies the source of `WebResourceRequested` event.
[v1_enum]
typedef enum COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS {
/// Indicates that web resource is requested from main page including dedicated workers and iframes.
COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS_DOCUMENT = 1,

/// Indicates that web resource is requested from shared worker.
COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS_SHARED_WORKER = 2,

/// Indicates that web resource is requested from service worker.
COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS_SERVICE_WORKER = 4,

/// Indicates that web resource is requested from any supported source.
COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS_ALL = 0XFFFFFFFF
} COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS;
cpp_quote("DEFINE_ENUM_FLAG_OPERATORS(COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS);")

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

/// 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

/// wildcard string for corresponding request source will be raised via
/// the `WebResourceRequested` event. To receive all raised events filters have
/// to be added before main page navigation.
///
/// The `uri` parameter value is a wildcard string matched against the URI
/// of the web resource request. This is a glob style
/// wildcard string in which a `*` matches zero or more characters and a `?`
/// matches exactly one character.
/// These wildcard characters can be escaped using a backslash just before
/// the wildcard character in order to represent the literal `*` or `?`.
///
/// The matching occurs over the URI as a whole string and not limiting
/// wildcard matches to particular parts of the URI.
/// The wildcard filter is compared to the URI after the URI has been
/// 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.

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

///
/// The `requestSourceKinds` is a mask of one or more `COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS`. OR
/// operation(s) can be applied to multiple `COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS` to
/// create a mask representing those data types. API returns `E_INVALIDARG` if `requestSourceKinds` equals to zero.
/// 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.

/// WebResourceRequested will be raised for all CoreWebView2s that have appropriate filters added in the
/// corresponding CoreWebView2Environment.
/// You should only add a WebResourceRequested filter for
/// COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS_SERVICE_WORKER or
/// COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS_SHARED_WORKER on
/// one CoreWebView2 to avoid handling the same WebResourceRequested
/// event multiple times.
///
/// | URI Filter String | Request URI | Match | Notes |
/// | ---- | ---- | ---- | ---- |
/// | `*` | `https://contoso.com/a/b/c` | Yes | A single * will match all URIs |
/// | `*://contoso.com/*` | `https://contoso.com/a/b/c` | Yes | Matches everything in contoso.com across all schemes |
/// | `*://contoso.com/*` | `https://example.com/?https://contoso.com/` | Yes | But also matches a URI with just the same text anywhere in the URI |
/// | `example` | `https://contoso.com/example` | No | The filter does not perform partial matches |
/// | `*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/` |

/// | `*example/` | `https://example` | Yes | Just like above, but this time the filter ends with a / just like the normalized URI |
/// | `https://xn--qei.example/` | `https://&#x2764;.example/` | Yes | Non-ASCII hostnames are normalized to punycode before wildcard comparison |
/// | `https://&#x2764;.example/` | `https://xn--qei.example/` | No | Non-ASCII hostnames are normalized to punycode before wildcard comparison |
HRESULT AddWebResourceRequestedFilterWithRequestSourceKinds(
[in] LPCWSTR const uri,
[in] COREWEBVIEW2_WEB_RESOURCE_CONTEXT const resourceContext,
[in] COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS const requestSourceKinds);

/// Removes a matching WebResource filter that was previously added for the
/// `WebResourceRequested` event. If the same filter was added multiple
/// 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.

[in] LPCWSTR const uri,
[in] COREWEBVIEW2_WEB_RESOURCE_CONTEXT const resourceContext,
[in] COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS const requestSourceKinds);
}

/// Event args for the `WebResourceRequested` event.
[uuid(572f38f9-c317-4b6c-8dd2-c8b894779bbb), object, pointer_default(unique)]
interface ICoreWebView2WebResourceRequestedEventArgs2 : ICoreWebView2WebResourceRequestedEventArgs {
/// The web resource request source.
[propget] HRESULT RequestSourceKinds(
[out, retval] COREWEBVIEW2_WEB_RESOURCE_REQUEST_SOURCE_KINDS* requestSourceKinds);
}
```

## .NET and WinRT
```c# (but really MIDL3)
namespace Microsoft.Web.WebView2.Core
{
[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;

Document = 0x00000001,
SharedWorker = 0x00000002,
ServiceWorker = 0x00000004,
All = 0xFFFFFFFF,
};

runtimeclass CoreWebView2
{
// ...
[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.

CoreWebView2WebResourceContext resourceContext,
CoreWebView2WebResourceRequestSourceKinds requestSourceKinds);

void RemoveWebResourceRequestedFilter(
String uri,
CoreWebView2WebResourceContext resourceContext,
CoreWebView2WebResourceRequestSourceKinds requestSourceKinds);
}
}

runtimeclass CoreWebView2WebResourceRequestedEventArgs
{
// ...
[interface_name("Microsoft.Web.WebView2.Core.ICoreWebView2WebResourceRequestedEventArgs2")]
{
// ICoreWebView2WebResourceRequestedEventArgs2 members
CoreWebView2WebResourceRequestSourceKinds RequestSourceKinds { get; };
}
}
}
```