Summary: | [GTK][EFL] Upgrade OpenWebRTC dependency | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alejandro G. Castro <alex> | ||||||||
Component: | WebCore Misc. | Assignee: | Alejandro G. Castro <alex> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | adam.bergkvist, gyuyoung.kim, jh718.park, ossy, pnormand | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 143211, 153540, 153541 | ||||||||||
Attachments: |
|
Description
Alejandro G. Castro
2016-01-26 09:32:28 PST
Created attachment 269893 [details]
Patch
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. (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. Adding Gyuyoung to check his opinion about Efl before landing this. Created attachment 269995 [details]
Patch
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. 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. Created attachment 270104 [details]
Patch
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. 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 :/ |