[webkit-reviews] review granted: [Bug 90764] WebCore needs WEBCORE_TESTING macro to mark methods being exported for testing. : [Attachment 152188] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Hajime Morrita
<morrita at google.com>'s request for review:
Bug 90764: WebCore needs WEBCORE_TESTING macro to mark methods being exported
for testing.
https://bugs.webkit.org/show_bug.cgi?id=90764

Attachment 152188: Patch
https://bugs.webkit.org/attachment.cgi?id=152188&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=152188&action=review


This looks great!  I'm happy not to have to edit these export lists.

> Source/WebCore/platform/PlatformExportMacros.h:36
> +#if defined(BUILDING_WebCore) || defined(BUILDING_WebKit) || \
> +    defined(STATICALLY_LINKED_WITH_WebCore) ||
defined(STATICALLY_LINKED_WITH_WebKit)

This list is somewhat mysterious, but I see you're just making it not
copy/paste code.

> Source/WebCore/platform/PlatformExportMacros.h:56
>  #define WEBKIT_EXPORTDATA __declspec(dllexport)
>  #else
>  #define WEBKIT_EXPORTDATA __declspec(dllimport)

Should we make these WTF_EXPORT_DECLARATION and WTF_IMPORT_DECLARATION
respectively?

> Source/WebCore/platform/PlatformExportMacros.h:70
> +#define WEBCORE_TESTING WTF_EXPORT_DECLARATION

I wonder about the parallelism in naming between WEBCORE_TESTING and
WEBKIT_EXPORTDATA.  WEBCORE_EXPORTTESTING is kind of long and ugly, so maybe
this is fine.