Skip to content
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 API:CoreWebView2Profile.Delete #2565

Open
wants to merge 1 commit into
base: MultipleFile_Erase
Choose a base branch
from

Conversation

yunate
Copy link
Contributor

@yunate yunate commented Jul 1, 2022

Add Api:CoreWebView2Profile.Delete:

  1. The user call this Delete API, the API send a MOJOM message to the browser process.
  2. The browser process calls API: profile_manager()->MaybeScheduleProfileForDeletion() to make the logic same as Edge:
    • Mark current profile as "schedule for deleting" and will be truly deleted at thread exit. This makes the Delete API do not
      need to wait for the local file to be deleted. So, the Delete Api could be treated as synchronous.
    • Close all WV2 under current profile.
  3. Add a new Enum's value WEBVIEW2_PROCESS_FAILED_REASON_PROFILE_DELETED which indicate the render process
    exited due to the related profile is deleted.
  4. It will be failure when try to create a "schedule for deleting" profile, the behavior is same as Edge.

So, since the API is synchronous, it does not need a call-back for the caller.
The Delete API will not try to release the client's profile class and WV2 class, but them should not be used any more for the user after Delete API is called,

@yunate yunate added the API Proposal Review WebView2 API Proposal for review. label Jul 1, 2022
@yunate yunate requested a review from david-risney July 1, 2022 08:00
specs/MultiProfile.md Outdated Show resolved Hide resolved
[uuid(1c1ae2cc-d5c2-ffe3-d3e7-7857035d23b7), object, pointer_default(unique)]
interface ICoreWebView2Profile3 : IUnknown {
/// Erase the profile completely. All webviews on this profile will be closed and
/// the profile directory on disk will be deleted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does all this happen synchronously during the call to Erase or does Erase return immediately and the work to close webviews and delete the profile directory happen asynchronously? Is there any need to make this method be async and tell the caller when the work closing webviews and deleting the profile directory is complete?

Copy link
Contributor Author

@yunate yunate Jul 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API will do as below steps:

  1. The caller calls Delete API, and the client will send a delete profile request MOJOM message to the browser (do not wait for response).
  2. The browser receives this MOJOM message do:
    • Set a will to delete flag to true;
    • Send Notify profile delete MOJOM message to all WV2 (Including the WV2 that calls the Api) on this profile;
  • Each WV2 receives the MOJOM message, and close the WV2;
  • When the WV2 closed, the browser check if the count of WV2 on this profile is zero (during this period, any create options will reset the flag to false.), if true means all WV2 closed, calls browser delete Api to make its behavior same as what Edge does.

This API seems like Emm... Exit() function, this API called, the WV2 will closed.
And as this API purpose, if any VW2 on a profile call Delete API, other VW2s on this profile will be all closed. This indicates that, all VW2s on this profile are equivalent, the WV2 which called Delete API is not special. So, we do not need to tell the caller when the work closing WV2 and deleting the profile directory is complete.

But maybe we can add a callback when each client (Including the WV2 that calls the Api) receives Notify profile delete MOJOM message in next PR.

image

This is sync when another creates on this profile.

  • if the create is before Notify message, this WV2 will be closed too.
  • if the create is after Notify message but before browser truly delete, we could set flag to false.
  • if the create is after browser delete or during delete (has a lock or run in a queue, so it same as after browser delete), a new profile with same name is created.

The Notify Delete function may could be a more common function, such as NotifyClientToClose(ReasonEnum).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense that its difficult to implement, but it seems like we have to have some way to tell the end developer that Delete is done and if it succeeded. Don't they need to know Delete has finished before closing their app so they don't close during the Delete? Or don't they need to know if Delete has finished before trying to create a new profile that might have the same name?

What is the scenario or use case for CoreWebView2Profile.Delete that the caller doesn't care if it succeeds or when it is done?

Copy link
Contributor Author

@yunate yunate Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design has an update, and now is:

client call Delete API ->
send MOJOM message to the browser process ->
browser close all WV2 and mark current profile as deleting ->
client invoke render process exited callback with reason WEBVIEW2_PROCESS_FAILED_REASON_PROFILE_DELETED

  1. The user call this Delete API, the API send a MOJOM message to the browser process.

  2. The browser process calls API: profile_manager()->MaybeScheduleProfileForDeletion() to make the logic same as Edge:

    • Mark current profile as "schedule for deleting" and will be truly deleted at thread exit. This makes the Delete API do not need to wait for the local file to be deleted. So, the Delete Api could be treated as synchronous.
    • Close all WV2 under current profile, the related render process will be closed.
  3. Add a new Enum's value WEBVIEW2_PROCESS_FAILED_REASON_PROFILE_DELETED which indicate the render process exited due to the related profile is deleted.

  4. It will be failure when try to create a "schedule for deleting" profile, the behavior is same as Edge.

So, since the API is synchronous, it does not need a call-back for the caller.
The Delete API will not try to release the client's profile class and WV2 class, but them should not be used any more for the user after Delete API is called.

specs/MultiProfile.md Outdated Show resolved Hide resolved
specs/MultiProfile.md Outdated Show resolved Hide resolved
specs/MultiProfile.md Outdated Show resolved Hide resolved
@yunate yunate force-pushed the MultipleFile_Erase-draft branch 3 times, most recently from 6ce6795 to bcb9543 Compare July 4, 2022 09:16
specs/MultiProfile.md Outdated Show resolved Hide resolved
[uuid(1c1ae2cc-d5c2-ffe3-d3e7-7857035d23b7), object, pointer_default(unique)]
interface ICoreWebView2Profile3 : IUnknown {
/// Erase the profile completely. All webviews on this profile will be closed and
/// the profile directory on disk will be deleted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense that its difficult to implement, but it seems like we have to have some way to tell the end developer that Delete is done and if it succeeded. Don't they need to know Delete has finished before closing their app so they don't close during the Delete? Or don't they need to know if Delete has finished before trying to create a new profile that might have the same name?

What is the scenario or use case for CoreWebView2Profile.Delete that the caller doesn't care if it succeeds or when it is done?

specs/MultiProfile.md Outdated Show resolved Hide resolved
@david-risney david-risney changed the title Add API:CoreWebView2Profile.Erase Add API:CoreWebView2Profile.Delete Jul 13, 2022
@yunate yunate force-pushed the MultipleFile_Erase-draft branch from bcb9543 to 470a1af Compare July 26, 2022 08:05
@yunate yunate requested a review from david-risney July 26, 2022 09:09
@yunate yunate force-pushed the MultipleFile_Erase-draft branch from 470a1af to ce9ff29 Compare July 27, 2022 05:14
@david-risney
Copy link
Contributor

If this is the old PR and https://github.com/MicrosoftEdge/WebView2Feedback/pull/2664/files is the new PR, can you please close this PR?

@david-risney david-risney added the review completed WebView2 API Proposal that's been reviewed and now needs final update and push label Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Proposal Review WebView2 API Proposal for review. review completed WebView2 API Proposal that's been reviewed and now needs final update and push
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants