Bug 164937 - [cmake][OpenWebRTC] Move SDPProcessorScriptResource rules to common WebCore
Summary: [cmake][OpenWebRTC] Move SDPProcessorScriptResource rules to common WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-11-18 11:17 PST by Olivier Blin
Modified: 2016-11-21 23:44 PST (History)
11 users (show)

See Also:


Attachments
Patch (3.23 KB, patch)
2016-11-18 11:21 PST, Olivier Blin
no flags Details | Formatted Diff | Diff
Patch (2.95 KB, patch)
2016-11-18 15:34 PST, Olivier Blin
no flags Details | Formatted Diff | Diff
Patch (3.20 KB, patch)
2016-11-18 15:58 PST, Olivier Blin
no flags Details | Formatted Diff | Diff
Patch (3.57 KB, patch)
2016-11-19 00:57 PST, Olivier Blin
no flags Details | Formatted Diff | Diff
Patch (3.47 KB, patch)
2016-11-19 01:09 PST, Olivier Blin
no flags Details | Formatted Diff | Diff
Patch (3.47 KB, patch)
2016-11-21 06:59 PST, Olivier Blin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Olivier Blin 2016-11-18 11:17:13 PST
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.
Comment 1 Olivier Blin 2016-11-18 11:21:26 PST
Created attachment 295168 [details]
Patch
Comment 2 Konstantin Tokarev 2016-11-18 13:15:56 PST
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.
Comment 3 Olivier Blin 2016-11-18 15:14:19 PST
(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
Comment 4 Olivier Blin 2016-11-18 15:34:45 PST
Created attachment 295202 [details]
Patch
Comment 5 Konstantin Tokarev 2016-11-18 15:38:33 PST
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)
Comment 6 Olivier Blin 2016-11-18 15:58:20 PST
Created attachment 295207 [details]
Patch
Comment 7 Konstantin Tokarev 2016-11-18 16:02:57 PST
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.
Comment 8 Olivier Blin 2016-11-18 16:28:02 PST
> 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?
Comment 9 Olivier Blin 2016-11-19 00:57:49 PST
Created attachment 295257 [details]
Patch
Comment 10 Olivier Blin 2016-11-19 01:09:06 PST
Created attachment 295258 [details]
Patch

We still need the intermediate variables
Comment 11 Konstantin Tokarev 2016-11-19 01:19:12 PST
LGTM
Comment 12 youenn fablet 2016-11-19 02:25:28 PST
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?
Comment 13 Philippe Normand 2016-11-21 03:59:21 PST
(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.
Comment 14 Olivier Blin 2016-11-21 06:59:47 PST
Created attachment 295293 [details]
Patch
Comment 15 youenn fablet 2016-11-21 08:01:42 PST
Comment on attachment 295293 [details]
Patch

Thanks!
Comment 16 WebKit Commit Bot 2016-11-21 08:25:59 PST
Comment on attachment 295293 [details]
Patch

Clearing flags on attachment: 295293

Committed r208948: <http://trac.webkit.org/changeset/208948>
Comment 17 WebKit Commit Bot 2016-11-21 08:26:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Olivier Blin 2016-11-21 10:02:51 PST
(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.
Comment 19 youenn fablet 2016-11-21 23:44:21 PST
> 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