76737 2012-01-20 13:59:49 -0800 Allow delayed DC allocation in HWndDC. 2012-01-23 18:03:40 -0800 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) PC Windows Vista RESOLVED FIXED P2 Normal --- 76303 76889 1 levin levin aroben ddkilzer dimich dslomov webkit.review.bot oldest_to_newest 539771 0 levin 2012-01-20 13:59:49 -0800 See summary. 539776 1 123375 levin 2012-01-20 14:04:36 -0800 Created attachment 123375 Patch 539777 2 levin 2012-01-20 14:06:30 -0800 I'll gladly accept a review from anyone who will provide one. 539779 3 webkit.review.bot 2012-01-20 14:08:13 -0800 Attachment 123375 did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/win/HWndDC.h:64: Use the class HWndDC instead of calling GetDC to avoid potential memory leaks. [runtime/leaky_pattern] [5] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 539849 4 123375 dslomov 2012-01-20 15:28:18 -0800 Comment on attachment 123375 Patch Looks good to me. Any usages in the code? 539851 5 levin 2012-01-20 15:29:47 -0800 (In reply to comment #4) > (From update of attachment 123375 [details]) > Looks good to me. Any usages in the code? See the last patch in https://bugs.webkit.org/show_bug.cgi?id=76303 I was asked to make it into smaller patches so this is part of my attempt to do so. 540568 6 123375 aroben 2012-01-23 08:38:42 -0800 Comment on attachment 123375 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123375&action=review > Source/WebCore/platform/win/HWndDC.h:37 > + explicit HWndDC() "explicit" is only needed for constructs that have one (and only one) required parameter. > Source/WebCore/platform/win/HWndDC.h:60 > + HDC changeWindowDC(HWND hwnd) "changeWindowDC" sounds like a function that takes an HDC, not an HWND. Maybe "setHwnd" would be a clearer name? > Source/WebCore/platform/win/HWndDC.h:68 > + void clearDC() I think this could just be called "clear". It would be nice if "myHwndDC = nullptr" did the same thing as "myHwndDC.clear()", to match classes like RefPtr and OwnPtr. 540572 7 levin 2012-01-23 08:41:52 -0800 (In reply to comment #6) > (From update of attachment 123375 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123375&action=review > > > Source/WebCore/platform/win/HWndDC.h:68 > > + void clearDC() > > I think this could just be called "clear". It would be nice if "myHwndDC = nullptr" did the same thing as "myHwndDC.clear()", to match classes like RefPtr and OwnPtr. My concern with this is that I don't know what the operator= should do if it isn't a nullptr. Maybe I could make it only work for nullptr.... I'll have to try to figure that out. 540963 8 levin 2012-01-23 16:37:03 -0800 Committed as http://trac.webkit.org/changeset/105656 Addressed everything except I didn't add the operator= for now. It wouldn't be used anywhere in my current set of changes anyway. We could add it later if it becomes important. 123375 2012-01-20 14:04:36 -0800 2012-01-23 08:38:42 -0800 Patch bug-76737-20120120140435.patch text/plain 2052 levin U3VidmVyc2lvbiBSZXZpc2lvbjogMTA1MTkxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOTVkZDk4NWJkMTY3NTE5 YmEyODVjZWNhZmFkNzY0MzI4OGExNWJhYS4uYjk0MmJmMmJlMzVjNGE5MDRlMWI5NzVhNzVkMDBi ZTYzMDRkZmZiZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDEyLTAxLTE4ICBEYXZp ZCBMZXZpbiAgPGxldmluQGNocm9taXVtLm9yZz4KKworICAgICAgICBBbGxvdyBkZWxheWVkIERD IGFsbG9jYXRpb24gaW4gSFduZERDLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9NzY3MzcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMh KS4KKworICAgICAgICBObyBuZXcgZnVuY3Rpb25hbGl0eSBleHBvc2VkIHNvIG5vIG5ldyB0ZXN0 cy4KKworICAgICAgICAqIHBsYXRmb3JtL3dpbi9IV25kREMuaDogQ2hhbmdlZCB0aGlzIHNsaWdo dGx5IHRvIGFsbG93CisgICAgICAgIGZvciBhbGxvY2F0aW5nIGEgd2luZG93IERDIGFmdGVyIHRo ZSBpbml0aWFsIGNyZWF0aW9uIHNpbmNlCisgICAgICAgIHRoaXMgcGF0dGVybiBvY2N1cnJzIGlu IHNldmVyYWwgcGxhY2VzIHNvIHRoaXMgbWFrZXMgaXQgZWFzeSB0bworICAgICAgICByZXBsYWNl IHRoZW0gaW4gYW4gdXBjb21pbmcgY2hhbmdlLgorICAgICAgICAoV2ViQ29yZTo6SFduZERDOjpI V25kREMpOgorICAgICAgICAoV2ViQ29yZTo6SFduZERDOjp+SFduZERDKToKKyAgICAgICAgKFdl YkNvcmU6OkhXbmREQzo6Y2hhbmdlV2luZG93REMpOgorICAgICAgICAoV2ViQ29yZTo6SFduZERD OjpjbGVhckRDKToKKwogMjAxMi0wMS0xNyAgQW5kcmVhcyBLbGluZyAgPGF3ZXNvbWVrbGluZ0Bh cHBsZS5jb20+CiAKICAgICAgICAgU3BhY2VTcGxpdFN0cmluZzogU2hhcmUgZXF1aXZhbGVudCBz dHJpbmcgcGllY2UgdmVjdG9ycy4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3Jt L3dpbi9IV25kREMuaCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3dpbi9IV25kREMuaAppbmRl eCAxMWRjMzMyNTdlYWIxMTM2MDVkNTg0ODcxNzZkMTRhOWRhZjgwYmQwLi44MDVkZGZmZmQxN2Nl ZWNhNWRmNzA2NTFjNWUxYTVhZjI3NzkwNTljIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9w bGF0Zm9ybS93aW4vSFduZERDLmgKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vd2luL0hX bmREQy5oCkBAIC0zNCw2ICszNCwxMiBAQCBuYW1lc3BhY2UgV2ViQ29yZSB7CiBjbGFzcyBIV25k REMgewogICAgIFdURl9NQUtFX05PTkNPUFlBQkxFKEhXbmREQyk7CiBwdWJsaWM6CisgICAgZXhw bGljaXQgSFduZERDKCkKKyAgICAgICAgOiBtX2h3bmQoMCkKKyAgICAgICAgLCBtX2hkYygwKQor ICAgIHsKKyAgICB9CisKICAgICBleHBsaWNpdCBIV25kREMoSFdORCBod25kKQogICAgICAgICA6 IG1faHduZChod25kKQogICAgICAgICAsIG1faGRjKDo6R2V0REMoaHduZCkpCkBAIC00OCw4ICs1 NCwyNCBAQCBwdWJsaWM6CiAKICAgICB+SFduZERDKCkKICAgICB7Ci0gICAgICAgIGlmIChtX2hk YykKLSAgICAgICAgICAgIDo6UmVsZWFzZURDKG1faHduZCwgbV9oZGMpOworICAgICAgICBjbGVh ckRDKCk7CisgICAgfQorCisgICAgSERDIGNoYW5nZVdpbmRvd0RDKEhXTkQgaHduZCkKKyAgICB7 CisgICAgICAgIGNsZWFyREMoKTsKKyAgICAgICAgbV9od25kID0gaHduZDsKKyAgICAgICAgbV9o ZGMgPSA6OkdldERDKGh3bmQpOworICAgICAgICByZXR1cm4gbV9oZGM7CisgICAgfQorCisgICAg dm9pZCBjbGVhckRDKCkKKyAgICB7CisgICAgICAgIGlmICghbV9oZGMpCisgICAgICAgICAgICBy ZXR1cm47CisgICAgICAgIDo6UmVsZWFzZURDKG1faHduZCwgbV9oZGMpOworICAgICAgICBtX2h3 bmQgPSAwOworICAgICAgICBtX2hkYyA9IDA7CiAgICAgfQogCiAgICAgb3BlcmF0b3IgSERDKCkK