Page MenuHome GnuPG
Files F20064603

review.txt
No OneTemporary
Actions

review.txt
View Options

nsIPCService: nsIStringStream doesn't seem to be scriptable. Hence it won't
serve the purpose. newStringChannel could be moved elsewhere (necko?)
nsIPipeChannel: True, it is only the implementation that isn't thread-safe. I
will change the comment
nsIProcessInfo: I don't think it should be merged with nsIProcess, although it
could be moved to xpcom and/or renamed. This interface provides info about the
current process, not about any newly created process (like nsIProcess)
nsIRandomService: This is necessary for Protozilla and could be useful otherwise
too. It too could be moved to XPCOM, if it isn't to stay in the extensions.
> MOVED INTO NSIIPCSERVICE
nsIPipeConsole: I will rename GetPipe to GetFileDesc
> DONE
nsIProcess: I could add another method initWithProcess to nsIPipeChannel to
initiate IPC using an nsIProcess. But you would need to add proprerties to
nsIProcess so that I can access the command arguments.
------- Additional Comments From Doug T 2001-08-28 11:15 -------
The api that I think should move to nsIProcess is GetEnv. This interface,
already has API related to the running of an exe, namely Kill().
> TODO
Lets get rid of the RandomNoiseService and interface. It is only used from the
nsIPCService so maybe a simplier implementation can live there. Maybe I am
missing something??
> DONE
nsIPCService.cpp:
Since mConsole is a nsCOMPtr, you do not need to initialize it to nsnull. Same
for mCookieStr (but I am a little less certain about this case).
> DONE
In the ~(), you do not have to explictly null mConsole.
> DONE
Should the Open() parameters on the nsIPipeConsole be turned into preferences -
or at least #define's?
> IPC does not have a UI at this time
In the Init(), you really don't need to have the local var pipeConsole. Just
use mConsole. Your really not using mConsole anywhere to test to see if Init()
failed.
> DONE
In GetConsole(), I do not think that you need to call mConsole.get(). Just use
mConsole and let the the nsCOMPtr do its magic. You do this in other places too
which need to be fixed.
> DONE
In ExecPipe, you init most of the out vars, but not all of them to null. You
should also init env?
> env is actually an input variable! (an array of strings)
When closing a nsIOutputStream, there is no need to explictly Flush as you do on
line 269 and in other places.
> DONE
Nit: use a 'while (1)' instead of a 'for (;;)' This is done in a couple places.
> TODO
also note, if you want to optimize the emptying of the inputstream, you could
call ReadSegments.
> TODO ???
nsIPCService.h
I see RegisterProc an UnregisterProc defined, but I don;t see that they are
implemented anywhere... Create too - in all of the headers?
> Deleted the methods
nsPipeChannel
This class is not threadsafe. Not sure if you intend it to be.
> DOES IT NEED TO BE?
don't init nscomptr's to null. General Rule.
If you do this:
nsCOMPtr<nsIURL> url( do_QueryInterface( aURI, &rv ) );
to test success, all you must do is check url, not rv or both.
> DONE
When getting an out string from a API, it is better to use nsXPIDL(C)String.
Change:
char* contentType = nsnull;
rv = MIMEService->GetTypeFromURI(url, &contentType);
to
nsXPIDLCString contentType;
rv = MIMEService->GetTypeFromURI(url, getter_Copies(contentType));
> DONE
In GetName, you should probably protect against a null mURI
> DONE
In OnDataAvailable,
I think that this sum is wrong. aLength is always the relative length.
mContentReceived += aLength - aSourceOffset;
> DONE
nsPipeConsole
> DONE
you do not have to have a local variable here, just use mPipeThread
nsCOMPtr<nsIThread> pipeThread;
rv = NS_NewThread(getter_AddRefs(pipeThread), this, 0, mThreadState);
> DONE
See if you can easily use a Lock instead of a Monitor? Monitors are much
heavier than a lock, and I don't see you using the CondVar properties of the
monitor.

File Metadata

Mime Type
text/x-c
Expires
Sun, Feb 23, 7:18 PM (3 h, 2 m)
Storage Engine
local-disk
Storage Format
Raw Data
Storage Handle
e8/bf/0827f9402eb81f7cb9f0a6924e3b

Event Timeline