Summary: | [WinCairo] Webfont rendering is broken. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | peavo | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | bfulgham, commit-queue, mrobinson | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Unspecified | ||||||||||
URL: | http://www.cern.org | ||||||||||
Attachments: |
|
Description
peavo
2013-06-17 06:56:25 PDT
Created attachment 204821 [details]
Patch
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! CC'ing mrobinson to double-check that there are no Gtk+ problems with this patch. 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. 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 :) Comment on attachment 204973 [details]
Patch
Thank you for making those changes! r=me.
Comment on attachment 204973 [details] Patch Clearing flags on attachment: 204973 Committed r151743: <http://trac.webkit.org/changeset/151743> All reviewed patches have been landed. Closing bug. |