[webkit-reviews] review granted: [Bug 120778] [Windows] Create GDIObject Class Template : [Attachment 211150] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Sep 10 09:38:14 PDT 2013
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.
More information about the webkit-reviews
mailing list