Bug 76737

Summary: Allow delayed DC allocation in HWndDC.
Product: WebKit Reporter: David Levin <levin>
Component: PlatformAssignee: David Levin <levin>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, ddkilzer, dimich, dslomov, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows Vista   
Bug Depends on:    
Bug Blocks: 76303, 76889    
Attachments:
Description Flags
Patch aroben: review+, aroben: commit-queue-

Description David Levin 2012-01-20 13:59:49 PST
See summary.
Comment 1 David Levin 2012-01-20 14:04:36 PST
Created attachment 123375 [details]
Patch
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] [5]
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.
Comment 8 David Levin 2012-01-23 16:37:03 PST
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.