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.
Created attachment 295168 [details] Patch
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.
(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
Created attachment 295202 [details] Patch
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)
Created attachment 295207 [details] Patch
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.
> 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?
Created attachment 295257 [details] Patch
Created attachment 295258 [details] Patch We still need the intermediate variables
LGTM
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?
(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.
Created attachment 295293 [details] Patch
Comment on attachment 295293 [details] Patch Thanks!
Comment on attachment 295293 [details] Patch Clearing flags on attachment: 295293 Committed r208948: <http://trac.webkit.org/changeset/208948>
All reviewed patches have been landed. Closing bug.
(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.
> 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