Bug 121037 – [Windows] Create SharedGDIObject Class Templates

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

This is the second step of Bug 120778.  It creates the SharedGDIObject class template (which is just a RefCounted object with a GDIObject member), and replace the RefCountedGDIHandle object with this.

[Brent Fulgham]

Created attachment 211224 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/14957051>

[Anders Carlsson]

Comment on attachment 211224 [details]
Patch

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

> Source/WebCore/platform/graphics/FontPlatformData.h:231
> +    RefPtr<SharedGDIObject<HFONT> > m_font;

No need for the extra space between > >

> Source/WebCore/platform/graphics/win/SharedGDIObject.h:38
> +    static PassRefPtr<SharedGDIObject> create(T object)
> +    {

I think this should take a GDIObject<T> and then use std::move.

> Source/WebKit/win/WebView.h:74
> +typedef WebCore::SharedGDIObject<HBITMAP> RefCountedHBITMAP;
> +typedef WebCore::SharedGDIObject<HRGN> RefCountedHRGN;

Do we really need these typedefs?

[Brent Fulgham]

Comment on attachment 211224 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontPlatformData.h:231
>> +    RefPtr<SharedGDIObject<HFONT> > m_font;
> 
> No need for the extra space between > >

OK!

>> Source/WebCore/platform/graphics/win/SharedGDIObject.h:38
>> +    {
> 
> I think this should take a GDIObject<T> and then use std::move.

I think we need both, unless we want to manually create the GDIObject<T> in the places that had previously worked with just HFONT/HBRUSH/etc.  I think leaving the (T object) version is a nice convenience.

>> Source/WebKit/win/WebView.h:74
>> +typedef WebCore::SharedGDIObject<HRGN> RefCountedHRGN;
> 
> Do we really need these typedefs?

Probably not. They are only used in two places; I've removed them.

[Brent Fulgham]

(In reply to comment #4)
> > I think this should take a GDIObject<T> and then use std::move.
> 
> I think we need both, unless we want to manually create the GDIObject<T> in the places that had previously worked with just HFONT/HBRUSH/etc.  I think leaving the (T object) version is a nice convenience.

Well, maybe not.  The compiler got confused by HFONT versus GDIObject<HFONT> and couldn't decide which to use.

I guess I'll just add the "adoptGDIObject" calls to the three or four places that use the bare HFONT interface.

[Anders Carlsson]

(In reply to comment #5)
> (In reply to comment #4)
> > > I think this should take a GDIObject<T> and then use std::move.
> > 
> > I think we need both, unless we want to manually create the GDIObject<T> in the places that had previously worked with just HFONT/HBRUSH/etc.  I think leaving the (T object) version is a nice convenience.
> 
> Well, maybe not.  The compiler got confused by HFONT versus GDIObject<HFONT> and couldn't decide which to use.
> 
> I guess I'll just add the "adoptGDIObject" calls to the three or four places that use the bare HFONT interface.

Yeah, that sounds good. It’s always better to be explicit about the cases where there’s single ownership of a resource.

[Brent Fulgham]

Created attachment 211242 [details]
Patch

[Anders Carlsson]

Comment on attachment 211242 [details]
Patch

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

> Source/WebCore/platform/graphics/win/SharedGDIObject.h:53
> +    SharedGDIObject(GDIObject<T> object)

I’d make this explicit.

[Brent Fulgham]

Committed r155476: <http://trac.webkit.org/changeset/155476>