Bug 182512 – [CMake] Make WebCore headers copies

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Don Olmstead]

WebCore headers should be copied during the build to match the behavior of Apple ports.

[Don Olmstead]

Created attachment 333125 [details]
WIP Patch

Lets see what crops up on the ports.

[EWS Watchlist]

Attachment 333125 [details] did not pass style-queue:


ERROR: Source/WebCore/CMakeLists.txt:1078:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/CMakeLists.txt:1087:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/CMakeLists.txt:1152:  There should be exactly one empty line instead of 0 between "accessibility/AccessibilityRenderObject.h" and "accessibility/win/AccessibilityObjectWrapperWin.h".  [list/emptyline] [5]
ERROR: Source/WebCore/CMakeLists.txt:1279:  There should be exactly one empty line instead of 0 between "css/StyleSheetList.h" and "css/parser/CSSParser.h".  [list/emptyline] [5]
ERROR: Source/WebCore/CMakeLists.txt:1422:  There should be exactly one empty line instead of 0 between "editing/WritingDirection.h" and "editing/cocoa/DataDetection.h".  [list/emptyline] [5]
ERROR: Source/WebCore/CMakeLists.txt:1423:  Alphabetical sorting problem. "editing/markup.h" should be before "editing/cocoa/DataDetection.h".  [list/order] [5]
ERROR: Source/WebCore/CMakeLists.txt:1561:  There should be exactly one empty line instead of 0 between "inspector/PageScriptDebugServer.h" and "inspector/agents/InspectorPageAgent.h".  [list/emptyline] [5]
ERROR: Source/WebCore/CMakeLists.txt:1871:  There should be exactly one empty line instead of 0 between "platform/cf/RunLoopObserver.h" and "platform/cf/win/CertificateCFWin.h".  [list/emptyline] [5]
Total errors found: 8 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 333142 [details]
WIP Patch

Still expecting failures on GTK/WPE but less likely on the Windows side.

[EWS Watchlist]

Attachment 333142 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npruntime.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/CMakeLists.txt:1078:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/CMakeLists.txt:1087:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npapi.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 7 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 333143 [details]
WIP Patch

Earlier patch was foiled by Windows paths being case insensitive. Trying again on GTK

[EWS Watchlist]

Attachment 333143 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npruntime.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/CMakeLists.txt:1078:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/CMakeLists.txt:1087:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npapi.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 6 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 333146 [details]
WIP Patch

[EWS Watchlist]

Attachment 333146 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npruntime.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/CMakeLists.txt:1078:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/CMakeLists.txt:1087:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npapi.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 7 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 333148 [details]
WIP Patch

Add dependency to WebCoreForwardingHeaders within the TestNetscapePlugIn target.

[EWS Watchlist]

Attachment 333148 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npruntime.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/CMakeLists.txt:1078:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/CMakeLists.txt:1087:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npapi.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 7 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 333148 [details]
WIP Patch

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

> Source/WebCore/CMakeLists.txt:1020
> +set(WebCore_FORWARDING_HEADERS

This is really unfortunate. Forwarding headers are supposed to make things easier for us, not harder. Having a 1500 line list of header files to copy around is not great.

Maybe we should take a step back and ask: since forwarding headers are causing problems, why do we have them in the first place? What is the real purpose here? Other software projects don't do this: it's a WebKitism, and I'm not sure it's justified. What would it take to get rid of forwarding headers entirely?

[Fujii Hironori]

Don can use DIRECTORIES instead of FILES of WEBKIT_MAKE_FORWARDING_HEADERS.

(In reply to Michael Catanzaro from comment #11)
> Maybe we should take a step back and ask: since forwarding headers are
> causing problems, why do we have them in the first place? What is the real
> purpose here? Other software projects don't do this: it's a WebKitism, and
> I'm not sure it's justified. What would it take to get rid of forwarding
> headers entirely?

That's because WebKit is using #include <JavaScriptCore/VM.h> style instread of #include <JavaScriptCore/runtime/VM.h> style.
We can remove all forwarding headers if we use the latter style.

Include header path is a part of API. This change is a API breakage.
If Safari, for example, is including those header, it might be difficult to change.

[Don Olmstead]

Created attachment 333355 [details]
Installed files for xcode frameworks

(In reply to Michael Catanzaro from comment #11)
> Comment on attachment 333148 [details]
> WIP Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333148&action=review
> 
> > Source/WebCore/CMakeLists.txt:1020
> > +set(WebCore_FORWARDING_HEADERS
> 
> This is really unfortunate. Forwarding headers are supposed to make things
> easier for us, not harder. Having a 1500 line list of header files to copy
> around is not great.
> 
> Maybe we should take a step back and ask: since forwarding headers are
> causing problems, why do we have them in the first place? What is the real
> purpose here? Other software projects don't do this: it's a WebKitism, and
> I'm not sure it's justified. What would it take to get rid of forwarding
> headers entirely?

Ok so this is going to be long winded because I am so far down the rabbit hole with this right now, but hopefully afterwards we can all see why the concept of forwarding headers needs to go away.

Within WebKit there are currently two types of forwarding headers.

Checked in files

This was the case for Source/WebCore/ForwardingHeaders where there was a directory structure matching the structure within Source/JavaScriptCore which would then point to the installed header. This was mainly caused by generated tools output, for example https://bugs.webkit.org/show_bug.cgi?id=182448 which was a dependent bug of https://bugs.webkit.org/show_bug.cgi?id=182347 which has many more examples.

These files can and should all go away.

Generated

This is what generate-forwarding-headers.pl is doing. It generates a file relative to the root Source directory that points to the actual file on disk and forwards includes to it.

Its actually been around for a long time, https://bugs.webkit.org/show_bug.cgi?id=44336, and appears to have been done despite the fact that Apple ports were copying since before then.

This file and its generated files should also go away.

So in the end my goal is to get rid of both of those types because they are causing problems, like with pragma once, and create an inconsistency between Xcode and CMake builds.

Remember that Xcode has this concept of public and private headers, the type of which is defined in Xcode. When you build a framework it installs those headers by copying their contents into Name.framework/Headers and Name.framework/PrivateHeaders and flattens them.

I just attached a text file with a find within the build directory when building through Xcode that Ross made for me which shows what goes where.

Currently the CMake function that makes this target is named WEBKIT_MAKE_FORWARDING_HEADERS. This is really a misnomer for what the function actually does which is an install of headers. WEBKIT_INSTALL_FRAMEWORK_HEADERS would probably be a better name. I'll start working through it and classifying headers like the above in https://bugs.webkit.org/show_bug.cgi?id=182593.

Having a listing of files is obviously a CMakeism. The macro Fujii created can just take directories which will glob and then create the proper targets. This obviously breaks down when a new file is added to an incremental build which is why CMake treats globbing as an anti-pattern.

Using whats there in generate-forwarding-headers.pl like what Konstantin suggested over in https://bugs.webkit.org/show_bug.cgi?id=182555#c17 wouldn't work because its not recursive, meaning it doesn't go looking inside the file it detects for more files. I have a script I'm using locally that is doing that which is why there are so many more files versus what actually is needed.

I am open to maybe piggy backing onto what Keith did with the unified source builds and making a Headers.txt or something along those lines but making things work first is my priority and cleanup is being saved for later. I did play with that some but decided to just use Fujii's work to get this task moving.

[Michael Catanzaro]

I don't want to start listing internal header files in CMakeLists.txt or Headers.txt or Sources.txt. But it's really starting to look like that might be what we have to do, isn't it....

(In reply to Fujii Hironori from comment #12)
> That's because WebKit is using #include <JavaScriptCore/VM.h> style instread
> of #include <JavaScriptCore/runtime/VM.h> style.
> We can remove all forwarding headers if we use the latter style.
> 
> Include header path is a part of API. This change is a API breakage.
> If Safari, for example, is including those header, it might be difficult to
> change.

Those headers are not part of the API. The API consists of headers that get installed. For JavaScriptCore, those are the headers under Source/JavaScriptCore/API. There should be no way for Safari to include VM.h, because it's not installed. Right? (Right?)

(In reply to Don Olmstead from comment #13)
> Remember that Xcode has this concept of public and private headers, the type
> of which is defined in Xcode. When you build a framework it installs those
> headers by copying their contents into Name.framework/Headers and
> Name.framework/PrivateHeaders and flattens them.

I think the real problem is that (a) the Xcode build is flattening the headers, which requires that we come up with some way to manually mimic this behavior, and (b) all the source code files in WebKit depend on flattened headers, so if we change it, we have to change the include style used in every source code file. We could write a script to do this for us, though.

Writing out the full path to the include directories, like Google does, seems best to me, e.g. #include "JavaScriptCore/runtime/VM.h". The alternative would be to add a bazillion include directories to every build target so that we can do #include "VM.h", but that seems far less desirable as then I lose important context (what sort of VM? oh, it's the JavaScriptCore VM).

That's my $0.02. I guess getting rid of flattened headers is maybe not likely, but that seems best to me.

[Don Olmstead]

Created attachment 333658 [details]
WIP Patch

WinCairo is building and hopefully AppleWin. GTK/WPE hopefully build

[EWS Watchlist]

Attachment 333658 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:45:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/CMakeLists.txt:233:  Alphabetical sorting problem. "bridge/npruntime_impl.h" should be before "bridge/jsc/BridgeJSC.h".  [list/order] [5]
ERROR: Source/WebCore/CMakeLists.txt:239:  Alphabetical sorting problem. "bridge/runtime_method.h" should be before "bridge/objc/WebScriptObjectPrivate.h".  [list/order] [5]
Total errors found: 4 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 333659 [details]
WIP Patch

Remove temp file...

[EWS Watchlist]

Attachment 333659 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:45:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/CMakeLists.txt:231:  Alphabetical sorting problem. "bridge/npruntime_impl.h" should be before "bridge/jsc/BridgeJSC.h".  [list/order] [5]
ERROR: Source/WebCore/CMakeLists.txt:237:  Alphabetical sorting problem. "bridge/runtime_method.h" should be before "bridge/objc/WebScriptObjectPrivate.h".  [list/order] [5]
Total errors found: 4 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 333661 [details]
WIP Patch

Trying again

[EWS Watchlist]

Attachment 333661 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:45:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/CMakeLists.txt:283:  Alphabetical sorting problem. "bridge/npruntime_impl.h" should be before "bridge/jsc/BridgeJSC.h".  [list/order] [5]
ERROR: Source/WebCore/CMakeLists.txt:289:  Alphabetical sorting problem. "bridge/runtime_method.h" should be before "bridge/objc/WebScriptObjectPrivate.h".  [list/order] [5]
Total errors found: 4 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 333663 [details]
WIP Patch

Move libwebrtc into the shared list

[EWS Watchlist]

Attachment 333663 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:45:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/CMakeLists.txt:1111:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/CMakeLists.txt:283:  Alphabetical sorting problem. "bridge/npruntime_impl.h" should be before "bridge/jsc/BridgeJSC.h".  [list/order] [5]
ERROR: Source/WebCore/CMakeLists.txt:289:  Alphabetical sorting problem. "bridge/runtime_method.h" should be before "bridge/objc/WebScriptObjectPrivate.h".  [list/order] [5]
Total errors found: 5 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 333665 [details]
WIP Patch

Remove some additional paths

[EWS Watchlist]

Attachment 333665 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:45:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/CMakeLists.txt:1111:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/CMakeLists.txt:283:  Alphabetical sorting problem. "bridge/npruntime_impl.h" should be before "bridge/jsc/BridgeJSC.h".  [list/order] [5]
ERROR: Source/WebCore/CMakeLists.txt:289:  Alphabetical sorting problem. "bridge/runtime_method.h" should be before "bridge/objc/WebScriptObjectPrivate.h".  [list/order] [5]
Total errors found: 6 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 333667 [details]
WIP Patch

Lets see if WPE likes this now

[EWS Watchlist]

Attachment 333667 [details] did not pass style-queue:


ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:45:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/CMakeLists.txt:1111:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/CMakeLists.txt:283:  Alphabetical sorting problem. "bridge/npruntime_impl.h" should be before "bridge/jsc/BridgeJSC.h".  [list/order] [5]
ERROR: Source/WebCore/CMakeLists.txt:289:  Alphabetical sorting problem. "bridge/runtime_method.h" should be before "bridge/objc/WebScriptObjectPrivate.h".  [list/order] [5]
Total errors found: 6 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

I feel like the GTK/WPE bots are having issues with this patch...

Michael any chance you can try and apply this and see what happens locally? I really need to get a GTK or WPE environment up and running locally...

[Zan Dobersek]

Created attachment 333679 [details]
GTK and WPE changes

This complements attachment #333667 [details], fixing GTK+ and WPE builds.

[Michael Catanzaro]

Thank you Zan!

[Don Olmstead]

Created attachment 333699 [details]
Patch

Lets try it again

[EWS Watchlist]

Attachment 333699 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npruntime.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:45:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/CMakeLists.txt:1111:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/CMakeLists.txt:283:  Alphabetical sorting problem. "bridge/npruntime_impl.h" should be before "bridge/jsc/BridgeJSC.h".  [list/order] [5]
ERROR: Source/WebCore/CMakeLists.txt:289:  Alphabetical sorting problem. "bridge/runtime_method.h" should be before "bridge/objc/WebScriptObjectPrivate.h".  [list/order] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npapi.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 8 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 333701 [details]
Patch

WPE is happy. Now lets see if AppleWin and GTK are happy.

[EWS Watchlist]

Attachment 333701 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npruntime.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:45:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/CMakeLists.txt:1109:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npapi.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 6 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 333704 [details]
Patch

Make GTK happy hopefully...

[Don Olmstead]

Created attachment 333708 [details]
Patch

Back to making WPE happy...

[EWS Watchlist]

Attachment 333708 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npruntime.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKitLegacy/win/WebKitPrefix.h:45:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npapi.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 5 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 333708 [details]
Patch

Attachment 333708 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/6485971

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 333714 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Don Olmstead]

(In reply to Build Bot from comment #38)
> Created attachment 333714 [details]
> Archive of layout-test-results from ews202 for win-future
> 
> The attached test failures were seen while running run-webkit-tests on the
> win-ews.
> Bot: ews202  Port: win-future  Platform:
> CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

I don't see why this would cause any problems with tests since its all build related.

Also think something is messed up with that WinCairo build as locally its fine.

[Keith Miller]

Comment on attachment 333708 [details]
Patch

r=me, ideally, we wouldn't need to list all the forwarded headers but I can't think of another way specify the subset of headers that need to be exported.

[Don Olmstead]

Comment on attachment 333708 [details]
Patch

Clearing flags on attachment: 333708

Committed r228431: <https://trac.webkit.org/changeset/228431>

[Don Olmstead]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37510435>

[Fujii Hironori]

Filed: Bug 182757 – REGRESSION(r228431) [CMake][Ninja] Fails to compile TestWebCore due to missing WebCore's derived headers

[Fujii Hironori]

Comment on attachment 333708 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:3295
> +    FILES ${WebCore_PRIVATE_FRAMEWORK_HEADERS}

Why is this variable named WebCore_PRIVATE_FRAMEWORK_HEADERS?
In my understanding, this is public framework headers because they are referred from external modules.

[Don Olmstead]

I'm told by perarne that the Apple Win EWS is having problems after this patch. I didn't think that a build change would add crashes especially since AppleWin already copied headers.

If it needs to be rolled out then we can do that. I'll keep looking into it tomorrow. If someone else wants to take a look its appreciated!

[Don Olmstead]

(In reply to Fujii Hironori from comment #45)
> Comment on attachment 333708 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333708&action=review
> 
> > Source/WebCore/CMakeLists.txt:3295
> > +    FILES ${WebCore_PRIVATE_FRAMEWORK_HEADERS}
> 
> Why is this variable named WebCore_PRIVATE_FRAMEWORK_HEADERS?
> In my understanding, this is public framework headers because they are
> referred from external modules.

If you look at https://bug-182512-attachments.webkit.org/attachment.cgi?id=333355 all the headers are listed as private within Apple's xcode setup so that's why.

[Don Olmstead]

(In reply to Don Olmstead from comment #46)
> I'm told by perarne that the Apple Win EWS is having problems after this
> patch. I didn't think that a build change would add crashes especially since
> AppleWin already copied headers.
> 
> If it needs to be rolled out then we can do that. I'll keep looking into it
> tomorrow. If someone else wants to take a look its appreciated!

Getting when running

Run-Time Check Failure #0 - The value of ESP was not properly saved across a function call.  This is usually a result of calling a function declared with one calling convention with a function pointer declared with a different calling convention.

I have a feeling that this might be due to config.h being removed in WebKitPrefix. I'm thinking of rolling this out then patching that separately to see what happens.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 182766

[Don Olmstead]

This is currently blocked on https://bugs.webkit.org/show_bug.cgi?id=182757. Because of that I'll bust off some of these smaller changes that were required into separate bugs and land those.

[Don Olmstead]

Created attachment 354708 [details]
WIP Patch

Let's see if GTK/WPE are happy with this.

[EWS Watchlist]

Attachment 354708 [details] did not pass style-queue:


ERROR: Source/WebCore/Headers.cmake:280:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/Headers.cmake:1169:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/Headers.cmake:182:  Alphabetical sorting problem. "bindings/js/JSCSSRuleCustom.h" should be before "bindings/js/JSCallbackData.h".  [list/order] [5]
ERROR: Source/WebCore/Headers.cmake:187:  Alphabetical sorting problem. "bindings/js/JSDOMAbstractOperations.h" should be before "bindings/js/JSDocumentCustom.h".  [list/order] [5]
ERROR: Source/WebCore/Headers.cmake:291:  Alphabetical sorting problem. "bridge/npruntime_impl.h" should be before "bridge/jsc/BridgeJSC.h".  [list/order] [5]
ERROR: Source/WebCore/Headers.cmake:297:  Alphabetical sorting problem. "bridge/runtime_method.h" should be before "bridge/objc/WebScriptObjectPrivate.h".  [list/order] [5]
ERROR: Source/WebCore/PlatformGTK.cmake:195:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/CMakeLists.txt:1635:  Alphabetical sorting problem. "Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp" should be before "Modules/mediastream/libwebrtc/LibWebRTCStatsCollector.cpp".  [list/order] [5]
Total errors found: 8 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 354710 [details]
WIP Patch

[EWS Watchlist]

Attachment 354710 [details] did not pass style-queue:


ERROR: Source/WebCore/Headers.cmake:280:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/Headers.cmake:1169:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/Headers.cmake:182:  Alphabetical sorting problem. "bindings/js/JSCSSRuleCustom.h" should be before "bindings/js/JSCallbackData.h".  [list/order] [5]
ERROR: Source/WebCore/Headers.cmake:187:  Alphabetical sorting problem. "bindings/js/JSDOMAbstractOperations.h" should be before "bindings/js/JSDocumentCustom.h".  [list/order] [5]
ERROR: Source/WebCore/Headers.cmake:291:  Alphabetical sorting problem. "bridge/npruntime_impl.h" should be before "bridge/jsc/BridgeJSC.h".  [list/order] [5]
ERROR: Source/WebCore/Headers.cmake:297:  Alphabetical sorting problem. "bridge/runtime_method.h" should be before "bridge/objc/WebScriptObjectPrivate.h".  [list/order] [5]
ERROR: Source/WebCore/PlatformGTK.cmake:195:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/CMakeLists.txt:1635:  Alphabetical sorting problem. "Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp" should be before "Modules/mediastream/libwebrtc/LibWebRTCStatsCollector.cpp".  [list/order] [5]
Total errors found: 8 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 354722 [details]
WIP Patch

[EWS Watchlist]

Attachment 354722 [details] did not pass style-queue:


ERROR: Source/WebCore/Headers.cmake:1168:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/Headers.cmake:296:  Alphabetical sorting problem. "bridge/runtime_method.h" should be before "bridge/objc/WebScriptObjectPrivate.h".  [list/order] [5]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 354746 [details]
WIP Patch

[EWS Watchlist]

Attachment 354746 [details] did not pass style-queue:


ERROR: Source/WebCore/PlatformMac.cmake:525:  There should be exactly one empty line instead of 0 between "platform/graphics/avfoundation/WebMediaSessionManagerMac.h" and "platform/graphics/avfoundation/objc/AVFoundationMIMETypeCache.h".  [list/emptyline] [5]
ERROR: Source/WebCore/PlatformAppleWin.cmake:68:  Alphabetical sorting problem. "platform/cf/win/CertificateCFWin.h" should be before "platform/network/cf/SocketStreamHandleImpl.h".  [list/order] [5]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 354747 [details]
WIP Patch

[Don Olmstead]

Created attachment 354768 [details]
WIP Patch

[EWS Watchlist]

Comment on attachment 354768 [details]
WIP Patch

Attachment 354768 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9983817

New failing tests:
http/wpt/css/css-animations/start-animation-001.html

[EWS Watchlist]

Created attachment 354771 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Don Olmstead]

Created attachment 354773 [details]
WIP Patch

[Don Olmstead]

Created attachment 355306 [details]
WIP Patch

[Don Olmstead]

Created attachment 355307 [details]
WIP Patch

[Don Olmstead]

Created attachment 355351 [details]
WIP Patch

[EWS Watchlist]

Attachment 355351 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npruntime.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npapi.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 4 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 355355 [details]
Patch

[EWS Watchlist]

Attachment 355355 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npruntime.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npapi.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 3 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 355357 [details]
Patch

[EWS Watchlist]

Attachment 355357 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npruntime.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npapi.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 3 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 355359 [details]
Patch

[EWS Watchlist]

Attachment 355359 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npruntime.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npapi.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 3 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 355361 [details]
Patch

[EWS Watchlist]

Attachment 355361 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npruntime.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npapi.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 3 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Bots are happy so this should be good for a review.

[Fujii Hironori]

Comment on attachment 355361 [details]
Patch

Don’t land this until 
Bug 182757 is addressed.

[Don Olmstead]

(In reply to Fujii Hironori from comment #77)
> Comment on attachment 355361 [details]
> Patch
> 
> Don’t land this until 
> Bug 182757 is addressed.

I'm not able to reproduce this on WinCairo through Ninja. This is with ninja running 36 instances of clang-cl. 

I do think we need to enumerate all the DerivedSources that need to be copied to make sure there are no issues but the current implementation of WEBKIT_MAKE_FORWARDING_HEADERS doesn't handle that and it would be easier to change that once everything is copying throughout WebKit.

[Don Olmstead]

Created attachment 367480 [details]
Patch

Let's see how the bots like this one

[EWS Watchlist]

Attachment 367480 [details] did not pass style-queue:


ERROR: Source/WebCore/PlatformWin.cmake:179:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/Headers.cmake:251:  Alphabetical sorting problem. "bridge/npruntime_impl.h" should be before "bridge/jsc/BridgeJSC.h".  [list/order] [5]
ERROR: Source/WebCore/Headers.cmake:257:  Alphabetical sorting problem. "bridge/runtime_method.h" should be before "bridge/objc/WebScriptObjectPrivate.h".  [list/order] [5]
ERROR: Source/WebCore/CMakeLists.txt:1675:  Alphabetical sorting problem. "Modules/mediastream/libwebrtc/LibWebRTCDataChannelHandler.cpp" should be before "Modules/mediastream/libwebrtc/LibWebRTCStatsCollector.cpp".  [list/order] [5]
ERROR: Source/WebKit/CMakeLists.txt:11:  No trailing spaces  [whitespace/trailing] [5]
Total errors found: 5 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 367481 [details]
Patch

[EWS Watchlist]

Attachment 367481 [details] did not pass style-queue:


ERROR: Source/WebCore/PlatformWin.cmake:179:  No trailing spaces  [whitespace/trailing] [5]
ERROR: Source/WebCore/Headers.cmake:246:  There should be no empty line between "bridge/NP_jsobject.h" and "bridge/runtime_method.h".  [list/emptyline] [5]
ERROR: Source/WebCore/Headers.cmake:255:  Alphabetical sorting problem. "bridge/npruntime_impl.h" should be before "bridge/jsc/BridgeJSC.h".  [list/order] [5]
Total errors found: 3 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 367483 [details]
Patch

Hopefully fix WPE and style

[EWS Watchlist]

Attachment 367483 [details] did not pass style-queue:


ERROR: Source/WebCore/Headers.cmake:254:  Alphabetical sorting problem. "bridge/npruntime_impl.h" should be before "bridge/jsc/BridgeJSC.h".  [list/order] [5]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 367485 [details]
Patch

[Don Olmstead]

Created attachment 367496 [details]
Patch

[Don Olmstead]

Created attachment 367497 [details]
Patch

[EWS Watchlist]

Attachment 367497 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/CMakeLists.txt:26:  Alphabetical sorting problem. "PALFrameworkHeaders" should be before "WebCorePrivateFrameworkHeaders".  [list/order] [5]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 367498 [details]
Patch

[Don Olmstead]

Created attachment 367499 [details]
Patch

[Don Olmstead]

Created attachment 367500 [details]
Patch

[EWS Watchlist]

Comment on attachment 367500 [details]
Patch

Attachment 367500 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11881859

New failing tests:
imported/w3c/web-platform-tests/service-workers/cache-storage/window/cache-abort.https.html

[EWS Watchlist]

Created attachment 367511 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Don Olmstead]

Created attachment 367540 [details]
Patch

[EWS Watchlist]

Attachment 367540 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 367543 [details]
Patch

[Don Olmstead]

Created attachment 367570 [details]
Patch

Hopefully fixes the AppleWin build. GTK one passed in the last patch but took over 2 hours to complete. WPE bots seem to be in a bad state so I'm not sure if everything is ok with that one.

I do think this is ready for review though.

[Don Olmstead]

Created attachment 367576 [details]
Patch

Fix bad include found by WPE bot.

[Fujii Hironori]

Comment on attachment 367576 [details]
Patch

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

> Source/WebCore/PlatformAppleWin.cmake:58
> +list(APPEND WebCore_PRIVATE_FORWARDING_HEADERS

WebCore_PRIVATE_FORWARDING_HEADERS should be WebCore_PRIVATE_FRAMEWORK_HEADERS. This is the reason for the AppleWin EWS failure.

> Source/WebCore/PlatformAppleWin.cmake:73
> +    list(APPEND WebCore_PRIVATE_INCLUDE_DIRECTORIES

Ditto.

[Fujii Hironori]

Comment on attachment 367576 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        will break the build whenever a file is added.

Yeah! explicitly listing is the right way to solve the repeating incremental build issue.

> Source/WebCore/ChangeLog:22
> +        * Headers.cmake: Added.

It would reduce the maintenance cost for header listing if you unify Headers.cmake and Sources.txt. How do you think this idea?

[Don Olmstead]

Created attachment 367601 [details]
Patch

Hopefully make AppleWin happy

[Don Olmstead]

(In reply to Fujii Hironori from comment #99)
> Comment on attachment 367576 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367576&action=review
> 
> > Source/WebCore/PlatformAppleWin.cmake:58
> > +list(APPEND WebCore_PRIVATE_FORWARDING_HEADERS
> 
> WebCore_PRIVATE_FORWARDING_HEADERS should be
> WebCore_PRIVATE_FRAMEWORK_HEADERS. This is the reason for the AppleWin EWS
> failure.
> 
> > Source/WebCore/PlatformAppleWin.cmake:73
> > +    list(APPEND WebCore_PRIVATE_INCLUDE_DIRECTORIES
> 
> Ditto.

Good catches!

(In reply to Fujii Hironori from comment #100)
> Comment on attachment 367576 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367576&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        will break the build whenever a file is added.
> 
> Yeah! explicitly listing is the right way to solve the repeating incremental
> build issue.
> 
> > Source/WebCore/ChangeLog:22
> > +        * Headers.cmake: Added.
> 
> It would reduce the maintenance cost for header listing if you unify
> Headers.cmake and Sources.txt. How do you think this idea?

I have some local patches that attempt to do something along this line. I was looking at this in terms of flattening PAL. Its super simple in CMake but there needs to be some kind of script for XCode.

I agree that this should be future work. I can gather up some of my patches and share where I was going with it.

[Fujii Hironori]

(In reply to Don Olmstead from comment #102)
> I have some local patches that attempt to do something along this line. I
> was looking at this in terms of flattening PAL. Its super simple in CMake
> but there needs to be some kind of script for XCode.

You need to do nothing for XCode because generate-unified-source-bundles.rb bundles only .cpp and .mm.
It is harmless for XCode to add .h files in Sources.txt.

> I agree that this should be future work. I can gather up some of my patches
> and share where I was going with it.

Got it.

[Fujii Hironori]

Comment on attachment 367601 [details]
Patch

Looks good to me. I'd like to wait EWS results and Moscow work time.

[Alex Christensen]

Comment on attachment 367601 [details]
Patch

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

Looks good to me

> Source/WebCore/Headers.cmake:1211
> +    plugins/npfunctions.h

It looks like the AppleWin build can't find this file from DumpRenderTree.

[Don Olmstead]

Created attachment 367735 [details]
Patch

Fix AppleWin

[Don Olmstead]

Created attachment 367736 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 367736 [details]
Patch

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

> Source/WebCore/Headers.cmake:1
> +set(WebCore_PRIVATE_FRAMEWORK_HEADERS

Manual maintenance of this header list sounds like a nightmare. Old macro at least supported directories

[Don Olmstead]

Created attachment 367738 [details]
Patch

Fix a file that was renamed

[Don Olmstead]

(In reply to Konstantin Tokarev from comment #108)
> Comment on attachment 367736 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367736&action=review
> 
> > Source/WebCore/Headers.cmake:1
> > +set(WebCore_PRIVATE_FRAMEWORK_HEADERS
> 
> Manual maintenance of this header list sounds like a nightmare. Old macro at
> least supported directories

Between when I used a script to enumerate the headers and posting this patch a file was renamed. This would force a rebuild because the directories would still have the old file. We know that CMake is bad about globbing so I don't really see any other options than enumerating things.

[Don Olmstead]

Created attachment 367742 [details]
Patch

[EWS Watchlist]

Attachment 367742 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npruntime.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npapi.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 3 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Don Olmstead]

Created attachment 367757 [details]
Patch

....

[EWS Watchlist]

Attachment 367757 [details] did not pass style-queue:


ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npruntime.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Tools/DumpRenderTree/TestNetscapePlugIn/ForwardingHeaders/WebKit/npapi.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 3 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 367757 [details]
Patch

Clearing flags on attachment: 367757

Committed r244443: <https://trac.webkit.org/changeset/244443>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Don Olmstead]

Created attachment 367773 [details]
Build fix

[Don Olmstead]

Reopening to land build fix

[Don Olmstead]

Created attachment 367774 [details]
Build fix

A header got added between the time the patch was submitted to EWS and landing.

[WebKit Commit Bot]

Comment on attachment 367774 [details]
Build fix

Clearing flags on attachment: 367774

Committed r244445: <https://trac.webkit.org/changeset/244445>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Don Olmstead]

Ok so looks like all the CMake based ports are green after the build fix.

Igalia folks if you have any problems after r244445 I would suggest doing a clean build. The aperez EWS bots were all having problems with this patch so hopefully those EWS bots can be looked at or maybe they'll end up nuking their WebKitBuild directory and get back to green.

[Konstantin Tokarev]

(In reply to Don Olmstead from comment #110)
> (In reply to Konstantin Tokarev from comment #108)
> > Comment on attachment 367736 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=367736&action=review
> > 
> > > Source/WebCore/Headers.cmake:1
> > > +set(WebCore_PRIVATE_FRAMEWORK_HEADERS
> > 
> > Manual maintenance of this header list sounds like a nightmare. Old macro at
> > least supported directories
> 
> Between when I used a script to enumerate the headers and posting this patch
> a file was renamed. This would force a rebuild because the directories would
> still have the old file. We know that CMake is bad about globbing so I don't
> really see any other options than enumerating things.

What about the following approach: for each SomeFile.cpp of WebCore if SomeFile.h exists we add it to header list? Probably there will be a few remaining headers, but there should be much less manual work.

As compared to generate-forwarding-headers.pl approach, which scans whichever headers we want to include, this method will do some excessive work in the clean build, however it doesn't require re-scanning sources on each build, reducing incremental build time, and guarantees that no WebKit port will be broken because of someone forgetting to add new port-independent header to the list