| Summary: | WebCore needs WEBCORE_TESTING macro to mark methods being exported for testing. | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||||||||||||||||||||
| Component: | Tools / Tests | Assignee: | Hajime Morrita <morrita> | ||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, ap, dglazkov, gustavo, philn, simonjam, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
|
Description
Hajime Morrita
2012-07-09 01:51:47 PDT
Created attachment 151395 [details]
Patch
Created attachment 151400 [details]
Patch
Comment on attachment 151400 [details] Patch Attachment 151400 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13188011 Comment on attachment 151400 [details] Patch Attachment 151400 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13184019 Comment on attachment 151400 [details] Patch Attachment 151400 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13168508 Created attachment 151433 [details]
Patch
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 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. Created attachment 151591 [details]
Patch
Created attachment 151603 [details]
Patch
Comment on attachment 151603 [details] Patch Attachment 151603 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13182302 Comment on attachment 151603 [details] Patch Attachment 151603 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13205221 Comment on attachment 151603 [details] Patch Attachment 151603 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13166680 Comment on attachment 151603 [details] Patch Attachment 151603 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13205225 Comment on attachment 151603 [details] Patch Attachment 151603 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13203226 Comment on attachment 151603 [details] Patch Attachment 151603 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13207194 Created attachment 151634 [details]
Patch
Comment on attachment 151634 [details] Patch Attachment 151634 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13180365 Comment on attachment 151634 [details] Patch Attachment 151634 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13211013 Created attachment 151658 [details]
Patch
Comment on attachment 151658 [details] Patch Attachment 151658 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13182383 Comment on attachment 151658 [details] Patch Attachment 151658 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13201291 Comment on attachment 151658 [details] Patch Attachment 151658 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13200286 Created attachment 152131 [details]
Patch
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 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
Created attachment 152188 [details]
Patch
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 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
> 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.
(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 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 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. Created attachment 152684 [details]
Patch for landing
Comment on attachment 152684 [details] Patch for landing Clearing flags on attachment: 152684 Committed r122805: <http://trac.webkit.org/changeset/122805> All reviewed patches have been landed. Closing bug. |