Bug 17336

Summary: Update WinCairo WebKit with Font/Text Support
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: alp, aroben, cedricv, hyatt, mitz
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
Patch to add font support to Cairo/Win backend.
none
Revised patch based on alp's comments
none
Second revision based on Alp and others comments.
none
Third revision based on dhyatt and mitzpettel's comments
none
Fourth revision based on mitzpettel's comments mitz: review+

Brent Fulgham
Reported 2008-02-12 19:53:05 PST
The attached patch updates the WebKit sources to support proper rendering of text using the Cairo back-end.
Attachments
Patch to add font support to Cairo/Win backend. (63.21 KB, patch)
2008-02-12 20:40 PST, Brent Fulgham
no flags
Revised patch based on alp's comments (63.91 KB, patch)
2008-02-13 19:40 PST, Brent Fulgham
no flags
Second revision based on Alp and others comments. (60.97 KB, patch)
2008-02-18 17:40 PST, Brent Fulgham
no flags
Third revision based on dhyatt and mitzpettel's comments (59.17 KB, patch)
2008-02-20 10:44 PST, Brent Fulgham
no flags
Fourth revision based on mitzpettel's comments (60.36 KB, patch)
2008-02-20 12:51 PST, Brent Fulgham
mitz: review+
Brent Fulgham
Comment 1 2008-02-12 20:40:51 PST
Created attachment 19102 [details] Patch to add font support to Cairo/Win backend.
Alp Toker
Comment 2 2008-02-13 10:44:00 PST
(In reply to comment #1) > Created an attachment (id=19102) [edit] > Patch to add font support to Cairo/Win backend. > I didn't have the chance to look over this fully yet. Some things I spotted: + static cairo_font_options_t* fontOptions; + if (!fontOptions) + fontOptions = cairo_font_options_create(); ^ Uninitialised variable, this will break in release builds. You should be fine with just: static cairo_font_options_t* fontOptions = cairo_font_options_create(); or you could keep the logic (it'll become useful later when Win implements central font options) and assign 0. +#if PLATFORM(CG) CGContextSaveGState(pctx); IntRect printRect = printerRect(printDC); @@ -2740,6 +2741,9 @@ HRESULT STDMETHODCALLTYPE WebFrame::spoo CGContextEndPage(pctx); CGContextRestoreGState(pctx); +#else + notImplemented(); +#endif ^ I'm already working on printing abstraction, so it's good that you just stubbed it out and didn't try to implement it. The rename of cgFont() to platformFont() is a clever shortcut, I like that :-) +#if PLATFORM(CG) typedef struct CGColor* CGColorRef; -typedef struct CGContext* CGContextRef; +typedef struct CGContext* ContextRef; +typedef const CGColorRef ColorRef; +#elif PLATFORM(CAIRO) +#include "GraphicsContext.h" +typedef WebCore::GraphicsContext* ContextRef; +typedef const WebCore::Color* ColorRef; +#endif ^ I'd just do #else here, though it doesn't matter much. I think there are a few more worthwhile code sharing opportunities here before, but didn't study the split closely enough to be sure yet.
Dave Hyatt
Comment 3 2008-02-13 10:50:49 PST
"Uninitialised variable, this will break in release builds." Incorrect. Statics inside functions in C++ are initialized to 0.
Alp Toker
Comment 4 2008-02-13 11:09:06 PST
(In reply to comment #3) > "Uninitialised variable, this will break in release builds." > > Incorrect. Statics inside functions in C++ are initialized to 0. > Technically correct but code like this gets moved around and might well end up in a pure C file some day. I'm pretty sure gcc will not zero stack-allocated pointers on some platforms unless you explicitly enable C99. I wouldn't write new code that relies on this feature.
Brent Fulgham
Comment 5 2008-02-13 11:37:06 PST
(In reply to comment #2) > Some things I spotted: > > + static cairo_font_options_t* fontOptions; > + if (!fontOptions) > + fontOptions = cairo_font_options_create(); > > ^ Uninitialised variable, this will break in release builds. > You should be fine with just: > > static cairo_font_options_t* fontOptions = cairo_font_options_create(); Oh -- I didn't even notice I had done this. I'll fix this immediately.
Brent Fulgham
Comment 6 2008-02-13 19:40:49 PST
Created attachment 19118 [details] Revised patch based on alp's comments Update against today's SVN and incorporating Alp's suggestions.
Brent Fulgham
Comment 7 2008-02-18 17:40:20 PST
Created attachment 19199 [details] Second revision based on Alp and others comments. Revised to increase code sharing between CG/Cairo.
Brent Fulgham
Comment 8 2008-02-20 10:44:05 PST
Created attachment 19231 [details] Third revision based on dhyatt and mitzpettel's comments * Switched from "platformFont" to explicit "cgFont" and "fontFace" members * Removed use of "platformPrivateInit/Destroy/etc." and switched to more descriptive names (initGDIFont/platformCommonDestroy/etc."
mitz
Comment 9 2008-02-20 11:44:33 PST
Comment on attachment 19231 [details] Third revision based on dhyatt and mitzpettel's comments + bool fontCreationFailed = (result->cgFont() == 0); This should just say + bool fontCreationFailed = !result->cgFont(); (similarly for the Cairo version). +#include <wtf/RetainPtr.h> should come last in FontCustomPlatformDataCairo.cpp. + FontCustomPlatformDataCairo(cairo_font_face_t* fontFace) + : m_fontFace(fontFace) + {} The initialization should be indented and the braces go on separate lines. +FontCustomPlatformDataCairo* createFontCustomPlatformData(SharedBuffer* buffer); No need to name the argument. + static cairo_font_options_t* fontOptions = 0; + if (!fontOptions) + fontOptions = cairo_font_options_create(); + cairo_font_options_set_antialias(fontOptions, CAIRO_ANTIALIAS_SUBPIXEL); No need to initialize the static variable to 0. But more importantly, Darin pointed out that you probably meant to include cairo_font_options_set_antialias() in the if block so that you only do it once. SimpleFontData::platformDestroy() in SimpleFontDataCairoWin.cpp should call platformCommonDestroy().
Brent Fulgham
Comment 10 2008-02-20 12:51:48 PST
Created attachment 19234 [details] Fourth revision based on mitzpettel's comments * Corrects formatting * SimpleFontData::platformDestroy() in SimpleFontDataCairoWin.cpp now calls platformCommonDestroy(). * cairo_font_options_set_antialias() now only called once during the initialization.
mitz
Comment 11 2008-02-20 14:39:33 PST
Comment on attachment 19234 [details] Fourth revision based on mitzpettel's comments r=me Please remove this line when landing: + WARNING: NO TEST CASES ADDED OR CHANGED
Matt Lilek
Comment 12 2008-02-20 18:14:28 PST
Note You need to log in before you can comment on or make changes to this bug.