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.

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.

+ 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.

OK. (I find the if-operator *much* easier to decipher.)

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.

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.

nit: comment style really varies in this file.

There is a system behind it, but OK to change it however the editor pleases.

why are you using wide strings for URLs? why not just use UTF-8?

Don't remember, anything is fine with me.

+ 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.