Bug 76227

Summary: check-webkit-style: should encourage the use of Own* classes for Windows DC.
Product: WebKit Reporter: David Levin <levin>
Component: Tools / TestsAssignee: David Levin <levin>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dimich, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 76235, 76281    
Bug Blocks: 76135    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description David Levin 2012-01-12 16:57:34 PST
Inspired by how long it took me to take down: https://bugs.webkit.org/show_bug.cgi?id=76203
Comment 1 David Levin 2012-01-12 17:03:16 PST
Created attachment 122340 [details]
Patch
Comment 2 Dmitry Lomov 2012-01-12 17:12:23 PST
Comment on attachment 122340 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122340&action=review

Looks good, 2 minor nits.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1679
> +              'Use OwnGetDC instead of GetDC to avoid potential memory leaks.')

I think OwnGetDC should be called DCRef, similar to RefPtr

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1685
> +              'Use OwnPtr<HDC> when calling CreateDC to avoid potential memory leaks.')

Maybe call out CreateCompatibleDC as well in the error message
Comment 3 David Levin 2012-01-12 17:48:34 PST
Created attachment 122351 [details]
Patch
Comment 4 Dmitry Lomov 2012-01-12 17:54:38 PST
Comment on attachment 122351 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=122351&action=review

> Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:3357
> +            'Use the class HwndDC instead of calling GetDCEx to avoid potential '

Is there a constructor on HwndDC to match GetDCEx signature? Maybe better to have HwndDC::Get and HwndDC::GetEx instead of constructors?
Comment 5 David Levin 2012-01-12 23:28:44 PST
Created attachment 122378 [details]
Patch
Comment 6 David Levin 2012-01-12 23:31:01 PST
(In reply to comment #4)
> (From update of attachment 122351 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122351&action=review
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:3357
> > +            'Use the class HwndDC instead of calling GetDCEx to avoid potential '
> 
> Is there a constructor on HwndDC to match GetDCEx signature?

Done.

> Maybe better to have HwndDC::Get and HwndDC::GetEx instead of constructors?
I see what you mean but I think that makes it harder to put on the stack and we're in luck because the parameter order is the same for GetDC and GetDCEx so it fits the typical overload pattern.

So I stayed with the constructors as discussed.
Comment 7 WebKit Review Bot 2012-01-12 23:34:21 PST
Attachment 122378 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/win/HwndDCWin.h:40:  Use the class HwndDC instead of calling GetDCEx to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 David Levin 2012-01-12 23:41:46 PST
Created attachment 122379 [details]
Patch
Comment 9 WebKit Review Bot 2012-01-12 23:44:04 PST
Attachment 122379 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/win/HwndDCWin.h:40:  Use the class HwndDC instead of calling GetDCEx to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 David Levin 2012-01-12 23:45:30 PST
(In reply to comment #9)
> Attachment 122379 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
> 
> Source/JavaScriptCore/wtf/win/HwndDCWin.h:40:  Use the class HwndDC instead of calling GetDCEx to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
> Total errors found: 1 in 5 files

Actually this is expected. It is flagging HWndDC. I suppose I could make that one file exempt from this check but it doesn't seem worth it.
Comment 11 David Levin 2012-01-13 10:51:58 PST
Created attachment 122456 [details]
Patch
Comment 12 WebKit Review Bot 2012-01-13 10:55:52 PST
Attachment 122456 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/win/HWndDCWin.h:40:  Use the class HWndDC instead of calling GetDCEx to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 WebKit Review Bot 2012-01-13 14:58:16 PST
Comment on attachment 122456 [details]
Patch

Clearing flags on attachment: 122456

Committed r104995: <http://trac.webkit.org/changeset/104995>
Comment 14 WebKit Review Bot 2012-01-13 14:58:22 PST
All reviewed patches have been landed.  Closing bug.