Yikes, what a long review. Sorry for the delay nevertheless.
darin wrote:
+mozSRoaming::mozSRoaming()
+ : mFiles(10)
+{
+ mIsRoaming = PR_FALSE;
+ mMethod = 0;
+ mRegistry = 0;
+}
nit: why not initialize all variables using the variable initializer list?
mFiles is an array or so, thus the custom constructor call via init
list. I thought the general style was to use the ctor body, thus the
rest, but both is fine with me.
nit: do you want to use NSPR logging instead? commenting out printf statements is a messy way to manage debug logging.
True, but NSPR log is even more messy IMHO - set up env vars each
session, figure out the right module name and log level etc.. We can
remove the printfs entirely, if they are a concern.
+
+ rv = ReadRoamingPrefs();
+ nsresult rv = NS_OK;
nit: no reason to initialize |rv| if you are going to assign a
value into it immediately.
There is: IIRC, I found myself moving around the definition and
adding/removing init all the time, IIRC even having it accidently
uninitalized at one point in this very module. Our code is not set in
stone, OTOH compilers may optimize this away anyways. For consistency
and safety, I always define and init nsresult at the very top.
nit: shouldn't this return a |const nsCStringArray*|? that way a caller
cannot modify the contents of mFiles. or is that desired?
No, not desired, IIRC, so you are right.
passing nsCOMPtr<nsIFile> by value has significant cost.
Huch? I assumes nsCOMPtr to be lightweight, only slightly more heavy
than a raw pointer and only increasing/decreasing a counter.
are you sure you want to do this? i would recommend using the standard XPCOM getter
style, which would be much more efficient, e.g.:
The idea was to have the result be a return value, so that I can use
nested function calls. XPCOM style doesn't allow that, which I find
extremly inconvient. In this case, however, I see that I don't use the
function in a nested way anyways and always check the return value, so
the XPCOM way would be better, yes, we could then maybe also return an
error code.
then callers can do the familiar:
nsCOMPtr<nsIFile> profileDir;
GetProfileDir(getter_AddRefs(profileDir));
if (profileDir)
or better yet:
nsCOMPtr<nsIFile> profileDir;
rv = GetProfileDir(getter_AddRefs(profileDir));
NS_ENSURE_SUCCESS(rv, rv);
BTW: I don't use the NS_ENSURE_SUCCESS macro in the code. I probably
should, it has proven helpful at other places, because it asserts at
the *lowest* point, near where the error actually appears, not at the
higher point where it's being catched, which makes tracking bugs a lot
easier. That's also the reason for the printfs, which can then be
omited:
+ if (NS_FAILED(rv))
+ {
+ //printf("error 0x%x\n", rv);
+ return rv;
+ }
(This whole thing can be replaced with the one-line macro)
+mozSRoaming::ConflictResolveUI(PRBool download, const nsCStringArray& files,
+ //nsCStringArray* result = new nsCStringArray();
why is this commented out? maybe it should be deleted?
I don't know anymore, sorry.
+ ioParamBlock->SetInt(0, download ? 1 : 2);
+ ioParamBlock->SetInt(1, files.Count());
should these indicies (0 and 1) correspond to some enum type? magic
numbers are bad. it is not at all clear to me (the casual code reader)
what they are supposed to mean.
It's defined in the long comment right above it ;-).
This just passes information to/from the JS code. I agree that the
nsIDialogParamBlock used is a bad solution, but I didn't know any other
one which wouldn't require considerably more [complicated] code.
enums give me some string to grep on to learn more about the identifier :-)
Yes, but these are also used in the JS code, so your grep would fail => misleading.
this seems to suggest that all of the filenames are ASCII. is this
strictly true? i assume these filenames all correspond to files that
live in our profile?
Yes.
again, what are all these magic numbers? 3, 4??
See above.
+ ? value == 1
+ : value == 2)
+ if (download
if ( (download && value == 1) || (value == 2) )
(or never mind...
OK. (I find the if-operator *much* easier to decipher.)
+nsresult mozSRoaming::Registry(nsCOMPtr<nsIRegistry>& result)
nit: this is a wierd way to use nsCOMPtr...
lol
See ProfileDir. Using XPCOM style is OK with me.
+ if (mRegistry)
+ {
+ result = mRegistry;
+ return NS_OK;
+ }
nit: perhaps you should change the sense of this to if (!mRegistry)
so you can share the common exit code.
Well, that's style. I always use this early-exit style (in non-XPCOM
code as |if (a) return a;| at the start) for caching functions, this
avoids long indented blocks.
nit: no need to initialize |rv| here either.
See above.
generally better to move declaration of nsXPIDLString (and other stack-based
instances of classes that define destructors) closer to where they are first
used. this way you don't have to run the destructor on them if you return
early before the objects are even used. this helps minimize code size bloat.
heh, well, I logically want to get the profile here, that's why I
declare that var, get the necessary profile manager and right
afterwards get the current profile. In JS, all that would be only one
statement. The destructor argument is intersting, I didn't think of
that, but the likelyness that the profile manager is not available is
extremely small, given that only the profile manager calls this code
;-). But the change is fine with me.
nit: use NSPR logging or remove the printfs. this nit applies throughout
the source.
OK, I'd opt for removal, then.
nit: no need to initialize |rv| since you are just going to overwrite it
immediately.
See above.
it seems kind of odd to fetch the nsIRegistry instance in this way given that
it happens to be a member variable (mRegistry).
Keep in mind that this code is run at most once at each of startup and shutdown.
nit: GetRegistryTree might be a better name for this.
OK. But the same would need to apply for ProfileDir(), FilesToRoam(), then.
+ mIsRoaming = enabled == 0 ? PR_FALSE : PR_TRUE;
change this to:
mIsRoaming = (enabled != 0);
heh, yes, indeed.
+ NS_ConvertUCS2toUTF8 files_pref(files_reg);
use NS_ConvertUTF16toUTF8 instead
OK
+void mozSRoaming::PrefsDone()
can this function be removed?
Probably. Alternatively, we could define it |inline|.
+{
+ //printf("mozSRoaming::CreateMethodHandler\n");
+ if (mMethod == 1)
+ return new mozSRoamingStream;
+ else if (mMethod == 2)
+ return new mozSRoamingCopy;
+ else // 0=unknown, e.g. prefs not yet read, or invalid
+ return 0;
+}
nit: "else" after a return statement is unnecessary.
Note that this (any many other comments) probably makes no difference in the resulting exe.
should mMethod be an enumeration type? magic numbers (1 & 2)... what do they
mean?
They are defined somewhere, they simply mean "use mozSRoamingStream"
and "use mozSRoamingCopy". I guess we could use an enum here, but the
values must stay, they are used in the user's registry, IIRC, even as
new values (= for new 'protocol handlers') are added.
+ So, I just power netlib up again and then power it down again by sending
+ the corresponding profile change notification.
this will have effect on SSL.
IIRC, I tested the code with SSL and it works, but not sure.
for
example, if you are fetching the profile over an SSL connection and the
user had already agreed to accept the site's certificate for this
session, then you will end up requiring the user tore-accept the certificate.
If you put your profile on an SSL-protected server and don't
permanently accept the server's certificate, you are insane. Any other
cases?
of course, i'm not sure how the UI for that would be handled anyways. perhaps we need a better event. restarting necko like this seems like a hack.
It is, as written. But adding a new event didn't (and doesn't) seem very tempting.
+ virtual ~mozSRoaming();
nit: does this really need to be virtual?
You asked it to be removed anyways. And the other functions are not virtual, so it's pointless.
(*phew* not even half through.)
+ nsresult RestoreCloseNet(PRBool restore);
i would recommend writing two inline helper functions for this:
nsresult RestoreNet() { return RestoreCloseNet(PR_TRUE); }
nsresult CloseNet() { return RestoreCloseNet(PR_FALSE); }
this way the callers can use these inline functions and the resulting binary
code will be just the same as you have it today.
Agreed.
CopyFile
declare this function |static|.
+ nsCOMPtr<nsIFile> fromFile;
+ rv = fromDir->Clone(getter_AddRefs(fromFile));
+ if (NS_FAILED(rv))
+ return rv;
+ rv = fromFile->Append(fileSubPath);
fileSubPath ... can it contains path delimiters?
Yes.
then i think you need AppendRelative which is defined on nsILocalFile.
+ nsXPIDLCString path1, path2;
+ fromFile->GetNativePath(path1);
+ toFileOld->GetNativePath(path2);
+ //printf("trying to copy from -%s- to -%s-\n", path1.get(), path2.get());
simply remove all of this.
Yes, oversight.
+ if (exists)
+ {
+ rv = toFileOld->Remove(PR_FALSE); // XXX needed?
what is the XXX comment about? can you resolve that? or can you
provide more information about why it may or may not be needed?
I don't know, if the following copy may fail, if the target file exists
(permissions wrong or whatever). Maybe you know. I don't care to find
out, this was mainly test code, which may be useful for some users.
+ rv = fromFile->CopyTo(toDir, fileSubPath);
note: fileSubPath must be a simple leaf name for this to work.
uh :-(
this looks very familiar... you sure you don't want to share code?
eh, 2 lines?
+ if (!mController)
+ return NS_ERROR_INVALID_ARG;
should this be changed to a debug-only NS_ASSERTION?
OK
+ // Get prefs
this comment seems wrong. do you mean "// Get registry" ??
I get these user prefs from the registry ;-)
+ rv = NS_NewNativeLocalFile(NS_ConvertUCS2toUTF8(remoteDirPref), PR_FALSE,
broken... use NS_NewLocalFile instead.
OK
+ mRemoteDir = do_QueryInterface(lf, &rv);
this QI is not necessary
OK
+ rv = lf->GetNativePath(path);
+ //printf("remote dir: -%s-\n", path.get());
[remove]
OK
+ mProfileDir->GetNativePath(path);
[remove]
OK
+ NS_ConvertASCIItoUCS2 fileL(file);
NS_ConvertASCIItoUTF16
Everywhere, probably,
OK
+ if (download)
+ {
+ if (!remoteExists)
+ continue;
+ else if (!remoteExists)
this seems wrong... should both cases really be checking |!remoteExists|??
Indeed! Got catch, thanks!
See below, this was probably supposed to be !profileExists / !remoteExists
+ /* actually, this code is not needed given how the last modified
+ code below works, but for readability and just in case... */
should there be a NS_WARNING here
OK
+ nsCString& file = *copyfiles.CStringAt(i);
use |const nsCString& file = ...| instead.
OK.
+/* ***** BEGIN LICENSE BLOCK *****
(I should have left my name out as well.)
+ * The contents of this file are too trivial to have copyright protection
+ * and are thus in the public domain.
+ *
+ * ***** END LICENSE BLOCK ***** */
just to be different?
lol, yeah, I admit it :-). I opt to be different when it makes sense to
me - in this case, this is a subtle political statement ;-). Really,
it's silly (and invalid) to claim copyright for that, and files where
the license plate is longer than the code just annoy me (scrolling
etc.).
Plant a license even on that, if you want.
+ * Version: NPL 1.1/GPL 2.0/LGPL 2.1
NPL??? shouldn't this use the MPL tri-license instead?
Yes, accident.
+#include "nsCOMPtr.h"
nit: why include nsCOMPtr.h here?
*shrug*
+class mozSRoamingProtocol
nit: this last "protected:" serves no purpose.
yup, accident.
nit: comment style really varies in this file.
There is a system behind it, but OK to change it however the editor pleases.
+void AppendElements(nsCStringArray& target, nsCStringArray& source)
this function should be declared |static| since it does not need
to have global visibility.
True.
might be good to target.SizeTo at the top of this loop if you anticipate appending a lot of items.
Good idea.
+ rv = ioserv->NewURI(NS_ConvertUCS2toUTF8(mRemoteBaseUrl),nsnull,nsnull,
maybe you want to just use NS_NewURI from nsNetUtils.h instead
OK
why are you using wide strings for URLs? why not just use UTF-8?
Don't remember, anything is fine with me.
+ rv = ioserv->NewFileURI(profiledir, getter_AddRefs(mProfileDir));
use NS_NewFileURI instead.
OK
clean up this apparent debugging code.
(Everywhere)
OK
+nsresult mozSRoamingStream::Download()
+{
+ return DownUpLoad(PR_TRUE);
+}
+nsresult mozSRoamingStream::Upload()
+{
+ return DownUpLoad(PR_FALSE);
+}
it's interesting that both of the mozSRoamingProtocol subclasses implement
Download and Upload in exactly the same way. should DownUpLoad be the
only virtual method then?
No, implementation detail. mozSRoamingProtocol was supposed to have a
number of other subclasses (not sure, if it will, due to the progress
dialog / JS disaster). It's not like this were a lot of code...
no need to initialize |rv| here... maybe move it down to where it is
first used.
See above.
+ Unfortunately, this doesn't work. I have to
+ block this function until the transfer is done, but have the transfer
+ itself and the progress UI working. darin says, I will get threading
+ issues then. He suggested to initiate the transfer from the dialog
I said this?
Yes, I can give you the IRC logs, if that helps.
It's not clear from your comments here what threading
issues have to do with this... where are the threads? Maybe we spoke
about spinning up a new thread for the downloads? That would be a problem
for sure since Necko is not threadsafe.
What I wanted to do was to put all the transfer logic into the C++
part, still have a dialog showing the progress and results to the user,
and not return |Download()| until all of that is finished (all
transfers done, dialog closed (automatically when successful, by user
when failure)).
As far I as I understood you, putting the transfer logic into JS was
the only way, because of said threading issues (which I didn't
understand, I have zero clue about Mozilla's threading).
It's too late now anyways, because all the code is written. I'm not
going to re-implement it in C++. But if I misunderstood you, we should
update the comment.
*phew*! done.