WPF - Refactor RenderHandler implementations #2652
Conversation
* Added a AbstractRenderHandler * Refactored the RenderHandlers to inherit from AbstractRenderHandler * Implemented the correct and thread safe dispose pattern
|
✅ Build CefSharp 71.0.0-CI2939 completed (commit 7c56fae9cf by @merceyz) |
CefSharp.Wpf/Rendering/Experimental/ByteArrayWritableBitmapRenderHandler.cs
Outdated
Show resolved
Hide resolved
|
✅ Build CefSharp 71.0.0-CI2949 completed (commit 5ac06b9c1e by @merceyz) |
amaitland
left a comment
amaitland
left a comment
There was a problem hiding this comment.
Comments inline, not quite sure what to do with ByteArrayWritableBitmapRenderHandler inheriting from AbstractRenderHandler, open to suggestions.
| /// </summary> | ||
| /// <seealso cref="CefSharp.Wpf.IRenderHandler" /> | ||
| public class ByteArrayWritableBitmapRenderHandler : IRenderHandler | ||
| public class ByteArrayWritableBitmapRenderHandler : AbstractRenderHandler |
There was a problem hiding this comment.
AbstractRenderHandler contains methods/properties that aren't used in this particular instance, so doesn't quite fit having it as a base class.
|
✅ Build CefSharp 72.0.0-CI2963 completed (commit 4b958907ae by @merceyz) |
| /// </summary> | ||
| public void Dispose() | ||
| { | ||
| Dispose(true); |
There was a problem hiding this comment.
If obj does not have a finalizer, the call to the SuppressFinalize method has no effect.
https://docs.microsoft.com/en-us/dotnet/api/system.gc.suppressfinalize?view=netframework-4.7.2
So looks like it's a NO OP, not sure why it's in the example.
There was a problem hiding this comment.
It's for when a derived class has a Finalizer
|
Will remove the AbstractRenderHandler inheritance from ByteArrayWritableBitmapRenderHandler and make the static properties internal. It might be worth creating an abstract class that implants just the Dispose pattern, will look at doing so if I have time. Little more testing and this should be merged in a few days, will take care of the changes when it comes to merge time. Thanks 👍 |
That was what i wanted to do for #2651 but it would only be possible on the OffScreen version. |
|
Broadly speaking that sounds fine, feel free to create an issue to track the task. There are quite a few instances where Dispose is called as the result of the unmanaged class/refptr being freed which won't be candidates for a base class. |
|
✅ Build CefSharp 72.0.0-CI2964 completed (commit addf5a8b3d by @amaitland) |
AbstractRenderHandler - change pixel properties to internal so they can be shared ByteArrayWritableBitmapRenderHandler WritableBitmapRenderHandler - run Format Document (changed fields to read only) Follow up to #2652
|
Follow up commit is 4a145bf Changed base class, didn't implement a base disposable class yet, will look at that as a issue of it's own. I guess technically it doesn't need to actually implement |
3 participants
AbstractRenderHandlerthat takes care of the basics of aIRenderHandlerRenderHandlers to inherit fromAbstractRenderHandler