see patch
Created attachment 83998 [details] Patch
Comment on attachment 83998 [details] Patch 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.
Comment on attachment 83998 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83998&action=review >> 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. Should I create a adoptWin32Handle ?!? Then this class can't be non-copyable or we need PassWin32Handle too? >> Source/WebCore/platform/win/Win32Handle.h:44 >> + CloseHandle(m_handle); > > Is it OK to call CloseHandle(0)? When will m_handle become 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. I don't like to use .get() all the time, but it makes sense to match the OwnPtr interface.
Comment on attachment 83998 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83998&action=review >>> 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. > > Should I create a adoptWin32Handle ?!? Then this class can't be non-copyable or we need PassWin32Handle too? Yeah, I'm not sure what the best idiom here is. I think what you have now is fine. >>> Source/WebCore/platform/win/Win32Handle.h:44 >>> + CloseHandle(m_handle); >> >> Is it OK to call CloseHandle(0)? > > When will m_handle become 0? It could be 0 if someone passed 0 to the constructor.
The version of this patch that landed in r80034 has neither a copy constructor nor the WTF_MAKE_NONCOPYABLE macro. This means the compiler will generate a default copy constructor, which will do the wrong thing.
(In reply to comment #5) > The version of this patch that landed in r80034 has neither a copy constructor nor the WTF_MAKE_NONCOPYABLE macro. This means the compiler will generate a default copy constructor, which will do the wrong thing. Argh! Missed to add the macro :-(
(In reply to comment #5) > The version of this patch that landed in r80034 has neither a copy constructor nor the WTF_MAKE_NONCOPYABLE macro. This means the compiler will generate a default copy constructor, which will do the wrong thing. Sorry again, committed r80042: <http://trac.webkit.org/changeset/80042> (In reply to comment #4) > >> Is it OK to call CloseHandle(0)? > > > > When will m_handle become 0? > > It could be 0 if someone passed 0 to the constructor. IMHO passing 0 is not right correct, but there's no problem to call CloseHandle(0). It will set GetLastError() to sth like "called with invalid handle" only.
(In reply to comment #7) > (In reply to comment #4) > > >> Is it OK to call CloseHandle(0)? > > > > > > When will m_handle become 0? > > > > It could be 0 if someone passed 0 to the constructor. > > IMHO passing 0 is not right correct, but there's no problem to call CloseHandle(0). It will set GetLastError() to sth like "called with invalid handle" only. If we don't want to allow 0, we could add an assertion.