Bug 121037 - [Windows] Create SharedGDIObject Class Templates
Summary: [Windows] Create SharedGDIObject Class Templates
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on: 120778
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-09 12:17 PDT by Brent Fulgham
Modified: 2013-09-10 14:40 PDT (History)
8 users (show)

See Also:


Attachments
Patch (23.75 KB, patch)
2013-09-10 12:34 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (27.35 KB, patch)
2013-09-10 14:23 PDT, Brent Fulgham
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2013-09-09 12:17:19 PDT
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.
Comment 1 Brent Fulgham 2013-09-10 12:34:02 PDT
Created attachment 211224 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2013-09-10 12:34:59 PDT
<rdar://problem/14957051>
Comment 3 Anders Carlsson 2013-09-10 12:39:20 PDT
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?
Comment 4 Brent Fulgham 2013-09-10 13:25:57 PDT
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.
Comment 5 Brent Fulgham 2013-09-10 13:31:09 PDT
(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.
Comment 6 Anders Carlsson 2013-09-10 13:39:06 PDT
(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.
Comment 7 Brent Fulgham 2013-09-10 14:23:52 PDT
Created attachment 211242 [details]
Patch
Comment 8 Anders Carlsson 2013-09-10 14:27:59 PDT
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.
Comment 9 Brent Fulgham 2013-09-10 14:40:37 PDT
Committed r155476: <http://trac.webkit.org/changeset/155476>