Bug 76227 – check-webkit-style: should encourage the use of Own* classes for Windows DC.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Levin]

Inspired by how long it took me to take down: https://bugs.webkit.org/show_bug.cgi?id=76203

[David Levin]

Created attachment 122340 [details]
Patch

[Dmitry Lomov]

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

[David Levin]

Created attachment 122351 [details]
Patch

[Dmitry Lomov]

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?

[David Levin]

Created attachment 122378 [details]
Patch

[David Levin]

(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.

[WebKit Review Bot]

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.

[David Levin]

Created attachment 122379 [details]
Patch

[WebKit Review Bot]

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.

[David Levin]

(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.

[David Levin]

Created attachment 122456 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

Comment on attachment 122456 [details]
Patch

Clearing flags on attachment: 122456

Committed r104995: <http://trac.webkit.org/changeset/104995>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.