Bug 17336 - Update WinCairo WebKit with Font/Text Support
Summary: Update WinCairo WebKit with Font/Text Support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-02-12 19:53 PST by Brent Fulgham
Modified: 2008-02-20 18:14 PST (History)
5 users (show)

See Also:


Attachments
Patch to add font support to Cairo/Win backend. (63.21 KB, patch)
2008-02-12 20:40 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Revised patch based on alp's comments (63.91 KB, patch)
2008-02-13 19:40 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Second revision based on Alp and others comments. (60.97 KB, patch)
2008-02-18 17:40 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Third revision based on dhyatt and mitzpettel's comments (59.17 KB, patch)
2008-02-20 10:44 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Fourth revision based on mitzpettel's comments (60.36 KB, patch)
2008-02-20 12:51 PST, Brent Fulgham
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 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.
Comment 1 Brent Fulgham 2008-02-12 20:40:51 PST
Created attachment 19102 [details]
Patch to add font support to Cairo/Win backend.
Comment 2 Alp Toker 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.
Comment 3 Dave Hyatt 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.

Comment 4 Alp Toker 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.
Comment 5 Brent Fulgham 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.
Comment 6 Brent Fulgham 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.
Comment 7 Brent Fulgham 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.
Comment 8 Brent Fulgham 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."
Comment 9 mitz 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().
Comment 10 Brent Fulgham 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.
Comment 11 mitz 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
Comment 12 Matt Lilek 2008-02-20 18:14:28 PST
Landed in <http://trac.webkit.org/projects/webkit/changeset/30441>.