Bug 76227 - check-webkit-style: should encourage the use of Own* classes for Windows DC.
Summary: check-webkit-style: should encourage the use of Own* classes for Windows DC.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on: 76235 76281
Blocks: 76135
  Show dependency treegraph
 
Reported: 2012-01-12 16:57 PST by David Levin
Modified: 2012-01-13 14:58 PST (History)
4 users (show)

See Also:


Attachments
Patch (5.99 KB, patch)
2012-01-12 17:03 PST, David Levin
no flags Details | Formatted Diff | Diff
Patch (6.47 KB, patch)
2012-01-12 17:48 PST, David Levin
no flags Details | Formatted Diff | Diff
Patch (7.81 KB, patch)
2012-01-12 23:28 PST, David Levin
no flags Details | Formatted Diff | Diff
Patch (7.80 KB, patch)
2012-01-12 23:41 PST, David Levin
no flags Details | Formatted Diff | Diff
Patch (7.86 KB, patch)
2012-01-13 10:51 PST, David Levin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.