Bug 55981 – Clean Up FontPlatformData Structure and Handling

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

[Brent Fulgham]

The existing FontPlatformData class is overly complicated.  Each port has its own (often incompatible) version, with inconsistent members and life cycle.

In fact, as Adam Roben pointed out previously, the main problem is that the FontPlatformData type provides the wrong layer of abstraction. It has morphed into an object type that attempts to encapsulate both
the underlying operating system's concept of a font/glyph (e.g., GDI, FreeType, Pango, etc.) and the mechanism used to render the font to screen (e.g., Cairo, CoreGraphics, Skia, etc.).  The various ports
have managed to cobble together a system that works, but in the process have created a Byzantine structure full of duplicated file names (FontPlatformData.h, FontCustomPlatformData.h, etc. for each platform) that generally expose incompatible API's and data structures.

This proposed change would separate the rendering process from the underlying font representation.  A Windows application using GDI fonts with a cairo rendering layer should largely share the same underlying font representation as CoreGraphics.  Similarly, all ports built on top of Cairo could use different font representations (e.g., GDI, Pango, FreeType) but still pipe the drawing logic through the Cairo rendering layer.

[Brent Fulgham]

Created attachment 85188 [details]
Initial idea for unifying FontPlatformData.h

As a first step, let's consolidate things into a single FontPlatformData.h file, analogous to the GraphicsContext class.  Later we should try to separate out the rendering and font aspects of this object.

[Brent Fulgham]

Maybe stuff like this could be a template:
#if OS(DARWIN)
    static NSFont* hashTableDeletedFontValue() { return reinterpret_cast<NSFont *>(-1); }
#elif PLATFORM(GTK)
    static PangoFont* hashTableDeletedFontValue() { return reinterpret_cast<PangoFont*>(-1); }
#endif

template <typename T>
static T* hashTableDeletedFontValue () { return reinterpret_cast<T*>(-1); }

[Brent Fulgham]

Question: Is BUILDING_ON_TIGER ever defined for Windows CG builds?  We could simplify some of the #if/def constructs if this case can be ignored.

[Brent Fulgham]

Created attachment 85256 [details]
Revamped version of header file, which compiles on Windows and Mac OS.

Revamped version of header file, which compiles on Windows and Mac OS.  Linux build will almost certainly break with this change; Chromium, Haiku, etc., are untried.

[Brent Fulgham]

Created attachment 85415 [details]
Full patch for Windows/Cocoa testing.

This patch builds and runs properly on Windows (CG and WinCairo).  WebCore builds on Cocoa, but it seems like the WebKit project doesn't know where to find 'config.h' for some reason.

This will almost certainly fail for GTK and others.  By flagging this for review I'm hoping the EWS bots will help me see what's wrong...

[WebKit Review Bot]

Attachment 85415 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/FontPlatformData.h:28:  Header file should not contain WebCore config.h. Should be: alphabetically sorted.  [build/include_order] [4]
Source/WebCore/platform/graphics/FontPlatformData.h:184:  The parameter name "orientation" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/FontPlatformData.h:185:  The parameter name "widthVariant" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/FontPlatformData.h:207:  The parameter name "fontFace" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/FontPlatformData.h:221:  The parameter name "font" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/FontPlatformData.h:265:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/platform/graphics/FontPlatformData.h:312:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/graphics/FontPlatformData.h:316:  The parameter name "nsFont" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/FontPlatformData.h:316:  The parameter name "cgFont" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 9 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Attachment 85415 [details] did not build on win:
Build output: http://queues.webkit.org/results/8133124

[Brent Fulgham]

Created attachment 85540 [details]
Adjusted with some of dhyatt's comments.

This version moves some common code to a new FontPlatformData.cpp file, and implements custom initialization routines in specific implementation classes.

[Martin Robinson]

Comment on attachment 85540 [details]
Adjusted with some of dhyatt's comments.

View in context: https://bugs.webkit.org/attachment.cgi?id=85540&action=review

I like where this is headed. :)

> Source/WebCore/platform/graphics/FontPlatformData.h:275
> +        return m_font == other.m_font

This does make an assumption that m_font is a pointer that will only be equal to other.m_font when they point to the exact same data. At least on the Freetype backend this will change the behavior of this operator overload. Perhaps this could become a platformDataIsEqual method. That would allow you to remove the #ifdefs below as well.

[Brent Fulgham]

Created attachment 85559 [details]
Correct build failure (WinCairo)

Updated with dhyatt's comments, tested on Windows builds (CG/Cairo).

[WebKit Review Bot]

Attachment 85559 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/FontPlatformData.cpp:30:  Use 'using namespace std;' instead of 'using std::min;'.  [build/using_std] [4]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Attachment 85559 [details] did not build on win:
Build output: http://queues.webkit.org/results/8149524

[Brent Fulgham]

Created attachment 85954 [details]
Update to build cleanly on Windows/WinCairo/Cocoa

[Brent Fulgham]

Comment on attachment 85954 [details]
Update to build cleanly on Windows/WinCairo/Cocoa

This also corrects the style warnings.

[Brent Fulgham]

I'd like to land the current patch as a first step in cleaning up the FontPlatformData (and other) items.  The first goal would be to get everyone sharing a common header file, then clean up the implementations to reduce duplication.

[Dave Hyatt]

Comment on attachment 85954 [details]
Update to build cleanly on Windows/WinCairo/Cocoa

r=me

[Brent Fulgham]

Created attachment 86995 [details]
Second step of FontPlatformData refactoring: GTK support

This patch provides GTK support, and unifies the WinCairo and GTK Cairo handling to reduce duplication.

[WebKit Review Bot]

Attachment 86995 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/FontPlatformData.h:252:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Source/WebCore/platform/graphics/win/FontCustomPlatformDataCairo.cpp:28:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp:88:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 5 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Created attachment 86998 [details]
Second step of FontPlatformData refactoring: GTK support (Style fixes)

Small update to get rid of some windows cr/lf pairs caused by some copy/pasting.

[Patrick R. Gansterer]

I like this changes, but PLEASE add bug number to your patch and add a "Committed r81977" comment here at the bug. ("webkit-patch land" does this for you without extra work)

r81977 broke many ports (at least EFL, WinCE and Wx) and it's very hard to find the context without bug number. :-(

If you do such ugly hacks (where a incremental build won't show build errors) at least search for "FontPlatformData.h" in _ALL_ directories. THX!

Here some build fixes:
http://trac.webkit.org/changeset/82018
http://trac.webkit.org/changeset/82027

[Brent Fulgham]

Created attachment 87390 [details]
Second step of FontPlatformData refactoring: GTK support (Style fixes), rebased to 82299.

[Martin Robinson]

Comment on attachment 86998 [details]
Second step of FontPlatformData refactoring: GTK support (Style fixes)

These changes great to me, but I don't think I can flip the r? switch since I contributed some code to this patch.

[Dave Hyatt]

Comment on attachment 87390 [details]
Second step of FontPlatformData refactoring: GTK support (Style fixes), rebased to 82299.

r=me

[Brent Fulgham]

Landed Second Step in http://trac.webkit.org/changeset/82314.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82314 might have broken Windows 7 Release (Tests)

[Eric Seidel (no email)]

Comment on attachment 85954 [details]
Update to build cleanly on Windows/WinCairo/Cocoa

Cleared David Hyatt's review+ from obsolete attachment 85954 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 87390 [details]
Second step of FontPlatformData refactoring: GTK support (Style fixes), rebased to 82299.

Cleared David Hyatt's review+ from obsolete attachment 87390 [details] so that this bug does not appear in http://webkit.org/pending-commit.