55334 2011-02-27 15:23:31 -0800 Add a handler class for Win32 HANDLE 2011-03-01 14:38:35 -0800 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) PC Windows XP RESOLVED FIXED P2 Normal --- 55336 1 paroga paroga aroben oldest_to_newest 358802 0 paroga 2011-02-27 15:23:31 -0800 see patch 358805 1 83998 paroga 2011-02-27 15:28:39 -0800 Created attachment 83998 Patch 358987 2 83998 aroben 2011-02-28 05:10:46 -0800 Comment on attachment 83998 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. 359016 3 83998 paroga 2011-02-28 06:13:20 -0800 Comment on attachment 83998 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. 359029 4 83998 aroben 2011-02-28 07:11:16 -0800 Comment on attachment 83998 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. 360219 5 aroben 2011-03-01 14:13:33 -0800 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. 360220 6 paroga 2011-03-01 14:14:42 -0800 (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 :-( 360245 7 paroga 2011-03-01 14:37:48 -0800 (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. 360246 8 aroben 2011-03-01 14:38:35 -0800 (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. 83998 2011-02-27 15:28:39 -0800 2011-02-28 07:11:16 -0800 Patch bug-55334.patch text/plain 3227 paroga ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCA0ZGMxZTZjLi4zNGVmOWU5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQg QEAKKzIwMTEtMDItMjcgIFBhdHJpY2sgR2Fuc3RlcmVyICA8cGFyb2dhQHdlYmtpdC5vcmc+CisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkIGEgaGFu ZGxlciBjbGFzcyBmb3IgV2luMzIgSEFORExFCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD01NTMzNAorCisgICAgICAgIFRoaXMgY2xhc3Mgd2lsbCBjYWxs IENsb3NlSGFuZGxlIGluIHRoZSBkZXN0cnVjdG9yIGZvciB2YWxpZCBoYW5kbGVzLgorCisgICAg ICAgICogcGxhdGZvcm0vd2luL1dpbjMySGFuZGxlLmg6IEFkZGVkLgorCiAyMDExLTAyLTI2ICBB ZGFtIEJhcnRoICA8YWJhcnRoQHdlYmtpdC5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgRXJp YyBTZWlkZWwuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS93aW4vV2luMzJI YW5kbGUuaCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3dpbi9XaW4zMkhhbmRsZS5oCm5ldyBm aWxlIG1vZGUgMTAwNjQ0CmluZGV4IDAwMDAwMDAuLjkxNTBlN2YKLS0tIC9kZXYvbnVsbAorKysg Yi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS93aW4vV2luMzJIYW5kbGUuaApAQCAtMCwwICsxLDY4 IEBACisvKgorICogQ29weXJpZ2h0IChDKSAyMDExIFBhdHJpY2sgR2Fuc3RlcmVyIDxwYXJvZ2FA cGFyb2dhLmNvbT4KKyAqCisgKiBSZWRpc3RyaWJ1dGlvbiBhbmQgdXNlIGluIHNvdXJjZSBhbmQg YmluYXJ5IGZvcm1zLCB3aXRoIG9yIHdpdGhvdXQKKyAqIG1vZGlmaWNhdGlvbiwgYXJlIHBlcm1p dHRlZCBwcm92aWRlZCB0aGF0IHRoZSBmb2xsb3dpbmcgY29uZGl0aW9ucworICogYXJlIG1ldDoK KyAqIDEuIFJlZGlzdHJpYnV0aW9ucyBvZiBzb3VyY2UgY29kZSBtdXN0IHJldGFpbiB0aGUgYWJv dmUgY29weXJpZ2h0CisgKiAgICBub3RpY2UsIHRoaXMgbGlzdCBvZiBjb25kaXRpb25zIGFuZCB0 aGUgZm9sbG93aW5nIGRpc2NsYWltZXIuCisgKiAyLiBSZWRpc3RyaWJ1dGlvbnMgaW4gYmluYXJ5 IGZvcm0gbXVzdCByZXByb2R1Y2UgdGhlIGFib3ZlIGNvcHlyaWdodAorICogICAgbm90aWNlLCB0 aGlzIGxpc3Qgb2YgY29uZGl0aW9ucyBhbmQgdGhlIGZvbGxvd2luZyBkaXNjbGFpbWVyIGluIHRo ZQorICogICAgZG9jdW1lbnRhdGlvbiBhbmQvb3Igb3RoZXIgbWF0ZXJpYWxzIHByb3ZpZGVkIHdp dGggdGhlIGRpc3RyaWJ1dGlvbi4KKyAqCisgKiBUSElTIFNPRlRXQVJFIElTIFBST1ZJREVEIEJZ IEFQUExFIEFORCBJVFMgQ09OVFJJQlVUT1JTICJBUyBJUyIgQU5EIEFOWQorICogRVhQUkVTUyBP UiBJTVBMSUVEIFdBUlJBTlRJRVMsIElOQ0xVRElORywgQlVUIE5PVCBMSU1JVEVEIFRPLCBUSEUg SU1QTElFRAorICogV0FSUkFOVElFUyBPRiBNRVJDSEFOVEFCSUxJVFkgQU5EIEZJVE5FU1MgRk9S IEEgUEFSVElDVUxBUiBQVVJQT1NFIEFSRQorICogRElTQ0xBSU1FRC4gSU4gTk8gRVZFTlQgU0hB TEwgQVBQTEUgT1IgSVRTIENPTlRSSUJVVE9SUyBCRSBMSUFCTEUgRk9SIEFOWQorICogRElSRUNU LCBJTkRJUkVDVCwgSU5DSURFTlRBTCwgU1BFQ0lBTCwgRVhFTVBMQVJZLCBPUiBDT05TRVFVRU5U SUFMIERBTUFHRVMKKyAqIChJTkNMVURJTkcsIEJVVCBOT1QgTElNSVRFRCBUTywgUFJPQ1VSRU1F TlQgT0YgU1VCU1RJVFVURSBHT09EUyBPUiBTRVJWSUNFUzsKKyAqIExPU1MgT0YgVVNFLCBEQVRB LCBPUiBQUk9GSVRTOyBPUiBCVVNJTkVTUyBJTlRFUlJVUFRJT04pIEhPV0VWRVIgQ0FVU0VEIEFO RAorICogT04gQU5ZIFRIRU9SWSBPRiBMSUFCSUxJVFksIFdIRVRIRVIgSU4gQ09OVFJBQ1QsIFNU UklDVCBMSUFCSUxJVFksIE9SIFRPUlQKKyAqIChJTkNMVURJTkcgTkVHTElHRU5DRSBPUiBPVEhF UldJU0UpIEFSSVNJTkcgSU4gQU5ZIFdBWSBPVVQgT0YgVEhFIFVTRSBPRgorICogVEhJUyBTT0ZU V0FSRSwgRVZFTiBJRiBBRFZJU0VEIE9GIFRIRSBQT1NTSUJJTElUWSBPRiBTVUNIIERBTUFHRS4K KyAqLworCisjaWZuZGVmIFdpbjMySGFuZGxlX2gKKyNkZWZpbmUgV2luMzJIYW5kbGVfaAorCisj aW5jbHVkZSA8bWVtb3J5PgorI2luY2x1ZGUgPHdpbmRvd3MuaD4KKworbmFtZXNwYWNlIFdlYkNv cmUgeworCitjbGFzcyBXaW4zMkhhbmRsZSB7CitwdWJsaWM6CisgICAgV2luMzJIYW5kbGUoKSA6 IG1faGFuZGxlKElOVkFMSURfSEFORExFX1ZBTFVFKSB7IH0KKyAgICBleHBsaWNpdCBXaW4zMkhh bmRsZShIQU5ETEUgaGFuZGxlKSA6IG1faGFuZGxlKGhhbmRsZSkgeyB9CisKKyAgICB+V2luMzJI YW5kbGUoKSB7IGNsZWFyKCk7IH0KKworICAgIHZvaWQgY2xlYXIoKQorICAgIHsKKyAgICAgICAg aWYgKCFpc1ZhbGlkKCkpCisgICAgICAgICAgICByZXR1cm47CisgICAgICAgIENsb3NlSGFuZGxl KG1faGFuZGxlKTsKKyAgICAgICAgbV9oYW5kbGUgPSBJTlZBTElEX0hBTkRMRV9WQUxVRTsKKyAg ICB9CisKKyAgICBib29sIGlzVmFsaWQoKSBjb25zdCB7IHJldHVybiBtX2hhbmRsZSAhPSBJTlZB TElEX0hBTkRMRV9WQUxVRTsgfQorCisgICAgb3BlcmF0b3IgSEFORExFKCkgY29uc3QgeyByZXR1 cm4gbV9oYW5kbGU7IH0KKyAgICBIQU5ETEUgZ2V0KCkgY29uc3QgeyByZXR1cm4gbV9oYW5kbGU7 IH0KKyAgICBIQU5ETEUgcmVsZWFzZSgpIHsgSEFORExFIHJldCA9IG1faGFuZGxlOyBtX2hhbmRs ZSA9IElOVkFMSURfSEFORExFX1ZBTFVFOyByZXR1cm4gcmV0OyB9CisKKyAgICBXaW4zMkhhbmRs ZSYgb3BlcmF0b3I9KEhBTkRMRSBoYW5kbGUpCisgICAgeworICAgICAgICBjbGVhcigpOworICAg ICAgICBtX2hhbmRsZSA9IGhhbmRsZTsKKyAgICB9CisgICAgV2luMzJIYW5kbGUmIG9wZXJhdG9y PShXaW4zMkhhbmRsZSYgb3RoZXIpIHsgc3dhcChvdGhlcik7IH0KKyAgICB2b2lkIHN3YXAoV2lu MzJIYW5kbGUmIG90aGVyKSB7IHN0ZDo6c3dhcChtX2hhbmRsZSwgb3RoZXIubV9oYW5kbGUpOyB9 CisKK3ByaXZhdGU6CisgICAgSEFORExFIG1faGFuZGxlOworfTsKKworfSAvLyBuYW1lc3BhY2Ug V2ViQ29yZQorCisjZW5kaWYgLy8gV2luMzJIYW5kbGVfaAo=