Needed to add Farstream as a backend for the GStreamer WebRTC implementation. It requires libnice and libcrypto as dependencies.
Created attachment 144115 [details] FLAGS for Farstream
Attachment 144115 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'GNUmakefile.am', u'Source/We..." exit_code: 1 Source/WebCore/ChangeLog:13: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 144115 [details] FLAGS for Farstream View in context: https://bugs.webkit.org/attachment.cgi?id=144115&action=review > Source/WebKit/gtk/GNUmakefile.am:66 > + $(HILDON_CFLAGS) \ > + $(LIBCRYPTO_CFLAGS) \ > + $(LIBFFTW_CFLAGS) \ hildong and libfftw don' t seem related, right? Merge conflicts maybe? > Source/WebKit/gtk/GNUmakefile.am:101 > + $(LIBFFTW_LIBS) \ Doesn' t seem related to Farstream
Created attachment 144132 [details] patch proposal removing unused deps
(In reply to comment #3) > (From update of attachment 144115 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144115&action=review > > > Source/WebKit/gtk/GNUmakefile.am:66 > > + $(HILDON_CFLAGS) \ > > + $(LIBCRYPTO_CFLAGS) \ > > + $(LIBFFTW_CFLAGS) \ > > hildong and libfftw don' t seem related, right? Merge conflicts maybe? > > > Source/WebKit/gtk/GNUmakefile.am:101 > > + $(LIBFFTW_LIBS) \ > > Doesn' t seem related to Farstream I believe you're right... Fixed, thanks for the heads up.
Comment on attachment 144132 [details] patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=144132&action=review > configure.ac:380 > +LIBFFTW_REQUIRED_VERSION=3.2.2 That can be removed too, didn' t notice it in the first patch. > configure.ac:1263 > + [AS_HELP_STRING([--with-media-stream-backend=@<:@farstream/gstreamer@:>@], [the media stream backend to use (default: farstream)])], So, hum... would it make sense to work on a single implementation? If the farstream implementation is more mature than the pure-gst one maybe the later can just be dropped?
(In reply to comment #6) > So, hum... would it make sense to work on a single implementation? If the farstream implementation is more mature than the pure-gst one maybe the later can just be dropped? I think I agree with Philippe here. More backends equal more maintenance in the end. It would be nice to hear at least a little bit about why the farstream backend might be preferred to the gstreamer one and whether or not it makes sense to drop one of them.
Created attachment 153502 [details] Patch
Comment on attachment 153502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=153502&action=review > Tools/gtk/jhbuild.modules:21 > + <dep package="farstream"/> We add new modules in jhbuild if they influence the layout tests results. This is not yet the case, can we do this in the patch where you plan to unskip the mediastream tests?
Created attachment 153532 [details] Patch
(In reply to comment #9) > (From update of attachment 153502 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=153502&action=review > > > Tools/gtk/jhbuild.modules:21 > > + <dep package="farstream"/> > > We add new modules in jhbuild if they influence the layout tests results. This is not yet the case, can we do this in the patch where you plan to unskip the mediastream tests? Agree, Actually I removed LayoutTests/platform/gtk/TestExpectations and most of them are Ok (I'm wondering if they're passing on google too)
(In reply to comment #11) > (In reply to comment #9) > > (From update of attachment 153502 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=153502&action=review > > > > > Tools/gtk/jhbuild.modules:21 > > > + <dep package="farstream"/> > > > > We add new modules in jhbuild if they influence the layout tests results. This is not yet the case, can we do this in the patch where you plan to unskip the mediastream tests? > > Agree, > > Actually I removed LayoutTests/platform/gtk/TestExpectations and most of them are Ok (I'm wondering if they're passing on google too) * remove SKIP of mediastream tests. Actually, since the implementation is quite big, I'm wondering if splitting those commits on very-small-uncompilable-commits makes sense.
(In reply to comment #12) > Actually, since the implementation is quite big, I'm wondering if splitting those commits on very-small-uncompilable-commits makes sense. In my opinion, an incomplete implementation is okay, but not commits that do not compile. One option here is to first stub out all classes that you need to implement and gradually add logical bits of the implementation.
Comment on attachment 153532 [details] Patch should we add farstream-dev to the build-bot?
(In reply to comment #14) > (From update of attachment 153532 [details]) > should we add farstream-dev to the build-bot? Yes, seems like it's missing in Gustavo's EWS and maybe buildslave. I installed libfarstream-0.1-dev on my EWS and one the Igalia buildslaves.
Comment on attachment 153532 [details] Patch Looks good but delaying cq+ until we are sure farstream-dev is installed on all bots. It's your call now, Gustavo :)
(In reply to comment #16) > (From update of attachment 153532 [details]) > Looks good but delaying cq+ until we are sure farstream-dev is installed on all bots. It's your call now, Gustavo :) Agreed.
(In reply to comment #17) > (In reply to comment #16) > > (From update of attachment 153532 [details] [details]) > > Looks good but delaying cq+ until we are sure farstream-dev is installed on all bots. It's your call now, Gustavo :) > > Agreed. Might be a good idea to disable media-stream on the default build, so people won't need to have farstream-dev installed to build webkit while the feature is not completed upstream. Although the build bot should keep media-stream enabled.
(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #16) > > > (From update of attachment 153532 [details] [details] [details]) > > > Looks good but delaying cq+ until we are sure farstream-dev is installed on all bots. It's your call now, Gustavo :) > > > > Agreed. > > Might be a good idea to disable media-stream on the default build, so people won't need to have farstream-dev installed to build webkit while the feature is not completed upstream. > This is already the case in configure.ac no? > Although the build bot should keep media-stream enabled. Yes it's enabled by default in build-webkit already, AFAIK.
(In reply to comment #19) > (In reply to comment #18) > > (In reply to comment #17) > > > (In reply to comment #16) > > > > (From update of attachment 153532 [details] [details] [details] [details]) > > > > Looks good but delaying cq+ until we are sure farstream-dev is installed on all bots. It's your call now, Gustavo :) > > > > > > Agreed. > > > > Might be a good idea to disable media-stream on the default build, so people won't need to have farstream-dev installed to build webkit while the feature is not completed upstream. > > > > This is already the case in configure.ac no? Well, should be, but there's something wrong when I do $ Tools/Scripts/build-webkit --gtk --debug I'm ending with --enable-media-stream in the autogen argument list. http://pastebin.com/wBq8TTNd
> > This is already the case in configure.ac no? > > Well, should be, but there's something wrong when I do > $ Tools/Scripts/build-webkit --gtk --debug > > I'm ending with --enable-media-stream in the autogen argument list. > http://pastebin.com/wBq8TTNd from Tools/Scripts/webkitperl/FeatureList.pm:293, I see that build-webkit enable media-stream for GTK by default. I'm not sure if we should remove that. What do you think Philippe?
(In reply to comment #21) > > > This is already the case in configure.ac no? > > > > Well, should be, but there's something wrong when I do > > $ Tools/Scripts/build-webkit --gtk --debug > > > > I'm ending with --enable-media-stream in the autogen argument list. > > http://pastebin.com/wBq8TTNd > > from Tools/Scripts/webkitperl/FeatureList.pm:293, I see that build-webkit enable media-stream for GTK by default. I'm not sure if we should remove that. What do you think Philippe? I was meaning that: AC_ARG_ENABLE(media_stream, AC_HELP_STRING([--enable-media-stream], [enable media stream support (incomplete) [default=no]]), [],[enable_media_stream=$enable_unstable_features]) and you saw already that build-webkit toggles this flag on. So everything's fine, I believe.
Created attachment 155304 [details] patch addind this patch for review again, since the build bot already has libfarstream-dev installed.
Created attachment 155305 [details] Patch * adding the correct file.
Comment on attachment 153532 [details] Patch Cleared Philippe Normand's review+ from obsolete attachment 153532 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 155305 [details] Patch Rejecting attachment 155305 [details] from review queue. danilo.cesar@collabora.co.uk does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have reviewer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your reviewer rights.
Comment on attachment 155305 [details] Patch Rejecting attachment 155305 [details] from commit-queue. danilo.cesar@collabora.co.uk does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
Comment on attachment 155305 [details] Patch Using the correct flags. Waiting for review/cq
Comment on attachment 155305 [details] Patch Clearing flags on attachment: 155305 Committed r124182: <http://trac.webkit.org/changeset/124182>
All reviewed patches have been landed. Closing bug.