Bug 153489 – [GTK][EFL] Upgrade OpenWebRTC dependency

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alejandro G. Castro]

We need to upgrade the OpenWebRTC dependency to be able to continue pushing patches but we have to upgrade also gstreamer to 1.7, which is not an option until the following bug is fixed:

For that reason we are going to stop compiling mediastream support by default, and running the tests. That way just people developing the technology can do it using a jhbuild moduleset that we are going to upload to the repository. Everyone else probably is not interested in using the unfinished code and we will make it compile by default when gstreamer is ready.

[Alejandro G. Castro]

Created attachment 269893 [details]
Patch

[Michael Catanzaro]

Comment on attachment 269893 [details]
Patch

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

> Tools/gtk/jhbuild-webrtc.modules:8
> +  <repository type="tarball" name="github-https"

I think "github-tarball" would be a better name.

> Tools/gtk/jhbuild-webrtc.modules:14
> +    <branch module="cisco/libsrtp/archive/v1.5.2.tar.gz" version="1.5.2"

I recommend using ${version} to make this easier to update.

<branch module="cisco/libsrtp/archive/v${version}.tar.gz" version="1.5.2"
        checkoutdir="libsrtp-${version}"

Just to make it a bit easier to upgrade versions in the future, for this and all the other modules.

> LayoutTests/platform/efl/TestExpectations:2733
> +webkit.org/b/153489 fast/mediastream [ Skip ]

You should file a different bug to unskip these tests that will remain open once this patch lands. I don't like linking test expectations to closed bugs.

> LayoutTests/platform/gtk/TestExpectations:2585
> +webkit.org/b/153489 fast/mediastream [ Skip ]

Ditto.

[Alejandro G. Castro]

(In reply to comment #2)
> Comment on attachment 269893 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269893&action=review
> 
> > Tools/gtk/jhbuild-webrtc.modules:8
> > +  <repository type="tarball" name="github-https"
> 
> I think "github-tarball" would be a better name.
> 
> > Tools/gtk/jhbuild-webrtc.modules:14
> > +    <branch module="cisco/libsrtp/archive/v1.5.2.tar.gz" version="1.5.2"
> 
> I recommend using ${version} to make this easier to update.
> 
> <branch module="cisco/libsrtp/archive/v${version}.tar.gz" version="1.5.2"
>         checkoutdir="libsrtp-${version}"
> 
> Just to make it a bit easier to upgrade versions in the future, for this and
> all the other modules.
> 
> > LayoutTests/platform/efl/TestExpectations:2733
> > +webkit.org/b/153489 fast/mediastream [ Skip ]
> 
> You should file a different bug to unskip these tests that will remain open
> once this patch lands. I don't like linking test expectations to closed bugs.
> 
> > LayoutTests/platform/gtk/TestExpectations:2585
> > +webkit.org/b/153489 fast/mediastream [ Skip ]
> 
> Ditto.

Thanks very much for the review Michael, I'll upload a new patch with your proposals.

[Alejandro G. Castro]

Adding Gyuyoung to check his opinion about Efl before landing this.

[Alejandro G. Castro]

Created attachment 269995 [details]
Patch

[Martin Robinson]

Comment on attachment 269995 [details]
Patch

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

Seems okay to me, but this needs a couple small changes.

> Tools/gtk/jhbuild-webrtc.modules:4
> +<?xml version="1.0"?>
> +<!DOCTYPE moduleset SYSTEM "moduleset.dtd">
> +<?xml-stylesheet type="text/xsl" href="moduleset.xsl"?>
> +<moduleset>

Do you mind trimming this down to the simply include the dependencies that you cannot get from your local system?

> LayoutTests/platform/gtk/TestExpectations:2586
> +
> +# Mediastream implementation requires new Gstreamer/OpenWebRTC
> +webkit.org/b/153540 fast/mediastream [ Skip ]
> +webkit.org/b/153540 http/tests/media/media-stream [ Skip ]

Please remove all the other mediastream and media-stream lines as well. Also, these should be placed in section 2 of the file.

[Alejandro G. Castro]

Thanks for the comment.

(In reply to comment #6)
> Comment on attachment 269995 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269995&action=review
> 
> Seems okay to me, but this needs a couple small changes.
> 
> > Tools/gtk/jhbuild-webrtc.modules:4
> > +<?xml version="1.0"?>
> > +<!DOCTYPE moduleset SYSTEM "moduleset.dtd">
> > +<?xml-stylesheet type="text/xsl" href="moduleset.xsl"?>
> > +<moduleset>
> 
> Do you mind trimming this down to the simply include the dependencies that
> you cannot get from your local system?
> 

I think it is, we just have packages and versions of packages required that are not similar to the ones in the common jhbuild file.

> > LayoutTests/platform/gtk/TestExpectations:2586
> > +
> > +# Mediastream implementation requires new Gstreamer/OpenWebRTC
> > +webkit.org/b/153540 fast/mediastream [ Skip ]
> > +webkit.org/b/153540 http/tests/media/media-stream [ Skip ]
> 
> Please remove all the other mediastream and media-stream lines as well.
> Also, these should be placed in section 2 of the file.

Fixed.

Thanks again.

[Alejandro G. Castro]

Created attachment 270104 [details]
Patch

[Alejandro G. Castro]

For EFL I've used an existent bug 87662, and made it dependent on the one following if gstreamer issue is solved to upgrade openwebrtc.

[Philippe Normand]

Comment on attachment 270104 [details]
Patch

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

> Tools/Scripts/run-gtk-tests:85
> +        SkippedTest("WebKit2/UserMedia", "WebKit2.UserMediaBasic", "We will not test by default until the upgrade gstreamer to 1.7", 153540),

s/1.7/1.9 :/

[Alejandro G. Castro]

landed https://trac.webkit.org/changeset/196006