Bug 76290 - [chromium] Convert uses of GetDC to HWndDC.
Summary: [chromium] Convert uses of GetDC to HWndDC.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on:
Blocks: 76237
  Show dependency treegraph
 
Reported: 2012-01-13 10:58 PST by David Levin
Modified: 2012-01-23 16:23 PST (History)
4 users (show)

See Also:


Attachments
Patch (16.49 KB, patch)
2012-01-13 11:11 PST, David Levin
dimich: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2012-01-13 10:58:40 PST
See summary.
Comment 1 David Levin 2012-01-13 11:11:17 PST
Created attachment 122467 [details]
Patch
Comment 2 James Robinson 2012-01-13 12:30:21 PST
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?
Comment 3 David Levin 2012-01-13 12:48:07 PST
(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 :).
Comment 4 Dmitry Lomov 2012-01-13 14:27:38 PST
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...
Comment 5 James Robinson 2012-01-13 14:37:42 PST
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.
Comment 6 David Levin 2012-01-19 09:56:12 PST
Dmitry Titov, maybe you could review this?
Comment 7 Dmitry Titov 2012-01-23 16:00:33 PST
Comment on attachment 122467 [details]
Patch

I looked through those files, change looks correct. Delaying ReleaseDC() until scope close is ok for these cases.
Comment 8 David Levin 2012-01-23 16:23:26 PST
Committed as http://trac.webkit.org/changeset/105654