Bug 98665 – Remove <wtf/Platform.h> include from header files.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dima Gorbik]

We don't want other clients that include WebKit headers to know about Platform.h.

[Dima Gorbik]

Created attachment 167561 [details]
Proposed fix 0.1

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/tests/minidom.c'..." exit_code: 1
Source/JavaScriptCore/API/tests/minidom.c:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/API/tests/testapi.c:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 7 files


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

[Eric Seidel (no email)]

Comment on attachment 167561 [details]
Proposed fix 0.1

Agreed!

[Dima Gorbik]

Style issues are because wtf/Assertions.h uses macros defined in the Platform.h. That's why they are swapped. Should I put Platform.h include above all other includes?

[Alexey Proskuryakov]

<wtf/Platform.h> is normally included through config.h, which is normally the second included file in every .cpp.

[WebKit Review Bot]

Comment on attachment 167561 [details]
Proposed fix 0.1

Rejecting attachment 167561 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 167561 [details]
Proposed fix 0.1

Clearing flags on attachment: 167561

Committed r131496: <http://trac.webkit.org/changeset/131496>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

I am puzzled by this change. It does not seem right to me.

What good is Assertions.h without Platform.h, since is uses macros in Platform.h? Is this because we have code that wants to use Assertions.h and has its own substitute for Platform.h, code outside WebKit?

[Darin Adler]

This change needed more discussion before the change was made. Not sure what to do about it now.

[Darin Adler]

The original philosophy was this:

1) Some headers are designed so they can be used outside WebKit. Those files should not include Platform.h and more importantly should not rely on things defined in Platform.h at all.

2) Other headers are designed for use only inside WebKit. Those files include Platform.h and use it.

After this change, it seems like there is a new class of header that can be used outside WebKit, but if used inside WebKit must be included after Platform.h. And presumably "config.h" is supposed to mitigate that problem. But I don’t understand how a header that uses the Platform.h macros can be used outside WebKit without Platform.h.

[Joseph Pecoraro]

(In reply to comment #10)
> This change needed more discussion before the change was made. Not sure what to do about it now.

I agree with Darin here. Assertions.h makes heavy use of WTF / Platform macros. If I just make a test file to include <wtf/Assertions.h> on its own this causes more then 20 errors (clang stops printing errors)

    #include <wtf/Assertions.h>

    int main(int argc, char *argv[])
    {
        ASSERT(true);
        return 0;
    }

Errors look like:

    #if !COMPILER(MSVC)
        ~~~~~~~~~^
    /Volumes/Data/Build/Debug/usr/local/include/wtf/Assertions.h:58:13: error: token is not a valid binary operator in a preprocessor subexpression

    #if HAVE(VARIADIC_MACRO)
        ~~~~^
    /Volumes/Data/Build/Debug/usr/local/include/wtf/Assertions.h:85:9: error: token is not a valid binary operator in a preprocessor subexpression

    /Volumes/Data/Build/Debug/usr/local/include/wtf/Assertions.h:137:1: error: unknown type name 'WTF_EXPORT_PRIVATE'
    WTF_EXPORT_PRIVATE void WTFReportAssertionFailure(const char* file, int line, const char* function, const char* assertion);
    ^
    /Volumes/Data/Build/Debug/usr/local/include/wtf/Assertions.h:137:20: error: expected unqualified-id


In order for this to compile at all I would need to Platform.h and ExportMacros.h. And then be sure to include them before Assertions.h (error prone):

    #include <wtf/Platform.h>
    #include <wtf/ExportMacros.h>
    #include <wtf/Assertions.h>

Removing Platform.h from WebCore headers makes sense. But, I think removing Platform.h from Assertions.h may have been a bad idea. Was there further work planned here?

[David Kilzer (:ddkilzer)]

Comment on attachment 167561 [details]
Proposed fix 0.1

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

>> Source/JavaScriptCore/API/tests/minidom.c:33
>> +#include <wtf/Platform.h>
> 
> Alphabetical sorting problem.  [build/include_order] [4]

Another way to fix this is to create a a prefix header for the minidom target, or to include the "config.h" header in all of the source files in API/tests.

>> Source/JavaScriptCore/API/tests/testapi.c:32
>> +#include <wtf/Platform.h>
> 
> Alphabetical sorting problem.  [build/include_order] [4]

Another way to fix this is to create a a prefix header for the minidom target, or to include the "config.h" header in all of the source files in API/tests.

> Source/WTF/wtf/Assertions.h:-45
> -#include <wtf/Platform.h>

This may need to be added back in the short term to address any build failures, although I think this is correct in the long term.  (Unless there is a special exception for Assertions.h, although I'm not aware of why that would be the case.)

Any project which uses WTF headers needs to know about <wtf/Platform.h> at a project level, which means it should appear in config.h or the project's prefix header.  That's true of JavaScriptCore, WebCore, WebKit and WebKit2 since they all include wtf/Platform.h in their project's config.h header (JSC, WebCore) or prefix header (WebKit) or both (WebKit2).

> Tools/DumpRenderTree/mac/MockGeolocationProvider.mm:26
> +#include <wtf/Platform.h>

Instead of including wtf/Platform.h here, it should include config.h instead:

#import "config.h"

[Alexey Proskuryakov]

Bug 101244 has a partial rollout of this change.