API review: Add Workers management APIs. #4556

monica-ch · 2024-05-13T23:03:27Z

This is the review for Workers Support in WebView2.

specs/Workers.md

david-risney · 2024-05-14T23:43:22Z

specs/Workers.md

+ wil::unique_cotaskmem_string scriptUrl;
+ CHECK_FAILURE(worker->get_ScriptUrl(&scriptUrl));
+
+ message << L"ScriptUrl: " << scriptUrl.get();


We want sample code that demonstrates doing something we expect end developers to actually write code for in their apps. The code you have here is more of sandbox / playground sort of thing where someone can try out the API rather than what we would expect the API to actually be used for.

As discussed, I added sample code for apps to utilize this worker monitoring for posting messages to worker for one sample of each worker. Please do suggest if it is not conveying it properly.

monica-ch · 2024-05-23T23:58:11Z

specs/Workers.md

+ {
+ String Scope { get; };
+
+ event Windows.Foundation.TypedEventHandler<CoreWebView2ServiceWorkerRegistration, IInspectable> Destroyed;


I've added this to notify apps that either the SW is unregistered, or we are destroying the object during profile destruction and such but I'm not sure if helps devs much. @david-risney What do you having of having this event on SWR? Do we notify apps that we are destroying the object and such?

You've got a list of these and an event for when one is added so having an event when one is destroyed seems good.

monica-ch · 2024-05-24T00:00:58Z

specs/Workers.md

+
+ event Windows.Foundation.TypedEventHandler<CoreWebView2ServiceWorkerRegistration, IInspectable> Destroyed;
+
+ Windows.Foundation.IAsyncOperation<CoreWebView2ServiceWorker> GetServiceWorkerAsync();


This gives info on active service worker as host apps in general interested in communicating with the active ones. Do you think we should match with the DOM one's like a method to get installing SW or waiting SW?

monica-ch · 2024-05-24T00:04:41Z

specs/Workers.md

+ {
+ String ScriptUri { get; };
+
+ CoreWebView2ServiceWorkerState State { get; };


Right now, we are dealing with active service workers, may be not needed as long as we provide a event to app that notifies SW state changes or methods to give access to other workers.

Good point. You probably want an event for StateChanged?

specs/Workers.md

david-risney · 2024-06-05T00:26:14Z

specs/Workers.md

+
+[uuid(0c0b03bd-ced2-5518-851a-3d3f71fb5c4b), object, pointer_default(unique)]
+interface ICoreWebView2ServiceWorkerRegistration : IUnknown {
+ /// A string representing a URL that defines a service worker's registration scope. It is relative to the base URL of the application.


This text says the property is relative. The next paragraph says it is a fully qualified URL. The fourth paragraph says it corresponds to the ServiceWorkerRegistration's scope which at least when calling register the scope is relative. Please make this clearer. This also applies to the above definition of scope in the registered event args.

Updated, during registration the scope is relative but in our API's it is full URL.

Also, I removed duplicated Scope from ICoreWebView2ServiceWorkerRegistrationEventArgs as devs can access the scope from the registration object.

david-risney · 2024-06-05T00:27:22Z

specs/Workers.md

+ ///
+ /// This corresponds to the `active` property of the `ServiceWorkerRegistration` object in the DOM.
+ /// For more information, see the [MDN documentation](https://developer.mozilla.org/en-US/docs/Web/API/ServiceWorkerRegistration/active).
+ HRESULT GetServiceWorker(


Should this be a property instead of a method? And should it be named 'ActiveServiceWorker' to sort of match the DOM name?

The reason it is created as async method because the CoreWebView2ServiceWokerRegistration object is created as soon as worker is registered in the web but there won't be any SW's available to interact with. The DOM has StateChanged event that notifies when the SW state changes so web can access install/active or waiting workers when it fires.

Do you think we should also provide similar event and make ActiveServiceWorker as property so when app receives this state they can access from ActiveServiceWorker property

Or

make ActiveServiceWorker property default to null and in the impl we monitor the SW state changes to active and create CoreWebView2ServiceWorker object and when app queries for this later we return the object?

specs/Workers.md

Co-authored-by: David Risney <dave@deletethis.net>

david-risney · 2024-06-18T16:52:53Z

specs/Workers.md

+ /// a service worker can control.
+ ///
+ /// By default, the `scope` is the directory where the service worker script resides.
+ /// For example, if the script is at https://example.com/sw.js, the default scope is


It would be good to use https://example.com/app/sw.js as your example. You say the default is the directory where the service worker script resides but the example may lead people to think its just the origin with root path. I would guess for https://example.com/app/sw.js the default scope is https://example.com/app/?

david-risney · 2024-06-18T16:54:16Z

specs/Workers.md

+
+[uuid(876f8390-f9a8-5264-9677-985d8453aeab), object, pointer_default(unique)]
+interface ICoreWebView2SharedWorker : IUnknown {
+ /// A string specified when creating a shared worker.


Doesn't mention the default value that the previous Name docs did. Probably want to make sure they are in sync (assuming they should be the same)

david-risney · 2024-06-18T16:55:27Z

specs/Workers.md

+
+ runtimeclass CoreWebView2ServiceWorkerRegistration
+ {
+ String Scope { get; };


Since this is an absolute URI (right?) probably good to name this ScopeUri

david-risney

Looks good except for the comments I left. Please address those and let me know when you have a PR to main and I can schedule a review thanks.

david-risney · 2024-07-01T17:05:01Z

specs/Workers.md

+===
+
+# Background
+Currently, WebView2 lacks comprehensive APIs for developers to fully utilize


May want to say the following to make it clear this is adding WebView2 APIs to interact with workers and not the ability to use workers at all. That is, WebView2 currently allows use of workers but with no way to interact with them via WebView2 APIs.

Suggested change

Currently, WebView2 lacks comprehensive APIs for developers to fully utilize

Currently, although workers can be used in web content in WebView2, WebView2 lacks comprehensive WebView2 APIs for developers to fully utilize

david-risney · 2024-07-01T17:06:40Z

specs/Workers.md

+===
+
+# Background
+Currently, WebView2 lacks comprehensive APIs for developers to fully utilize


Would be good to add a note to the background that this API spec covers some basics for workers and that future API specs will add additional worker features (with examples & links to the other API specs if available)

david-risney · 2024-07-01T17:09:07Z

specs/Workers.md

+
+ // You can dispatch a message to the worker with the URL you want to
+ // fetch data from the host app.
+ worker.PostMessage("{type: 'FETCH_URL',url: 'https://example.com/data.json'");


For all the messages posted and parsed in the sample code, please change a name or the structure to say 'MyMessage' or something like that so the reader knows that this is sample app code specific data and that you can't just post a message 'FETCH_URL' and have it do something.

Suggested change

worker.PostMessage("{type: 'FETCH_URL',url: 'https://example.com/data.json'");

worker.PostMessage("{ MyMessage: { type: 'FETCH_URL', url: 'https://example.com/data.json' } ");

david-risney · 2024-07-01T17:09:52Z

specs/Workers.md

+ LogMessage("Dedicated Worker has been created with the script located at the " + scriptUri);
+
+ // Send a message to the dedicated worker to offload fetch request from the host app.
+ dedicatedWorker.PostMessage(dedicatedWorker);


Should this be calling PostMessageToWorker?

monica-ch added 2 commits May 13, 2024 15:10

Add workers spec

d561005

added checks

13bc4b4

monica-ch requested a review from david-risney May 14, 2024 14:54

monica-ch added the API Proposal Review WebView2 API Proposal for review. label May 14, 2024

david-risney requested changes May 14, 2024

View reviewed changes

Per feedback - reference dom apis and example of api usage in workers

4ed60d9

monica-ch commented May 23, 2024

View reviewed changes

monica-ch commented May 24, 2024

View reviewed changes

david-risney requested changes Jun 5, 2024

View reviewed changes

monica-ch and others added 2 commits June 5, 2024 11:09

Update specs/Workers.md

bc81bbf

Co-authored-by: David Risney <dave@deletethis.net>

address feedback-2

28f1039

monica-ch requested a review from david-risney June 6, 2024 20:04

david-risney reviewed Jun 18, 2024

View reviewed changes

remove state and add activated event

9dcc110

david-risney requested review from aluhrs13 and vickiez July 1, 2024 17:02

david-risney approved these changes Jul 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API review: Add Workers management APIs. #4556

API review: Add Workers management APIs. #4556

monica-ch commented May 13, 2024

david-risney May 14, 2024

monica-ch May 23, 2024

monica-ch May 23, 2024

david-risney Jun 18, 2024

monica-ch May 24, 2024

monica-ch May 24, 2024

david-risney Jun 18, 2024

david-risney Jun 5, 2024

monica-ch Jun 6, 2024 •

edited

Loading

david-risney Jun 5, 2024

monica-ch Jun 6, 2024

david-risney Jun 18, 2024

david-risney Jun 18, 2024

david-risney Jun 18, 2024

david-risney left a comment

david-risney Jul 1, 2024

david-risney Jul 1, 2024

david-risney Jul 1, 2024

david-risney Jul 1, 2024


		event Windows.Foundation.TypedEventHandler<CoreWebView2ServiceWorkerRegistration, IInspectable> Destroyed;

		Windows.Foundation.IAsyncOperation<CoreWebView2ServiceWorker> GetServiceWorkerAsync();

	Currently, WebView2 lacks comprehensive APIs for developers to fully utilize
	Currently, although workers can be used in web content in WebView2, WebView2 lacks comprehensive WebView2 APIs for developers to fully utilize

	worker.PostMessage("{type: 'FETCH_URL',url: 'https://example.com/data.json'");
	worker.PostMessage("{ MyMessage: { type: 'FETCH_URL', url: 'https://example.com/data.json' } ");

API review: Add Workers management APIs. #4556

Are you sure you want to change the base?

API review: Add Workers management APIs. #4556

Conversation

monica-ch commented May 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monica-ch Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-risney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monica-ch Jun 6, 2024 •

edited

Loading