[webkit-reviews] review granted: [Bug 120778] [Windows] Create GDIObject Class Template : [Attachment 211150] Patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 120778: [Windows] Create GDIObject Class Template
https://bugs.webkit.org/show_bug.cgi?id=120778

Attachment 211150: Patch
https://bugs.webkit.org/attachment.cgi?id=211150&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
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.