Subframeworks move around from SDK to SDK and OS release to OS release, but just using the top-level umbrella framework guarantees forward compatibility. Because of an error in the Chromium build files that leaked into WebKit, some code was reaching into ApplicationServices.framework to #include things in subframeworks. In order to fix the build file leak in Chromium and stop doing this, the offenders in WebKit need to be fixed.
Created attachment 146362 [details] Don’t reach into subframeworks, just #include <ApplicationServices/ApplicationServices.h>
Created attachment 146363 [details] Updated to current trunk (r119733)
Comment on attachment 146363 [details] Updated to current trunk (r119733) View in context: https://bugs.webkit.org/attachment.cgi?id=146363&action=review > WebCore/platform/graphics/GlyphBuffer.h:42 > +#if (PLATFORM(WX) && OS(DARWIN)) || USE(SKIA_ON_MAC_CHROMIUM) Not your fault, but I think SKIA_ON_MAC_CHROMIUM is always on by default now, so this could be something like #if (OS(DARWIN) && (PLATFORM(WX) || PLATFORM(CHROMIUM))) Cary, could you verify? Should we keep these around or just nuke them?
Comment on attachment 146363 [details] Updated to current trunk (r119733) For files that are shared with Apple (e.g., WebCore/platform/graphics/mac/ComplexTextControllerCoreText.mm) this would make things less stable, not more so, since edits to the Apple side of the #include list would not be reflected in the Chromium build. Is it permissible to replace the Apple side as well?
I don’t know what you mean by “replace the Apple side.” I was trying to scope my patch to be a minimal-change one that only touched Chromium. In the general sense, #include <ApplicationServices/ApplicationServices.h> is never a problem, although in newer SDKs, CoreGraphics.framework has been promoted to be a top-level framework instead of being a subframework under ApplicationServices.framework. I’m not sure what Apple’s requirements for SDK use are, but they (and other WebKit ports on the Mac) may have reasons to prefer including things of the form <CoreGraphics/xxx.h>. In Chromium’s case, we still need to operate with the old 10.5 SDK, and will soon switch to the 10.6 SDK, in which CoreGraphics.framework is still a subframework, and we’re supposed to disallow subframework includes of this form. I don’t know exactly when Chrome’s compiler flag leak into WebKit was introduced, but it probably happened when we switched to using Skia, which means that this change merely restores the old (correct) state of affairs, where Chrome code (including WebKit built for Chrome) couldn’t reach into subframeworks of ApplicationServices.framework.
Comment on attachment 146363 [details] Updated to current trunk (r119733) What I mean by 'the Apple side' is #if USE(CG). Sorry for the confusion. What I was attempting to say that if, in the future, the USE(CG) bracketed code in GlyphBuffer.h line 38 or the code following the #else in ComplexTextControllerCoreText.mm line 38 is edited in a way that changes the behavior of the rest of the file, the Chromium port won't benefit from that change. If this is not a concern, then feel free to ignore. To address Stephen's question, yes, USE(SKIA_ON_MAC_CHROMIUM) should be deprecated and replaced with PLATFORM(CHROMIUM) && OS(DARWIN). The code base with CG+Chromium doesn't compile and isn't maintained.
Created attachment 147097 [details] Don’t use SKIA_ON_MAC_CHROMIUM I don’t think that the divergent paths for #includes are worth worrying about. <ApplicationServices/ApplicationServices.h> brings in both <CoreGraphics/CGGeometry.h> and <CoreText/CoreText.h>, and more. I’m not sure if that change is desirable for the Apple CG port, which appears content (on the basis of the existing code) to reach into subframeworks in this manner, and both CoreGraphics and CoreText have been promoted to top-level frameworks in SDKs newer than what Chrome can build with. This version of the patch assumes that OS(DARWIN) && PLATFORM(CHROMIUM) is always Skia, and doesn’t use SKIA_ON_MAC_CHROMIUM at all.
Comment on attachment 147097 [details] Don’t use SKIA_ON_MAC_CHROMIUM lgtm
Comment on attachment 147097 [details] Don’t use SKIA_ON_MAC_CHROMIUM The patch is not applying because it's diffed at Source, rather than at the webkit checkout root. Please rebuild the patch and re-upload so we can get some EWS coverage.
Created attachment 147134 [details] Patch rooted above Source
Comment on attachment 147134 [details] Patch rooted above Source Thanks, this one seems to apply on the EWS.
Comment on attachment 147134 [details] Patch rooted above Source Looks good, assuming the bots are OK with it. r=me
Comment on attachment 147134 [details] Patch rooted above Source The bots are OK with it. Let’s do it.
Comment on attachment 147134 [details] Patch rooted above Source Clearing flags on attachment: 147134 Committed r120122: <http://trac.webkit.org/changeset/120122>
All reviewed patches have been landed. Closing bug.
One more cropped up yesterday. It was added by a Chromium contributor. This wouldn’t have happened if I was able to land https://chromiumcodereview.appspot.com/10535059/ first, but I was waiting for the WebKit roll. Patch coming…
Created attachment 147333 [details] Fix new violation in platform/graphics/harfbuzz/ng/HarfBuzzFaceCoreText.cpp
Attachment 147333 [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/harfbuzz/ng/HarfBuzzFaceCoreText.cpp:34: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 147337 [details] Style-conformant patch for HarfBuzzFaceCoreText.cpp
Comment on attachment 147337 [details] Style-conformant patch for HarfBuzzFaceCoreText.cpp OK. r=me
Comment on attachment 147337 [details] Style-conformant patch for HarfBuzzFaceCoreText.cpp Clearing flags on attachment: 147337 Committed r120234: <http://trac.webkit.org/changeset/120234>