Bug 88413

Created attachment 146037 [details] Patch Add support for building Media Stream feature.

Comment on attachment 146037 [details] Patch LGTM. BTW, will you support media stream feature for EFL port ?

(In reply to comment #2) > (From update of attachment 146037 [details]) > LGTM. BTW, will you support media stream feature for EFL port ? Thanks for your review. I tried to enable this feature by default, but some of the tests are not passing, so it is turned off for now. I filed a meta bug #87662 for adding missing implementation and enabling this feature for the EFL port.

Ok, Looks fine.

Comment on attachment 146037 [details] Patch LGTM.

Sources that need to be compiled for supporting Media Stream feature has already been added to CMake build system in bug 88849. So, I will consider this bug for adding EFL platform specific changes only.

Created attachment 147385 [details] Patch Add support for building Media Stream feature.

Comment on attachment 147385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147385&action=review > Source/WebKit/efl/WebCoreSupport/UserMediaClientEfl.h:19 > + */ Trivial nit : Add an empty line.

Comment on attachment 147385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147385&action=review Does it make sense to sort of add this functionality to the port even if UserMediaClientEfl only contains stubs? > Source/WebCore/PlatformEfl.cmake:286 > + LIST(APPEND WebCore_INCLUDE_DIRECTORIES > + "${WEBCORE_DIR}/platform/mediastream/gstreamer" > + ) > + LIST(APPEND WebCore_SOURCES > + platform/mediastream/gstreamer/DeprecatedPeerConnectionHandler.cpp > + platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp > + ) We generally use a 4-space indentation for CMake files. I generally favor not using these conditionals, as the .cpp files are protected by #ifdefs anyway. But we might just change all those checks at once in another patch.

(In reply to comment #9) > (From update of attachment 147385 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147385&action=review > > Does it make sense to sort of add this functionality to the port even if UserMediaClientEfl only contains stubs? > This is an initial patch to add support for building media stream feature on the EFL port. This feature is turned off for now because some of the tests are still failing. As I said in the comment 3, I filed a meta bug for adding missing implementation before enabling the feature. GTK and Blackberry ports enabled this by default, but the tests are skipped it seems. > > Source/WebCore/PlatformEfl.cmake:286 > > + LIST(APPEND WebCore_INCLUDE_DIRECTORIES > > + "${WEBCORE_DIR}/platform/mediastream/gstreamer" > > + ) > > + LIST(APPEND WebCore_SOURCES > > + platform/mediastream/gstreamer/DeprecatedPeerConnectionHandler.cpp > > + platform/mediastream/gstreamer/MediaStreamCenterGStreamer.cpp > > + ) > > We generally use a 4-space indentation for CMake files. Initially I thought of using 4-space indentation, but switched to 2-space for consistency reasons. EFL's CMake files are using 2-space in most of the places. I think we should fix this in all the EFL's CMake files in a separate bug. >I generally favor not using these conditionals, as the .cpp files are protected by #ifdefs anyway. But we might just change all those checks at once in another patch. Make sense to have a separate bug to fix all at once.

Created attachment 147566 [details] patch Updated patch fixing review comments.

(In reply to comment #10) > This is an initial patch to add support for building media stream feature on the EFL port. This feature is turned off for now because some of the tests are still failing. As I said in the comment 3, I filed a meta bug for adding missing implementation before enabling the feature. GTK and Blackberry ports enabled this by default, but the tests are skipped it seems. Do you already have some more patches up your sleeve? I'm usually a bit uncomfortable with adding stub source files, as they sometimes end up not being updated with actual implementations. Wouldn't it the case to implement the code in UserMediaClientEfl in this same patch?

Comment on attachment 147566 [details] patch As discussed on #webkit-efl, I am going to remove stubs I added in the patch for now and keep only build system changes.

Created attachment 148330 [details] patch Removed stub sources. Patch contains only minor build system changes.

Comment on attachment 148330 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=148330&action=review > Source/WebCore/PlatformEfl.cmake:285 > + platform/mediastream/gstreamer/DeprecatedPeerConnectionHandler.cpp These files already are being protected by #if ENABLE(MEDIA_STREAM). So, in this case, I don't like to guard twice.

Created attachment 148532 [details] patch Removed unnecessary ifdef guard.

Comment on attachment 148532 [details] patch LGTM.

Comment on attachment 148532 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=148532&action=review > Source/cmake/OptionsEfl.cmake:79 > +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_MEDIA_STREAM OFF) Isn't it redundant since it's also OFF by default in WebKitFeatures.cmake?

Created attachment 148913 [details] Patch

Comment on attachment 148532 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=148532&action=review >> Source/cmake/OptionsEfl.cmake:79 >> +WEBKIT_OPTION_DEFAULT_PORT_VALUE(ENABLE_MEDIA_STREAM OFF) > > Isn't it redundant since it's also OFF by default in WebKitFeatures.cmake? You are right. Removed it now.

Comment on attachment 148913 [details] Patch Looks fine, thanks.

Comment on attachment 148913 [details] Patch Clearing flags on attachment: 148913 Committed r121180: <http://trac.webkit.org/changeset/121180>

All reviewed patches have been landed. Closing bug.