-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Improved dispose pattern in ChromiumWebBrowser #2651
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
Conversation
* Added thread safe dispose pattern * Added IsDisposed
|
✅ Build CefSharp 71.0.0-CI2936 completed (commit 09afdada13 by @merceyz) |
|
✅ Build CefSharp 71.0.0-CI2937 completed (commit 3ded02c549 by @merceyz) |
amaitland
left a comment
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.
Other than adding a comment which I've posted inline for discussion the OffScreen changes look like they can be merged.
Any objection if I merge this one class at a time? (Will take care of that manually myself). Rather than trying to review and test everything all at once, and potentially having this go stale, I think it makes sense to work through them one by one. OffScreen, WPF and WinForms is probably the order of lest to most complex.
|
✅ Build CefSharp 71.0.0-CI2947 completed (commit d479d1f93f by @amaitland) |
I would like to keep these changes together and not split them up, however it's up to you.
The code in all three is basically the same, only difference is |
It'll be some time before I have a chance to review them all at once. What advantage do you see in trying to merge this all at once? They are unrelated changes from a code point of view.
It's the sort of thing that's easy to introduce a subtle change, the structure looks fine. |
Less noise in the history, but mainly because i'd like to make more changes to all three at the same time for other things. But go ahead, it's fine by me. |
What sort of things exactly? Related to |
|
✅ Build CefSharp 72.0.0-CI2967 completed (commit d74425cd2e by @amaitland) |
|
✅ Build CefSharp 72.0.0-CI2969 completed (commit 5f087f80af by @amaitland) |
First thing that comes to mind that is sort of related is a disposed check in all public functions. |
|
Please keep the scope of this |
|
Of course |
|
✅ Build CefSharp 72.0.0-CI2970 completed (commit 4f094adbbd by @merceyz) |
|
Will hopefully review/merge the |
|
✅ Build CefSharp 73.0.0-CI3020 completed (commit acbabb79ce by @amaitland) |
|
@merceyz Thoughts on a |
|
@amaitland The |
IsDisposedproperty toIWebBrowserdisposingis set totrueResolves #1954
EDIT:
As this is getting merged in parts, here are the relevant commits
OffScreen: 58fac38 b1ef9e8