As the current Objc impl is cross-platform already, let's move it to the base class?
Created attachment 393988 [details] Patch
Comment on attachment 393988 [details] Patch r=me once the bots are happy
Ld /Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/WebKitBuild/Release/TestWebKitAPI normal x86_64 cd /Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/Tools/TestWebKitAPI export MACOSX_DEPLOYMENT_TARGET=10.14 /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -L/Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/WebKitBuild/Release -F/Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/WebKitBuild/Release -F/Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/Tools/TestWebKitAPI/../../WebKitLibraries/WebKitPrivateFrameworkStubs/Mac/101400 -F/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/PrivateFrameworks -F/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/PrivateFrameworks -iframework /Application s/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/Quartz.framework/Frameworks -filelist /Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPI.build/Objects-normal/x86_64/TestWebKitAPI.LinkFileList -Xlinker -rpath -Xlinker @loader_path -mmacosx-version-min=10.14 -Xlinker -object_path_lto -Xlinker /Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPI.build/Objects-normal/x86_64/TestWebKitAPI_lto.o -stdlib=libc++ -fobjc-link-runtime -Wl,-unexported_symbol -Wl,__ZN7testing4Test16TearDownTestCaseEv -Wl,-unexported_symbol -Wl,__ZN7testing4Test13SetUpTestCaseEv -lgtest -force_load /Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/WebKitBuild/Release/libTestWebKitAPI.a -framework JavaScriptCore -framework WebKit -lWebCoreTestSupport -framework Network -framework PDFKit -framework Cocoa -framework Carbon -framework CoreGraphics -framework CoreLocation -framework CoreText -framework IOKit -lboringssl -licucore -framework LocalAuthentication -framework QuartzCore -framework Security -sectcreate __TEXT __info_plist /Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPI.build/Objects-normal/x86_64/Processed-Info.plist -Xlinker -dependency_info -Xlinker /Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/WebKitBuild/TestWebKitAPI.build/Release/TestWebKitAPI.build/Objects-normal/x86_64/TestWebKitAPI_dependency_info.dat -o /Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/WebKitBuild/Release/TestWebKitAPI ld: warning: directory not found for option '-F/Volumes/Data/worker/macOS-Mojave-Release-Build-EWS/build/Tools/TestWebKitAPI/../../WebKitLibraries/WebKitPrivateFrameworkStubs/Mac/101400' Undefined symbols for architecture x86_64: "WebCore::FloatSize::toJSONObject() const", referenced from: WebCore::MediaSample::toJSONString() const in libTestWebKitAPI.a(SampleMap.o) ld: symbol(s) not found for architecture x86_64 clang: error: linker command failed with exit code 1 (use -v to see invocation) I don't understand the error... FloatSize.cpp seems to be in TestWebKitAPI sources already.
I don't understand the failure either. I applied your patch locally and it builds without error.
Comment on attachment 393988 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=393988&action=review > Source/WebCore/platform/MediaSample.h:101 > + virtual String toJSONString() const Nit: this doesn't need to be virtual.
Created attachment 394165 [details] Patch
Committed r258807: <https://trac.webkit.org/changeset/258807>
<rdar://problem/60724828>
Comment on attachment 394165 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394165&action=review > Source/WebCore/platform/MediaSample.h:112 > + String toJSONString() const > + { > + auto object = JSON::Object::create(); > + > + object->setObject("pts"_s, presentationTime().toJSONObject()); > + object->setObject("dts"_s, decodeTime().toJSONObject()); > + object->setObject("duration"_s, duration().toJSONObject()); > + object->setInteger("flags"_s, static_cast<unsigned>(flags())); > + object->setObject("presentationSize"_s, presentationSize().toJSONObject()); > + > + return object->toJSONString(); > + } Why inlined? No handy .cpp file to put it in?
Comment on attachment 394165 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394165&action=review >> Source/WebCore/platform/MediaSample.h:112 >> + } > > Why inlined? No handy .cpp file to put it in? No MediaSample.cpp indeed. This is the only method with more than one line body, I wasn't sure a dedicated .cpp file made sense in this case.