[webkit-reviews] review denied: [Bug 90764] WebCore needs WEBCORE_EXPORT_TESTING macro to mark methods being exported for testing. : [Attachment 151433] Patch

[bugzilla-daemon at webkit.org]

Hajime Morrita <morrita at google.com> has denied	review:
Bug 90764: WebCore needs WEBCORE_EXPORT_TESTING macro to mark methods being
exported for testing.
https://bugs.webkit.org/show_bug.cgi?id=90764

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

------- Additional Comments from Hajime Morrita <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=151433&action=review


Hi Alexey, thanks for your feedback! I'll address them shortly.

>> Source/WTF/wtf/ExportMacros.h:54
>> +#define WTF_HIDDEN_DECL
> 
> Why are you renaming these? I think that having the word "declaration" in
every declaration's name is an overkill.
> 
> In any case, please don't abbreviate.

I'll rename it to eliminate the abbreviation.
Point here is that I want to move the compiler specific definitions out from
USE(EXPORT_MACROS)
since USE(EXPORT_MACROS) and USE(EXPORT_MACROS_FOR_TESTING) is orthogonal
(EXPORT_MACROS is disabled on windows but I'd like to enable FOR_TESTING on
windows)

>> Source/WTF/wtf/ExportMacros.h:67
>> +#endif
> 
> This looks confusing. Is WTF_SHOULD_EXPORT_SYMBOLS_IF_NEEDED only about
symbols exported from WTF? But why is it true when statically linking with WTF?


This is just an extraction from the original conditional of line 70 and lie 96.

But I admit the name was bad. Will rename somehow.

>> Source/WTF/wtf/ExportMacros.h:110
>> +#if defined(WTF_SHOULD_EXPORT_SYMBOLS_IF_NEEDED)
> 
> I'm not sure if this phrase means much. All code should do things that are
needed. Can you rephrase or explain this, perhaps as a normal English sentence
first?

OK. will do.

>> Source/WTF/wtf/ExportMacros.h:111
>> +#define WTF_EXPORT_TESTING WTF_EXPORT_DECL
> 
> "WTF export testing" does not parse when using English grammar. Please
correct.

True. will fix.

>> Source/WTF/wtf/ExportMacros.h:127
>> +#endif
> 
> No-op here.

Will fix.