Bug 117695 - [WinCairo] Webfont rendering is broken.
Summary: [WinCairo] Webfont rendering is broken.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Unspecified
: P2 Normal
Assignee: Nobody
URL: http://www.cern.org
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-17 06:56 PDT by peavo
Modified: 2013-06-19 11:17 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.16 KB, patch)
2013-06-17 07:11 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (8.28 KB, patch)
2013-06-18 08:02 PDT, peavo
no flags Details | Formatted Diff | Diff
Patch (8.39 KB, patch)
2013-06-19 01:11 PDT, peavo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description peavo 2013-06-17 06:56:25 PDT
The native font handle created from webfont data is invalid.
In much the same way it's done in WinApple, we first need to install the webfont, create a native font handle from the new font name,
and pass the font handle to the FontPlatformData object, where it's needed to get the glyphs.
Comment 1 peavo 2013-06-17 07:11:11 PDT
Created attachment 204821 [details]
Patch
Comment 2 Brent Fulgham 2013-06-17 22:23:30 PDT
Comment on attachment 204821 [details]
Patch

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

I think this looks good, but I'd like Martin Robinson to double-check that we aren't doing something bad to the Gtk+ port.  I also had a few questions I'd like you to double-check for me before I r+ the patch. Thanks!

> Source/WebCore/platform/graphics/FontPlatformData.h:109
> +    FontPlatformData(HFONT, cairo_font_face_t*, float size, bool bold, bool italic);

It seems like this should break Gtk+, but the EWS says its okay... It seems like this should maybe be protected by USE(WINCAIRO).

> Source/WebCore/platform/graphics/win/FontCustomPlatformDataCairo.cpp:82
>  FontCustomPlatformData* createFontCustomPlatformData(SharedBuffer* buffer)

Should this be declared static now? It's not used outside this module, is it?

> Source/WebCore/platform/graphics/win/FontCustomPlatformDataCairo.h:-49
> -    cairo_font_face_t* m_fontFace;

Is there no benefit to holding the cairo_font_fact_t*?  Is it needed anywhere else?

> Source/WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp:60
> +    : m_font(RefCountedGDIHandle<HFONT>::create(font))

Good!
Comment 3 Brent Fulgham 2013-06-17 22:24:10 PDT
CC'ing mrobinson to double-check that there are no Gtk+ problems with this patch.
Comment 4 Martin Robinson 2013-06-17 22:44:32 PDT
Comment on attachment 204821 [details]
Patch

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

>> Source/WebCore/platform/graphics/FontPlatformData.h:109
>> +    FontPlatformData(HFONT, cairo_font_face_t*, float size, bool bold, bool italic);
> 
> It seems like this should break Gtk+, but the EWS says its okay... It seems like this should maybe be protected by USE(WINCAIRO).

I think that perhaps GTK+ and EFL are not including this file, but are instead including WebKit/Source/WebCore/platform/graphics/freetype/FontPlatformData.h. Looks good to me. :)
Comment 5 peavo 2013-06-18 07:30:10 PDT
(In reply to comment #2)

Thanks for reviewing, Brent!

> > Source/WebCore/platform/graphics/win/FontCustomPlatformDataCairo.cpp:82
> >  FontCustomPlatformData* createFontCustomPlatformData(SharedBuffer* buffer)
> 
> Should this be declared static now? It's not used outside this module, is it?

Yes, it's used by WebCore\loader\cache\CachedFont.cpp.

> > Source/WebCore/platform/graphics/win/FontCustomPlatformDataCairo.h:-49
> > -    cairo_font_face_t* m_fontFace;
> 
> Is there no benefit to holding the cairo_font_fact_t*?  Is it needed anywhere else?

I don't think it's currently needed elsewhere (no compile errors). The FontCustomPlatformData object is only used to create a FontPlatformData object via the fontPlatformData() method, and the cairo_font_face_t pointer is only used to create a cairo_scaled_font_t pointer in the FontPlatformData constructor, so I believe it can be a temporary variable.
Comment 6 peavo 2013-06-18 08:02:01 PDT
Created attachment 204915 [details]
Patch
Comment 7 peavo 2013-06-18 08:03:28 PDT
(In reply to comment #6)
> Created an attachment (id=204915) [details]
> Patch

Modified patch according to review, moved windows specific constructor into PLATFORM(WIN) section.
Comment 8 Brent Fulgham 2013-06-18 09:15:50 PDT
Comment on attachment 204915 [details]
Patch

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

This patch looks great.  Could you just make the "#elif" change I asked for when you land the change?

> Source/WebCore/platform/graphics/FontPlatformData.h:107
>  #if USE(CAIRO)

nit: This should probably have been an "#elif USE(CAIRO)", since we never use both graphics packages together (they are exclusive).
Comment 9 peavo 2013-06-19 01:11:15 PDT
Created attachment 204973 [details]
Patch
Comment 10 peavo 2013-06-19 01:21:03 PDT
(In reply to comment #8)

> nit: This should probably have been an "#elif USE(CAIRO)", since we never use both graphics packages together (they are exclusive).

Changed the patch to use "#elif USE(CAIRO)". If the patch is ok, could you also commit+ it, I don't have committer rights :)
Comment 11 Brent Fulgham 2013-06-19 10:53:54 PDT
Comment on attachment 204973 [details]
Patch

Thank you for making those changes!  r=me.
Comment 12 WebKit Commit Bot 2013-06-19 11:17:49 PDT
Comment on attachment 204973 [details]
Patch

Clearing flags on attachment: 204973

Committed r151743: <http://trac.webkit.org/changeset/151743>
Comment 13 WebKit Commit Bot 2013-06-19 11:17:51 PDT
All reviewed patches have been landed.  Closing bug.