Bug 136521 – [Cocoa] Some WebKitLegacy headers migrated from WebCore incorrectly contain WEBCORE_EXPORT

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

[Cocoa] Some WebKitLegacy headers migrated from WebCore incorrectly contain WEBCORE_EXPORT

[Andy Estes]

Headers migrated from WebCore to WebKitLegacy might contain the WEBCORE_EXPORT macro. Since we don't control the clients of WebKitLegacy, we can't guarantee that this macro will be defined in the translation units including these headers.

Previously we've been defining WEBCORE_EXPORT in some of the projects we control, but this isn't a systematic fix. Since WEBCORE_EXPORT meaningless in API headers, let's teach postprocess-headers.sh to strip it out.

[Andy Estes]

Created attachment 237609 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/config.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


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

[Andy Estes]

On second thought, it probably makes more sense to do the stripping in MigrateHeaders.make, since that only processes the headers that could contain WEBCORE_EXPORT.

[Andy Estes]

Created attachment 237646 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/config.h:33:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


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

[Andy Estes]

Committed r173283: <http://trac.webkit.org/changeset/173283>

[Daniel Bates]

Comment on attachment 237646 [details]
Patch

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

I know this patch was already reviewed and landed. I noticed some minor nits.

> Source/WebKit/mac/ChangeLog:12
> +        This removes WEBCORE_EXPORT and a single following space character but preserves preceeding whitespace.

This regular expression doesn't preserve leading whitespace. It preserves leading space characters. I further remark about this in my comment in file Source/WebKit/mac/MigrateHeaders.make.

> Source/WebKit/mac/MigrateHeaders.make:240
> +WEBCORE_HEADER_REPLACE_RULES = -e s/\<WebCore/\<WebKitLegacy/ -e s/DOMDOMImplementation/DOMImplementation/ -e "s/(^ *)WEBCORE_EXPORT /\1/"

This is OK as-is. Obviously, this regular expression won't match lines that start with tab characters or are a mix of space and tab characters. In theory, we shouldn't have tab characters in source files per the WebKit Code Style Guideline.

> Tools/DumpRenderTree/config.h:30
> +#include <WebCore/PlatformExportMacros.h>
>  #include <wtf/Platform.h>
>  #include <wtf/ExportMacros.h>
>  #include <runtime/JSExportMacros.h>

These headers aren't listed in alphabetical order. Please sort these headers according to the output of the UNIX sort command.

Also, it's unnecessary to explicitly include headers <wtf/Platform.h> and <wtf/ExportMacros.h> as <WebCore/PlatformExportMacros.h> includes them. For completeness, <wtf/ExportMacros.h> is also included by <runtime/JSExportMacros.h>.

> Tools/TestWebKitAPI/config.h:33
> +#include <WebCore/PlatformExportMacros.h>
>  #include <wtf/Platform.h>
>  #include <wtf/ExportMacros.h>
>  #include <runtime/JSExportMacros.h>

Ditto.

> Tools/WebKitTestRunner/config.h:36
> +#include <WebCore/PlatformExportMacros.h>
>  #include <WebKit/WebKit2_C.h>
>  #include <wtf/Platform.h>
>  #include <wtf/ExportMacros.h>
>  #include <runtime/JSExportMacros.h>

Ditto.

[Andy Estes]

(In reply to comment #8)
> (From update of attachment 237646 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237646&action=review
> 
> I know this patch was already reviewed and landed. I noticed some minor nits.
> 
> > Source/WebKit/mac/ChangeLog:12
> > +        This removes WEBCORE_EXPORT and a single following space character but preserves preceeding whitespace.
> 
> This regular expression doesn't preserve leading whitespace. It preserves leading space characters. I further remark about this in my comment in file Source/WebKit/mac/MigrateHeaders.make.

True, my terminology was imprecise. I only meant to preserve spaces.

> 
> > Source/WebKit/mac/MigrateHeaders.make:240
> > +WEBCORE_HEADER_REPLACE_RULES = -e s/\<WebCore/\<WebKitLegacy/ -e s/DOMDOMImplementation/DOMImplementation/ -e "s/(^ *)WEBCORE_EXPORT /\1/"
> 
> This is OK as-is. Obviously, this regular expression won't match lines that start with tab characters or are a mix of space and tab characters. In theory, we shouldn't have tab characters in source files per the WebKit Code Style Guideline.

Right. We shouldn't have tab characters in header files, so I don't account for them.

> 
> > Tools/DumpRenderTree/config.h:30
> > +#include <WebCore/PlatformExportMacros.h>
> >  #include <wtf/Platform.h>
> >  #include <wtf/ExportMacros.h>
> >  #include <runtime/JSExportMacros.h>
> 
> These headers aren't listed in alphabetical order. Please sort these headers according to the output of the UNIX sort command.

Oops, I didn't see the existing sorting mistake. Thanks for pointing it out.

> 
> Also, it's unnecessary to explicitly include headers <wtf/Platform.h> and <wtf/ExportMacros.h> as <WebCore/PlatformExportMacros.h> includes them. For completeness, <wtf/ExportMacros.h> is also included by <runtime/JSExportMacros.h>.

Ok, I'll clean this up.

Thanks Dan!

[Andy Estes]

Addressed Dan's feedback in r173288: <http://trac.webkit.org/changeset/173288>