Bug 39473 – [DRT/Chromium] Link resources and load Ahem font for Windows

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kent Tamura]

[DRT/Chromium] Link resources and load Ahem font for Windows

[Kent Tamura]

Created attachment 56677 [details]
Patch

[Tony Chang]

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]

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

Created attachment 56683 [details]
Patch (rev.2)

[Tony Chang]

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]

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

Created attachment 56690 [details]
Patch (rev.3)

[Kent Tamura]

(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 on attachment 56690 [details]
Patch (rev.3)

ok.

[Kent Tamura]

Comment on attachment 56690 [details]
Patch (rev.3)

Clearing flags on attachment: 56690

Committed r60005: <http://trac.webkit.org/changeset/60005>

[Kent Tamura]

All reviewed patches have been landed.  Closing bug.