-
Notifications
You must be signed in to change notification settings - Fork 64
API Review: Service Worker Post Message Setting #5465
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
base: main
Are you sure you want to change the base?
Conversation
3ee583d to
61ffa14
Compare
| m_sampleUri = m_appWindow->GetLocalUri(L"index.html"); | ||
| CHECK_FAILURE(m_webView->Navigate(m_sampleUri.c_str())); | ||
|
|
||
| // Setup WebMessageReceived event to receive message from main thread. |
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.
Is there a race window where the service worker could try to post a message before we register the handler?
If we are worried about events coming from the old page, maybe the place to register these event handlers is in NavigationStarted?
| CHECK_FAILURE(webViewSettingsStaging->put_IsWebViewScriptApisForServiceWorkerEnabled(!isEnabled)); | ||
|
|
||
| MessageBox( | ||
| nullptr, |
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.
Should probably use reinterpret_cast<HWND>(m_appWindow.Id().Value) as the parent window for the message box.
| { | ||
| std::wstringstream message{}; | ||
| message << L"Message: " << std::endl << L"Service Worker Message from Main thread" << std::endl; | ||
| m_appWindow->AsyncMessageBox(message.str(), L"Message from Service Worker"); |
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.
| m_appWindow->AsyncMessageBox(message.str(), L"Message from Service Worker"); | |
| m_appWindow->AsyncMessageBox(message.str(), L"Message from Service Worker (relayed by Main Thread)"); | |
| // check if chrome and webview objects are available in service worker script. | ||
| sampleUri = GetLocalUri("index.html"); | ||
| webView.CoreWebView2.Navigate(sampleUri); | ||
| } |
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.
The bottom half of the C++ sample is missing from the C# sample. I'm guessing it's something like
webView.CoreWebView2.WebMessageReceived += (s, e) => {
var message = e.TryGetWebMessageAsString();
if (message == "MessageFromMainThread") {
m_appWindow.AsyncMessageBox($"""
Message:
{message}
Service Worker Message from Main thread
""", "Message from Service Worker");
}
};
WebView.CoreWebView2.Profile.ServiceWorkerManager.ServiceWorkerRegistered += (s, e) => {
var serviceWorkerRegistration = e.ServiceWorkerRegistration;
var serviceWorker = serviceWorkerRegistration.ActiveServiceWorker;
if (serviceWorker != null) {
SetupEventsOnServiceWorker(serviceWorker);
} else {
serviceWorkerRegistration.ServiceWorkerActivated += (s2, e2) => {
SetupEventsOnServiceWorker(e2.ActiveServiceWorker);
};
}
};
}
private void SetupEventsOnServiceWorker(CoreWebView2ServiceWorker serviceWorker)
{
serviceWorker.WebMessageReceived += (s, e) => {
var message = e.TryGetWebMessageAsString();
m_appWindow.AsyncMessageBox($"""
Message:
{message}
""", "Message from Service Worker");
};
}| { | ||
| wil::unique_cotaskmem_string scopeUri; | ||
| CHECK_FAILURE(serviceWorkerRegistration->get_ScopeUri(&scopeUri)); | ||
| std::wstring scopeUriStr(scopeUri.get()); |
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.
Unused variable.
| CHECK_FAILURE(m_webView->Navigate(m_sampleUri.c_str())); | ||
|
|
||
| // Setup WebMessageReceived event to receive message from main thread. | ||
| m_webView->add_WebMessageReceived( |
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.
Presumably should avoid double-registering if the setting is toggled multiple times.
| { | ||
| [interface_name("Microsoft.Web.WebView2.Core.ICoreWebView2StagingSettings")] | ||
| { | ||
| Boolean IsWebViewScriptApisForServiceWorkerEnabled { get; set; }; |
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.
Plural verb? Plural "Workers"?
| Boolean IsWebViewScriptApisForServiceWorkerEnabled { get; set; }; | |
| Boolean AreWebViewScriptApisForServiceWorkersEnabled { get; set; }; | |
Maybe tweak for better reading:
Boolean AreWebViewScriptApisEnabledForServiceWorkers { get; set; };
| { | ||
| runtimeclass CoreWebView2Settings | ||
| { | ||
| [interface_name("Microsoft.Web.WebView2.Core.ICoreWebView2StagingSettings")] |
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.
Precedent is to increment a number suffix of the interface, for new properties on CoreWebView2Settings, rather than insert adjectives. (see ICoreWebView2Settings12 adding IsNonClientRegionSupportEnabled. If we haven't otherwise already changed patterns, stay consistent for new properties.
|
|
||
| ## Win32 C++ | ||
| ```cpp | ||
| interface ICoreWebView2Settings : IUnknown { |
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 was meant to be ICoreWebView2StagingSettings to align with interface_name in c# below, correct? (and would change to ICoreWebView2SettingsN if we change to that pattern.)
No description provided.