Consolidate soft-linked CMTimeMakeWithSeconds() function into a single header/source file. Advantages: - In the future, using a soft-linked function that is already defined is as easy as including a new header file in any source file. - Reduces duplicate code for soft-linking the same framework and the same method in multiple source files. - Reduces static variables that cause dirty pages for soft-linking the same framework and the same method in multiple source files. - New code puts static soft-link methods into the WebCore namespace. - Inline method moved to header, making it more likely to be inlined and thus fixing weak external symbol issues worked around in Bug 141607. Disadvantages: - This strategy requires separate macros for header (declaration) and source (implementation) files, but this is mitigated by extra declarations which will cause the compile to fail if they don't match.
Created attachment 246668 [details] Feedback on approach and test compile v1
Attachment 246668 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:138: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 246668 [details] Feedback on approach and test compile v1 View in context: https://bugs.webkit.org/attachment.cgi?id=246668&action=review > Source/WebCore/Configurations/WebCore.unexp:-55 > -_CMTimeMakeWithSeconds Why just this one symbol? There are logs of CoreMedia symbols here. > Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:8229 > + <ClCompile Include="..\platform\cocoa\CoreMediaSoftLink.cpp" /> Windows probably shouldn't include things in the cocoa directory.
(In reply to comment #3) > Comment on attachment 246668 [details] > Feedback on approach and test compile > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246668&action=review > > > Source/WebCore/Configurations/WebCore.unexp:-55 > > -_CMTimeMakeWithSeconds > > Why just this one symbol? There are logs of CoreMedia symbols here. Yes, there are a lot of symbols. I want to get just one working with this new strategy before moving more (all) of them. That should result in fewer broken builds. As for why CMTimeMakeWithSeconds(), I'm targeting one of the "highest value" symbols (which are duplicated the most) per Bug 141625 Comment #0 and which was worked around for Bug 141607. > > Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:8229 > > + <ClCompile Include="..\platform\cocoa\CoreMediaSoftLink.cpp" /> > > Windows probably shouldn't include things in the cocoa directory. Correct. I suppose "cf" is the best existing subdirectory. I wanted to get a first pass at a patch up for the EWS bots to chew on, since I'll undoubtedly have Windows fixes to make, so I didn't fix this in the first patch.
Comment on attachment 246668 [details] Feedback on approach and test compile v1 View in context: https://bugs.webkit.org/attachment.cgi?id=246668&action=review > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:78 > +#include "CoreMediaSoftLink.h" > #include "CoreMediaSoftLinking.h" If the strategy in this patch is what we want to use going forward, we should rename CoreMediaSoftLinking.h to CoreMediaSoftLinkWin.h (to disambiguate it from CoreMediaSoftLink.h).
Errors with patch v1: CoreMediaSoftLink.cpp 1>c:\cygwin\home\buildbot\webkit\source\webcore\platform\cocoa\CoreMediaSoftLink.h(42): warning C4273: 'CMTimeMakeWithSeconds' : inconsistent dll linkage (..\platform\cocoa\CoreMediaSoftLink.cpp) C:\cygwin\home\buildbot\WebKit\WebKitLibraries\win\include\CoreMedia/CMTime.h(63) : see previous definition of 'CMTimeMakeWithSeconds' 1>..\platform\cocoa\CoreMediaSoftLink.cpp(48): error C2732: linkage specification contradicts earlier specification for 'softLink_CMTimeMakeWithSeconds' ..\platform\cocoa\CoreMediaSoftLink.cpp(48) : see declaration of 'softLink_CMTimeMakeWithSeconds' 1>..\platform\cocoa\CoreMediaSoftLink.cpp(48): warning C4211: nonstandard extension used : redefined extern to static 1>..\platform\graphics\avfoundation\MediaTimeAVFoundation.cpp(31): fatal error C1083: Cannot open include file: 'CoreMediaSoftLink.h': No such file or directory 1>..\platform\graphics\avfoundation\cf\MediaPlayerPrivateAVFoundationCF.cpp(77): fatal error C1083: Cannot open include file: 'CoreMediaSoftLink.h': No such file or directory
Created attachment 246673 [details] Feedback on approach and test compile v2
(In reply to comment #7) > Created attachment 246673 [details] > Feedback on approach and test compile v2 - Attempted to fix Windows compiler errors. - Moved CoreMediaSoftLink.{cpp,h} from WebCore/platform/cocoa to WebCore/platform/cf.
Attachment 246673 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:138: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #7) > Created attachment 246673 [details] > Feedback on approach and test compile v2 More Windows build errors: CoreMediaSoftLink.cpp 1>c:\cygwin\home\buildbot\webkit\source\webcore\platform\cf\CoreMediaSoftLink.h(42): warning C4273: 'CMTimeMakeWithSeconds' : inconsistent dll linkage (..\platform\cf\CoreMediaSoftLink.cpp) C:\cygwin\home\buildbot\WebKit\WebKitLibraries\win\include\CoreMedia/CMTime.h(63) : see previous definition of 'CMTimeMakeWithSeconds' 1>..\platform\cf\CoreMediaSoftLink.cpp(48): error C2732: linkage specification contradicts earlier specification for 'softLink_CMTimeMakeWithSeconds' ..\platform\cf\CoreMediaSoftLink.cpp(48) : see declaration of 'softLink_CMTimeMakeWithSeconds' 1>..\platform\cf\CoreMediaSoftLink.cpp(48): error C2086: 'CMTime (__cdecl *__cdecl WebCore::softLinkCMTimeMakeWithSeconds)(Float64,int32_t)' : redefinition c:\cygwin\home\buildbot\webkit\source\webcore\platform\cf\CoreMediaSoftLink.h(42) : see declaration of 'WebCore::softLinkCMTimeMakeWithSeconds' MediaTimeAVFoundation.cpp 1>C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\cf\CoreMediaSoftLink.h(42): warning C4273: 'CMTimeMakeWithSeconds' : inconsistent dll linkage (..\platform\graphics\avfoundation\MediaTimeAVFoundation.cpp) C:\cygwin\home\buildbot\WebKit\WebKitLibraries\win\include\CoreMedia/CMTime.h(63) : see previous definition of 'CMTimeMakeWithSeconds' MediaPlayerPrivateAVFoundationCF.cpp 1>C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\cf\CoreMediaSoftLink.h(42): warning C4273: 'CMTimeMakeWithSeconds' : inconsistent dll linkage (..\platform\graphics\avfoundation\cf\MediaPlayerPrivateAVFoundationCF.cpp) C:\cygwin\home\buildbot\WebKit\WebKitLibraries\win\include\CoreMedia/CMTime.h(63) : see previous definition of 'CMTimeMakeWithSeconds'
Created attachment 246686 [details] Feedback on approach and test compile v3
Attachment 246686 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:132: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #11) > Created attachment 246686 [details] > Feedback on approach and test compile v3 * Removed the additional type checks for the soft-linked method in both SOFT_LINK_DLL_IMPORT_FUNCTION_DECL() and SOFT_LINK_DLL_IMPORT_FUNCTION_IMPL(): - WTF_EXTERN_C_BEGIN \ - resultType functionName parameterDeclarations; \ - WTF_EXTERN_C_END \ * Removed 'static' from the declaration of the softLink##functionName declaration in SOFT_LINK_DLL_IMPORT_FUNCTION_DECL().
Comment on attachment 246686 [details] Feedback on approach and test compile v3 View in context: https://bugs.webkit.org/attachment.cgi?id=246686&action=review > Source/WebCore/platform/win/SoftLinking.h:129 > + static resultType(callingConvention*softLink##functionName) parameterDeclarations = init##functionName; \ I don't think this should be static..
Comment on attachment 246686 [details] Feedback on approach and test compile v3 View in context: https://bugs.webkit.org/attachment.cgi?id=246686&action=review > Source/WebCore/platform/cf/CoreMediaSoftLink.h:40 > +#include <CoreMedia/CoreMedia.h> You can write this as <CoreMedia/CMTime.h> on Windows, too. I think using the "CoreMedia.h" header is a holdover from older versions of our support libraries.
(In reply to comment #13) > (In reply to comment #11) > > Created attachment 246686 [details] > > Feedback on approach and test compile v3 > > * Removed the additional type checks for the soft-linked method in both > SOFT_LINK_DLL_IMPORT_FUNCTION_DECL() and > SOFT_LINK_DLL_IMPORT_FUNCTION_IMPL(): > - WTF_EXTERN_C_BEGIN \ > - resultType functionName parameterDeclarations; \ > - WTF_EXTERN_C_END \ > > * Removed 'static' from the declaration of the softLink##functionName > declaration in SOFT_LINK_DLL_IMPORT_FUNCTION_DECL(). Errors in this patch: CoreMediaSoftLink.cpp 1>..\platform\cf\CoreMediaSoftLink.cpp(48): error C2370: 'WebCore::softLinkCMTimeMakeWithSeconds' : redefinition; different storage class c:\cygwin\home\buildbot\webkit\source\webcore\platform\cf\CoreMediaSoftLink.h(42) : see declaration of 'WebCore::softLinkCMTimeMakeWithSeconds' 1>..\platform\cf\CoreMediaSoftLink.cpp(48): error C3861: 'softLinkCMTimeMakeWithSeconds': identifier not found
(In reply to comment #14) > Comment on attachment 246686 [details] > Feedback on approach and test compile v3 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246686&action=review > > > Source/WebCore/platform/win/SoftLinking.h:129 > > + static resultType(callingConvention*softLink##functionName) parameterDeclarations = init##functionName; \ > > I don't think this should be static.. Yes, just like the Mac version. Not sure why it's so hard to see that when looking at the macro. Thanks!
Created attachment 246698 [details] Feedback on approach and test compile v4
Attachment 246698 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:132: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
This looks good on Windows now!
Comment on attachment 246698 [details] Feedback on approach and test compile v4 View in context: https://bugs.webkit.org/attachment.cgi?id=246698&action=review Looks great! r=me > Source/WebCore/platform/cf/CoreMediaSoftLink.cpp:44 > +SOFT_LINK_LIBRARY(CoreMedia) I wonder if this DEBUG_ALL case could be handled inside the Macro definition somehow, so we could just call it SOFT_LINK_FRAMEWORK on both platforms? > Source/WebCore/platform/cf/CoreMediaSoftLink.cpp:48 > +SOFT_LINK_DLL_IMPORT_FUNCTION_IMPL(CoreMedia, CMTimeMakeWithSeconds, CMTime, __cdecl, (Float64 seconds, int32_t preferredTimeScale), (seconds, preferredTimeScale)) It's a shame we need two different macros for this. I wonder if they could be consolidated. > Source/WebCore/platform/win/SoftLinking.h:117 > +#define SOFT_LINK_DLL_IMPORT_FUNCTION_DECL(functionName, resultType, callingConvention, parameterDeclarations, parameterNames) \ I don't think we ever have a calling convention other than "__cdecl". We might be able to just have that in the implementation, which would give the same signature on Windows and Mac.
Comment on attachment 246698 [details] Feedback on approach and test compile v4 Based on an in-person discussion with Brent, I'm going to make mac/SoftLinking.h and win/SoftLinking.h so similar that I won't have to have separate #if PLATFORM(COCOA)/#elif PLATFORM(WIN)/#endif statements in the CoreMediaSoftLink.{cpp,h} files.
Created attachment 246825 [details] Patch v5
Attachment 246825 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:134: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246830 [details] Patch v6
Attachment 246830 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:134: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #25) > Created attachment 246830 [details] > Patch v6 This patch attempts to fix the weak external symbol by not reimplementing the extern "C" function as an inline function. The iOS/Mac port now behaves identically to the Windows port by prepending "softLink_" to the inline function name.
Comment on attachment 246830 [details] Patch v6 Oops. I see the issue here. We now can't include CoreMediaSoftLink.h in CoreMediaSoftLink.cpp because the method will be redefined. New patch forthcoming.
Created attachment 246833 [details] Patch v7
Attachment 246833 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/cf/CoreMediaSoftLink.cpp:29: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:134: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 5 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #30) > Attachment 246833 [details] did not pass style-queue: > > ERROR: Source/WebCore/platform/cf/CoreMediaSoftLink.cpp:29: Found other > header before a header this file implements. Should be: config.h, primary > header, blank line, and then alphabetically sorted. [build/include_order] > [4] This is what caused Patch v6 to fail. Will need to update style checker to ignore this rule for these special *SoftLink.cpp files.
Comment on attachment 246833 [details] Patch v7 View in context: https://bugs.webkit.org/attachment.cgi?id=246833&action=review > Source/WebCore/platform/win/SoftLinking.h:128 > +#define SOFT_LINK_FUNCTION_IMPL(library, functionName, resultType, __cdecl, parameterDeclarations, parameterNames) \ The '__cdecl' here is getting treated like a parameter name, and breaking the build on Windows! Please just get rid of the '__cdecl' value and we should be all set.
Created attachment 246837 [details] Patch v8
Comment on attachment 246837 [details] Patch v8 This version of the patch builds for me correctly on Windows.
Attachment 246837 [details] did not pass style-queue: ERROR: Source/WebCore/platform/mac/SoftLinking.h:117: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:130: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/mac/SoftLinking.h:135: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/platform/cf/CoreMediaSoftLink.cpp:29: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/platform/win/SoftLinking.h:134: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 5 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r180287: <http://trac.webkit.org/changeset/180287>
Comment on attachment 246837 [details] Patch v8 View in context: https://bugs.webkit.org/attachment.cgi?id=246837&action=review > Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:78 > +#include "CoreMediaSoftLink.h" > #include "CoreMediaSoftLinking.h" :-(
Comment on attachment 246837 [details] Patch v8 View in context: https://bugs.webkit.org/attachment.cgi?id=246837&action=review >> Source/WebCore/platform/graphics/avfoundation/cf/MediaPlayerPrivateAVFoundationCF.cpp:78 >> #include "CoreMediaSoftLinking.h" > > :-( It's just a growing pain until this work is complete. :-)