Bug 209287 – Make the MediaSample::toJSONString method generic

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Philippe Normand]

As the current Objc impl is cross-platform already, let's move it to the base class?

[Philippe Normand]

Created attachment 393988 [details]
Patch

[Eric Carlson]

Comment on attachment 393988 [details]
Patch

r=me once the bots are happy

[Philippe Normand]

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.

[Eric Carlson]

I don't understand the failure either. I applied your patch locally and it builds without error.

[Eric Carlson]

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.

[Philippe Normand]

Created attachment 394165 [details]
Patch

[Philippe Normand]

Committed r258807: <https://trac.webkit.org/changeset/258807>

[Radar WebKit Bug Importer]

<rdar://problem/60724828>

[Darin Adler]

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?

[Philippe Normand]

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.