Bug 120778 – [Windows] Create GDIObject Class Template

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

Create a GDIObject class template that responsibility for creation and destruction of GDI resources in a safe manner.  This is currently handled by various specializations of the generic OwnPtr template class, but shoe-horning GDI objects into this design has made OwnPtr more complicated than it needs to be. It also prevents us from merging OwnPtr and OwnArrayPtr.

This work will be done in a couple of steps:
1. Create the GDIObject class template, and replace any bare GDI object (e.g., HFONT, HBRUSH, etc.) use with this helper wrapper, and replace OwnPtr<#GDI Thing#> with this new class.
2. Create the SharedGDIObject class template (which is just a RefCounted object with a GDIObject member), and replace the RefCountedGDIHandle object with this.

[Radar WebKit Bug Importer]

<rdar://problem/14919459>

[Brent Fulgham]

Created attachment 211062 [details]
Initial WIP Patch.

[WebKit Commit Bot]

Attachment 211062 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/platform/graphics/ca/win/CACFLayerTreeHost.cpp', u'Source/WebCore/platform/win/GDIObject.h', u'Source/WebCore/platform/win/ScrollbarThemeWin.cpp', u'Source/WebCore/rendering/RenderThemeWin.cpp']" exit_code: 1
Source/WebCore/platform/win/GDIObject.h:79:  Missing spaces around &&  [whitespace/operators] [3]
Source/WebCore/platform/win/GDIObject.h:82:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 211062 [details]
Initial WIP Patch.

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

> Source/WebCore/platform/win/GDIObject.h:48
> +#if COMPILER_SUPPORTS(CXX_RVALUE_REFERENCES)

This is always true now.

> Source/WebCore/platform/win/GDIObject.h:55
> +    typedef T ValueType;
> +    typedef ValueType* PtrType;

No need to do this anymore. The pointer type will always be T.

> Source/WebCore/platform/win/GDIObject.h:58
> +    GDIObject(const T& obj) : m_obj(obj) { }

I think we need an explicit adopt function for adopting a pointer.

> Source/WebCore/platform/win/GDIObject.h:64
> +    operator HGDIOBJ () { return static_cast<HGDIOBJ>(m_obj); }
> +    operator ValueType () { return m_obj; }

Non-explicit conversion operators are pretty dangerous. I think you should get rid of them.

> Source/WebCore/platform/win/GDIObject.h:67
> +    ValueType release() WARN_UNUSED_RETURN;

Let’s call this leak() instead.

> Source/WebCore/platform/win/GDIObject.h:99
> +    T m_obj;

m_obj can just be m_object.

> Source/WebCore/platform/win/GDIObject.h:166
> +template<typename T, typename U> inline bool operator==(const GDIObject<T>& a, U* b)
> +{
> +    return a.get() == b; 
> +}
> +
> +template<typename T, typename U> inline bool operator==(T* a, const GDIObject<U>& b) 
> +{
> +    return a == b.get(); 
> +}
> +
> +template<typename T, typename U> inline bool operator!=(const GDIObject<T>& a, U* b)
> +{
> +    return a.get() != b; 
> +}
> +
> +template<typename T, typename U> inline bool operator!=(T* a, const GDIObject<U>& b)
> +{
> +    return a != b.get(); 
> +}

Not sure we need all these operators - checking for GDI object equality seems uncommon.

> Source/WebCore/platform/win/GDIObject.h:188
> +using WebCore::GDIObject;

I don’t think we want this.

[Brent Fulgham]

Comment on attachment 211062 [details]
Initial WIP Patch.

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

>> Source/WebCore/platform/win/GDIObject.h:48
>> +#if COMPILER_SUPPORTS(CXX_RVALUE_REFERENCES)
> 
> This is always true now.

OK!

>> Source/WebCore/platform/win/GDIObject.h:58
>> +    GDIObject(const T& obj) : m_obj(obj) { }
> 
> I think we need an explicit adopt function for adopting a pointer.

You mean as a class member? Or do you want the whole PassRefPtr infrastructure?

>> Source/WebCore/platform/win/GDIObject.h:64
>> +    operator ValueType () { return m_obj; }
> 
> Non-explicit conversion operators are pretty dangerous. I think you should get rid of them.

Removed!

>> Source/WebCore/platform/win/GDIObject.h:67
>> +    ValueType release() WARN_UNUSED_RETURN;
> 
> Let’s call this leak() instead.

Done.

>> Source/WebCore/platform/win/GDIObject.h:99
>> +    T m_obj;
> 
> m_obj can just be m_object.

Done.

>> Source/WebCore/platform/win/GDIObject.h:166
>> +}
> 
> Not sure we need all these operators - checking for GDI object equality seems uncommon.

Removed!

>> Source/WebCore/platform/win/GDIObject.h:188
>> +using WebCore::GDIObject;
> 
> I don’t think we want this.

Done.

[Brent Fulgham]

I discovered that the OwnPtr stuff was actually being used outside of WebCore (in test and other classes), so I moved the implementation to WTF.

I was not able to remove HBITMAP and HDC from OwnPtr (yet) because they are still being used as part of the RefCountedGDIObject class.  This will be addressed in Bug 121037.

[Brent Fulgham]

Created attachment 211126 [details]
Patch

[WebKit Commit Bot]

Attachment 211126 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.vcxproj/WTF.vcxproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj.filters', u'Source/WTF/WTF.vcxproj/copy-files.cmd', u'Source/WTF/wtf/OwnPtrCommon.h', u'Source/WTF/wtf/win/GDIObject.h', u'Source/WTF/wtf/win/OwnPtrWin.cpp', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/page/win/FrameCGWin.cpp', u'Source/WebCore/page/win/FrameCairoWin.cpp', u'Source/WebCore/page/win/FrameWin.cpp', u'Source/WebCore/page/win/FrameWin.h', u'Source/WebCore/platform/graphics/ca/win/CACFLayerTreeHost.cpp', u'Source/WebCore/platform/graphics/win/FontCacheWin.cpp', u'Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp', u'Source/WebCore/platform/graphics/win/FontCustomPlatformDataCairo.cpp', u'Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp', u'Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp', u'Source/WebCore/platform/graphics/win/GraphicsContextWin.cpp', u'Source/WebCore/platform/graphics/win/SimpleFontDataWin.cpp', u'Source/WebCore/platform/win/CursorWin.cpp', u'Source/WebCore/platform/win/DragImageCGWin.cpp', u'Source/WebCore/platform/win/DragImageCairoWin.cpp', u'Source/WebCore/platform/win/DragImageWin.cpp', u'Source/WebCore/platform/win/PasteboardWin.cpp', u'Source/WebCore/platform/win/ScrollbarThemeWin.cpp', u'Source/WebCore/plugins/win/PluginViewWin.cpp', u'Source/WebCore/rendering/RenderThemeWin.cpp', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/FullscreenVideoController.cpp', u'Source/WebKit/win/FullscreenVideoController.h', u'Source/WebKit/win/WebCoreSupport/EmbeddedWidget.cpp', u'Source/WebKit/win/WebNodeHighlight.cpp', u'Source/WebKit/win/WebView.cpp']" exit_code: 1
Source/WebCore/platform/graphics/win/GraphicsContextWin.cpp:117:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebCore/platform/win/DragImageWin.cpp:185:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebKit/win/WebNodeHighlight.cpp:145:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebKit/win/FullscreenVideoController.cpp:491:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebCore/platform/win/PasteboardWin.cpp:747:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebCore/platform/win/PasteboardWin.cpp:748:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebCore/platform/win/CursorWin.cpp:57:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebCore/platform/win/CursorWin.cpp:82:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebCore/platform/win/CursorWin.cpp:83:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebKit/win/WebView.cpp:898:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebKit/win/WebView.cpp:997:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebKit/win/WebView.cpp:1041:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebKit/win/WebView.cpp:1115:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebCore/platform/win/DragImageCGWin.cpp:97:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebCore/platform/win/DragImageCGWin.cpp:128:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WTF/wtf/win/GDIObject.h:67:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/win/GDIObject.h:70:  Missing spaces around &&  [whitespace/operators] [3]
Source/WebCore/plugins/win/PluginViewWin.cpp:1071:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebCore/platform/win/DragImageCairoWin.cpp:119:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebCore/platform/win/DragImageCairoWin.cpp:158:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Source/WebCore/page/win/FrameCGWin.cpp:61:  Use adoptPtr and OwnPtr<HDC> when calling CreateCompatibleDC to avoid potential memory leaks.  [runtime/leaky_pattern] [5]
Total errors found: 21 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 211126 [details]
Patch

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

> Source/WTF/wtf/win/GDIObject.h:41
> +typedef struct HBITMAP__* HBITMAP;
> +typedef struct HBRUSH__* HBRUSH;
> +typedef struct HDC__* HDC;
> +typedef struct HFONT__* HFONT;
> +typedef struct HPALETTE__* HPALETTE;
> +typedef struct HPEN__* HPEN;
> +typedef struct HRGN__* HRGN;

Do you really need to forward declare these? Since ::DeleteObject is used below maybe windows.h is already brought in somehow?

> Source/WTF/wtf/win/GDIObject.h:77
> +

I think you can remove this newline too.

> Source/WTF/wtf/win/GDIObject.h:78
> +    GDIObject(const T& obj) : m_object(obj) { }

No need for this to be const T& - it can just be T. Also, obj -> object.

> Source/WTF/wtf/win/GDIObject.h:98
> +template<typename T> inline GDIObject<T>::GDIObject(GDIObject<T>&& o)
> +    : m_object(o.leak())

I think you should name the variable “other” instead of o.

> Source/WTF/wtf/win/GDIObject.h:106
> +    T obj = m_object;
> +    m_object = o.leak();
> +    ASSERT(!obj || m_object != obj);

A better idiom for writing operator= is:

auto obj = std::move(other);
swap(obj);
return *this;

> Source/WTF/wtf/win/GDIObject.h:120
> +template<typename T> inline GDIObject<T>& GDIObject<T>::operator=(T obj)
> +{
> +    T object = m_object;
> +    m_object = obj;
> +    ASSERT(!object || m_object != object);
> +    deleteObject(object);
> +
> +    return *this;
> +}

Is this member really needed?

> Source/WTF/wtf/win/GDIObject.h:127
> +
> +

Extra newline!

> Source/WTF/wtf/win/GDIObject.h:148
> +#endif // GDIObject_h

You should add using WTF::GDIObject; outside of the WTF namespace, that way you don’t need the WTF prefix.

> Source/WebCore/page/win/FrameWin.h:38
> +WTF::GDIObject<HBITMAP> imageFromRect(const Frame*, IntRect&);
> +WTF::GDIObject<HBITMAP> imageFromSelection(Frame*, bool forceWhiteText);

No need for WTF.

> Source/WebCore/platform/graphics/ca/win/CACFLayerTreeHost.cpp:60
> +using WTF::adoptGDIObject;

This should also go into the GDIObject.h!

> Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:104
> +    GDIObject<HBITMAP> bitmap = adoptGDIObject(static_cast<HBITMAP>(::GetCurrentObject(hdc, OBJ_BITMAP)));

auto!

> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:140
> +    GDIObject<HBITMAP> bitmap = adoptGDIObject(static_cast<HBITMAP>(::GetCurrentObject(hdc, OBJ_BITMAP)));

auto!

[Brent Fulgham]

Comment on attachment 211126 [details]
Patch

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

Thanks for looking this thing over. I'm rebuilding now with corrections.

>> Source/WTF/wtf/win/GDIObject.h:77
>> +
> 
> I think you can remove this newline too.

Done.

>> Source/WTF/wtf/win/GDIObject.h:78
>> +    GDIObject(const T& obj) : m_object(obj) { }
> 
> No need for this to be const T& - it can just be T. Also, obj -> object.

Done.

>> Source/WTF/wtf/win/GDIObject.h:98
>> +    : m_object(o.leak())
> 
> I think you should name the variable “other” instead of o.

Done.

>> Source/WTF/wtf/win/GDIObject.h:106
>> +    ASSERT(!obj || m_object != obj);
> 
> A better idiom for writing operator= is:
> 
> auto obj = std::move(other);
> swap(obj);
> return *this;

Done.

>> Source/WTF/wtf/win/GDIObject.h:120
>> +}
> 
> Is this member really needed?

Checking -- I'll comment it out and rebuild.

>> Source/WTF/wtf/win/GDIObject.h:127
>> +
> 
> Extra newline!

OK!

>> Source/WTF/wtf/win/GDIObject.h:148
>> +#endif // GDIObject_h
> 
> You should add using WTF::GDIObject; outside of the WTF namespace, that way you don’t need the WTF prefix.

Done.

>> Source/WebCore/page/win/FrameWin.h:38
>> +WTF::GDIObject<HBITMAP> imageFromSelection(Frame*, bool forceWhiteText);
> 
> No need for WTF.

OK!

>> Source/WebCore/platform/graphics/ca/win/CACFLayerTreeHost.cpp:60
>> +using WTF::adoptGDIObject;
> 
> This should also go into the GDIObject.h!

OK!

>> Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:104
>> +    GDIObject<HBITMAP> bitmap = adoptGDIObject(static_cast<HBITMAP>(::GetCurrentObject(hdc, OBJ_BITMAP)));
> 
> auto!

OK!

>> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:140
>> +    GDIObject<HBITMAP> bitmap = adoptGDIObject(static_cast<HBITMAP>(::GetCurrentObject(hdc, OBJ_BITMAP)));
> 
> auto!

OK!

[Brent Fulgham]

Created attachment 211150 [details]
Patch

[Brent Fulgham]

Hopefully this patch will be the final one!  I think I took care of everything.

[WebKit Commit Bot]

Attachment 211150 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/WTF.vcxproj/WTF.vcxproj', u'Source/WTF/WTF.vcxproj/WTF.vcxproj.filters', u'Source/WTF/WTF.vcxproj/copy-files.cmd', u'Source/WTF/wtf/OwnPtrCommon.h', u'Source/WTF/wtf/win/GDIObject.h', u'Source/WTF/wtf/win/OwnPtrWin.cpp', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/page/win/FrameCGWin.cpp', u'Source/WebCore/page/win/FrameCairoWin.cpp', u'Source/WebCore/page/win/FrameWin.cpp', u'Source/WebCore/page/win/FrameWin.h', u'Source/WebCore/platform/graphics/ca/win/CACFLayerTreeHost.cpp', u'Source/WebCore/platform/graphics/win/FontCacheWin.cpp', u'Source/WebCore/platform/graphics/win/FontCustomPlatformData.cpp', u'Source/WebCore/platform/graphics/win/FontCustomPlatformDataCairo.cpp', u'Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp', u'Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp', u'Source/WebCore/platform/graphics/win/GraphicsContextWin.cpp', u'Source/WebCore/platform/graphics/win/SimpleFontDataWin.cpp', u'Source/WebCore/platform/win/CursorWin.cpp', u'Source/WebCore/platform/win/DragImageCGWin.cpp', u'Source/WebCore/platform/win/DragImageCairoWin.cpp', u'Source/WebCore/platform/win/DragImageWin.cpp', u'Source/WebCore/platform/win/PasteboardWin.cpp', u'Source/WebCore/platform/win/ScrollbarThemeWin.cpp', u'Source/WebCore/plugins/win/PluginViewWin.cpp', u'Source/WebCore/rendering/RenderThemeWin.cpp', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/FullscreenVideoController.cpp', u'Source/WebKit/win/FullscreenVideoController.h', u'Source/WebKit/win/WebCoreSupport/EmbeddedWidget.cpp', u'Source/WebKit/win/WebNodeHighlight.cpp', u'Source/WebKit/win/WebView.cpp', u'Tools/ChangeLog', u'Tools/DumpRenderTree/win/PixelDumpSupportWin.cpp', u'Tools/Scripts/webkitpy/style/checkers/cpp.py', u'Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py']" exit_code: 1
Source/WTF/wtf/win/GDIObject.h:69:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/win/GDIObject.h:72:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 2 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 211150 [details]
Patch

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

> Source/WTF/wtf/win/GDIObject.h:66
> +    GDIObject<T>& operator=(T);

I don't think this should be exposed since it takes ownership of the passed in object.

> Source/WTF/wtf/win/GDIObject.h:118
> +/*template<typename T> inline GDIObject<T>& GDIObject<T>::operator=(T other)
> +{
> +    auto object = m_object;
> +    m_object = other;
> +    ASSERT(!object || m_object != object);
> +    deleteObject(object);
> +
> +    return *this;
> +}*/

Commented out, I guess it's not needed after all!

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:477
>     if (desiredItalic && !matchData.m_chosen.lfItalic && synthesizeItalic)
>         matchData.m_chosen.lfItalic = 1;

Not part of this patch, but I think createGDIFont should return a GDIObject<HFONT> - as should any other create functions we have that return GDI objects.

> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:587
> +    hfont.leak(); // result now owns the HFONT.

Can't you call hfont.leak() when creating the FontPlatformData? Even better, change FontPlatformData to take an OwnPtr<HFONT> and use std::move.

> Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:2
> + * Copyright (C) 2003-2008, 2010, 2013 Apple Inc. All rights reserved.

I think we have to explicitly list all the copyright years, so don't change this to use a range.

> Source/WebCore/platform/graphics/win/GraphicsContextWin.cpp:2
> + * Copyright (C) 2003-2008, 2013 Apple Inc. All rights reserved.

I think we have to explicitly list all the copyright years, so don't change this to use a range.

> Source/WebCore/platform/win/DragImageCairoWin.cpp:123
> +    hbmp = allocImage(dstDC.get(), dstSize, &targetContext);

Should make allocImage return a GDIObject!

> Source/WebCore/platform/win/DragImageWin.cpp:192
> +    SelectObject(workingDC.get(), image);

::SelectObject?

> Source/WebCore/plugins/win/PluginViewWin.cpp:470
> +            HRGN rgn = ::CreateRectRgn(0, 0, 0, 0);

I'd rather see GDIObject<HRGN> here and then a call to .leak() in SetWindowRgn.

> Source/WebCore/plugins/win/PluginViewWin.cpp:473
> +            HRGN rgn = ::CreateRectRgn(m_clipRect.x(), m_clipRect.y(), m_clipRect.maxX(), m_clipRect.maxY());

Ditto.

> Source/WebCore/plugins/win/PluginViewWin.cpp:481
> +            HRGN rgn = ::CreateRectRgn(m_clipRect.x(), m_clipRect.y(), m_clipRect.maxX(), m_clipRect.maxY());

Ditto.

[Brent Fulgham]

Comment on attachment 211150 [details]
Patch

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

>> Source/WTF/wtf/win/GDIObject.h:66
>> +    GDIObject<T>& operator=(T);
> 
> I don't think this should be exposed since it takes ownership of the passed in object.

I'll move this to private.

>> Source/WTF/wtf/win/GDIObject.h:118
>> +}*/
> 
> Commented out, I guess it's not needed after all!

Right -- I'll remove it.

>> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:477
>>         matchData.m_chosen.lfItalic = 1;
> 
> Not part of this patch, but I think createGDIFont should return a GDIObject<HFONT> - as should any other create functions we have that return GDI objects.

I filed https://bugs.webkit.org/show_bug.cgi?id=121100 to do this.

>> Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp:2
>> + * Copyright (C) 2003-2008, 2010, 2013 Apple Inc. All rights reserved.
> 
> I think we have to explicitly list all the copyright years, so don't change this to use a range.

OK!

>> Source/WebCore/platform/graphics/win/GraphicsContextWin.cpp:2
>> + * Copyright (C) 2003-2008, 2013 Apple Inc. All rights reserved.
> 
> I think we have to explicitly list all the copyright years, so don't change this to use a range.

OK! I've searched for this in all files, and reverted where I did this change.

>> Source/WebCore/platform/win/DragImageWin.cpp:192
>> +    SelectObject(workingDC.get(), image);
> 
> ::SelectObject?

Yes!

>> Source/WebCore/plugins/win/PluginViewWin.cpp:470
>> +            HRGN rgn = ::CreateRectRgn(0, 0, 0, 0);
> 
> I'd rather see GDIObject<HRGN> here and then a call to .leak() in SetWindowRgn.

Done. I made the same change in EmbeddedWidget.cpp, which has the same code pattern.

>> Source/WebCore/plugins/win/PluginViewWin.cpp:473
>> +            HRGN rgn = ::CreateRectRgn(m_clipRect.x(), m_clipRect.y(), m_clipRect.maxX(), m_clipRect.maxY());
> 
> Ditto.

Ditto.

>> Source/WebCore/plugins/win/PluginViewWin.cpp:481
>> +            HRGN rgn = ::CreateRectRgn(m_clipRect.x(), m_clipRect.y(), m_clipRect.maxX(), m_clipRect.maxY());
> 
> Ditto.

Ditto.

[Brent Fulgham]

Comment on attachment 211150 [details]
Patch

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

>> Source/WebCore/platform/graphics/win/FontCacheWin.cpp:587
>> +    hfont.leak(); // result now owns the HFONT.
> 
> Can't you call hfont.leak() when creating the FontPlatformData? Even better, change FontPlatformData to take an OwnPtr<HFONT> and use std::move.

I'd like to make this change in Bug 121100, as this patch is already over 60KB.  I can't leak() when creating the FontPlatformData, because in the case of a failure we want GDIObject to dealloc the HFONT, so leak() would be inappropriate in that case.  I like your move idea better, but that will involve more changes to other calls, hence the second bug.

[Brent Fulgham]

Committed r155454: <http://trac.webkit.org/changeset/155454>