Bug 88413 – [EFL] Add support for building with ENABLE_MEDIA_STREAM

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Sudarsana Nagineni (babu)]

Add support for building Media Stream feature with ENABLE_MEDIA_STREAM flag.

[Sudarsana Nagineni (babu)]

Created attachment 146037 [details]
Patch

Add support for building Media Stream feature.

[Gyuyoung Kim]

Comment on attachment 146037 [details]
Patch

LGTM. BTW, will you support media stream feature for EFL port ?

[Sudarsana Nagineni (babu)]

(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.

[Gyuyoung Kim]

Ok, Looks fine.

[Chris Dumez]

Comment on attachment 146037 [details]
Patch

LGTM.

[Sudarsana Nagineni (babu)]

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.

[Sudarsana Nagineni (babu)]

Created attachment 147385 [details]
Patch

Add support for building Media Stream feature.

[Gyuyoung Kim]

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.

[Raphael Kubo da Costa (:rakuco)]

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.

[Sudarsana Nagineni (babu)]

(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.

[Sudarsana Nagineni (babu)]

Created attachment 147566 [details]
patch

Updated patch fixing review comments.

[Raphael Kubo da Costa (:rakuco)]

(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?

[Sudarsana Nagineni (babu)]

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.

[Sudarsana Nagineni (babu)]

Created attachment 148330 [details]
patch

Removed stub sources. Patch contains only minor build system changes.

[Gyuyoung Kim]

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.

[Sudarsana Nagineni (babu)]

Created attachment 148532 [details]
patch

Removed unnecessary ifdef guard.

[Gyuyoung Kim]

Comment on attachment 148532 [details]
patch

LGTM.

[Raphael Kubo da Costa (:rakuco)]

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?

[Sudarsana Nagineni (babu)]

Created attachment 148913 [details]
Patch

[Sudarsana Nagineni (babu)]

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.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 148913 [details]
Patch

Looks fine, thanks.

[WebKit Review Bot]

Comment on attachment 148913 [details]
Patch

Clearing flags on attachment: 148913

Committed r121180: <http://trac.webkit.org/changeset/121180>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.