Bug 90764 – WebCore needs WEBCORE_TESTING macro to mark methods being exported for testing.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Hajime Morrita]

WEBCORE_TESTING aims to reduce the burden to edito symbol files just for testing APIs.
This will save editing of GTK and Windows ports symbol files.

[Hajime Morrita]

Created attachment 151395 [details]
Patch

[Hajime Morrita]

Created attachment 151400 [details]
Patch

[Early Warning System Bot]

Comment on attachment 151400 [details]
Patch

Attachment 151400 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13188011

[Build Bot]

Comment on attachment 151400 [details]
Patch

Attachment 151400 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13184019

[Build Bot]

Comment on attachment 151400 [details]
Patch

Attachment 151400 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13168508

[Hajime Morrita]

Created attachment 151433 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 151433 [details]
Patch

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

> Source/WTF/wtf/ExportMacros.h:54
> -#define WTF_EXPORT __declspec(dllexport)
> -#define WTF_IMPORT __declspec(dllimport)
> -#define WTF_HIDDEN
> +#define WTF_EXPORT_DECL __declspec(dllexport)
> +#define WTF_IMPORT_DECL __declspec(dllimport)
> +#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.

> Source/WTF/wtf/ExportMacros.h:67
> +#if defined(BUILDING_WTF) || defined(STATICALLY_LINKED_WITH_WTF) || (PLATFORM(WX) && defined(BUILDING_JavaScriptCore))
> +#define WTF_SHOULD_EXPORT_SYMBOLS_IF_NEEDED 1
> +#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?

> 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?

> 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.

> Source/WTF/wtf/ExportMacros.h:127
> +// See note in wtf/Platform.h for more info on EXPORT_MACROS.
> +#if USE(EXPORT_MACROS_FOR_TESTING)
> +#if OS(WINDOWS) && !COMPILER(GCC)
> +#else
> +#endif
> +#endif

No-op here.

[Hajime Morrita]

Comment on attachment 151433 [details]
Patch

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.

[Hajime Morrita]

Created attachment 151591 [details]
Patch

[Hajime Morrita]

Created attachment 151603 [details]
Patch

[Build Bot]

Comment on attachment 151603 [details]
Patch

Attachment 151603 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13182302

[WebKit Review Bot]

Comment on attachment 151603 [details]
Patch

Attachment 151603 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13205221

[Early Warning System Bot]

Comment on attachment 151603 [details]
Patch

Attachment 151603 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13166680

[Early Warning System Bot]

Comment on attachment 151603 [details]
Patch

Attachment 151603 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13205225

[Gyuyoung Kim]

Comment on attachment 151603 [details]
Patch

Attachment 151603 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13203226

[Build Bot]

Comment on attachment 151603 [details]
Patch

Attachment 151603 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13207194

[Hajime Morrita]

Created attachment 151634 [details]
Patch

[Build Bot]

Comment on attachment 151634 [details]
Patch

Attachment 151634 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13180365

[Early Warning System Bot]

Comment on attachment 151634 [details]
Patch

Attachment 151634 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13211013

[Hajime Morrita]

Created attachment 151658 [details]
Patch

[Build Bot]

Comment on attachment 151658 [details]
Patch

Attachment 151658 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13182383

[Gustavo Noronha (kov)]

Comment on attachment 151658 [details]
Patch

Attachment 151658 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13201291

[Early Warning System Bot]

Comment on attachment 151658 [details]
Patch

Attachment 151658 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13200286

[Hajime Morrita]

Created attachment 152131 [details]
Patch

[WebKit Review Bot]

Comment on attachment 152131 [details]
Patch

Attachment 152131 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13205986

New failing tests:
http/tests/w3c/webperf/approved/navigation-timing/html/test_performance_attributes_exist_in_object.html

[WebKit Review Bot]

Created attachment 152182 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Hajime Morrita]

Created attachment 152188 [details]
Patch

[WebKit Review Bot]

Comment on attachment 152188 [details]
Patch

Attachment 152188 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13236076

New failing tests:
http/tests/w3c/webperf/approved/navigation-timing/html/test_performance_attributes_exist_in_object.html

[WebKit Review Bot]

Created attachment 152210 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Adam Barth]

> http/tests/w3c/webperf/approved/navigation-timing/html/test_performance_attributes_exist_in_object.html

Sorry for the noise.  I've marked this test as flaky.

[Hajime Morrita]

(In reply to comment #30)
> > http/tests/w3c/webperf/approved/navigation-timing/html/test_performance_attributes_exist_in_object.html
> 
> Sorry for the noise.  I've marked this test as flaky.

Thanks for your help! What do you think about this patch btw? ;-)

[Adam Barth]

Comment on attachment 152188 [details]
Patch

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.

[Hajime Morrita]

Comment on attachment 152188 [details]
Patch

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

Thanks for taking looks this quick! I'll rebase and land this shortly.

>> Source/WebCore/platform/PlatformExportMacros.h:36
>> +    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.

yup.this is a combination of historical artifact and tricky build configurations...
Some ports put WebCore into WebKit and some do not. some make WebCore static and link it into WebKit, etc.

>> 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.

Yup. I'm choosing this short name since this would be here and there soon.

[Hajime Morrita]

Created attachment 152684 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 152684 [details]
Patch for landing

Clearing flags on attachment: 152684

Committed r122805: <http://trac.webkit.org/changeset/122805>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.