https://treeherder.mozilla.org/#/jobs?repo=try&revision=602a16361574 https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea8f377a8e34

Comment on attachment 8550787 [details] [diff] [review] templatize_nss_ctors Review of attachment 8550787 [details] [diff] [review]: ----------------------------------------------------------------- In general, this looks good. It's definitely a lot clearer and cleaner than those macros. However, let's update the style as well. Two spaces for indentation, */& in type names go on the left (e.g. void** aResult, not void **aResult), braces always go around conditional bodies, with the opening brace on the same line, and keep to < 80 characters when possible. r- for now. ::: security/manager/ssl/src/nsNSSModule.cpp @@ +49,4 @@ > > +MOZ_ALWAYS_INLINE static bool IsProcessDefault() > +{ > + return GeckoProcessType_Default == XRE_GetProcessType(); This isn't in the style guide, but generally it's more common to switch the operands here (see the implementation of EnsureNSSInitializedChromeOrContent for example, which incidentally you could change to call this function instead). @@ +53,5 @@ > } > > +template<class InstanceClass> > +MOZ_ALWAYS_INLINE static nsresult > +Instantiate(REFNSIID aIID, void **aResult) nit: void** aResult @@ +56,5 @@ > +MOZ_ALWAYS_INLINE static nsresult > +Instantiate(REFNSIID aIID, void **aResult) > +{ > + InstanceClass* inst; > + inst = new InstanceClass(); We may as well have InstanceClass* inst = new InstanceClass(); all on one line. @@ +65,4 @@ > } > > +template<class InstanceClass, > + nsresult (InstanceClass::*InitMethod)()> Can we use references to class functions instead of pointers to class functions? @@ +66,5 @@ > > +template<class InstanceClass, > + nsresult (InstanceClass::*InitMethod)()> > +MOZ_ALWAYS_INLINE static nsresult > +Instantiate(REFNSIID aIID, void **aResult) nit: void** aResult @@ +69,5 @@ > +MOZ_ALWAYS_INLINE static nsresult > +Instantiate(REFNSIID aIID, void **aResult) > +{ > + InstanceClass* inst; > + inst = new InstanceClass(); Same here - let's have these two statements all on one line. @@ +70,5 @@ > +Instantiate(REFNSIID aIID, void **aResult) > +{ > + InstanceClass* inst; > + inst = new InstanceClass(); > + NS_ADDREF(inst); Same here with nsCOMPtr instead of manual refcounting, if possible. @@ +72,5 @@ > + InstanceClass* inst; > + inst = new InstanceClass(); > + NS_ADDREF(inst); > + nsresult rv = (inst->*InitMethod)(); > + if(NS_SUCCEEDED(rv)) { nit: if (NS_SUCCEEDED... Also, since it's possible to not take this branch, let's set *aResult = nullptr in case it doesn't get taken. @@ +83,5 @@ > +template<EnsureNSSOperator ensureOperator, > + class InstanceClassChrome, > + class InstanceClassContent> > +static nsresult > +Constructor(nsISupports *aOuter, REFNSIID aIID, void **aResult) nit: nsISupports* aOuter, void** aResult @@ +93,5 @@ > + rv = NS_ERROR_NO_AGGREGATION; > + return rv; > + } > + > + if (!EnsureNSSInitialized(ensureOperator)) nit: braces around conditional bodies @@ +96,5 @@ > + > + if (!EnsureNSSInitialized(ensureOperator)) > + return NS_ERROR_FAILURE; > + > + if (IsProcessDefault()) same @@ +102,5 @@ > + else > + rv = Instantiate<InstanceClassContent>(aIID, aResult); > + > + if (ensureOperator == nssLoadingComponent) > + { nit: brace at the end of the previous line @@ +103,5 @@ > + rv = Instantiate<InstanceClassContent>(aIID, aResult); > + > + if (ensureOperator == nssLoadingComponent) > + { > + if (NS_SUCCEEDED(rv)) nit: braces around bodies @@ +141,5 @@ > +{ > + nsresult rv; > + > + *aResult = nullptr; > + if (nullptr != aOuter) { Again, let's switch these operands. @@ +143,5 @@ > + > + *aResult = nullptr; > + if (nullptr != aOuter) { > + rv = NS_ERROR_NO_AGGREGATION; > + return rv; Let's simplify this to 'return NS_ERROR_NO_AGGREGATION' and move the declaration of rv closer to where it's used. @@ +232,3 @@ > static const mozilla::Module::CIDEntry kNSSCIDs[] = { > + { &kNS_NSSCOMPONENT_CID, false, nullptr, Constructor<nssLoadingComponent, nsNSSComponent, > + &nsNSSComponent::Init> }, Let's break these lines so they're < 80 characters.

Fixed style nits. (In reply to David Keeler [:keeler] (use needinfo?) from comment #1) > @@ +65,4 @@ > > } > > > > +template<class InstanceClass, > > + nsresult (InstanceClass::*InitMethod)()> > > Can we use references to class functions instead of pointers to class > functions? Looks like we can't. http://stackoverflow.com/questions/21952386/why-doesnt-reference-to-member-exist-in-c > @@ +70,5 @@ > > +Instantiate(REFNSIID aIID, void **aResult) > > +{ > > + InstanceClass* inst; > > + inst = new InstanceClass(); > > + NS_ADDREF(inst); > > Same here with nsCOMPtr instead of manual refcounting, if possible. nsCOMPtr didn't work because of the ambiguous conversion to nsISupports*. I used nsRefPtr instead. > @@ +72,5 @@ > > + InstanceClass* inst; > > + inst = new InstanceClass(); > > + NS_ADDREF(inst); > > + nsresult rv = (inst->*InitMethod)(); > > + if(NS_SUCCEEDED(rv)) { > > nit: if (NS_SUCCEEDED... > Also, since it's possible to not take this branch, let's set *aResult = > nullptr in case it doesn't get taken. Instantiate() is only called from Constructor() and Constructor() sets aResult to nullptr at the top of the function.

Resolved conflicts with bug 1114882.

Comment on attachment 8553779 [details] [diff] [review] Use templates instead of macros for NSS module favtory construntors, v3 Review of attachment 8553779 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. However, I have some suggestions for further refactoring that will make it much less likely we'll do something wrong in the future (see in particular the third comment). r- for now. ::: security/manager/ssl/src/nsNSSModule.cpp @@ +55,5 @@ > +template<class InstanceClass> > +MOZ_ALWAYS_INLINE static nsresult > +Instantiate(REFNSIID aIID, void** aResult) > +{ > + nsRefPtr<InstanceClass> inst = new InstanceClass(); Actually, this was a poor suggestion on my part. The underlying code for QueryInterface (as far as I can tell) uses NS_ADDREF / NS_RELEASE, so we should stick to that to be sure that this refcounting is compatible. (And, indeed, when I compile with this patch there are errors relating to this.) @@ +62,4 @@ > } > > +template<class InstanceClass, > + nsresult (InstanceClass::*InitMethod)()> Looks like these two lines will fit on one line. @@ +136,5 @@ > + class InstanceClassContent, > + nsresult (InstanceClassChrome::*InitMethodChrome)(), > + nsresult (InstanceClassContent::*InitMethodContent)()> > +static nsresult > +Constructor(nsISupports* aOuter, REFNSIID aIID, void** aResult) I didn't realize there were two very similar Constructor functions as a result of sometimes having an init method and sometimes not. The code has already diverged unintentionally. I think having two of these functions could be a source of bugs in the future. We should define the constructor that doesn't take init methods in terms of the other one, like so: return Constructor<ensureOperator, InstanceClassChrome, InstanceClassContent, nullptr, nullptr>(aOuter, aIID, aResult); If we also only have one Instantiate method (and define the other one as 'return Instantiate<InstanceClass, nullptr>;'), it actually makes the code quite clean (all we have to do is something like 'if (InitMethod) { ... }' in Instantiate). Make sure that the code for handling ensureOperator == nssEnsureChromeOrContent is in the implementation of Constructor. @@ +226,5 @@ > +// Use the special factory constructor for everything this module implements, > +// because all code could potentially require the NSS library. > +// Our factory constructor takes an optional EnsureNSSOperator template parameter. > +// Only for the nsNSSComponent, set this to nssLoadingComponent. > +// For classes available from a content process, set this to nssEnsureOnChromeOnly. A couple of these lines are >80 chars. @@ +260,5 @@ > + { &kNS_CRYPTO_HMAC_CID, false, nullptr, Constructor<nsCryptoHMAC> }, > + { &kNS_CERT_PICKER_CID, false, nullptr, Constructor<nsCertPicker> }, > + { &kNS_NTLMAUTHMODULE_CID, false, nullptr, > + Constructor<nssEnsure, nsNTLMAuthModule, &nsNTLMAuthModule::InitTest> }, > + { &kNS_STREAMCIPHER_CID, false, nullptr, Constructor<nsStreamCipher> }, nsStreamCipher was removed recently, by the way.

I couldn't use nullptr for member function pointer template argument because of a MSVC 2013 bug :( https://connect.microsoft.com/VisualStudio/feedback/details/807614/vc-2013-doesnt-accept-nullptr-as-default-value-for-member-function-pointer-template-argument

Since bug 1186064 has been fixed, I think we can move forward with this. Any interest in doing so?

Sure.

> z:/build/build/src/security/manager/ssl/nsNSSModule.cpp(227): error C2440: 'initializing': > cannot convert from 'overloaded-function' to 'mozilla::Module::ConstructorProcPtr' :( Build does not fail locally. Are compilers on automation too old?

Ah, Win32 calling convention bites me again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=463853b36cf247ba8c26f1f0577605a8399b7bc2

Comment on attachment 8831405 [details] Bug 1122973 - Use templates instead of macros for NSS module factory constructors. https://reviewboard.mozilla.org/r/107982/#review109410 LGTM with comments addressed, but I think we should also get another set of eyes on this. ::: security/manager/ssl/nsNSSModule.cpp:58 (Diff revision 2) > - Init) > +MOZ_ALWAYS_INLINE static nsresult > +Instantiate(REFNSIID aIID, void** aResult) > +{ > + InstanceClass* inst = new InstanceClass(); > + NS_ADDREF(inst); > + nsresult rv = InitMethod != nullptr ? (inst->*InitMethod)() : NS_OK; nit: no need for comparison to nullptr (as in, we can just say `InitMethod ? (inst->*InitMethod)() : NS_OK`) ::: security/manager/ssl/nsNSSModule.cpp:72 (Diff revision 2) > + nsresult (InstanceClass::*InitMethod)() = nullptr> > +static nsresult > +Constructor(nsISupports* aOuter, REFNSIID aIID, void** aResult) > +{ > + *aResult = nullptr; > + if (aOuter != nullptr) { nit: same here w/nullptr comparison ::: security/manager/ssl/nsNSSU2FToken.h:33 (Diff revision 2) > // For nsNSSShutDownObject > virtual void virtualDestroyNSSReference() override; > void destructorSafeDestroyNSSReference(); > > + // Required to satisfy Win32 where nsresult != NS_IMETHOD > + nsresult Initialize() { return Init(); } Let's actually fix this by removing init from nsINSSU2FToken.idl.

Comment on attachment 8831405 [details] Bug 1122973 - Use templates instead of macros for NSS module factory constructors. https://reviewboard.mozilla.org/r/107982/#review109410 > nit: no need for comparison to nullptr (as in, we can just say `InitMethod ? (inst->*InitMethod)() : NS_OK`) I had to add the comparison to silence compiler warnings on Linux/macOS. > Let's actually fix this by removing init from nsINSSU2FToken.idl. Done.

Comment on attachment 8831405 [details] Bug 1122973 - Use templates instead of macros for NSS module factory constructors. https://reviewboard.mozilla.org/r/107982/#review110924 Sorry for the delay. Anyways, thanks for doing this. Not having all these horrible macros is a lot nicer.

Pushed by VYV03354@nifty.ne.jp: https://hg.mozilla.org/integration/autoland/rev/3a1ed6f4ed6a Use templates instead of macros for NSS module factory constructors. r=Cykesiopka,keeler