-
Notifications
You must be signed in to change notification settings - Fork 51
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
API Review: Periodic Background Sync and Background Sync API #4540
base: api-periodic-sync-and-background-sync
Are you sure you want to change the base?
API Review: Periodic Background Sync and Background Sync API #4540
Conversation
``` | ||
|
||
|
||
# Examples |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you say in here why we end devs shouldn't do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.