Bug 88569 – Chromium Mac: don’t #include things in subframeworks of ApplicationServices.framework

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Mark Mentovai]

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.

[Mark Mentovai]

Created attachment 146362 [details]
Don’t reach into subframeworks, just #include <ApplicationServices/ApplicationServices.h>

[Mark Mentovai]

Created attachment 146363 [details]
Updated to current trunk (r119733)

[Stephen White]

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?

[Cary Clark]

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?

[Mark Mentovai]

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.

[Cary Clark]

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.

[Mark Mentovai]

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.

[Cary Clark]

Comment on attachment 147097 [details]
Don’t use SKIA_ON_MAC_CHROMIUM

lgtm

[Stephen White]

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.

[Mark Mentovai]

Created attachment 147134 [details]
Patch rooted above Source

[Mark Mentovai]

Comment on attachment 147134 [details]
Patch rooted above Source

Thanks, this one seems to apply on the EWS.

[Stephen White]

Comment on attachment 147134 [details]
Patch rooted above Source

Looks good, assuming the bots are OK with it.  r=me

[Mark Mentovai]

Comment on attachment 147134 [details]
Patch rooted above Source

The bots are OK with it. Let’s do it.

[WebKit Review Bot]

Comment on attachment 147134 [details]
Patch rooted above Source

Clearing flags on attachment: 147134

Committed r120122: <http://trac.webkit.org/changeset/120122>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Mentovai]

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…

[Mark Mentovai]

Created attachment 147333 [details]
Fix new violation in platform/graphics/harfbuzz/ng/HarfBuzzFaceCoreText.cpp

[WebKit Review Bot]

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.

[Mark Mentovai]

Created attachment 147337 [details]
Style-conformant patch for HarfBuzzFaceCoreText.cpp

[Stephen White]

Comment on attachment 147337 [details]
Style-conformant patch for HarfBuzzFaceCoreText.cpp

OK.  r=me

[WebKit Review Bot]

Comment on attachment 147337 [details]
Style-conformant patch for HarfBuzzFaceCoreText.cpp

Clearing flags on attachment: 147337

Committed r120234: <http://trac.webkit.org/changeset/120234>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.