Bug 76235 – HwndDC is a better name than OwnGetDC.

• Component: Web Template Framework
• Status: RESOLVED FIXED

[David Levin]

Based on feedback in https://bugs.webkit.org/show_bug.cgi?id=76227#c2 and subsequent discussion.

[David Levin]

Created attachment 122349 [details]
Patch

[Dmitry Lomov]

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.

[David Levin]

Created attachment 122355 [details]
Patch

[Dmitry Titov]

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()

[WebKit Review Bot]

Comment on attachment 122355 [details]
Patch

Clearing flags on attachment: 122355

Committed r104895: <http://trac.webkit.org/changeset/104895>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Since HWND is “handle to window”, I think it should be HWnd, when things are mixed case, not Hwnd.

[David Levin]

(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

[Sam Weinig]

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.

[David Levin]

(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.

[David Levin]

(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).

[Sam Weinig]

(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.

[David Levin]

(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

[Darin Adler]

(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.