Bug 182980

Summary: [CMake] Split declaration of JSC headers into public and private
Product: WebKit Reporter: Don Olmstead <don.olmstead>
Component: Tools / TestsAssignee: Don Olmstead <don.olmstead>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, annulen, commit-queue, ews, keith_miller, lforschler, mcatanzaro, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
mcatanzaro: review-
Patch
don.olmstead: review-
Patch
none
Patch none

Description Don Olmstead 2018-02-20 11:57:31 PST
JavaScriptCore headers in Xcode are listed as both private and public. The CMake declarations should reflect that.
Comment 1 Don Olmstead 2018-02-20 11:58:31 PST
Created attachment 334291 [details]
Patch

Splits everything into PUBLIC_FRAMEWORK_HEADERS and PRIVATE_FRAMEWORK_HEADERS.
Comment 2 Michael Catanzaro 2018-02-20 12:25:56 PST
Comment on attachment 334291 [details]
Patch

Problem is the public headers are different for different ports. GTK and WPE hardcode the list of non-Objective C headers in PlatformGTK.cmake and PlatformWPE.cmake. Best to clean this up now: add two separate lists of public framework headers, one for the C API, and one for Objective C. And get rid of the WPE and GTK-specific lists.
Comment 3 Don Olmstead 2018-02-20 13:08:12 PST
Created attachment 334297 [details]
Patch

Fixing based on review comments
Comment 4 Don Olmstead 2018-02-20 13:42:23 PST
Comment on attachment 334297 [details]
Patch

Ugh a windows file didn't end up here... Another patch it is...
Comment 5 Don Olmstead 2018-02-20 13:47:35 PST
Created attachment 334300 [details]
Patch
Comment 6 Build Bot 2018-02-20 13:48:48 PST
Attachment 334300 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/PlatformMac.cmake:20:  Alphabetical sorting problem. "API/JSContext.h" should be before "API/JavaScriptCore.h".  [list/order] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Michael Catanzaro 2018-02-20 15:45:41 PST
Comment on attachment 334300 [details]
Patch

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

Windows EWS is still sad.

> Source/JavaScriptCore/CMakeLists.txt:343
> +    API/JavaScript.h

I guess it makes sense to placate style checker and move it down below JSValueRef.h.

> Source/JavaScriptCore/PlatformMac.cmake:18
> +list(APPEND JavaScriptCore_PUBLIC_FRAMEWORK_HEADERS

Good idea!
Comment 8 Don Olmstead 2018-02-20 16:19:45 PST
Created attachment 334315 [details]
Patch

Hopefully making style checker and winders happy
Comment 9 Don Olmstead 2018-02-21 11:26:26 PST
Michael we good now?
Comment 10 Michael Catanzaro 2018-02-21 11:40:43 PST
Comment on attachment 334315 [details]
Patch

I think so!
Comment 11 Don Olmstead 2018-02-21 11:41:49 PST
Comment on attachment 334315 [details]
Patch

Thanks!
Comment 12 WebKit Commit Bot 2018-02-21 14:02:08 PST
Comment on attachment 334315 [details]
Patch

Clearing flags on attachment: 334315

Committed r228898: <https://trac.webkit.org/changeset/228898>
Comment 13 WebKit Commit Bot 2018-02-21 14:02:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Radar WebKit Bug Importer 2018-02-21 14:03:56 PST
<rdar://problem/37760307>