Bug 36190 - Lessen Include Path Build Fragility (FontPlatformData.h)
Summary: Lessen Include Path Build Fragility (FontPlatformData.h)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Critical
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-16 13:57 PDT by Brent Fulgham
Modified: 2010-03-18 14:21 PDT (History)
2 users (show)

See Also:


Attachments
Patch to disambiguate FontPlatformData.h includes. (16.28 KB, patch)
2010-03-16 14:11 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Address FontPlatformData.h include confusion by moving WinCairo declarations into platform/graphics/cairo version of file. (26.11 KB, patch)
2010-03-17 17:04 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Same as last revision, but with check-webkit-style fixes. (27.69 KB, patch)
2010-03-18 10:36 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Uploaded wrong version of diff. This is the correct file. (I.e., attachment 50975 with check-webkit-style fixes.) (27.35 KB, patch)
2010-03-18 10:38 PDT, Brent Fulgham
aroben: 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 2010-03-16 13:57:55 PDT
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.
Comment 1 Brent Fulgham 2010-03-16 14:11:27 PDT
Created attachment 50837 [details]
Patch to disambiguate FontPlatformData.h includes.
Comment 2 Adam Roben (:aroben) 2010-03-16 15:05:46 PDT
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.
Comment 3 Brent Fulgham 2010-03-17 17:04:20 PDT
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.
Comment 4 WebKit Review Bot 2010-03-17 17:08:59 PDT
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.
Comment 5 Brent Fulgham 2010-03-18 10:36:22 PDT
Created attachment 51044 [details]
Same as last revision, but with check-webkit-style fixes.

Minor whitespace changes to conform to check-webkit-style.
Comment 6 Brent Fulgham 2010-03-18 10:38:52 PDT
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.
Comment 7 WebKit Review Bot 2010-03-18 10:43:53 PDT
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.
Comment 8 Adam Roben (:aroben) 2010-03-18 11:29:22 PDT
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
Comment 9 Brent Fulgham 2010-03-18 14:21:05 PDT
Landed in http://trac.webkit.org/changeset/56192.