http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/WebKit.gyp&exact_package=chromium&q=file:gyp%20file:third_party/webkit%20:gtest&type=cs&l=710 makes the webkit target depend on test code in the shared-library build. This is terrible for various reasons: - It violates the ODR by whole-sale linking in static libraries which are also linked into the main binary directly (http://crbug.com/112539) - It holds non-test code hostage to the standards of test-code (e.g. http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/WebKit.gyp&exact_package=chromium&q=file:gyp%20file:third_party/webkit%20:gtest&type=cs&l=763) - It makes the build bigger than it needs to be The madness must stop.
Created attachment 130908 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
I think it is great that you are taking this on. I think it makes good sense to make our build of WebKit.DLL export symbols from WebCore that are used by our unit tests. I am concerned about using WTF_EXPORT outside of the WTF code base. I think it would be better to invent a similar define for each module. So, WebCore should have its own WEBCORE_EXPORT most likely. This may be something we need to discuss more broadly with the community as other ports use the other way of exporting symbols from WebCore (.def / WebCore.exp). -Darin
Created attachment 130918 [details] Patch
(In reply to comment #3) > I am concerned about using WTF_EXPORT outside of the WTF code base. I think it would be better to invent a similar define for each module. So, WebCore should have its own WEBCORE_EXPORT most likely. In looking for where to put the proposed new WEBCORE_EXPORT I found Source/WebCore/platform/PlatformExportMacros.h which already defines WEBKIT_EXPORTDATA as what I would have put in WEBCORE_EXPORT. So the second patch updates to use that instead of WTF_EXPORT{,DATA}. SGTY? > This may be something we need to discuss more broadly with the community as other ports use the other way of exporting symbols from WebCore (.def / WebCore.exp). ISTM there's already quite a lot of use of _EXPORT macros (across ports) so not sure if this is needed. Adam: do you have an opinion on this?
Comment on attachment 130918 [details] Patch Attachment 130918 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11906194
> Adam: do you have an opinion on this? I agree with fishd that this is a change we should discuss with the boarder community before making. Different ports have different needs for exporting symbols from WebCore. It's not obvious me to what's best. The simplest solution would be to create a Chromium equivalent to WebCore.exp.in that lists the symbols that Chromium wants to export from WebCore. However, maintaining that file is a pain and I wouldn't wish to impose the maintenance of another such file on the rest of the community.
Comment on attachment 130918 [details] Patch Attachment 130918 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11915210
Comment on attachment 130918 [details] Patch Attachment 130918 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/11911212
(In reply to comment #7) > I agree with fishd that this is a change we should discuss with the boarder community before making. Sent email to webkit-dev. Let the opinions flow!
Comment on attachment 130918 [details] Patch Attachment 130918 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11903360
Comment on attachment 130918 [details] Patch Attachment 130918 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11894426
Comment on attachment 130918 [details] Patch I would probably prefer to introduce a WEBCORE_EXPORT macro, possibly renaming WEBKIT_EXPORTDATA to be that. That seems clearer to me. Otherwise, this patch looks to correctly implement the idea of exporting functions from WebCore to support the Chromium port's unit tests.
(In reply to comment #13) > (From update of attachment 130918 [details]) > I would probably prefer to introduce a WEBCORE_EXPORT macro, possibly renaming WEBKIT_EXPORTDATA to be that. That seems clearer to me. Went to see how much work it'd be to do this, and AFAICT *nobody* uses it today. How worried do I need to be about hidden codebases? (safari, other ports not part of the third_party/WebKit tree in a chromium checkout) I'll do the rename as part of this patch if there are really no other use sites.
> How worried do I need to be about hidden codebases? The WebKit project explicitly doesn't care about hidden codebases. That's the carrot for joining the community.
Created attachment 131197 [details] Patch
(In reply to comment #16) > Created an attachment (id=131197) [details] > Patch Renamed s/WEBKIT_EXPORTDATA/WEBCORE_EXPORT/g and added an explicit #include to hopefully greenify the red EWS runs from the last patch.
Comment on attachment 131197 [details] Patch Attachment 131197 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11937252
Comment on attachment 131197 [details] Patch Attachment 131197 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11939261
Created attachment 131200 [details] Patch Even more explicit includes of PlatformExportMacros.h
Created attachment 131245 [details] Patch rename WEBCORE_EXPORT to WEBCORE_EXPORT_PRIVATE per morrita@ preference
Comment on attachment 131245 [details] Patch I'm clearing the review flag from this patch since it's not clear how we should proceed here.
Thanks Adam. FTR, my plan is to wait for bug 82948 to finish getting squashed before re-visiting this.
Makes sense.