[webkit-reviews] review granted: [Bug 188010] [GStreamer] Make codecparsers optionnal : [Attachment 345766] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Thibault Saunier
<tsaunier at gnome.org>'s request for review:
Bug 188010: [GStreamer] Make codecparsers optionnal
https://bugs.webkit.org/show_bug.cgi?id=188010

Attachment 345766: Patch

https://bugs.webkit.org/attachment.cgi?id=345766&action=review




--- Comment #2 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 345766
  --> https://bugs.webkit.org/attachment.cgi?id=345766
Patch

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

> Source/WebCore/platform/GStreamer.cmake:111
> +	       message(WARNING "WebRTC or MediaStream enabled but GStreamer <
1.10 - Support won't be built.")

Nobody is going to see this warning... use a fatal error if you want the user
to see a problem. And this is definitely a problem worth seeing. Also, follow
the format of our comparable error messages:

message(FATAL_ERROR "GStreamer 1.10 is needed for ENABLE_MEDIA_STREAM or
ENABLE_WEB_RTC")

> Source/cmake/OptionsGTK.cmake:429
>  include(GStreamerChecks)
> +if (ENABLE_MEDIA_STREAM OR ENABLE_WEB_RTC)
> +    if (PC_GSTREAMER_VERSION VERSION_LESS "1.10")
> +	   SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC FALSE)
> +	   SET_AND_EXPOSE_TO_BUILD(WEBRTC_WEBKIT_BUILD FALSE)
> +    else ()
> +	   SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC TRUE)
> +	   SET_AND_EXPOSE_TO_BUILD(WEBRTC_WEBKIT_BUILD TRUE)
> +    endif ()
> +else ()
> +    SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC FALSE)
> +    SET_AND_EXPOSE_TO_BUILD(WEBRTC_WEBKIT_BUILD FALSE)
> +endif ()

Surely this should be done in GStreamerChecks.cmake?

> Source/cmake/OptionsWPE.cmake:158
> +if (ENABLE_MEDIA_STREAM OR ENABLE_WEB_RTC)
> +    if (PC_GSTREAMER_VERSION VERSION_LESS "1.10")
> +	   SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC FALSE)
> +	   SET_AND_EXPOSE_TO_BUILD(WEBRTC_WEBKIT_BUILD FALSE)
> +    else ()
> +	   SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC TRUE)
> +	   SET_AND_EXPOSE_TO_BUILD(WEBRTC_WEBKIT_BUILD TRUE)
> +    endif ()
> +else ()
> +    SET_AND_EXPOSE_TO_BUILD(USE_LIBWEBRTC FALSE)
> +    SET_AND_EXPOSE_TO_BUILD(WEBRTC_WEBKIT_BUILD FALSE)
> +endif ()

Ditto