Bug 175542 – [PAL] Add PALPrefix to project

• Status: NEW
• Component: Platform

[Don Olmstead]

XCode is implicitly including WebCorePrefix in the WebCore project. Emulate the same thing within PAL.

[Don Olmstead]

Created attachment 318050 [details]
WIP Patch

Copies the contents of WebCorePrefix.h/cpp for PALPrefix.h/cpp.

[Build Bot]

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


ERROR: Source/WebCore/PAL/PALPrefix.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/PALPrefix.h:61:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/PALPrefix.h:91:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/PAL/PALPrefix.h:145:  "windows.h" already included at Source/WebCore/PAL/PALPrefix.h:124  [build/include] [4]
Total errors found: 4 in 5 files


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

[Myles C. Maxfield]

Comment on attachment 318050 [details]
WIP Patch

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

> Source/WebCore/PAL/PALPrefix.cpp:26
> +#include "PALPrefix.h"

What is the point of this .cpp file? Can we remove both this one and the one from WebCore?

> Source/WebCore/PAL/PALPrefix.h:104
> +#if OS(WINDOWS)
> +#ifndef CF_IMPLICIT_BRIDGING_ENABLED
> +#define CF_IMPLICIT_BRIDGING_ENABLED
> +#endif
> +
> +#ifndef CF_IMPLICIT_BRIDGING_DISABLED
> +#define CF_IMPLICIT_BRIDGING_DISABLED
> +#endif

Might want to unify this block with the previous OS(WINDOWS) block

> Source/WebCore/PAL/PALPrefix.h:127
> +#if OS(WINDOWS)

Ditto.

> Source/WebCore/PAL/PALPrefix.h:132
> +// FIXME <rdar://problem/8208868> Remove support for obsolete ColorSync API, CoreServices header in CoreGraphics
> +// We can remove this once the new ColorSync APIs are available in an internal Safari SDK.
> +#include <ColorSync/ColorSync.h>

This radar is 8 years old; surely we can remove this by now.

> Source/WebCore/PAL/pal/system/mac/SoundMac.mm:-29
> -#import <AppKit/AppKit.h>

Why did you remove this?

[Ross Kirsling]

> > Source/WebCore/PAL/pal/system/mac/SoundMac.mm:-29
> > -#import <AppKit/AppKit.h>
> 
> Why did you remove this?

I added this line in https://bugs.webkit.org/show_bug.cgi?id=173999 as a per-file solution; this bug aims to do things the "right way" (i.e. per-project).

[Tim Horton]

No, we should be /able/ to build without the prefix header, it’s just about build time optimization. See the commment at the top of the file.

[Don Olmstead]

(In reply to Tim Horton from comment #5)
> No, we should be /able/ to build without the prefix header, it’s just about
> build time optimization. See the commment at the top of the file.

We were making some changes in future patches that were relying on WebCorePrefix being used which is why we made this particular patch. If we should be able to build without this and its just a build time optimization perhaps we should approach this differently.

[Myles C. Maxfield]

(In reply to Don Olmstead from comment #6)
> (In reply to Tim Horton from comment #5)
> > No, we should be /able/ to build without the prefix header, it’s just about
> > build time optimization. See the commment at the top of the file.
> 
> We were making some changes in future patches that were relying on
> WebCorePrefix being used which is why we made this particular patch. If we
> should be able to build without this and its just a build time optimization
> perhaps we should approach this differently.

Currently, it seems that the prefix header *is* required for building, contrary to the claim.

We should either:
1) Remove the claim that we can build without the prefix header
2) Fix the places which fail to build without the prefix header

I don't have an opinion about which of these we should do, but maybe Tim does.

[Tim Horton]

(In reply to Myles C. Maxfield from comment #7)
> (In reply to Don Olmstead from comment #6)
> > (In reply to Tim Horton from comment #5)
> > > No, we should be /able/ to build without the prefix header, it’s just about
> > > build time optimization. See the commment at the top of the file.
> > 
> > We were making some changes in future patches that were relying on
> > WebCorePrefix being used which is why we made this particular patch. If we
> > should be able to build without this and its just a build time optimization
> > perhaps we should approach this differently.
> 
> Currently, it seems that the prefix header *is* required for building,
> contrary to the claim.
> 
> We should either:
> 1) Remove the claim that we can build without the prefix header
> 2) Fix the places which fail to build without the prefix header
> 
> I don't have an opinion about which of these we should do, but maybe Tim
> does.

The comment makes it pretty clear which we should do.