-
Notifications
You must be signed in to change notification settings - Fork 55
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
Multiprofile's Delete API modify #3069
base: MultipleFile_Delete
Are you sure you want to change the base?
Conversation
d40cfb0
to
71ec5f8
Compare
71ec5f8
to
3b46d95
Compare
14b4dec
to
009cc73
Compare
009cc73
to
70e7a7f
Compare
/// Indicates that the reason of webview2 closed is its corresponding | ||
/// Profile has been or marked as deleted. | ||
COREWEBVIEW2_CLOSED_REASON_PROFILE_DELETED, | ||
} COREWEBVIEW2_CLOSED_REASON; |
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 two other cases we should add here (we can get the urgent work's implementation in first, and the less urgent parts implementation in a later PR) are for normal webview2 shutdown, and for browser process crash. Maybe this:
/// The CoreWebView2 has closed because the corresponding
/// CoreWebView2Controller had its Close method called either
/// explicitly or implicitly.
COREWEBVIEW2_CLOSED_REASON_SHUTDOWN,
/// The CoreWebView2 has closed because its browser process
/// crashed. The Closed event will be raised after the ProcessFailed
/// event is raised.
COREWEBVIEW2_CLOSED_REASON_BROWSER_PROCESS_FAILURE,
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.
Ok, I will create a work item to trace this things.
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.
Additional entries for this:
/// The CoreWebView2 has closed because the corresponding
/// CoreWebView2Controller had its Close method called either
/// explicitly or implicitly.
COREWEBVIEW2_CLOSED_REASON_SHUTDOWN,
/// The CoreWebView2 has closed because its browser process
/// crashed. The Closed event will be raised after the ProcessFailed
/// event is raised.
COREWEBVIEW2_CLOSED_REASON_BROWSER_PROCESS_FAILURE,
/// The CoreWebView2 has closed because its browser process
/// has exited.
COREWEBVIEW2_CLOSED_REASON_BROWSER_PROCESS_EXITED,
/// The CoreWebView2 has closed because its parent window
/// was closed.
COREWEBVIEW2_CLOSED_REASON_PARENT_WINDOW_CLOSED
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.
Do I need to add those to the API SPEC, or add them one by one in a later PR?
/// Remove an event handler previously added with `add_Closed`. | ||
HRESULT remove_Closed( | ||
[in] EventRegistrationToken token); | ||
} |
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.
In addition we should add a property where you can check if the CoreWebView2 is closed or not. This way end dev's asynchronous code that may be in progress when the profile is deleted can check if the CoreWebView2 has closed and take appropriate action.
/// Returns TRUE if the CoreWebView2 is closed. Once closed, CoreWebView2
/// events will no longer be raised and methods will immediately return failure
/// unless otherwise noted in the API documentation. This property will continue
/// to work after the CoreWebView2 is closed.
[propget] HRESULT IsClosed([out] BOOL* value);
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 think do not need this function. Because, close webview and Closed event handle function called are in a same therad and a same loop. So, if the Closed event handle function called, the user has known the webview has been closed, they can record a flag in this function; if before Closed event handle function called, the webview is always still alive.
60e59fc
to
70e7a7f
Compare
specs/MultiProfile.md
Outdated
/// The CoreWebView2 has closed because the corresponding | ||
/// CoreWebView2Controller had its Close method called either explicitly or | ||
/// implicitly. | ||
COREWEBVIEW2_CLOSED_REASON_SHUTDOWN, |
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.
If developers explicitly called Close()
to close the webview, wondering whether we need to raise the Closed
event.
And, what scenario do you imagine that the Close()
method is called implicitly? Is it already covered by other enums?
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.
remove this Close event's Enum value and add them in latter PR.
Before, we have implemented profile's Delete API, but it has some bugs, so we should modify it.
At present, the relationship between profile and WebView is:
So, when Delete API is called, we should raise an event handle to tell every webview2 under this profile that the profile has called Delete API.
Local files delete logic of Delete API will follow Edge’s delete logic. And the Edge’s profile delete logic is:
The Delete API new design is based on the above fact. When Delete API is called, the actions are: