Bug 164937 – [cmake][OpenWebRTC] Move SDPProcessorScriptResource rules to common WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Olivier Blin]

OpenWebRTC cmake build rules have been moved to a common gstreamer directory (bug 154116) and SDPProcessorScriptResource has been moved in openwebrtc directory (bug 163778).
But the SDP_PROCESSOR_SCRIPTS are still living in PlatformGTK.cmake
They should be moved to the common place in platform/GStreamer.cmake

This is needed to build OpenWebRTC support in other gstreamer-based ports, WPE in my case, probably EFL and Qt as well.

[Olivier Blin]

Created attachment 295168 [details]
Patch

[Konstantin Tokarev]

Modules/mediastream/sdp.js is certainly not specific to GStreamer, it used used by Mac port too, so it should be moved to WebCore/CMakeLists.txt

I assume the same is true for SDPProcessorScriptResource.cpp, just nobody took care to fix building MediaStream in Mac port with cmake.

[Olivier Blin]

(In reply to comment #2)
> Modules/mediastream/sdp.js is certainly not specific to GStreamer, it used
> used by Mac port too, so it should be moved to WebCore/CMakeLists.txt
> 
> I assume the same is true for SDPProcessorScriptResource.cpp, just nobody
> took care to fix building MediaStream in Mac port with cmake.

Actually, it seems the SDP dependencies are wrong.
It lists Source/WebCore/platform/mediastream/openwebrtc/SDPProcessorScriptResource.cpp, but the file is no actually in the Source/WebCore/platform/mediastream/ directory (moved by Youenn in bug 163940, which is about fixing the Mac bots).

So, instead of moving the cmake rules to the gstreamer directory, we should actually move them into the main WebCore/CMakeLists.txt, as you suggests.

Thanks for the review

[Olivier Blin]

Created attachment 295202 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 295202 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295202&action=review

> Source/WebCore/CMakeLists.txt:3674
> +        platform/mediastream/SDPProcessorScriptResource.cpp

This file should be moved to the main sources list, because its contents are already guarded with ENABLE(WEB_RTC)

[Olivier Blin]

Created attachment 295207 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 295207 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295207&action=review

> Source/WebCore/CMakeLists.txt:3673
> +endif ()

Sorry for not spotting all issues at once :)

Following if() block checks if WebCore_SDP_PROCESSOR_SCRIPTS is set, now it's always true, so both blocks can be merged into single if (ENABLE_MEDIA_STREAM) block.

[Olivier Blin]

> Following if() block checks if WebCore_SDP_PROCESSOR_SCRIPTS is set, now
> it's always true, so both blocks can be merged into single if
> (ENABLE_MEDIA_STREAM) block.

Just this then:

if (ENABLE_MEDIA_STREAM)
    MAKE_JS_FILE_ARRAYS(
        ${DERIVED_SOURCES_WEBCORE_DIR}/SDPProcessorScriptsData.cpp
        ${DERIVED_SOURCES_WEBCORE_DIR}/SDPProcessorScriptsData.h
        ${WEBCORE_DIR}/Modules/mediastream/sdp.js
        ${WEBCORE_DIR}/platform/mediastream/SDPProcessorScriptResource.cpp
    )
endif ()

Youenn, is that ok?

[Olivier Blin]

Created attachment 295257 [details]
Patch

[Olivier Blin]

Created attachment 295258 [details]
Patch

We still need the intermediate variables

[Konstantin Tokarev]

LGTM

[youenn fablet]

This seems mostly good to me.
I am just wondering whether we should use ENABLE_WEB_RTC for SDP stuff instead of ENABLE_MEDIA_STREAM?

[Philippe Normand]

(In reply to comment #12)
> This seems mostly good to me.
> I am just wondering whether we should use ENABLE_WEB_RTC for SDP stuff
> instead of ENABLE_MEDIA_STREAM?

Makes sense.

[Olivier Blin]

Created attachment 295293 [details]
Patch

[youenn fablet]

Comment on attachment 295293 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 295293 [details]
Patch

Clearing flags on attachment: 295293

Committed r208948: <http://trac.webkit.org/changeset/208948>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Olivier Blin]

(In reply to comment #15)
> Comment on attachment 295293 [details]
> Patch
> 
> Thanks!


Thanks all for the careful review!

By the way, would it make sense to move WebRTC specific classes to a specific subdirectory under WebCore/Modules?
It would help to make explicit what is for the MediaStream API, and what is for WebRTC.

[youenn fablet]

> By the way, would it make sense to move WebRTC specific classes to a
> specific subdirectory under WebCore/Modules?
> It would help to make explicit what is for the MediaStream API, and what is
> for WebRTC.

It might be worth splitting mediastream in Modules and platform, although there will be obviously some interactions between the two