Bug 17336 – Update WinCairo WebKit with Font/Text Support

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

[Brent Fulgham]

The attached patch updates the WebKit sources to support proper rendering of text using the Cairo back-end.

[Brent Fulgham]

Created attachment 19102 [details]
Patch to add font support to Cairo/Win backend.

[Alp Toker]

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

"Uninitialised variable, this will break in release builds."

Incorrect.  Statics inside functions in C++ are initialized to 0.

[Alp Toker]

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

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

Created attachment 19118 [details]
Revised patch based on alp's comments

Update against today's SVN and incorporating Alp's suggestions.

[Brent Fulgham]

Created attachment 19199 [details]
Second revision based on Alp and others comments.

Revised to increase code sharing between CG/Cairo.

[Brent Fulgham]

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

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]

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.

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]

Landed in <http://trac.webkit.org/projects/webkit/changeset/30441>.