Bug 76235

Summary: HwndDC is a better name than OwnGetDC.
Product: WebKit Reporter: David Levin <levin>
Component: Web Template FrameworkAssignee: David Levin <levin>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 76227    
Attachments:
Description Flags
Patch
none
Patch none

Description David Levin 2012-01-12 17:34:04 PST
Based on feedback in https://bugs.webkit.org/show_bug.cgi?id=76227#c2 and subsequent discussion.
Comment 1 David Levin 2012-01-12 17:35:35 PST
Created attachment 122349 [details]
Patch
Comment 2 Dmitry Lomov 2012-01-12 17:46:56 PST
Comment on attachment 122349 [details]
Patch

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

Looks good.

> Source/WebKit/chromium/src/win/WebScreenInfoFactory.cpp:65
> +    HwndDC hdc(0);

Probably makes sense to make a pass and replace more usages of ::GetDC with this new class.
Comment 3 David Levin 2012-01-12 18:19:38 PST
Created attachment 122355 [details]
Patch
Comment 4 Dmitry Titov 2012-01-12 18:29:37 PST
Comment on attachment 122355 [details]
Patch

r=me, I understand there is a separate bug filed to follow up on Dmitry's suggestion to comb the files for other usages of ::GetDC()
Comment 5 WebKit Review Bot 2012-01-12 20:44:23 PST
Comment on attachment 122355 [details]
Patch

Clearing flags on attachment: 122355

Committed r104895: <http://trac.webkit.org/changeset/104895>
Comment 6 WebKit Review Bot 2012-01-12 20:44:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Darin Adler 2012-01-13 09:04:32 PST
Since HWND is “handle to window”, I think it should be HWnd, when things are mixed case, not Hwnd.
Comment 8 David Levin 2012-01-13 09:32:51 PST
(In reply to comment #7)
> Since HWND is “handle to window”, I think it should be HWnd, when things are mixed case, not Hwnd.

Patch: https://bugs.webkit.org/show_bug.cgi?id=76281
Comment 9 Sam Weinig 2012-01-13 12:56:06 PST
I have two concerns with this patch:

1) It is missing a header guard.
2) I don't think WTF is the right layer for it. It might fit nicely in WebCore/platform though.
Comment 10 David Levin 2012-01-13 13:01:16 PST
(In reply to comment #9)
> I have two concerns with this patch:
> 
> 1) It is missing a header guard.

Of course :(

> 2) I don't think WTF is the right layer for it. It might fit nicely in WebCore/platform though.

I'm fine with its 3rd move (OwnGetDC->HwndDC -> HWndDC -> platform/win/HWndDC). 

However, it feels odd to me that we have OwnPtr<HDC> in the wtf layer (which works for Create*DC) but avoid the thing that can own a GetDC result.
Comment 11 David Levin 2012-01-13 13:18:02 PST
(In reply to comment #10)
> (In reply to comment #9)
> > I have two concerns with this patch:
> > 
> > 1) It is missing a header guard.
> 
> Of course :(

Fixed http://trac.webkit.org/changeset/104976

Waiting on #2 based on your thoughts (and I might land a few other patches first and then do the file move since I already have them done).
Comment 12 Sam Weinig 2012-01-13 13:50:07 PST
(In reply to comment #10)
> (In reply to comment #9)
> > I have two concerns with this patch:
> > 
> > 1) It is missing a header guard.
> 
> Of course :(
> 
> > 2) I don't think WTF is the right layer for it. It might fit nicely in WebCore/platform though.
> 
> I'm fine with its 3rd move (OwnGetDC->HwndDC -> HWndDC -> platform/win/HWndDC). 
> 
> However, it feels odd to me that we have OwnPtr<HDC> in the wtf layer (which works for Create*DC) but avoid the thing that can own a GetDC result.

Memory management smart pointers have always had an odd relationship with WTF, nevertheless I still think this one should move up to platform/win.
Comment 13 David Levin 2012-01-13 15:25:39 PST
(In reply to comment #12) 
> Memory management smart pointers have always had an odd relationship with WTF, nevertheless I still think this one should move up to platform/win.

Patch: https://bugs.webkit.org/show_bug.cgi?id=76314
Comment 14 Darin Adler 2012-01-16 16:10:49 PST
(In reply to comment #10)
> However, it feels odd to me that we have OwnPtr<HDC> in the wtf layer

That is a mistake I made. I don’t think we should be using OwnPtr for things that aren't simple new/delete, but I couldn’t resist expanding it for various Windows handle types. I suppose there’s more to be said on the subject, but that’s the basics.