Bug 90764 - WebCore needs WEBCORE_TESTING macro to mark methods being exported for testing.
Summary: WebCore needs WEBCORE_TESTING macro to mark methods being exported for testing.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hajime Morrita
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-09 01:51 PDT by Hajime Morrita
Modified: 2012-07-16 21:33 PDT (History)
8 users (show)

See Also:


Attachments
Patch (20.57 KB, patch)
2012-07-09 21:42 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (13.89 KB, patch)
2012-07-09 22:37 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (11.41 KB, patch)
2012-07-10 03:52 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (11.51 KB, patch)
2012-07-10 21:46 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (11.41 KB, patch)
2012-07-10 22:45 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (11.51 KB, patch)
2012-07-11 01:04 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (11.52 KB, patch)
2012-07-11 02:40 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Patch (15.05 KB, patch)
2012-07-12 19:02 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-03 (537.49 KB, application/zip)
2012-07-13 01:25 PDT, WebKit Review Bot
no flags Details
Patch (15.04 KB, patch)
2012-07-13 01:37 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (346.46 KB, application/zip)
2012-07-13 04:00 PDT, WebKit Review Bot
no flags Details
Patch for landing (15.04 KB, patch)
2012-07-16 20:00 PDT, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2012-07-09 01:51:47 PDT
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.
Comment 1 Hajime Morrita 2012-07-09 21:42:52 PDT
Created attachment 151395 [details]
Patch
Comment 2 Hajime Morrita 2012-07-09 22:37:05 PDT
Created attachment 151400 [details]
Patch
Comment 3 Early Warning System Bot 2012-07-09 23:17:48 PDT
Comment on attachment 151400 [details]
Patch

Attachment 151400 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13188011
Comment 4 Build Bot 2012-07-10 00:24:21 PDT
Comment on attachment 151400 [details]
Patch

Attachment 151400 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13184019
Comment 5 Build Bot 2012-07-10 02:14:23 PDT
Comment on attachment 151400 [details]
Patch

Attachment 151400 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13168508
Comment 6 Hajime Morrita 2012-07-10 03:52:23 PDT
Created attachment 151433 [details]
Patch
Comment 7 Alexey Proskuryakov 2012-07-10 06:17:56 PDT
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.
Comment 8 Hajime Morrita 2012-07-10 17:21:54 PDT
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.
Comment 9 Hajime Morrita 2012-07-10 21:46:58 PDT
Created attachment 151591 [details]
Patch
Comment 10 Hajime Morrita 2012-07-10 22:45:54 PDT
Created attachment 151603 [details]
Patch
Comment 11 Build Bot 2012-07-10 23:07:36 PDT
Comment on attachment 151603 [details]
Patch

Attachment 151603 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13182302
Comment 12 WebKit Review Bot 2012-07-10 23:13:08 PDT
Comment on attachment 151603 [details]
Patch

Attachment 151603 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13205221
Comment 13 Early Warning System Bot 2012-07-10 23:14:48 PDT
Comment on attachment 151603 [details]
Patch

Attachment 151603 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13166680
Comment 14 Early Warning System Bot 2012-07-10 23:19:12 PDT
Comment on attachment 151603 [details]
Patch

Attachment 151603 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13205225
Comment 15 Gyuyoung Kim 2012-07-10 23:36:46 PDT
Comment on attachment 151603 [details]
Patch

Attachment 151603 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13203226
Comment 16 Build Bot 2012-07-11 00:02:51 PDT
Comment on attachment 151603 [details]
Patch

Attachment 151603 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13207194
Comment 17 Hajime Morrita 2012-07-11 01:04:27 PDT
Created attachment 151634 [details]
Patch
Comment 18 Build Bot 2012-07-11 01:47:51 PDT
Comment on attachment 151634 [details]
Patch

Attachment 151634 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13180365
Comment 19 Early Warning System Bot 2012-07-11 01:57:55 PDT
Comment on attachment 151634 [details]
Patch

Attachment 151634 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13211013
Comment 20 Hajime Morrita 2012-07-11 02:40:52 PDT
Created attachment 151658 [details]
Patch
Comment 21 Build Bot 2012-07-11 03:16:13 PDT
Comment on attachment 151658 [details]
Patch

Attachment 151658 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13182383
Comment 22 Gustavo Noronha (kov) 2012-07-11 03:35:51 PDT
Comment on attachment 151658 [details]
Patch

Attachment 151658 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13201291
Comment 23 Early Warning System Bot 2012-07-11 04:15:19 PDT
Comment on attachment 151658 [details]
Patch

Attachment 151658 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13200286
Comment 24 Hajime Morrita 2012-07-12 19:02:06 PDT
Created attachment 152131 [details]
Patch
Comment 25 WebKit Review Bot 2012-07-13 01:25:35 PDT
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
Comment 26 WebKit Review Bot 2012-07-13 01:25:39 PDT
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
Comment 27 Hajime Morrita 2012-07-13 01:37:51 PDT
Created attachment 152188 [details]
Patch
Comment 28 WebKit Review Bot 2012-07-13 04:00:51 PDT
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
Comment 29 WebKit Review Bot 2012-07-13 04:00:56 PDT
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
Comment 30 Adam Barth 2012-07-13 10:15:03 PDT
> 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.
Comment 31 Hajime Morrita 2012-07-16 17:23:51 PDT
(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? ;-)
Comment 32 Adam Barth 2012-07-16 17:36:16 PDT
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.
Comment 33 Hajime Morrita 2012-07-16 18:57:25 PDT
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.
Comment 34 Hajime Morrita 2012-07-16 20:00:03 PDT
Created attachment 152684 [details]
Patch for landing
Comment 35 WebKit Review Bot 2012-07-16 21:33:37 PDT
Comment on attachment 152684 [details]
Patch for landing

Clearing flags on attachment: 152684

Committed r122805: <http://trac.webkit.org/changeset/122805>
Comment 36 WebKit Review Bot 2012-07-16 21:33:44 PDT
All reviewed patches have been landed.  Closing bug.