Bug 55334

Summary: Add a handler class for Win32 HANDLE
Product: WebKit Reporter: Patrick R. Gansterer <paroga>
Component: PlatformAssignee: Patrick R. Gansterer <paroga>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Bug Depends on:    
Bug Blocks: 55336    
Attachments:
Description Flags
Patch aroben: review+

Description Patrick R. Gansterer 2011-02-27 15:23:31 PST
see patch
Comment 1 Patrick R. Gansterer 2011-02-27 15:28:39 PST
Created attachment 83998 [details]
Patch
Comment 2 Adam Roben (:aroben) 2011-02-28 05:10:46 PST
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 3 Patrick R. Gansterer 2011-02-28 06:13:20 PST
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 4 Adam Roben (:aroben) 2011-02-28 07:11:16 PST
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.
Comment 5 Adam Roben (:aroben) 2011-03-01 14:13:33 PST
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.
Comment 6 Patrick R. Gansterer 2011-03-01 14:14:42 PST
(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 :-(
Comment 7 Patrick R. Gansterer 2011-03-01 14:37:48 PST
(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.
Comment 8 Adam Roben (:aroben) 2011-03-01 14:38:35 PST
(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.