Bug 76290 – [chromium] Convert uses of GetDC to HWndDC.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[David Levin]

See summary.

[David Levin]

Created attachment 122467 [details]
Patch

[James Robinson]

Comment on attachment 122467 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:-80
> -        ReleaseDC(0, dc);

i'm not totally sure about this change - it looks like this means the ReleaseDC() call will be delayed quite a bit - in this case it could be held across a synchronous IPC when previously it would not. Are you sure this is OK?

> Source/WebCore/rendering/RenderThemeChromiumWin.cpp:-187
> -                ReleaseDC(0, hdc);

similarly here you're changing the order of the ReleaseDC() and DeleteObject() calls. are you sure that's OK?

[David Levin]

(In reply to comment #2)
> (From update of attachment 122467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122467&action=review
> 
> > Source/WebCore/platform/graphics/chromium/GlyphPageTreeNodeChromiumWin.cpp:-80
> > -        ReleaseDC(0, dc);
> 
> i'm not totally sure about this change - it looks like this means the ReleaseDC() call will be delayed quite a bit - in this case it could be held across a synchronous IPC when previously it would not. Are you sure this is OK?


Why does that matter? This is the DC for the entire screen so it isn't window specific. It doesn't matter if we hold it a little longer. It is just important that it is deleted. I suspect the only reason that it was put here is to avoid having to put a ReleaseDC in each clause.

(fwiw, the gdi object limit is quite large if you aren't leaking them -- which is what inspired this whole change.)

> > Source/WebCore/rendering/RenderThemeChromiumWin.cpp:-187
> > -                ReleaseDC(0, hdc);
> 
> similarly here you're changing the order of the ReleaseDC() and DeleteObject() calls. are you sure that's OK?

Yes, the font and the dc have no relation at that point.

Here's where the two are mixed together and also separated due to the SelectObject.
                HGDIOBJ hObject = SelectObject(hdc, hFont);
                TEXTMETRIC tm;
                GetTextMetrics(hdc, &tm);
                SelectObject(hdc, hObject);
So at the point of DeleteObject and ReleaseDC for the font and dc, the two items are independent.

Ideally HFONT would be in a OwnPtr<HFONT> but that is a different issue and I didn't wish to boil the ocean completely :).

[Dmitry Lomov]

Comment on attachment 122467 [details]
Patch

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

Looks good. It is cool - weird that all used are for hWnd = 0 though....

> Source/WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:283
>      HGDIOBJ oldFont = static_cast<HFONT>(SelectObject(hdc, hfont));

Not in this patch it it will be awesome to have a RAII abstraction for SelectObject as well...

[James Robinson]

OK, sounds like you want a reviewer with a bit more knowledge of how these windows things work (not me). Anyone else please feel free to review this.

[David Levin]

Dmitry Titov, maybe you could review this?

[Dmitry Titov]

Comment on attachment 122467 [details]
Patch

I looked through those files, change looks correct. Delaying ReleaseDC() until scope close is ok for these cases.

[David Levin]

Committed as http://trac.webkit.org/changeset/105654