Bug 39473

Summary: [DRT/Chromium] Link resources and load Ahem font for Windows
Product: WebKit Reporter: Kent Tamura <tkent>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, rolandsteiner, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Windows Vista   
Bug Depends on:    
Bug Blocks: 35902    
Attachments:
Description Flags
Patch
none
Patch (rev.2)
none
Patch (rev.3) none

Kent Tamura
Reported 2010-05-20 23:34:12 PDT
[DRT/Chromium] Link resources and load Ahem font for Windows
Attachments
Patch (7.23 KB, patch)
2010-05-20 23:43 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Patch (rev.2) (7.90 KB, patch)
2010-05-21 02:09 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Patch (rev.3) (7.79 KB, patch)
2010-05-21 03:04 PDT, Kent Tamura 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Kent Tamura
Comment 1 2010-05-20 23:43:09 PDT
Created attachment 56677 [details] Patch
Tony Chang
Comment 2 2010-05-20 23:51:36 PDT
Comment on attachment 56677 [details] Patch > + // Load AHEM font. > + // The DRT executable file is placed at: > + // - src/chrome/{Debug,Release}/DumpRenderTree.exe or > + // - WebKit/WebKit/chromium/{Debug,Release}/DumpRenderTree.exe. > + // So, WebKit top directory is "../../third_party/WebKit" or "../../..". It might be easier to just copy AHEM____.TTF next to DumpRenderTree.exe with a gyp rule. It makes the code for finding it a lot simpler.
Kent Tamura
Comment 3 2010-05-21 01:37:35 PDT
(In reply to comment #2) > (From update of attachment 56677 [details]) > > + // Load AHEM font. > > + // The DRT executable file is placed at: > > + // - src/chrome/{Debug,Release}/DumpRenderTree.exe or > > + // - WebKit/WebKit/chromium/{Debug,Release}/DumpRenderTree.exe. > > + // So, WebKit top directory is "../../third_party/WebKit" or "../../..". > > It might be easier to just copy AHEM____.TTF next to DumpRenderTree.exe with a gyp rule. It makes the code for finding it a lot simpler. It's a good idea! Thanks.
Kent Tamura
Comment 4 2010-05-21 02:09:00 PDT
Created attachment 56683 [details] Patch (rev.2)
Tony Chang
Comment 5 2010-05-21 02:19:02 PDT
Comment on attachment 56683 [details] Patch (rev.2) The Apple win code just uses AddFontResourceEx instead of AddFontMemResourceEx. I don't have a preference between the two functions, but it might be less code. Anyway, I think this is fine too. LGTM with two style nits. > + ::PathRemoveFileSpec(modulePath); > + WCHAR ahemPath[_MAX_PATH]; > + struct _stat ahemStat; Nit: Drop struct since it's not needed in C++. > + DWORD numFonts = 1; > + HANDLE h = ::AddFontMemResourceEx(fontBuffer, size, 0, &numFonts); Nit: handle or fontHandle would be a more descriptive variable name.
Kent Tamura
Comment 6 2010-05-21 02:47:01 PDT
(In reply to comment #5) > (From update of attachment 56683 [details]) > The Apple win code just uses AddFontResourceEx instead of AddFontMemResourceEx. I don't have a preference between the two functions, but it might be less code. Anyway, I think this is fine too. LGTM with two style nits. I tried to use AddFontResourceEx(), but it failed on my Vista machine. I couldn't find the failure reasons and workarounds other than AddFontMemResourceEx(). (AddFontResourceEx() doesn't provide an error code!) > > + ::PathRemoveFileSpec(modulePath); > > + WCHAR ahemPath[_MAX_PATH]; > > + struct _stat ahemStat; > > Nit: Drop struct since it's not needed in C++. > > > + DWORD numFonts = 1; > > + HANDLE h = ::AddFontMemResourceEx(fontBuffer, size, 0, &numFonts); > > Nit: handle or fontHandle would be a more descriptive variable name. I'll fix them.
Kent Tamura
Comment 7 2010-05-21 03:04:49 PDT
Created attachment 56690 [details] Patch (rev.3)
Kent Tamura
Comment 8 2010-05-21 03:06:34 PDT
(In reply to comment #5) > > + WCHAR ahemPath[_MAX_PATH]; > > + struct _stat ahemStat; > > Nit: Drop struct since it's not needed in C++. Unfortunately, removing "struct" caused an error. Probably because _stat is a function name too. > > + HANDLE h = ::AddFontMemResourceEx(fontBuffer, size, 0, &numFonts); > > Nit: handle or fontHandle would be a more descriptive variable name. Done.
Dimitri Glazkov (Google)
Comment 9 2010-05-21 07:41:21 PDT
Comment on attachment 56690 [details] Patch (rev.3) ok.
Kent Tamura
Comment 10 2010-05-22 10:17:20 PDT
Comment on attachment 56690 [details] Patch (rev.3) Clearing flags on attachment: 56690 Committed r60005: <http://trac.webkit.org/changeset/60005>
Kent Tamura
Comment 11 2010-05-22 10:17:31 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.