RESOLVED FIXED 36190
Lessen Include Path Build Fragility (FontPlatformData.h)
https://bugs.webkit.org/show_bug.cgi?id=36190
Summary Lessen Include Path Build Fragility (FontPlatformData.h)
Brent Fulgham
Reported 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.
Attachments
Patch to disambiguate FontPlatformData.h includes. (16.28 KB, patch)
2010-03-16 14:11 PDT, Brent Fulgham
no flags
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
Same as last revision, but with check-webkit-style fixes. (27.69 KB, patch)
2010-03-18 10:36 PDT, Brent Fulgham
no flags
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+
Brent Fulgham
Comment 1 2010-03-16 14:11:27 PDT
Created attachment 50837 [details] Patch to disambiguate FontPlatformData.h includes.
Adam Roben (:aroben)
Comment 2 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.
Brent Fulgham
Comment 3 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.
WebKit Review Bot
Comment 4 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.
Brent Fulgham
Comment 5 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.
Brent Fulgham
Comment 6 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.
WebKit Review Bot
Comment 7 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.
Adam Roben (:aroben)
Comment 8 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
Brent Fulgham
Comment 9 2010-03-18 14:21:05 PDT
Note You need to log in before you can comment on or make changes to this bug.