Bug 170997 – Clean up ICU headers

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

[Andy VanWagoner]

Ideally we should not need the ICU headers in the WebKit source, so if possible, we should remove them.

If we cannot remove them, then all ICU headers should be on the same version (iirc 55.1 is the minimum version for all platforms).

[JF Bastien]

We seem to have 4 copies, and as best I can tell from the project files we probably don't build for earlier than 10.10 at this point.

[Andy VanWagoner]

I found the ICU headers in Xcode.app/Contents but it looks like it is only part of the AppleTV, iPhone, & Watch platforms.

/Applications/Xcode.app/Contents/Developer/Platforms/AppleTVOS.platform/Developer/SDKs/AppleTVOS.sdk/usr/include/unicode
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS.sdk/usr/include
/Applications/Xcode.app/Contents/Developer/Platforms/WatchOS.platform/Developer/SDKs/WatchOS.sdk/usr/include/unicode

The MacOSX platform does *not* have a corresponding include folder.

/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/unicode

Should the README in the icu folders of the projects be updated to make it more clear that it's not just Mac OS X 10.4, but also all subsequent versions of Mac OS X, that require the headers?

[JF Bastien]

(In reply to Andy VanWagoner from comment #2)
> I found the ICU headers in Xcode.app/Contents but it looks like it is only
> part of the AppleTV, iPhone, & Watch platforms.
> 
> /Applications/Xcode.app/Contents/Developer/Platforms/AppleTVOS.platform/
> Developer/SDKs/AppleTVOS.sdk/usr/include/unicode
> /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/
> Developer/SDKs/iPhoneOS.sdk/usr/include
> /Applications/Xcode.app/Contents/Developer/Platforms/WatchOS.platform/
> Developer/SDKs/WatchOS.sdk/usr/include/unicode
> 
> The MacOSX platform does *not* have a corresponding include folder.
> 
> /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/
> Developer/SDKs/MacOSX.sdk/usr/include/unicode
> 
> Should the README in the icu folders of the projects be updated to make it
> more clear that it's not just Mac OS X 10.4, but also all subsequent
> versions of Mac OS X, that require the headers?

Dan was just telling me the same thing :)

I think you're correct: the READMEs are misleading, it's 10.4 and up.

[Andy VanWagoner]

I've started by simply updating all the existing files to v55.1. Should I add headers we don't use yet, as long as they are part of the C API?

[JF Bastien]

(In reply to Andy VanWagoner from comment #4)
> I've started by simply updating all the existing files to v55.1. Should I
> add headers we don't use yet, as long as they are part of the C API?

I think this would be fine since we already support 55.1. Of course, use of newer APIs will need the same gymnastics to avoid breaking older ports.

Could you also update the READMEs to avoid the confusing w.r.t. 10.4? It seems the issue it mentions is here to stay, not a temporary thing as it makes it sound.

[Andy VanWagoner]

Created attachment 308046 [details]
Patch

[JF Bastien]

Comment on attachment 308046 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308046&action=review

Looks good, will wait for bots to be happy too.

> Source/JavaScriptCore/icu/unicode/platform.h:127
> + *  http://www.chromium.org/nativeclient

lol

> Source/JavaScriptCore/icu/unicode/uchar.h:42
> +#define U_UNICODE_VERSION "7.0"

Does this indirectly make us support new things through the system library? i.e. new things in the .h opt us into thing the library supported but were off due to some header-declared feature set? I don't know how most of ICU works :)

[Andy VanWagoner]

(In reply to JF Bastien from comment #7)
> Comment on attachment 308046 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308046&action=review
> 
> Looks good, will wait for bots to be happy too.
> 
> > Source/JavaScriptCore/icu/unicode/platform.h:127
> > + *  http://www.chromium.org/nativeclient
> 
> lol
> 
> > Source/JavaScriptCore/icu/unicode/uchar.h:42
> > +#define U_UNICODE_VERSION "7.0"
> 
> Does this indirectly make us support new things through the system library?
> i.e. new things in the .h opt us into thing the library supported but were
> off due to some header-declared feature set? I don't know how most of ICU
> works :)

Considering the system headers are used on most platforms, including iOS, Apple TV, and Apple Watch, I don't think this opts us into anything in particular. There are a few defines that can cause the build to fail if left their original value, and so I preserved the edits we had in them and documented it.

This particular define is actually the following in my iPhoneOS.platform version:

#define U_UNICODE_VERSION "8.0"

[JF Bastien]

OK this looks good. iOS-sim fails seem unrelated.

I'll revert if anything breaks, but this seems pretty low risk. Thanks for taking this on :)

[JF Bastien]

Comment on attachment 308046 [details]
Patch

cq+

[WebKit Commit Bot]

Comment on attachment 308046 [details]
Patch

Clearing flags on attachment: 308046

Committed r215722: <http://trac.webkit.org/changeset/215722>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.