Bug 55334 - Add a handler class for Win32 HANDLE
Summary: Add a handler class for Win32 HANDLE
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Patrick R. Gansterer
URL:
Keywords:
Depends on:
Blocks: 55336
  Show dependency treegraph
 
Reported: 2011-02-27 15:23 PST by Patrick R. Gansterer
Modified: 2011-03-01 14:38 PST (History)
1 user (show)

See Also:


Attachments
Patch (3.15 KB, patch)
2011-02-27 15:28 PST, Patrick R. Gansterer
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.