Bug 76738

Summary: Make OwnPtr<HDC> work for the Chromium Windows port.
Product: WebKit Reporter: David Levin <levin>
Component: New BugsAssignee: David Levin <levin>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, bfulgham, dimich, dslomov, jianli
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 77073    
Attachments:
Description Flags
Patch jianli: review+

David Levin
Reported 2012-01-20 14:22:37 PST
Make OwnPtr<HDC> work for the Chromium Windows port.
Attachments
Patch (2.17 KB, patch)
2012-01-20 14:23 PST, David Levin 		jianli: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
David Levin
Comment 1 2012-01-20 14:23:56 PST
Created attachment 123379 [details] Patch
Dmitry Lomov
Comment 2 2012-01-20 16:09:22 PST
Comment on attachment 123379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123379&action=review Looks good! > Source/JavaScriptCore/ChangeLog:8 > + * JavaScriptCore.gyp/JavaScriptCore.gyp: Added OwPptrWin to the typo
Jian Li
Comment 3 2012-01-20 16:30:36 PST
Comment on attachment 123379 [details] Patch Looks good. Please fix the typo pointed out by dslomov before committing.
David Levin
Comment 4 2012-01-20 16:38:14 PST
Committed as http://trac.webkit.org/changeset/105555
Andy Estes
Comment 5 2012-01-25 23:52:40 PST
This broke Apple's Windows port in a subtle way. We define OS(WINDOWS) but not OS(WIN), so the deleteOwnPtr() overloads in OwnPtrCommon.h were stripped by the preprocessor, causing Windows-allocated resources to be deleted by fastmalloc. What's the right fix here? We should agree on either OS(WIN) or OS(WINDOWS).
Andy Estes
Comment 6 2012-01-26 00:11:06 PST
In fact, it looks like this patch was just wrong. Platform.h defines WTF_OS_WINDOWS, so this patch should have had OS(WINDOWS), not OS(WIN). I'll fix this in <https://bugs.webkit.org/show_bug.cgi?id=77073>.
David Levin
Comment 7 2012-01-26 05:13:24 PST
(In reply to comment #6) > In fact, it looks like this patch was just wrong. Platform.h defines WTF_OS_WINDOWS, so this patch should have had OS(WINDOWS), not OS(WIN). I'll fix this in <https://bugs.webkit.org/show_bug.cgi?id=77073>. Thank you very much.
Brent Fulgham
Comment 8 2012-01-26 12:34:18 PST
This change breaks the WinCairo build. There is no such thing as OS(WIN) (see <JavaScriptCore/Platform.h>) This should have been coded as OS(WINDOWS).
David Levin
Comment 9 2012-01-26 12:37:48 PST
(In reply to comment #8) > This change breaks the WinCairo build. > > There is no such thing as OS(WIN) (see <JavaScriptCore/Platform.h>) > > This should have been coded as OS(WINDOWS). Yes. It has been fixed. See comment #6.
Brent Fulgham
Comment 10 2012-01-26 13:33:28 PST
Excellent. I just got the change -- thanks for tackling it so quickly :-)
Note You need to log in before you can comment on or make changes to this bug.