Bug 36190 – Lessen Include Path Build Fragility (FontPlatformData.h)

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

The use of the common file name "FontPlatformData.h" across multiple platforms means that the ordering of include paths determines which version of this file is used during a build.

A recent source change (http://trac.webkit.org/changeset/55510) broke the WinCairo build because it moved the Gtk-specific version of FontPlatformData.h into the platform/graphics/cairo directory, which is now used preferentially to the historical version in platform/graphics/win.

After discussing the issue with Adam Roben, we decided that the best long-term solution is to rename this header file 'FontPlatformDataWin.h', to disambiguate the file intended for the build.

The attached patch makes this proposed change.

[Brent Fulgham]

Created attachment 50837 [details]
Patch to disambiguate FontPlatformData.h includes.

[Adam Roben (:aroben)]

Comment on attachment 50837 [details]
Patch to disambiguate FontPlatformData.h includes.

My biggest complaint with this approach is that the next time someone adds #include "FontPlatformData.h" to a file, they will break Apple's Windows build. We need to find some way to avoid that.

[Brent Fulgham]

Created attachment 50975 [details]
Address FontPlatformData.h include confusion by moving WinCairo declarations into platform/graphics/cairo version of file.

Move WinCairo implementation to platform/graphics/cairo; move cg-specific FontPlatformData.h.  Remove cairo-specific stuff from the cg/FontPlatformData.h file.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp:135:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp:136:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp:137:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp:138:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/graphics/win/FontPlatformDataCairoWin.cpp:139:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WARNING: Could not read file. Skipping: "WebCore/platform/graphics/win/FontPlatformData.h"
WebCore/platform/graphics/cairo/FontPlatformData.h:41:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/win/RefCountedHFONT.h:23:  #ifndef header guard has wrong style, please use: RefCountedHFONT_h  [build/header_guard] [5]
WebCore/platform/graphics/win/RefCountedHFONT.h:36:  More than one command on the same line in if  [whitespace/parens] [4]
WebCore/platform/graphics/cg/FontPlatformData.h:24:  #ifndef header guard has wrong style, please use: FontPlatformData_h  [build/header_guard] [5]
WebCore/platform/graphics/cg/FontPlatformData.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/cg/FontPlatformData.h:76:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/graphics/cg/FontPlatformData.h:77:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/graphics/cg/FontPlatformData.h:78:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/graphics/cg/FontPlatformData.h:79:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 14 in 8 files


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

[Brent Fulgham]

Created attachment 51044 [details]
Same as last revision, but with check-webkit-style fixes.

Minor whitespace changes to conform to check-webkit-style.

[Brent Fulgham]

Created attachment 51045 [details]
Uploaded wrong version of diff.  This is the correct file.  (I.e., attachment 50975 [details] with check-webkit-style fixes.)

Corrects check-webkit-style warnings.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WARNING: Could not read file. Skipping: "WebCore/platform/graphics/win/FontPlatformData.h"
WebCore/platform/graphics/cairo/FontPlatformData.h:41:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 8 files


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

[Adam Roben (:aroben)]

Comment on attachment 51045 [details]
Uploaded wrong version of diff.  This is the correct file.  (I.e., attachment 50975 [details] with check-webkit-style fixes.)

> -    bool isHashTableDeletedValue() const {
> +    bool isHashTableDeletedValue() const
> +    {
>  #if defined(USE_FREETYPE)
>          return m_pattern == hashTableDeletedFontValue();
>  #elif defined(USE_PANGO)
>          return m_font == hashTableDeletedFontValue();
> +#elif PLATFORM(WIN)
> +        return m_font.isHashTableDeletedValue();
>  #endif
>      };
>  
> +
>  #ifndef NDEBUG
>      String description() const;
>  #endif

Too much whitespace in there. (And the semicolon after the closing brace isn't needed.)

> @@ -122,6 +161,13 @@ public:
>  
>      PangoContext* m_context;
>      PangoFont* m_font;
> +#elif PLATFORM(WIN)
> +private:
> +    void platformDataInit(HFONT font, float size, HDC hdc, WCHAR* faceName);

You could remove the "font" and "hdc" parameter names here.

> Index: WebCore/platform/graphics/cg/FontPlatformData.h

Having this Windows-specific file in the cg/ directory seems a little confusing. Mac uses CG, too (and many files in the platform/graphics/cg directory), for example. But I guess it's OK for now.

> +++ WebCore/platform/graphics/win/RefCountedHFONT.h	(revision 0)
> @@ -0,0 +1,59 @@
> +/*
> + * This file is part of the internal font implementation.  It should not be included by anyone other than
> + * FontCairo.cpp, FontMac.cpp, FontWin.cpp and Font.cpp.

I don't think this comment is needed anymore.

r=me

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/56192.