API Review: Periodic Background Sync and Background Sync API #4540
Conversation
JosephJin0815
changed the title
| ``` | ||
|
|
||
|
|
||
| # Examples |
There was a problem hiding this comment.
Sample code section should be above the API details section.
| wil::com_ptr<ICoreWebView2ServiceWorkerManager> serviceWorkerManager; | ||
| CHECK_FAILURE(webViewProfile3->get_ServiceWorkerManager(&serviceWorkerManager)); | ||
|
|
||
| if (serviceWorkerManager) |
There was a problem hiding this comment.
You call CHECK_FAILURE, so can we remove the if (serviceWorkerManager) check? Is it valid for get_ServiceWorkerManager to return S_OK and provide a nullptr serviceWorkerManager? If not then just rely on CHECK_FAILURE and can remove if check.
| ICoreWebView2ServiceWorkerRegistration* serviceWorkerRegistration) | ||
| -> HRESULT | ||
| { | ||
| if (serviceWorkerRegistration) |
There was a problem hiding this comment.
You should be checking the HRESULT error and don't need to check if serviceWorkerRegistration is non-nullptr?
| auto webViewProfile3 = webView2Profile.try_query<ICoreWebView2Profile3>(); | ||
| CHECK_FEATURE_RETURN_EMPTY(webViewProfile3); | ||
| wil::com_ptr<ICoreWebView2ServiceWorkerManager> serviceWorkerManager; | ||
| CHECK_FAILURE(webViewProfile3->get_ServiceWorkerManager(&serviceWorkerManager)); |
There was a problem hiding this comment.
It looks like this API depends on the worker API. Do we need to wait for the worker API spec to finish first? Or will it be good to consider them at the same time?
| } | ||
| else if (wcscmp(message.c_str(), L"GetPeriodicSyncRegistrations") == 0) | ||
| { | ||
| ShowAllSyncRegistrationInfos(COREWEBVIEW2_SERVICE_WORKER_SYNCHRONIZATION_KIND_PERIODIC_SYNC); |
There was a problem hiding this comment.
Same question as previous about the scenario for the sample code. This looks like sandbox / playground sort of code to play with the API rather than sample code that demonstrates something an end dev would actually do with this API in their code.
| /// The minimum interval time, in milliseconds, at which the minimum time | ||
| /// interval between periodic background synchronizations should occur. | ||
| /// You should not call `CoreWebView2ServiceWorkerSyncRegistrationManager.DispatchPeriodicSyncEventAsync` | ||
| /// to dispatch periodic synchronization tasks less than its minimum time interval. |
There was a problem hiding this comment.
Can you say in here why we end devs shouldn't do this?
There was a problem hiding this comment.
I believe we said in chat that you would add something like 'This is because the periodic sync web APIs standard says it will not be dispatched more often than the minimum time interval and the web content may not handle the event being raised more often.'
| /// interval between periodic background synchronizations should occur. | ||
| /// You should not call `CoreWebView2ServiceWorkerSyncRegistrationManager.DispatchPeriodicSyncEventAsync` | ||
| /// to dispatch periodic synchronization tasks less than its minimum time interval. | ||
| /// This property is always returns an invalid value `-1` for the background synchronization registration. |
There was a problem hiding this comment.
| /// This property is always returns an invalid value `-1` for the background synchronization registration. | |
| /// This property is `-1` for all background synchronization registrations. | |
| /// The minimum interval time, in milliseconds, at which the minimum time | ||
| /// interval between periodic background synchronizations should occur. | ||
| /// You should not call `CoreWebView2ServiceWorkerSyncRegistrationManager.DispatchPeriodicSyncEventAsync` | ||
| /// to dispatch periodic synchronization tasks less than its minimum time interval. |
There was a problem hiding this comment.
I believe we said in chat that you would add something like 'This is because the periodic sync web APIs standard says it will not be dispatched more often than the minimum time interval and the web content may not handle the event being raised more often.'
|
|
||
| event Windows.Foundation.TypedEventHandler<CoreWebView2ServiceWorkerSyncRegistrationManager, CoreWebView2ServiceWorkerSyncRegisteredEventArgs> PeriodicSyncRegistered; | ||
|
|
||
| Windows.Foundation.IAsyncOperation<IVectorView<CoreWebView2ServiceWorkerSyncRegistrationInfo>> GetSyncRegistrationsAsync(CoreWebView2ServiceWorkerSynchronizationKind Kind); |
There was a problem hiding this comment.
There's already two events for background and periodic seems like it would make sense to also have two separate methods to get background and periodic registrations?
This is a review for the new Periodic Background Sync and Background Sync API.