[webkit-reviews] review granted: [Bug 55334] Add a handler class for Win32 HANDLE : [Attachment 83998] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 55334: Add a handler class for Win32 HANDLE
https://bugs.webkit.org/show_bug.cgi?id=55334

Attachment 83998: Patch
https://bugs.webkit.org/attachment.cgi?id=83998&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=83998&action=review

r=me, but I'm not marking it cq+ because I think you should make a few changes.


> Source/WebCore/platform/win/Win32Handle.h:36
> +    explicit Win32Handle(HANDLE handle) : m_handle(handle) { }

It would be nice if we could be more explicit that this function adopts the
passed-in HANDLE.

> Source/WebCore/platform/win/Win32Handle.h:44
> +	   CloseHandle(m_handle);

Is it OK to call CloseHandle(0)?

> Source/WebCore/platform/win/Win32Handle.h:50
> +    operator HANDLE() const { return m_handle; }

I don't think we need this. We can just use .get() instead. That would match
our WTF smart pointers better.

> Source/WebCore/platform/win/Win32Handle.h:59
> +    Win32Handle& operator=(Win32Handle& other) { swap(other); }

This is surprising behavior. I think it would make more sense just to make
Win32Handle be non-copyable.