Bug 88569 - Chromium Mac: don’t #include things in subframeworks of ApplicationServices.framework
Summary: Chromium Mac: don’t #include things in subframeworks of ApplicationServices.f...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P2 Normal
Assignee: Mark Mentovai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-07 12:32 PDT by Mark Mentovai
Modified: 2012-06-13 12:21 PDT (History)
4 users (show)

See Also:


Attachments
Don’t reach into subframeworks, just #include <ApplicationServices/ApplicationServices.h> (4.25 KB, patch)
2012-06-07 12:40 PDT, Mark Mentovai
no flags Details | Formatted Diff | Diff
Updated to current trunk (r119733) (4.22 KB, patch)
2012-06-07 12:45 PDT, Mark Mentovai
no flags Details | Formatted Diff | Diff
Don’t use SKIA_ON_MAC_CHROMIUM (5.63 KB, patch)
2012-06-12 09:22 PDT, Mark Mentovai
senorblanco: review-
Details | Formatted Diff | Diff
Patch rooted above Source (5.76 KB, patch)
2012-06-12 12:25 PDT, Mark Mentovai
no flags Details | Formatted Diff | Diff
Fix new violation in platform/graphics/harfbuzz/ng/HarfBuzzFaceCoreText.cpp (1.36 KB, patch)
2012-06-13 09:09 PDT, Mark Mentovai
no flags Details | Formatted Diff | Diff
Style-conformant patch for HarfBuzzFaceCoreText.cpp (1.40 KB, patch)
2012-06-13 09:28 PDT, Mark Mentovai
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Mentovai 2012-06-07 12:32:34 PDT
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.
Comment 1 Mark Mentovai 2012-06-07 12:40:01 PDT
Created attachment 146362 [details]
Don’t reach into subframeworks, just #include <ApplicationServices/ApplicationServices.h>
Comment 2 Mark Mentovai 2012-06-07 12:45:09 PDT
Created attachment 146363 [details]
Updated to current trunk (r119733)
Comment 3 Stephen White 2012-06-12 08:39:24 PDT
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 4 Cary Clark 2012-06-12 08:45:26 PDT
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?
Comment 5 Mark Mentovai 2012-06-12 08:53:32 PDT
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 6 Cary Clark 2012-06-12 09:01:55 PDT
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.
Comment 7 Mark Mentovai 2012-06-12 09:22:18 PDT
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 8 Cary Clark 2012-06-12 09:24:56 PDT
Comment on attachment 147097 [details]
Don’t use SKIA_ON_MAC_CHROMIUM

lgtm
Comment 9 Stephen White 2012-06-12 10:35:36 PDT
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.
Comment 10 Mark Mentovai 2012-06-12 12:25:53 PDT
Created attachment 147134 [details]
Patch rooted above Source
Comment 11 Mark Mentovai 2012-06-12 12:30:25 PDT
Comment on attachment 147134 [details]
Patch rooted above Source

Thanks, this one seems to apply on the EWS.
Comment 12 Stephen White 2012-06-12 12:54:38 PDT
Comment on attachment 147134 [details]
Patch rooted above Source

Looks good, assuming the bots are OK with it.  r=me
Comment 13 Mark Mentovai 2012-06-12 13:59:27 PDT
Comment on attachment 147134 [details]
Patch rooted above Source

The bots are OK with it. Let’s do it.
Comment 14 WebKit Review Bot 2012-06-12 14:39:31 PDT
Comment on attachment 147134 [details]
Patch rooted above Source

Clearing flags on attachment: 147134

Committed r120122: <http://trac.webkit.org/changeset/120122>
Comment 15 WebKit Review Bot 2012-06-12 14:39:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Mark Mentovai 2012-06-13 09:09:11 PDT
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…
Comment 17 Mark Mentovai 2012-06-13 09:09:42 PDT
Created attachment 147333 [details]
Fix new violation in platform/graphics/harfbuzz/ng/HarfBuzzFaceCoreText.cpp
Comment 18 WebKit Review Bot 2012-06-13 09:13:00 PDT
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.
Comment 19 Mark Mentovai 2012-06-13 09:28:59 PDT
Created attachment 147337 [details]
Style-conformant patch for HarfBuzzFaceCoreText.cpp
Comment 20 Stephen White 2012-06-13 11:17:57 PDT
Comment on attachment 147337 [details]
Style-conformant patch for HarfBuzzFaceCoreText.cpp

OK.  r=me
Comment 21 WebKit Review Bot 2012-06-13 12:21:34 PDT
Comment on attachment 147337 [details]
Style-conformant patch for HarfBuzzFaceCoreText.cpp

Clearing flags on attachment: 147337

Committed r120234: <http://trac.webkit.org/changeset/120234>
Comment 22 WebKit Review Bot 2012-06-13 12:21:40 PDT
All reviewed patches have been landed.  Closing bug.