Bug 136521 - [Cocoa] Some WebKitLegacy headers migrated from WebCore incorrectly contain WEBCORE_EXPORT
Summary: [Cocoa] Some WebKitLegacy headers migrated from WebCore incorrectly contain W...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-03 20:10 PDT by Andy Estes
Modified: 2014-09-04 16:14 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.38 KB, patch)
2014-09-03 20:25 PDT, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (6.80 KB, patch)
2014-09-04 14:10 PDT, Andy Estes
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2014-09-03 20:10:58 PDT
[Cocoa] Some WebKitLegacy headers migrated from WebCore incorrectly contain WEBCORE_EXPORT
Comment 1 Andy Estes 2014-09-03 20:15:36 PDT
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.
Comment 2 Andy Estes 2014-09-03 20:25:11 PDT
Created attachment 237609 [details]
Patch
Comment 3 WebKit Commit Bot 2014-09-03 20:26:55 PDT
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.
Comment 4 Andy Estes 2014-09-03 21:14:26 PDT
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.
Comment 5 Andy Estes 2014-09-04 14:10:51 PDT
Created attachment 237646 [details]
Patch
Comment 6 WebKit Commit Bot 2014-09-04 14:12:51 PDT
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.
Comment 7 Andy Estes 2014-09-04 14:36:30 PDT
Committed r173283: <http://trac.webkit.org/changeset/173283>
Comment 8 Daniel Bates 2014-09-04 14:46:41 PDT
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.
Comment 9 Andy Estes 2014-09-04 14:51:03 PDT
(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!
Comment 10 Andy Estes 2014-09-04 16:14:13 PDT
Addressed Dan's feedback in r173288: <http://trac.webkit.org/changeset/173288>