To enable building a standalone WebRTC library, nsISocketTransportService and nsIDNS need to be able to built into a stand alone library that maybe used by the WebRTC library.

Comment on attachment 8522523 [details] [diff] [review] Mini Necko v1 Review of attachment 8522523 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/dns/nsHostResolver.cpp @@ +215,1 @@ > Telemetry::Accumulate(Telemetry::DNS_BLACKLIST_COUNT, mBlacklistedCount); As discussed on IRC: a significant amount of these are only there for the Telemetry::Accumulate() calls, so we might want to stub/define those out instead.

Comment on attachment 8522523 [details] [diff] [review] Mini Necko v1 Review of attachment 8522523 [details] [diff] [review]: ----------------------------------------------------------------- This looks OK from an xpcom point of view, modulo the point raised below. This is really in large part necko-stuff anyway, so I don't know that there's a lot for me to say here. ::: xpcom/libxpcomrt/XPCOMRTInit.cpp @@ +101,5 @@ > NS_ShutdownXPCOMRT() > { > nsresult rv = NS_OK; > + { > + nsCOMPtr<nsIThread> thread = do_GetCurrentThread(); Why are you scoping things here? (I can sort of believe it's needed, but I would like clarification on this. This change also seems like something that should go in the main xpcomrt patch, so that the network-related changes here are smaller and more obvious.)

Comment on attachment 8522523 [details] [diff] [review] Mini Necko v1 Review of attachment 8522523 [details] [diff] [review]: ----------------------------------------------------------------- A couple thoughts - let's call it standalone instead of mini. We may want to expand this to support something like the iPhone over time. this looks really easy to regress through normal necko hacking. how do we avoid that? I concur a telemetry stub will get rid of a ton of ifdefs, please do that I'm concerned that some of the stubs, like GetURLSpecFromFile() shouldn't just remove code and return NS_OK - that seems like a recipe for disaster. They should at least throw and if you think this code shouldn't be called at all then they should assert in the stubbed path. This applies to a lot of the call sites. ::: netwerk/base/src/nsSocketTransportService2.cpp @@ +4,4 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #include "nsSocketTransportService2.h" > +#if !defined(MOZILLA_XPCOMRT_API) if you can rearrange this to be 1 or 2 ifdefs in the header list I would rather have that than preserve the sorting. ::: netwerk/mini/nsNetModuleMini.cpp @@ +18,5 @@ > +#include "nsPISocketTransportService.h" > +#include "nscore.h" > + > +extern const mozilla::Module kNeckoMiniModule; > + let's put this code in namespace mozilla::net @@ +20,5 @@ > + > +extern const mozilla::Module kNeckoMiniModule; > + > +nsresult > +NS_InitNetModuleMini() don't use the NS_ prefix anymore when we're in the namespace

Updated to address review comments.

Added missing new files.

Added missing new files.

Comment on attachment 8544894 [details] [diff] [review] Necko Standalone v3 I believe all review comments have been addressed.

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #3) > Comment on attachment 8522523 [details] [diff] [review] > Mini Necko v1 > > Review of attachment 8522523 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks OK from an xpcom point of view, modulo the point raised below. > This is really in large part necko-stuff anyway, so I don't know that > there's a lot for me to say here. > > ::: xpcom/libxpcomrt/XPCOMRTInit.cpp > @@ +101,5 @@ > > NS_ShutdownXPCOMRT() > > { > > nsresult rv = NS_OK; > > + { > > + nsCOMPtr<nsIThread> thread = do_GetCurrentThread(); > > Why are you scoping things here? (I can sort of believe it's needed, but I > would like clarification on this. This change also seems like something > that should go in the main xpcomrt patch, so that the network-related > changes here are smaller and more obvious.) The scoping comes from the code I cribbed from. I lost the comment explaining the scoping: Block it so that the COMPtr will get deleted before we hit servicemanager shutdown. I put the comment back in. I misunderstood what you meant about moving the reformatting of the XPCOMRT Init function until right now. I'll try and address that and up load new patches for Bug 1093934 and this bug.

Comment on attachment 8545564 [details] [diff] [review] Part 1: Necko Standalone v5 Fixed the formatting issue in earlier patch so there is less xpcom in this patch.

This is good. thanks. I'm deeply concerned about how easy it would be to regress this. is a MOZILLA_XPCOMRT_API part of the ci? Is it considered a non-regressable target?

>I'm deeply concerned about how easy it would be to regress this. is a MOZILLA_XPCOMRT_API part of the ci? Is it considered a non-regressable target? Together with this landing, a patch should land (bug 1101651) that causes the WebRTC unit tests to be linked against this library.

(In reply to Patrick McManus [:mcmanus] from comment #13) > This is good. thanks. > > I'm deeply concerned about how easy it would be to regress this. is a > MOZILLA_XPCOMRT_API part of the ci? Is it considered a non-regressable > target? Yes, this will make things more complicated. The patches in bug 1101651 link against the MOZILLA_XPCOMRT_API targets so link breakage should be noticeable at build time. I do not believe the WebRTC unit test would reveal functionality breakage. Perhaps there are unit tests I can tie into to get better coverage?

I think build time failure is fine.. what test suite in the CI makes that build? I want to make sure some variation of a try run would show the failure.

Comment on attachment 8545564 [details] [diff] [review] Part 1: Necko Standalone v5 Review of attachment 8545564 [details] [diff] [review]: ----------------------------------------------------------------- please double check regressions to this can be diagnosed via try server r=mcmanus

(In reply to Patrick McManus [:mcmanus] from comment #17) > Comment on attachment 8545564 [details] [diff] [review] > Part 1: Necko Standalone v5 > > Review of attachment 8545564 [details] [diff] [review]: > ----------------------------------------------------------------- > > please double check regressions to this can be diagnosed via try server > r=mcmanus I have verified that mediapipeline_unittest, sdp_unittest, buffered_stun_socket_unittest, ice_unittest, nrappkit_unittest, rlogringbuffer_unittest, runnable_utils_unittest, simpletokenbucket_unittest, sockettransportservice_unittest, TestSyncRunnable, transport_unittests, and turn_unittest are all linked against standalone XPCOM and Necko and run on try.

Comment on attachment 8545564 [details] [diff] [review] Part 1: Necko Standalone v5 Review of attachment 8545564 [details] [diff] [review]: ----------------------------------------------------------------- I thought I set the r+ flag on this when I left my last comment, but I just saw it in my queue. Sorry!

Rebased to tip of inbound. Carry forward r+ from :mcmanus

Comment on attachment 8552754 [details] [diff] [review] Part 2: Unicode lib needed by standalone Necko v3 Small moz.build addition to enable building standalone WebRTC library.

Comment on attachment 8552754 [details] [diff] [review] Part 2: Unicode lib needed by standalone Necko v3 Review of attachment 8552754 [details] [diff] [review]: ----------------------------------------------------------------- You didn't need build peer review for this. But I'll give it to you.

Rebased to current inbound. Carry forward r+ from mcmanus

Rebased to current inbound carry forward r+ from :gps

Rebased to current inbound. Carry forward r+ from mcmanus

Rebased to current inbound. Carry forward r+ from gps

https://hg.mozilla.org/integration/fx-team/rev/e1f294f7ca90 https://hg.mozilla.org/integration/fx-team/rev/c7da4d4c09aa

These were backed out so I could back out bug 1101651 for test failures: https://hg.mozilla.org/integration/fx-team/rev/1f1973da3d17 https://hg.mozilla.org/integration/fx-team/rev/eebb50c44389

Rebased to current inbound. Carry forward r+ from mcmanus

Rebased to current inbound. Carry forward r+ from gps

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f160bf4fd43 https://hg.mozilla.org/integration/mozilla-inbound/rev/3451d72b8e1a

https://hg.mozilla.org/mozilla-central/rev/6f160bf4fd43 https://hg.mozilla.org/mozilla-central/rev/3451d72b8e1a