-
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
Add Spec: Critical Kill Switch #4574
base: user/xiaqu/critical-kill-switch
Are you sure you want to change the base?
Add Spec: Critical Kill Switch #4574
Conversation
Co-authored-by: David Risney <[email protected]>
|
||
void WebView_CriticalRestartRequired(object sender, object e) | ||
{ | ||
System.Threading.SynchronizationContext.Current.Post((_) => |
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 C++ and C# code should match. Right now they don't really. The C# uses this post to run this code async where the c++ does not. You don't need to do it in either since RestartIfSelectedByUser could handle this.
The C# code shows an outer method WebView_CoreWebView2InitializationCompleted that registers the event handler and the C++ just has some code that registers the event handler.
/// `CriticalRestartRequired` will give developer the ability to prompt user for restart, | ||
/// thus resolve in faster resolution time. | ||
/// | ||
/// Critical Update is only applying payload; thus, version is not important. But for apps |
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 is good to say that a CriticalRestartRequired doesn't necessarily mean there is a new webview2 runtime version, but do we want to say that it will never mean that? Do we want to say that CriticalRestartRequired is only for ECS configuration or leave it more vague so we can apply it to webview2 updates in the future if we like? @aluhrs13, @fabiorocha
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.
Yeah I agree, we shouldn't be too precise about what this does or doesn't do, just that it's something that WV2 believes you should somewhat urgently restart for. I could see us doing something in the future where updates with important CVE fixes might fire this event.
Co-authored-by: David Risney <[email protected]>
Co-authored-by: David Risney <[email protected]>
Add spec for critical kill switch