|Summary:||Allow delayed DC allocation in HWndDC.|
|Product:||WebKit||Reporter:||David Levin <levin>|
|Component:||Platform||Assignee:||David Levin <levin>|
|Severity:||Normal||CC:||aroben, ddkilzer, dimich, dslomov, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
|Bug Blocks:||76303, 76889|
Description David Levin 2012-01-20 13:59:49 PST
Comment 2 David Levin 2012-01-20 14:06:30 PST
I'll gladly accept a review from anyone who will provide one.
Comment 3 WebKit Review Bot 2012-01-20 14:08:13 PST
Attachment 123375 [details] 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]  Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Dmitry Lomov 2012-01-20 15:28:18 PST
Comment on attachment 123375 [details] Patch Looks good to me. Any usages in the code?
Comment 5 David Levin 2012-01-20 15:29:47 PST
(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.
Comment 6 Adam Roben (:aroben) 2012-01-23 08:38:42 PST
Comment on attachment 123375 [details] 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.
Comment 7 David Levin 2012-01-23 08:41:52 PST
(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.