It should be possible to build and run most of the WebRTC unit tests with the standalone WebRTC library. This will help verify the standalone library and help detect build breakage.

Comment on attachment 8554047 [details] [diff] [review] 0003-Bug-1101651-Part-3-share-common-moz.build-for-webrtc-unit-tests-db2092b.patch I'm not sure if it makes sense to make a common build file since the end goal is to only have one set of unit tests, but until the standalone supports GMP and nsPref the unit tests need to be built twice.

Comment on attachment 8554898 [details] [diff] [review] 0001-Bug-1101651-Part-1-xpcomrt-version-of-dom-media-library-need-for-standalone-webrtcs-4082709.patch Updated patch after rebase.

Comment on attachment 8554899 [details] [diff] [review] 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-caf1ba1.patch Updated patch after rebase.

Comment on attachment 8554900 [details] [diff] [review] 0003-Bug-1101651-Part-3-share-common-moz.build-for-webrtc-unit-tests-d31e1f6.patch Updated patch after rebase.

Comment on attachment 8554898 [details] [diff] [review] 0001-Bug-1101651-Part-1-xpcomrt-version-of-dom-media-library-need-for-standalone-webrtcs-4082709.patch Review of attachment 8554898 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/AudioSegment.h @@ +198,5 @@ > MOZ_ASSERT(channels == segmentChannelCount); > output.SetLength(channels); > bufferPtrs.SetLength(channels); > +#if !defined(MOZILLA_XPCOMRT_API) > +// FIXME XPCOMRT perhaps more descriptive, or point to a bug? @@ +211,5 @@ > T* out = output[i].AppendElements(outSize); > uint32_t outFrames = outSize; > > +#if !defined(MOZILLA_XPCOMRT_API) > +// FIXME XPCOMRT ditto ::: dom/media/SimpleImageBuffer.cpp @@ +15,5 @@ > + mWidth = width; > + mHeight = height; > + if (!mBuffer || (size > mBufferSize)) { > + if (mBuffer) { > + delete []mBuffer; Nit: add a space (or move it). In the tree, there are 9 samples like this, and 1000 of delete[] foo or delete [] foo. @@ +25,5 @@ > + } > + } > + > + if (mBuffer) { > + if (frame && (size > 0)) { memcpy((void *)mBuffer, (const void*)frame, size); } no one-line "if (foo) { blah; }" @@ +41,5 @@ > + > +const unsigned char* > +SimpleImageBuffer::GetImage(unsigned int* size) const > +{ > + if (size) { *size = mSize; } ditto ::: dom/media/SimpleImageBuffer.h @@ +21,5 @@ > + const unsigned char* GetImage(unsigned int* size) const; > + void GetWidthAndHeight(int* width, int* height) const > + { > + if (width) { *width = mWidth; } > + if (height) { *height = mHeight; } newlines @@ +27,5 @@ > + > +protected: > + ~SimpleImageBuffer() > + { > + delete []mBuffer; add a space ::: dom/media/standalone/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk': the moz.build changes will need build peer review; look good to me

Comment on attachment 8554899 [details] [diff] [review] 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-caf1ba1.patch Review of attachment 8554899 [details] [diff] [review]: ----------------------------------------------------------------- leaving open to decide on how to handle the unittest directory breakdown ::: media/mtransport/testlib/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +include('../objs.mozbuild') non-trivial moz.build changes need build peer review ::: media/webrtc/signaling/test/moz.build @@ +9,2 @@ > 'jsep_session_unittest', > + #'mediaconduit_unittests', This is a problem... ::: media/webrtc/signaling/test/signaling_unittests.cpp @@ +4309,3 @@ > match = offer.find("profile-level-id"); > ASSERT_NE(std::string::npos, match); > + if (match != std::string::npos) { Do we need the if() right after asserting it's not npos? ::: media/webrtc/signaling/test/xul/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk': Why is this directory (and the tests) called "xul"? I'd far prefer to leave the main directory as the primary tests, and have a 'standalone' subdir that includes files from the main. Is that not possible?

Comment on attachment 8554900 [details] [diff] [review] 0003-Bug-1101651-Part-3-share-common-moz.build-for-webrtc-unit-tests-d31e1f6.patch Review of attachment 8554900 [details] [diff] [review]: ----------------------------------------------------------------- r+ modulo the directory name/split issue (and build peer approval).

(In reply to Randell Jesup [:jesup] from comment #18) > Comment on attachment 8554899 [details] [diff] [review] > 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using- > standalone-WebRTC-library-caf1ba1.patch > > Review of attachment 8554899 [details] [diff] [review]: > ----------------------------------------------------------------- > > leaving open to decide on how to handle the unittest directory breakdown > I have no problem switching it. It probably makes more sense to put the standalone tests in their own directory. The original thinking was that the unit tests would only be built once with the XPCOMRT. Unfortunately the missing parts (GMP, nsPrefs, nsIOService etc) in XPCOMRT make that unrealistic in the short term. Eventually the standalone WebRTC library will need to support these missing features but that is something that can be dealt with if/when it happens. > ::: media/mtransport/testlib/moz.build > @@ +3,5 @@ > > +# This Source Code Form is subject to the terms of the Mozilla Public > > +# License, v. 2.0. If a copy of the MPL was not distributed with this > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > + > > +include('../objs.mozbuild') > > non-trivial moz.build changes need build peer review > > ::: media/webrtc/signaling/test/moz.build > @@ +9,2 @@ > > 'jsep_session_unittest', > > + #'mediaconduit_unittests', > > This is a problem... > It is only commented out of the xpcomrt version because it uses GMP. See Bug 1121677. I thought I had a comment in there with the bug, must have gone missing in one of the many rebases. I'll add it back in. > ::: media/webrtc/signaling/test/signaling_unittests.cpp > @@ +4309,3 @@ > > match = offer.find("profile-level-id"); > > ASSERT_NE(std::string::npos, match); > > + if (match != std::string::npos) { > > Do we need the if() right after asserting it's not npos? > I'm pretty sure it was need at the time I added it because the unit test was crashing but may not be need now. I will remove it to verify. > ::: media/webrtc/signaling/test/xul/moz.build > @@ +3,5 @@ > > +# This Source Code Form is subject to the terms of the Mozilla Public > > +# License, v. 2.0. If a copy of the MPL was not distributed with this > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > + > > +if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk': > > Why is this directory (and the tests) called "xul"? > > I'd far prefer to leave the main directory as the primary tests, and have a > 'standalone' subdir that includes files from the main. Is that not possible? I will switch things around so that standalone is the subdir.

Comment on attachment 8558845 [details] [diff] [review] 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-15020315-0a98df9.patch Updated patch to address comments.

Updated patch to address review comments. carry forward r+ from :rjesup

Comment on attachment 8558845 [details] [diff] [review] 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-15020315-0a98df9.patch Review of attachment 8558845 [details] [diff] [review]: ----------------------------------------------------------------- glandium should take this since he did the Gyp integration last and has the libraries story more paged in than I do.

Comment on attachment 8558845 [details] [diff] [review] 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-15020315-0a98df9.patch Review of attachment 8558845 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/mtransport/standalone/moz.build @@ +4,5 @@ > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk': > + Library('mtransport_standalone') If you don't need this library in that case, don't recurse in this directory instead. ::: media/mtransport/testlib/moz.build @@ +72,5 @@ > +DEFINES['R_DEFINED_UINT2'] = 'uint16_t' > +DEFINES['R_DEFINED_INT4'] = 'int32_t' > +DEFINES['R_DEFINED_UINT4'] = 'uint32_t' > +DEFINES['R_DEFINED_INT8'] = 'int64_t' > +DEFINES['R_DEFINED_UINT8'] = 'uint64_t' Why do we need to build mtransport *three* times? (Also, that's three moz.builds with 98% the same content) ::: media/webrtc/moz.build @@ +99,5 @@ > build_for_test=1 > ) > + GYP_DIRS['signalingtest'].variables.update( > + build_for_standalone=0 > + ) GYP_DIRS['signalingtest'].variables.update( build_for_test=1, build_for_standalone=0, ) ::: toolkit/toolkit.mozbuild @@ +63,5 @@ > '/media/webrtc', > '/media/mtransport/third_party', > '/media/mtransport/build', > '/media/mtransport/standalone', > + '/media/mtransport/testlib', I think it's time to make directory traversal for /media/webrtc and /media/mtransport more "normal" (as in, have those directories be in DIRS in /media/{webrtc,mtransport}/moz.build.

Comment on attachment 8558845 [details] [diff] [review] 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-15020315-0a98df9.patch Review of attachment 8558845 [details] [diff] [review]: ----------------------------------------------------------------- One concern is that the proliferation of "if defined(MOZILLA_INTERNAL_API) && !defined(MOZILLA_XPCOMRT_API)" everywhere; it'll be easy for people to mess up. Maybe define a single thing to check everywhere? "if !defined(MOZILLA_EXTERNAL_LINKAGE)"?? r+ with note that glandium's comments need to be dealt with. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h @@ +624,5 @@ > NS_ABORT_IF_FALSE(CheckThreadInt(), "Wrong thread"); > } > bool CheckThreadInt() const { > +#if defined(MOZILLA_INTERNAL_API) && !defined(MOZILLA_XPCOMRT_API) > +// FIXME XPCOMRT Work needed here it seems ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp @@ +169,5 @@ > mMainThread(mParent->GetMainThread()), > mSTSThread(mParent->GetSTSThread()), > mProxyResolveCompleted(false) { > +#if defined(MOZILLA_XPCOMRT_API) > + // FIXME Bug 1126039 Standalone XPCOMRT does not currently support nsIProtocolProxyService or nsIIOService // TODO(bug N) please ::: media/webrtc/signaling/test/FakeMediaStreams.h @@ +33,5 @@ > class MediaSegment; > }; > > +class Fake_VideoSink { > + public: indent wrong

Rebased patch to latest inbound. carry forward r+ from :rjesup

Updated patch to address review comments. carry forward r+ from :rjesup

Rebased to latest inbound. carry forward r+ from :dminor

https://hg.mozilla.org/integration/fx-team/rev/b84eb39d1b52 https://hg.mozilla.org/integration/fx-team/rev/d02fb013d5b7 https://hg.mozilla.org/integration/fx-team/rev/af45e9fc2953

There was static analysis bustage https://treeherder.mozilla.org/logviewer.html#?job_id=2543871&repo=fx-team that was fixed by ehsan's patch https://hg.mozilla.org/integration/fx-team/rev/4c9711732a18 But then there were also mochitest-3 failures https://treeherder.mozilla.org/logviewer.html#?job_id=2543122&repo=fx-team and https://treeherder.mozilla.org/logviewer.html#?job_id=2544493&repo=fx-team So this bug and bug 1097804 and bug 1093934 are all backed out now. https://hg.mozilla.org/integration/fx-team/rev/bba2048d3424 https://hg.mozilla.org/integration/fx-team/rev/8cbe5dac41e5 https://hg.mozilla.org/integration/fx-team/rev/2fb78f64b625 https://hg.mozilla.org/integration/fx-team/rev/6cf9b3b2c363

Rebased patch to latest inbound. carry forward r+ from :rjesup

Updated patch to address mochitest failure. carry forward r+ from :rjesup

Rebased to latest inbound. carry forward r+ from :dminor

I have identified the mochi test failure and the code is now able to pass with this latest patch.

Hi, Part 2 failed to apply: patching file media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp Hunk #6 FAILED at 383 1 out of 42 hunks FAILED -- saving rejects to file media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh 1-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-15040315-0e5f89b.patch

Rebased patch to latest inbound. carry forward r+ from :rjesup

Rebased patch to latest inbound. carry forward r+ from :rjesup

Rebased to latest inbound. carry forward r+ from :dminor

Hi Randall, sorry this failed again like patching file media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp Hunk #5 FAILED at 312 1 out of 42 hunks FAILED -- saving rejects to file media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh 1-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-15040715-eb47f92.patch could you take a look, thanks!

(In reply to Carsten Book [:Tomcat] from comment #44) > Hi Randall, > > sorry this failed again like patching file > media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp > Hunk #5 FAILED at 312 > 1 out of 42 hunks FAILED -- saving rejects to file > media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp.rej > patch failed, unable to continue (try -v) > patch failed, rejects left in working dir > errors during apply, please fix and refresh > 1-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC- > library-15040715-eb47f92.patch > > > could you take a look, thanks! Unfortunately this patch bit rots rapidly, sometimes it will have bit rotted before I've even completed a try run with it.

Rebased patch to latest inbound. carry forward r+ from :rjesup

Rebased patch to latest inbound. carry forward r+ from :rjesup

Rebased to latest inbound. carry forward r+ from :dminor

https://hg.mozilla.org/integration/mozilla-inbound/rev/2aef619be58e https://hg.mozilla.org/integration/mozilla-inbound/rev/7c2a1eaed3ac https://hg.mozilla.org/integration/mozilla-inbound/rev/e615b3d21889

https://hg.mozilla.org/mozilla-central/rev/2aef619be58e https://hg.mozilla.org/mozilla-central/rev/7c2a1eaed3ac https://hg.mozilla.org/mozilla-central/rev/e615b3d21889

Patch 2 breaks build --with-system-nspr for me ============================== ERROR PROCESSING MOZBUILD FILE ============================== The error occurred while processing the following file or one of the files it includes: ./mozilla-central/media/webrtc/signaling/test/standalone/moz.build The error occurred when validating the result of the execution. The reported error is: USE_LIBS contains "/nsprpub/lib/libc/src/plc4", but there is no "plc4" LIBRARY_NAME in nsprpub/lib/libc/src. Am I doing something wrong?

Comment on attachment 8590317 [details] [diff] [review] 0002-Bug-1101651-Part-2-Enable-WebRTC-unit-tests-to-be-built-using-standalone-WebRTC-library-15040909-40de3e8.patch Review of attachment 8590317 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/mtransport/test/moz.build @@ +40,5 @@ > LOCAL_INCLUDES += [ > '/media/mtransport/third_party/nrappkit/src/port/linux/include', > ] > + USE_LIBS += [ > + 'static:/nsprpub/lib/libc/src/plc4', You really shouldn't do this. Just USE_LIBS += ['nspr'] That's what causes comment 51.