I'm not sure when I'll finish -- maybe you should just land this and
I'll do it after? None of these are showstoppers to landing, needless
to say.
Is conrad landing soon?
/be
client/src/ipcService.h
- template usage ok? typedef ipcList<class ipcClientQuery> ipcClientQueryQ;
- //PRBool InWaitMessage() { return mInWaitMessage; } vs. #if 0 for members
- how about a major doc-comment talking about ipcService class?
- can you use an nsCOMPtr<ipcTransport> for mTransport?
client/src/ipcSocketProviderUnix.h
- XP_UNIX downcast in ~ipcTransport could use NS_STATIC_CAST? avoid .get()?
Why does only Unix need the ClearTransport call?
- why a monitor instead of just a lock, or a lock and a condvar?
- if (!mMonitor), nsAutoMonitor will assert but not crash, but you'll race
among multiple threads; can't fail the ctor, check in Init method?
client/src/ipcModuleFactory.cpp
- Mixing #if 0 with commenting out to hide ipcService{R,Unr}egisterProc
client/src/ipcService.cpp
- Use PR_STATIC_CALLBACK, not static PRBool PR_CALLBACK, for max portability
oh wait, this matters only on WIN16 -- still, worth using that macro?
- ipcClientQuery::IsCanceled()
use nsnull, not NULL, and do you need to use .get() to compare to 0?
i thought not any longer (old gcc bug, we're past it?)
- ipcClientQuery::OnQueryComplete()
- check for malloc failure
- move const char *lastName and *lastTarget inside the loop bodies, no need
for useless inits and outer-scope decls
- Use nsnull not NULL? (throughout, last time i'll mention it -- ah, there's
an nsnull in the ~ipcService assertion)
- ipcService::Init()
just return mTransport->Init(this); (no need for rv dance, unless you have
to normalize NS_COMFALSE or another success code into NS_OK)
- ipcService::OnIPCMClientID()
unchecked new in mTransport->SendMsg call
- ipcService::OnIPCMClientInfo() and ipcService::OnIPCMError() seem awfully
similar, but they're short; just wondering if they could share code (not
source code, compiled code)
- hey, new failure checks and NS_ERROR_OUT_OF_MEMORY! i thought i was out to
lunch wanting those earlier (Photon's philosophy is very big-virtual-memory
system: who cares? you'll thrash to death first; put up a dialog and close
other apps; whatever -- but i digress)
- whoops, ipcService::QueryClientByName() does not null test query = new ...
- ditto for ipcService::QueryClientByID()
- more unchecked new calls in SetMessageObserver -- check throughout
- does passing null to SendMsg make it fail immediately with NS_ERROR_OOM?
it fails, but not with OOM -- shouldn't it? or else callers here should not
pass in new ... (potentially pass in null)
- why does the ipcService observe "profile-change-net-..."? what's the -net-
mean in those strings? comments would be helpful here
- why the PRUint8 vs. char types in const pointer params that seem to require
casts without adding much value?
- does SendMsg always consume its msg param?
it depends on whether SendMsg_Internal does; i see a case in the Unix version
that returns early due to evil NS_ENSURE_TRUE's hidden return, w/o freeing
msg....
client/src/ipcSocketProviderUnix.cpp
- ipcIOLayerConnect
- do the stat after fstat OSX 10.1.5 nonsense only for that platform
- ipcSocketProviderUnix::NewSocket
- don't you need to null *securityInfo before returning NS_OK? or is that
not an out param?
client/src/ipcTransport.cpp
- general comment: are you using a monitor just to get a lock and a condvar,
or do you need reentrancy? it seems better to me to use a separate lock and
condvar if you don't need a monitor, but if that just bloats the code, then
never mind