Bug 117695 – [WinCairo] Webfont rendering is broken.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

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.

Created attachment 204821 [details]
Patch

[Brent Fulgham]

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!

[Brent Fulgham]

CC'ing mrobinson to double-check that there are no Gtk+ problems with this patch.

[Martin Robinson]

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. :)

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

Created attachment 204915 [details]
Patch

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

[Brent Fulgham]

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).

Created attachment 204973 [details]
Patch

(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 :)

[Brent Fulgham]

Comment on attachment 204973 [details]
Patch

Thank you for making those changes!  r=me.

[WebKit Commit Bot]

Comment on attachment 204973 [details]
Patch

Clearing flags on attachment: 204973

Committed r151743: <http://trac.webkit.org/changeset/151743>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.