This is an implementation proposal for bug 17048 and bug 124026. In this case, the minimal work to get some roaming support, with some Netscape Communicator 4.x compatibility, running will be done. Only certain files will be exchanged with the server, e.g. - bookmarks.html - cookies.txt, cookie permissions (cookperm.txt) - mail filters - address book - history More single files can be added relatively easily. Mail will not be exchanged, because it's stored in a lot of files and you can use IMAP for that. Not sure about user prefs (prefs.js, user.js), because prefs.js is partly machine-dependant (-> more work) and might partly not be backwards-compatible to 4.x. Not sure, if exchanging private keys and passwords is a good idea. As a protocol, HTTP PUT and/or MOVE will be used. For bonus points, HTTPS and WebDAV are also supported (should not be too hard), but that will probably need special funding. There SHOULD be some simple UI for specifying the server and the files exchanged. If there's not enough funding for building that in Mozilla and it turns out that using (and modifying) an external application can to the job, this shall fulfill the specs as well. But that's a matter of last resort. The specs might change slightly, if the majority agrees.

-> me to not spam others

Ben, This sounds pretty good. Could there be any support for encryption of the files residing on the remote server? Most UNIX systems give some level of read access to users' directories; by encrypting the files directly, we know they're secure on the server & over the network even w/o SSL. - Adam

Adam -- that's not a mozilla change exactly -- it's a change to the mod_roaming server....

Matthew Miller, no. That won't help over the network. Adam, this will break 4.x-compatibility.

Ben -- oh, right. I was assuming something like: files sent over ssl, and then the remote server encrypts them in some other way upon receipt. But that's basically silly. :)

For 4.x compatibility, encryption could be a checkbox. If someone requires that, they can simply flip the checkbox off. But by encrypting the files before they're sent, users would know that their files are basically secure, even on a server that they know to be insecure. I'm just thinking about someone casually reading through someone's cookie file, for example. With very basic encryption, this would be difficult. - Adam

Adam -- it's true, that doesn't sound so difficult. But it's definitely not part of this bug -- create a new RFE for it.

Should we leave room in this enhancement for bug 10488, website offline viewing? I'm not suggesting we DO that bug as part of this one; I'm simply saying that perhaps we could/should leave a hole that will allow them to easily piggy-back on what we've done. - Adam

Adam -- seems highly unrelated... NS 4.x didn't have a feature like that in its roaming access that I can remember (or see). Perhaps you meant to put that comment on a different bug?

I guess I was thinking that some of the code we write for this bug could have utility in that one... What, I'm not sure. It seemed so obvious when I posted it. :) - Adam

WARNGING: Please not comment here unless you either - are a Mozilla developer - donated money (see bug 124026) - want to help coding I started working on this. Code is planned to live in profile/session-roaming/ (or similar). Part of the README: When the users selected a profile, we will check, if it's a roaming profile and where the data lies. If necessary, we will contact the server and download the data as files. We will overwrite local profile files with the downloaded ones. Then, the profile works as if it were fully local. When the user then logs out (shuts down Mozilla or switches to another profile), we upload the local files, overwriting those on the server. We compare the modification times of files and ask the user (-> GUI) before overwriting newer files. Following cconrad's advise, I do not hook up using nsIProfileChangeStatus, but in nsProfile directly. That just calls |nsSRoamingSync|. This in turn uses various protocol handlers like |nsSRoamingHTTP| to do the upload/download. These in turn may use generic protocol handlers like the netwerk HTTP protocol to do that.

s/cconrad/Conrad Carlen/ Conrad, please see above, if that's OK with you.

Sounds good - and I like your WARNING: ;-) Why would custom protocol handlers be needed?

> Why would custom protocol handlers be needed? They'd hook up roaming with the generic protocol implementations. E.g. the HTTP one reads the http url, username etc., then downloads each file by driving netwerk's HTTP impl..

Conrad, nsProfile::ShutDownCurrentProfile (which you suggested) is never called during shutdown (at least here on Unix without QuickLaunch). Any other suggestions where to hook up the upload?

It's being called from here: http://lxr.mozilla.org/mozilla/source/xpfe/bootstrap/nsAppRunner.cpp#804 How could that not be happening for you? Notice, though, that code is passing "0" for the type of shutdown. It should be passing SHUTDOWN_PERSIST but that call was added recently and quickly and we didn't investigate the amount of redundant work that could happen by passing SHUTDOWN_PERSIST. Either way, ShutdownCurrentProfile() should be getting called. Come to think of it, the uploading also needs to be triggered when switching to another profile in SetCurrentProfile().

Status update: I have a very rudimentaary version of roaming working. Somewhat working is: - Central roaming switchboard code - A "copy" implementation of the "protocol handler" - it copies files from/to another location in the local filesystem - Conflict resolve UI (lets you select which files to keep, if both have changed) Outstanding is (mainly): - HTTP protocol handler (I have some code, but it doesn't work) - Progress UI - Prefs reading (I tried to read normal prefs, but that doesn't work. Maybe not too surprising.) - Bugs - opt build exits at startup (might be independant of my code; debug build works) - ShutDownCurrentProfile not being called - Conflict resolve UI layout I won't be able to work on it in the next week, and I have an assignment in the next months. I will try to work on it in my remaining free time, but can't make promises. :-(

Seems like the last sentence was a bit misleading. This bug has the highest priority to me and I will work on it as much as I can. I want to get it finished ASAP. I just won't be able to work on it full-time.

This is the current state, no change in the last month. Primary intention is backup and giving donators some confidence. License: Unlike stated in the source file, this is not MPL (open-source) as long as it is done and I'm paid. You may not use it for daily work, but you are welcome to look at it or try it out.

BTW: I am trying to work on this a bit in the next days.

Forgot some parts. Same disclaimers.

Just curious...are you looking into any of the Publishing capabilities of the Editor functionality of Mozilla to avoid possible duplicate code? It seems to me that the publishing functionality represents the same thing when syncing roaming files. Also doesn't the bookmarks and many other roaming files have a last modified attributes that could be used for identifying possible changes in the roaming files which would trigger synching of the files? Although some of this may be at a per link level, but from the file modified attribute that also might be usable. I would think whenever a change occurred (except for rapidly changing files like history) writing would be preferrible (unless you have an extremely large roaming file(s)).

> are you looking into any of the Publishing capabilities of the > Editor functionality of Mozilla to avoid possible duplicate code? Yes, that is part of the general networking code and I try to reuse it.

I wish I had found this bug earlier but oh well. I have a rough implementation of remote bookmarks working in bug 158964.

Cool. Ben, can we have an update on how your work is going?

I am fighting with undocumented APIs and features of webbrowserpersist (upload/download, multiple files).

To give a short sample of what I spent my time on: I took a close look at Composer's publishing functionality. I extracted 1000 lines of publishing code (of out several thousand lines of code) and tried to understand and adapt it for roaming, only to find out that it uses WebBrowserPersist by passing a URL into a *file* argument (both have completely different interfaces). Although WebBrowserPersist is a public API for embedders, this is documented nowhere: not in the API, and not even in any comments I found, neither in the editor nor webbrowserpersist. I had to read and compare the editor with the internal webbrowserpersist code to actually believe it. Now, I am wondering, if I should extract the nice-looking progress dialog and authentication from Composer or if I can use nsIDownload for the progress dialog of the upload in similarily "creative" ways.

I had a long and very productive meeting yesterday with brade, cmanske and partly darin. We discussed how to implement the http transfer (considering threading and blocking issues) and esp. the progress UI. I have an attack plan now that we think should work, I am implementing and trying it out now.

Status update: Working: I used the Composer Publishing UI as base: - The progress dialog comes up, listing the files to be transfered - Download backend works - Upload code written, not sure, if it works - The status/progress feedback works partially only To be done: - Make upload, feedback work - Esp. auth prompts Other parts to be done: - prefs backend - prefs ui - check for conflicts (ui done) - polish progress and conflict dialogs

> To be done: > - Make upload, feedback work > - Esp. auth prompts Done, done, done. In others, it can now up- and download to a HTTP server and request necessary passwords. But it's still all very rough, work-in-progress. I have no idea where I should get the prefs from - the prefs service seems still unfunctional at this point. Any clueful hints that might save me some digging?

Current state of work. For status, see updates above. Work in progress.

This is required by the above patch. It makes some smaller changes to netwerk, mostly: - WriteFrom implementation for FileStreams - Changing some error codes that are defined twice

(Legal crap: Same restrictions as above still apply.)

Ben, what's the status of this one?

No significant progress, I couldn't work on this in the last 2-3 weeks. Will resume work soon.

Can the target milestone be updated, to future if necessary?

I am still planning to have this done this year, unless personal things prevent it. Thus, 1.3beta. The attached code is now under the MPL, because I have now written contracts.

Conflict resultion (avoidance of overwriting of newer files) won't make it this year, due to bug 186016, unless I get external help. I'll have a hard time to make it this year anyways, but I will try to keep my promises. :-( Pref backend is working. I followed ccarlen's repeated advise to use the Mozilla app registry to store the roaming settings, not the prefs system (see earlier comments). Part of the reason for the decision was that he told me that he plans to migrate the file format to XML in the 1.3 timeframe. I know that many sysadmins will want to edit the settings using scripts to remote-administrate lots of users, and plaintext files are a great help in that case. Another (smaller) part was that I finally found nsIRegistry.idl (which is in xpcom/components/). I previously only looked at modules/libreg/ and was horrified by the header files (without any documentation) I found there. With my patch, I'll add a README to that dir, so that the next developer looking there knows about the IDL file. I worked on the prefs UI, but that's unfinished. Remaining is mostly error handling and polish. I.e. removing all (most of?) the XXX cruft that has been left during development, testing, trying to make it work in non-standard situations etc.. I will also have to finally decide on where the code should live and make the build work for that case. Candidates are: - profile/src / profile/resources - profile/sroaming - extensions/sroaming Opinions, please, esp. from ccarlen. Oh, and thanks guys for not harrassing me with comments asking for update on the bug .).

Trying to keep my promise of "this year" :-/ *sigh*, here is the current state of work. I will attach the source (under MPL) and upload a Linux debug test build of Beonex Communicator to my server. Have fun! :-) (Hopefully it works for you ;-P ) It is mostly functional, modulo the parts mentioned below. Mainly: - it is still buggy, esp. in edge/failure-cases - the username in the pref dialog is ignored - there is no conflict resolution yet (e.g. when mozilla crashed, the next start will use the files from the server, you also can't run 2 instances of Mozilla/Beonex Communicator concurrently against the same roaming profile) - I have to clean things up for checkin my todo list: now - XXXs - rename pref functions - username, $USERID in URL don't work yet - interpret http responses - make passed profile dir a url, what about mac? - interpret old webbrowserpersist onstatechange function for useful stuff - small stuff - error handling - empty pref - cancel by user in progress dialog: xpconnect errors - ftp - xpconnect errors after upload - http - no user name (upload to <http://server/roaming/ben/localstore.rdf> gives NS_OK) - wrong path in url (but working server) - test same situations with ftp - non existant files: stops during upload, doesn't during download - if something fails horribly (e.g. a JS error in the prompt* functions), then the UI reports success or is completely malfunctional. - ui: (error/success) messages - copy protocol - no remote files yet Later: - mod/upload times - remove printfs - build - dir - contents.rdf correct? - windows, mac - polish - conflict ui - progress ui - focus cancel button? Enhance (with additional funding) - protocols - LDAP - IMAP - password encrypt - ProfileManager

This is the source in a zipfile. Extract it to mozilla/profile/, then apply mozilla/profile/patches/124029-4-full-changes.diff to mozilla/ (i.e. the top-level of the tree).

The source patch is against 1.0.x. Trunk is untested. To enable roaming, go to prefs dialog. For remote servers, as URL, enter e.g. ftp://ben@server//home/ben/roaming/ [note the double-slash after the host] http://ben@server/roaming/ben/ Copy File should work as well. During the first upload, you get a conflict. left stands for the "remote" dir, right for the "local", internal Mozilla profile. Select the right boxes. Note that your password is stored in plaintext in the Mozilla registry file. I thought it was a problem until I realized that we currently use plaintext protocols (HTTP, FTP) anyway. (I might look into HTTPS later.) Hit OK before switching pref panes, or you'll lose your changes. You also have to open the "Item Selection" pane once, unselect all inexistant files (e.g. addressbook) and press OK. (just noticed those bugs :-( ). Gee, is this all still buggy :-( *sigh* But you will see lots of debug info running by on the console. You'll probably find a hint about most problems in there somewhere. This build is really only for the most adventurous users who like to play with new functionality or are dying to use roaming. If you want something that works, wait a bit more until the bugs are fixed. Hopefully not too long. <http://download.developer.beonex.com/communicator/0.8/dev/beonex-comm-0.8.2-dev-roaming-4-linux-i586-debug.tar.bz2> Hope you enjoy it nevertheless :)

Very cool. So what happens now? Will this automatically be checked into the Mozilla tree, or does somebody else have to review it, or is not ready for that at all yet? What's the procedure/timeline?

adding bug 136492 as blocked by this (if bugzilla will allow me!).

> I will also have to finally decide on where the code should live and make the > build work for that case. Candidates are: > - profile/src / profile/resources > - profile/sroaming > - extensions/sroaming > Opinions, please, esp. from ccarlen. I'd need a decision on this now. I'd like to check the source into the Beonex tree to work from there and produce preliminary builds, but before I can do that, I need to know the final location of the files. Part of the code, esp. the transfer code written in JS and possibly the progress dialog, is of general nature. For example, with some changes, the editor publishing code might use it. Or I am successfully using it for another project which has to replicate remote files locally for pre-caching. In that case, the UI is a progress meter in the status bar, so the generality of the code has been proven to some extend. Because e.g. editor publishing cannot depend on extensions/, a location there would be disadvanturous in the long term, although I like the idea of isolated code in extensions/. ccarlen, what do you think? It's your call.

I believe the Calendar project now has "remote storage" with syncing. Their efforts, I think, have parallels to yours. Since the calendar is well on its way to becoming a part of the default builds maybe some type of harmonization with their structure would prove advantageous?

I'm in favor of putting it in extensions. Having it in extensions means that the mainline tree has to be able to build without it. Is profile/src going to be dependent on interfaces in the new module?

> I'm in favor of putting it in extensions. nod, but that means we can't reuse the code :-( > Is profile/src going to be dependent on interfaces in the new module? Yes. The IDL would have to live in profile/public, profile/src/nsProfile.cpp would try to get an implementation, and if that fails, just ignore the error and go on.

Yeah, then put it in profile/sroaming. Actually, I'd rather just call it "roaming" instead of "sroaming." The "roaming" subdir can have it's own "public" a "resource" subdirs - keep this stuff together as a module and in the makefiles. This should be its own module for header deps, too - called "profileroaming."

Ian Pottinger, thanks for the note, but I fear it's too late for that now, the code is almost done. They are free to reuse mine, though ;-). Conrad, oki, thanks. I used "sroaming" for session roaming, because the code is limited in design by the idea of transfer of single files, we can't sync dynamically, but there are ideas floating around for that (ACAP; RDF for bookmarks etc.), so I didn't want to imply that this is the end-all roaming implementation for Mozilla. But I'll use just "roaming", if you prefer. I guess the profile prefix makes is unambiguous enough. Thanks for the note, I can now proceed, although I am not sure how to do that header stuff. I'll look into it.

> although I am not sure how to do that header stuff You just need to add: MODULE = profileroaming to the makefile which exports headers, generates XPIDL. See other Makefiles in "public" dirs for example.

This is raising my footprint radar :) Guys, the IDL can always be in the main tree and then the extension can provide an implementation of it if the extension is available (we will be doing this for xmlprettyprint soon, for example). Putting extra features things like this in extensions is a very good idea. At a *minimum*, one must be able to compile without this stuff, and it would be very nice if it was in a DLL so one could just drop roaming in at binary time. That makes it possible for a packager to package profile roaming separately, or an installer to conditionally install it, etc. etc. As for code reuse, if you want the progress dialog to be reused it needs to be moved to a separate module from the profile stuff anyway--editor shouldn't have to import profile roaming just to be able to do a progress dialog. So I'd say it's a moot point for the current world.

> and it would be very nice if it was in a DLL so one could just drop roaming in at binary time. Of course - that goes without saying.

> if you want the progress dialog to be reused it needs to be > moved to a separate module from the profile stuff anyway Where should that be?

> > if you want the progress dialog to be reused it needs to be > > moved to a separate module from the profile stuff anyway > > Where should that be? You could create extensions/upload_progress and have the Composer stuff try to use that. However, in my view, you shouldn't do that until there is a need and it's demonstrable that composer (or someone) can and will use it. leaf can move the revision history over when that time comes, so there's no loss involved if you put that decision off that decision until later.

> You could create extensions/upload_progress and have the Composer stuff try > to use that. As I understood, core code cannot depend on extensions/. > so there's no loss involved if you put that decision off that decision > until later. hm, not sure. I am unpassionate about that location question anyways. I just need a workable (in the logn term) decision soon.

For progress UI, you should use nsIWebProgressListener as the iface - which is core. Then, you could make a XUL implementation of that in entensions, and non-XUL apps could implement that iface with their own UI. Whatever you do with progress UI, it needs to be something with a contract ID that non-XUL embeddors can override.

> Putting extra features things like this in extensions is a very good idea. Agreed. > For progress UI, you should use nsIWebProgressListener as the iface I unfortuantely had to write the transfer logic in JS (threading, explanation see patch), so I didn't use an IDL interface, just callbacks within JS. Basically the same design like the editor publish dialog where the original code came from, just a bit cleaned up and abstracted. I originally wanted to write the transfer login in C++ and use XUL/JS behind IDL for the progress UI, but said threading problem got in my way and destroyed my plans :-(. I'll now implement it as an extension with an IDL in profile/ to invoke it during startup (failure-tolerant), as mentioned above. If the transfer stuff wants to be used by core code, we'll have to move a few files.

> I'll now implement it as an extension with an IDL in profile/ Done. extensions/sroaming/ I'll attach the changes to profile/, netwerk/ and configure. profile: mozISRoaming.idl is still subject to change. We can still change that together with the extensions/ changes. netwerk: Darin, not sure, if I mentioned that before, but I noticed that the same error code was assigned twice (2 different errors had the same code). I fixed that (that's part of the netwerk changes), but could you please make sure that this doesn't happen again by cleaning the netwerk error codes up? configure: I won't check in the change to configure, just configure.in, that's just for your convience. I'll check extensions/sroaming/ into the Beonex Communicator branch BEONEX_0_8_BRANCH, you can get the source from there, so I don't have to attach a zipfile or lengthly patch with all the new source files every time. So, build and try the code, either - check out the complete Beonex Communicator code, it will include the current roaming code or - (for trunk) 1. apply the patches to profile/, netwerk/ and configure that I am about to attach. 2. fetch extensions/sroaming/ from the Beonex Communicator branch (|cvs co -r BEONEX_0_8_BRANCH extensions/sroaming/|) 3. fetch profile/public/mozISRoaming.idl from the same branch, in case I had to make changes to that As for changes to the main roaming code, I cleaned the code up a lot in the last month and also fixed a few bugs. It is still a work in progress, though.

I thought there was some kind of loose moratorium on new extensions without staff permission, given the existence of sites like mozdev.org

Good timing, alecf, to say that just when I am done. You said in bug 17048 comment 137 "We just need someone to step up and actually do the work." I did (with the help of the kind donators) and now you're saying this should not be part of Mozilla?

FYI: I am not going to move this code again, to any other place in the tree or to mozdev, or do any other massive from-the-ground up changes. I just worked on this build stuff the whole night and day. I asked several times where this should be. You knew this bug, you had lots of time to object. If mozilla.org doesn't accept it in extensions/sroaming/, that's bad luck for Mozilla, I can't help it, I did my part. It will still be available in Beonex Communicator. (I knew it. I knew Alec would do that. That's why I asked so often.)

This easily predates any such moratorium, loose or otherwise -- the original bug is from 1999. It's just taken a while. :) C'mon, there's lots of people waiting with bated breath for mozilla to have this 4xp feature. Let's not have some sort of political squabbling get in the way of its implementation.

yes, that's exactly what I was saying. In fact if you rearrange the letters in that comment, you'll see that it is an anagram for "I'm out to get you" But in all seriousness, I never said anything about where the code went. I'm not on mozilla.org staff, I'm simply delivering what I've heard.. I don't see how it is a difficult thing for you to simply ask staff@mozilla.org for permission. If I'm wrong, I'm wrong...

Comment on attachment 115496 [details] [diff] [review] profile/ changes, version 5 >Index: profile/public/MANIFEST_IDL >=================================================================== The Mac CFM builds have not been unplugged on the ports page. Until they are, you'll need to edit profile/macbuild/ProfileServicesIDL.xml in order to not break those builds. >Index: profile/public/Makefile.in >=================================================================== >RCS file: /cvsroot/mozilla/profile/public/Makefile.in,v >retrieving revision 1.13 >diff -u -u -w -r1.13 Makefile.in >--- profile/public/Makefile.in 28 Dec 2002 01:14:24 -0000 1.13 >+++ profile/public/Makefile.in 25 Feb 2003 12:27:17 -0000 >@@ -37,6 +37,7 @@ > nsIProfileInternal.idl \ > nsIProfileStartupListener.idl \ > nsIProfileChangeStatus.idl \ >+ mozISRoaming.idl \ I'm still not happy with the name of this interface. (1) The "S" bothers me. It's not obvious that it stands for "Session" and, even so, I'm not sure if that distinction needs to be made in the iface name. If it's really important to you to make the "Session" distinction, make it "Session" and not abbreviate to "S." (2) The prefix mozI instead of nsI is a pain. For instance, with embedding apps, when I want to see what interfaces are being used, I can search for "nsI*.h" and get a pretty good list. If people start using all sorts of different prefixes, that falls apart. Anyway "ns" doesn't stand for "Netscape" - it stands for "namespace." Consistency, when it comes to the gas pedal being the rightmost pedal, and interfaces beginning with "nsI", is a good thing :-) > $(NULL) > > ifeq ($(OS_ARCH),WINNT) >Index: profile/public/mozISRoaming.idl >=================================================================== <snip> >+ >+[scriptable, uuid(ab62465c-494c-446e-b671-930bb98a7bc4)] >+interface mozISRoaming : nsISupports { >+ void BeginSession(); >+ void EndSession(); >+ >+ boolean isRoaming(); // should be here? >+ >+ void Encrypt(inout wstring password); // dito >+ void Decrypt(inout wstring password); // dito This seems odd. Why would somebody need to use this iface for that purpose instead of nsISecretDecoderRing? >Index: profile/resources/content/profileSelection.xul >=================================================================== <snip> >@@ -46,7 +46,7 @@ > style="width: 42em;" > onclose="onExit();" > onload="StartUp();"> >- >+<!-- XXX are you sure that you don't mean onunload instead of onclose? BenB--> If this is wrong, fix it, file a bug, or whatever, but don't just check in a comment which asks a question. > <stringbundle id="bundle_profileManager" > src="chrome://communicator/locale/profile/profileManager.properties"/> > <stringbundle id="bundle_brand" >Index: profile/src/nsProfile.cpp >=================================================================== >RCS file: /cvsroot/mozilla/profile/src/nsProfile.cpp,v <snip> > >+ // Roaming download >+ // Tolerate errors. Maybe the roaming extension isn't installed. >+ nsCOMPtr <mozISRoaming> roam = do_CreateInstance(kSRoamingCID, &rv); >+ if (NS_SUCCEEDED(rv)) >+ roam->BeginSession(); >+ > // Phase 4: Notify observers that the profile has changed - Here they respond to new profile > observerService->NotifyObservers(subject, "profile-do-change", context.get()); > >@@ -1330,6 +1338,12 @@ > // Phase 3: Notify observers of a profile change > observerService->NotifyObservers(subject, "profile-before-change", context.get()); > } >+ >+ // Roaming upload >+ // Tolerate errors. Maybe the roaming extension isn't installed. >+ nsCOMPtr <mozISRoaming> roam = do_CreateInstance(kSRoamingCID, &rv); >+ if (NS_SUCCEEDED(rv)) >+ roam->EndSession(); > > gDirServiceProvider->SetProfileDir(nsnull); > UpdateCurrentProfileModTime(PR_TRUE); I really need to factor the profile shutting down code so most is shared between SetCurrentProfile() and ShutDownCurrentProfile(). File a bug. Until then, I think you want to do the upload in both places.

Comment on attachment 115494 [details] [diff] [review] Netwerk part, version 5 >Index: netwerk/base/public/nsISocketTransport.idl >- const unsigned long STATUS_RESOLVING = 0x804b0003; >- const unsigned long STATUS_CONNECTED_TO = 0x804b0004; >+ const unsigned long STATUS_RESOLVING = 0x804b0050; >+ const unsigned long STATUS_CONNECTED_TO = 0x804b0051; ... >- const unsigned long STATUS_WAITING_FOR = 0x804b000a; >+ const unsigned long STATUS_WAITING_FOR = 0x804b0052; these numeric constants are hardcoded throughout the XPFE because these constants were formerly not available from script (as they are now). if you really want to change these values then you will have to search all of the code for places where these numeric literals appear. check for both hex as well as decimal encodings. my suggestion would be to not try to change them. afterall, there is no need. it does not matter that these overlap with error codes listed in nsNetErrors.h... these are status codes specific to nsITransportEventSink and have nothing to do with normal nsresult error codes. >Index: netwerk/base/src/nsFileStreams.cpp > nsFileOutputStream::WriteFrom(nsIInputStream *inStr, PRUint32 count, PRUint32 *_retval) ... why do you need this? the file streams intentionally do not support this method. if you need something like this then you should wrap the file stream using a buffered stream. otherwise, your patch is just adding bloat to necko. >Index: netwerk/protocol/http/src/nsHttpChannel.cpp >+ NS_WARNING("nsHttpChannel::SetContentLength not implmented"); ... >+ NS_WARNING("nsHttpChannel::Open not implmented"); make these NS_NOTREACHED("nsHttpChannel::Func") instead, where "Func" = "SetContentLength" | "Open"

First, thanks for the fast reviews. ccarlen: > The Mac CFM builds have not been unplugged on the ports page. Until they are, > you'll need to edit profile/macbuild/ProfileServicesIDL.xml in order to not > break those builds. *sigh*, I thought that the point of moving it to ports status was that we d0on't have to bother with those strange build files. I guess I can do the change for the IDL file, but I don't think I know enough about the mac build files to do it for main sroaming code. > (1) The "S" bothers me. It's not obvious that it stands for "Session" and, > even so, I'm not sure if that distinction needs to be made in the iface > If it's really important to you to make the "Session" distinction, make > it "Session" and not abbreviate to "S." I think it might become important in the future, if jkreiser or others work on a kind of roaming that immediately notifies all running instances of changes. In this case, "beginsession" and "endsession" have no meaning, it would probably more the Observer pattern. In such a world, my (legacy?) "nsIRoaming" interface would be confusing, I think, thus the distinction. I can change it to SessionRoaming. > (2) The prefix mozI instead of nsI is a pain. For instance, with embedding > apps, when I want to see what interfaces are being used, I can search for > "nsI*.h" and get a pretty good list. If people start using all sorts of > different prefixes, that falls apart. Anyway "ns" doesn't stand for "Netscape" > - it stands for "namespace." Consistency, when it comes to the gas pedal being > the rightmost pedal, and interfaces beginning with "nsI", is a good thing :-) But the point of namespaces is to be different ;-). There are at least 2 extensions whose interfaces don't start with nsI, namely xmlterm (mozI), sql (mozI) and inspector (inI). I guess it would be even more confusing, if the interface would be called nsI and the implementation files moz, meaning that I have to change all the C++ code. Do you really think that's necessary? > This seems odd. Why would somebody need to use this iface for that purpose > instead of nsISecretDecoderRing? I wasn't sure, which implementation I wanted to use or should use. The profile is not started up yet, so I'm unsure, if I even can use general Mozill facilities. even if I can, what if the user enabled strong security with master password? He'd then (if it works at all) have to enter the master password for roaming to work, then we might change the profile world and he has to enter the master password again during runtime of the profile, which would mean that he entered the master password for roaming only, destroying the sense of a master password, making things only confusing. I don't like to have this there, thus the source code comments. I'd like to get rid of it. It's currently not implemented anyways, I just store plaintext passwords (it's the user's harddisk, after all). I said the interface is subject to change :). > > onunload instead of onclose? > If this is wrong, fix it, file a bug, or whatever n/m (seems like I was wrong) > I really need to factor the profile shutting down code so most is shared > between SetCurrentProfile() and ShutDownCurrentProfile(). File a bug. Bug 194939 > Until then, I think you want to do the upload in both places. Did you mean upload/download? Or that I should upload in SetCurrentProfile? The Shutdown code is not always executed? We must not upload during startup or after switching to a profile, otherwise we'd overwrite the changes possibly made by another instance with our older local profile. darin: > these numeric constants are hardcoded throughout the XPFE duh. Can we then change the constants in nsNetErrors.h? > it does not matter that > these overlap with error codes listed in nsNetErrors.h... these are > status codes specific to nsITransportEventSink and have nothing to do > with normal nsresult error codes. To my code (and that of Editor publishing, BTW), it does matter. It assumes error codes unique within Mozilla. I make decisions based on the error code in a central place. I also note down the error codes and later generate user messages from them. I wouldn't have noticed it, if it hadn't been a problem I ran into :-(. IIRC, I was totally confused why I get errors completely out of context (but still within the context of network / file transfer), like a redirection for FTP (huh?), until I realized what was the problem. > > WriteFrom > why do you need this? > you should wrap the file stream using a buffered stream IIRC, I do that now, I'll have to check, if that's still needed. If so, please add a comment somewhere where people see it, why it isn't implemented and what people should do. at least in the source file (IIRC, you do that in another place as well), better yet also in the interface. > make these NS_NOTREACHED("nsHttpChannel::Func") instead k. I'll wait for answers before makign actual changes to the code.

> these are status codes specific to nsITransportEventSink Oh, I think I understand you now (just woke up, sorry). You're saying that these code can only ever appear in nsITransportEventSink? (I can't find that interface, did you mean nsIProgressEvent?) And that they never appear in the same place as error codes? nsIProgressEventSink (where I got them from): * Notify the EventSink with a status message for the URL load.<BR> * @param status - A status code denoting the type of notification. This * can be a message to be displayed (e.g. for file I/O, * STATUS_READ_FROM, or STATUS_WROTE_TO), or can be an event * to be programmatically handled. * @param statusArg - An argument or arguments to the status notification. * These arguments will be formatted into any status or error * message. Multiple arguments can be passed by delimiting them * with newline ('\n') characters. void onStatus(in nsIRequest request, in nsISupports ctxt, in nsresult status, in wstring statusArg); From the wording of interface, it seems to me that I can indeed get normal error codes from |status|, which means that we do have a collision. Even if not, it would still be an IMHO unnecessary hassle to have them overlap. At least let's add a comment to the interface that these are not error codes, but the status codes from nsISocketTransport only. Without further notice, nsresult implies generic Mozilla error codes to me.

ops, darin wasn't on cc. see above.

A little status update: I've been working (in part) on the conflict resolution. The code for it is written, and seems to work in general, but it doesn't work properly yet. I also moved from the 1.0 branch to the trunk and have significant problems with networking (spurious errors, non-reported errors etc.), which I have to track down, what the problem is.

Hi ben, I tried your patch, it is a great work, very helpful. Do you have plan when to check it into trunk?

when it's done (tm) ;-P. Seriously, I have no idea when or how to fix the problems I mentioned. The rest is mainly a bit UI and polish. For the current state of work, see the plan.txt at the top of the sroaming dir.

Any update?

I haven't been able to work on it in the last months, but I am working on it as we speak, and fixed 2 important bugs yesterday.

Ben, Your fix is in your own branch? Do you have plan to merge your changes to trunk? If you don't have, I could help since we have a patch which is based on your patch and supply LDAP roaming function.

heh, wow :-) I planned to get this ready for use first before pursueing checkin. You can somewhat track my progress using the plan.txt file (I just commited an update). If you want to get it on the trunk earlier, you are *more than* welcome to drive that. The worst problem right now is that the new dynamic profile change feature causes the whole network library to shut down during the profile teardown - *before* I need to upload, so the upload fails, of course. I tried to power up and then tear down netlib manually, but that didn't work. I'll try tomorrow to undo the change introduced by dynamic profiles, to see, if that fixes things for me. (Note that it works just fine on the 1.0 branch.)

> the new dynamic profile change feature causes the whole network library to shut down during the profile teardown - *before* I need to upload, so the upload fails, of course Why don't you just do the upload on a private profile change notification that's sent out before "profile-change-net-teardown?" Make up a new observer topic ("profile-change-pre-teardown" or something) and stick in a call to NotifyObservers() here: http://lxr.mozilla.org/seamonkey/source/profile/src/nsProfile.cpp#1197

IIRC, I tried something like that already, but I'll recheck tomorrow. Thanks for the advise, though.

The upload problem is fixed, at least for HTTP. It turned out that my previous workaround worked, but there was *another* change in the 1.0->1.4 time frame which also caused the upload to fail. After I noticed and fixed that, it works. (checked in current code into my branch.) So, the code is roughly working with 1.4 now. Yay :). There are still a number of bugs left which I need to fix, though.

good news: the known bad bugs are all fixed. left is only UI polish and code cleanup.

Good to hear. Since 1.4 is the end of the line for the Moz suite, I guess we wont be seeing this as a part of a regular Moz release. Is this correct? Are there already any plans regarding Firebird/Thunderbird?

Removing myself from the cc list

Comment on attachment 125927 [details] [diff] [review] Netwerk part, version 6 >Index: netwerk/base/src/nsBufferedStreams.cpp >@@ -37,6 +37,7 @@ ... >+#include "prmem.h" why not use C++ new[]/delete[]? >@@ -568,8 +569,26 @@ > NS_IMETHODIMP > nsBufferedOutputStream::WriteFrom(nsIInputStream *inStr, PRUint32 count, PRUint32 *_retval) > { >- NS_NOTREACHED("WriteFrom"); >- return NS_ERROR_NOT_IMPLEMENTED; >+ nsresult rv; >+ char* buf = (char*)PR_Malloc(count); missing check for OOM: if (!buf) return NS_ERROR_OUT_OF_MEMORY; >+ rv = inStr->Read(buf, count, &read);

Comment on attachment 125932 [details] [diff] [review] profile/ changes, version 6 >Index: profile/src/nsProfile.cpp > // if shutDownType is not a well know value, skip the notifications should be ... well know_n_ value ... (not your fault, but it wouldn't hurt to fix it)

Comment on attachment 125934 [details] [diff] [review] Default prefs, version 6 >Index: modules/libpref/src/init/all.js >+// display some general warning to the user about making backups, security etc. missing , before etc. :)

Removed myself also. Mozilla roaming is no longer needed.

Duh. Why did nobody tell me that earlier? /me packs his things and goes home. ;-P (Please don't comment when you uncc yourself.)

OK, I am practically through with my to do list and declaring this done. Yay :-) I went through the review items above and fixed them. I'll attach patches against the trunk, where I had to make changes to Mozilla code. The roaming code (the extensions/sroaming/ part) can be fetched as usual from CVS, branch BEONEX_0_8_BRANCH. Please review. I'll create test builds later. Firebird: I made the extension work with Firebird (not with existing releases, though, because some small changes to core Mozilla code are necessary for the code to work). The prefs dialog logic is quite fragile, due to the Communicator pref window which loads/unloads panels with every switch by the user, while tabs (which I used for Firebird) don't do that. Gave me a lot of pain and costed a lot of time. And I just discovered that firebird decides it's OK to exit after the roaming progress window opened and closed during startup, probably confusing my window with the main window. I don't know where the problem is or how to fix it, and I am tired, and it looks like Firebird/XRE bug anyways, so I filed bug 209880. Apart from that, roaming works with Firebird.

Comment on attachment 125933 [details] [diff] [review] xpcom/ changes, version 6 oops! r=darin

Comment on attachment 125927 [details] [diff] [review] Netwerk part, version 6 >Index: netwerk/base/public/nsISocketTransport.idl >+ * nsITransportEventSink status codes. >+ * Although these look like XPCOM error codes and are passed in an >+ * nsresult variable, they are *not* error codes. They *do* overlap with >+ * other, existing error codes in netlib. These status codes are >+ * confined within a very limited context where no error codes may appear, >+ * so there is no ambiguity, it may just be just inconvient. revised text: * nsITransportEventSink status codes. * * Although these look like XPCOM error codes and are passed in an nsresult * variable, they are *not* error codes. Note that while they *do* overlap * with existing error codes in Necko, these status codes are confined * within a very limited context where no error codes may appear, so there * is no ambiguity. > nsBufferedOutputStream::WriteFrom(nsIInputStream *inStr, PRUint32 count, PRUint32 *_retval) > { >+ nsresult rv; >+ char* buf = (char*)PR_Malloc(count); >+ >+ PRUint32 read; >+ rv = inStr->Read(buf, count, &read); ... >+ rv = Write(buf, read, _retval); ... > } hmm... this isn't really done right. this should be implemented without the intermediate buffer copy. heck, this is a buffered output stream. clearly, there is a buffer that can be passed to the input stream's Read method ;-) >Index: netwerk/base/src/nsFileStreams.cpp >+ /* File streams intentionally do not support this method. >+ If you need something like this, then you should wrap >+ the file stream using a buffered stream. */ use //-style comments, and mention nsIBuffered{In,Out}putStream so folks can grep for it if need be.

What are the odds this will make 1.4?

I'm waiting for ccarlen's review atm, but given that RC2 is already out, not good

Comment on attachment 125931 [details] [diff] [review] Changes to top-level build system, version 6 You don't need to check in the change to mozilla/configure - it gets generated automatically when a change to configure.in is checked in. Without that, r=ccarlen

Comment on attachment 125932 [details] [diff] [review] profile/ changes, version 6 >? profile/full.diff >Index: profile/public/MANIFEST_IDL >=================================================================== >RCS file: /cvsroot/mozilla/profile/public/Attic/MANIFEST_IDL,v >retrieving revision 1.2 Dead file - removed from the tree. >Index: profile/public/Makefile.in >=================================================================== >RCS file: /cvsroot/mozilla/profile/public/Makefile.in,v >retrieving revision 1.14 >diff -u -r1.14 Makefile.in >--- profile/public/Makefile.in 2 May 2003 03:59:20 -0000 1.14 >+++ profile/public/Makefile.in 18 Jun 2003 17:01:06 -0000 >@@ -37,6 +37,7 @@ > XPIDLSRCS = \ > nsIProfileInternal.idl \ > nsIProfileStartupListener.idl \ >+ nsISessionRoaming.idl \ > $(NULL) > > ifeq ($(OS_ARCH),WINNT) >Index: profile/public/mozISRoaming.idl >=================================================================== >RCS file: profile/public/mozISRoaming.idl >diff -N profile/public/mozISRoaming.idl >Index: profile/public/nsIProfile.idl >=================================================================== >RCS file: /cvsroot/mozilla/profile/public/nsIProfile.idl,v >retrieving revision 1.30 >diff -u -r1.30 nsIProfile.idl >--- profile/public/nsIProfile.idl 28 Sep 2001 20:10:02 -0000 1.30 >+++ profile/public/nsIProfile.idl 18 Jun 2003 17:01:06 -0000 >@@ -85,5 +85,3 @@ > void deleteProfile(in wstring name, in boolean canDeleteFiles); > void cloneProfile(in wstring profileName); > }; >- >-#endif /* nsIProfile_h__ */ >Index: profile/public/nsISessionRoaming.idl >=================================================================== >RCS file: profile/public/nsISessionRoaming.idl >+ >+%{C++ >+ >+#define NS_SESSIONROAMING_CID \ >+ { /* {ab62465c-494c-446e-b671-930bb98a7bc4} */ \ >+ 0xab62465c, \ >+ 0x494c, \ >+ 0x446e, \ >+ { 0xb6, 0x71, 0x93, 0x0b, 0xb9, 0x8a, 0x7b, 0xc4 } \ >+ } A CID really shouldn't be in an .idl file. >Index: profile/resources/content/profileSelection.xul >=================================================================== >RCS file: /cvsroot/mozilla/profile/resources/content/profileSelection.xul,v >@@ -46,7 +46,7 @@ > style="width: 42em;" > onclose="onExit();" > onload="StartUp();"> >- >+ Let's not check in a new version for his change, please. > <stringbundle id="bundle_profileManager" > src="chrome://communicator/locale/profile/profileManager.properties"/> > <stringbundle id="bundle_brand" >Index: profile/src/nsProfile.cpp >=================================================================== > #include "nsIDocShell.h" >@@ -162,6 +163,7 @@ > static NS_DEFINE_CID(kPrefMigrationCID, NS_PREFMIGRATION_CID); > static NS_DEFINE_CID(kPrefConverterCID, NS_PREFCONVERTER_CID); > static NS_DEFINE_IID(kCookieServiceCID, NS_COOKIESERVICE_CID); >+static NS_DEFINE_CID(kSessionRoamingCID, NS_SESSIONROAMING_CID); Use the contract ID, not the CID. Sorry, bad examples here, but, if we're writing new code... r=ccarlen with CID issues addressed.

Comment on attachment 125934 [details] [diff] [review] Default prefs, version 6 So, which files get roamed is not per-profile, but per-product? Not what I thought we were going to have but, for this stage, OK

> So, which files get roamed is not per-profile, but per-product? Hm? No, that's just the default, what is initially selected when you enable roaming for the first time in the prefs dialog. The user settings are stored per-profile in the Mozilla app registry (see previous discussion). Conrad, could you please also review the real roaming code in extensions/sroaming/ (see above about how to get it - it's over 7000 lines of code)?

Is there a (Win) build somewhere where we could test (oogle over) this fix? :)

Ben, I tried your patch and meet a problem that I can't moveTo the temp file to final file. It will throw an exception FILE_ACCESS_DENIED. It happens on download finished. Did you meet the same problem? Thanks!

Not that I know, no.

benb: most likely you're using a single volume on linux for your testing, whereas pete is probably on solaris and /tmp is a ramdisk and the destination is some other volume. moveTo would fail for a move across volumes (and i believe that's the right rv for such a failure), if you get that failure you need to use copyTo() followed by remove().

I tried copyTo. But it will failed if the file already exists. So, we should first remove the dest file and then copyTo and then remove. Is there any better solution?

arg. Silly me assumed that mv would do what |mv| does. But if Pete Zha is talking about the place I think he is (in the "stream protocol", where I move bookmarks.html.tmp to bookmarks.html), then they are in the same dir, the profile dir.

On windows, if the file is open, you will not be able to rename of delete it. But on unix we can do that. So, I think that's the reason of the exception happens on windows but not on Linux/Unix. We should find another way to do the mv on windows.

> We should find another way Anybody knows such a way? I should not write to the file directly. I did that before, but netwerk unfortunately overwrites the old file with a 0-byte file, if e.g. the authentication failed. outch. (and there are possibly other, rare cases for corruption, like failure of connection etc.) So - I can create a buffer large enough to hold arbitary-sized files in memory and only write the buffer when the transfer completed successfully or - I need to create 2 more channels and another async transfer to read fromt he temp file and write to the real file (ugly) or - we need to find another solution. BTW: Conrad, any progress on the review?

> BTW: Conrad, any progress on the review? Ben, I checked out extensions/sroaming from your branch and it looks good - just a few comments. Are you going to post a -N diff with those files added to the trunk? It's easier to add review comments to a patch.

Exactly what are the steps to get doing using roaming in Mozilla? Checklist for what to apply and what to check out please?

As patch, for ccarlen > looks good - just a few comments Great :-). I was already scared :).

Changing milestone as 1.4a is history.

I found the information of roaming is not in the preference ( I don't mean UI ). So, it is not autoconfigable. My question is: is there any way to manage the roaming information centrally for some enterprise users?

Ben, Do you have any idea about how we can roaming Messenger Filters? Since the account is dynamic created in preference, I can't find a way to implement it. One workarround solution is: We could create an roaming entry which can be customized by user. User could select any file on the local machine and give it a name on server. This way, different instance could setup a customized file with same name on server but with different file name on local. Do you agree this solution? Or you have better idea? Another problem. I see you don't enable user to roaming Preference(prefs.js). Is it because that file include many local path and file name? Since Netscape4 supports to roaming Preference, we'd better find a solution to do it. Thanks for your time! Pete

Where are we on this? Any news? Will this make 1.5?

Conrad, did you have a chance to review my code, so we can check it in?

> Another problem. I see you don't enable user to roaming Preference(prefs.js). > Is it because that file include many local path and file name? Yes, see initial description. > Do you have any idea about how we can roaming Messenger Filters? You could do the same I did for the password / wallet file. These have changing filenames as well, and I added a provision in the UI to dynamically read the name. You could hook up there as well and add the mail filters. Note that there may be an arbitary number of filters, but you should be able to add them to the bottom of the prefs files list UI, and generate a suitable UI name for it. (The rest of the code then won't be to display a nice name for it, but shoudl fall back to the filename.) The other problem is that the file lies in a subdir, and we potentially have n file with the same filename. IIRC, the current code assumes the same filename on server and client (but not sure), and doesn't have a way to create a directory on the server. You'd have to figure something out to work around that, like generating a unique name and copying the file locally before upload some similar, or alternatively fix the backend to support different filenames on server and client, but that could get tricky. > is there any way to manage the roaming information centrally > for some enterprise users? In the discussion above in this bug, you see that there is (or was?) an XML-converter for the Mozilla registry planned. The idea was that a company-wide script exports the registry, modifies it to include the roaming settings (that's also why the username can be set separately from the URL, to make that easier), and imports it into the Mozilla app registry again. This could happen at the Windows profile creation or remotely using Novell tools or whatever. Once the user has the right roaming account set up (should be a one-time action), you can manage his Mozilla settings on the roaming server. Pete Zha meanwhile wrote me that he modified the roaming code to use prefs instead of the registry. I used the registry, because I ran into problems with unsing the prefs and Conrad Carlen repeatedly suggested me (here and per email) to use the registry for that, so I eventually did. Pete Zha now reverted that and says it works. I didn't look at the code or tired it, so I cannot make any statements about it. However, I'd prefer if my code went into CVS unmodified at first. If Pete Zha's solution indeed works well and is better (well possible), it could be checked in separately afterwards, but we then at least have a revision history.

I confirmed the code to work with HTTPS. It breaks on Windows, because the file is still open while moveTo. You need to change // flush if (file.fos) // only for download file.fos.flush(); if (file.bos) // dito file.bos.flush(); to // flush/close - only for download if (file.bos && file.fos) { file.bos.flush(); file.fos.flush(); file.bos.close(); file.fos.close(); } (A simple close(), which also flushes, didn't work for me, IIRC, because of the combination of these output streams.)

changed per darin's comments

Comment on attachment 131104 [details] [diff] [review] netwerk patch, v7 instead of the code in WriteFrom here, I could use inStr->ReadSegments and call ::Write in the reader function. tell me if you prefer that.

Comment on attachment 131104 [details] [diff] [review] netwerk patch, v7 >Index: base/src/nsBufferedStreams.cpp > nsBufferedOutputStream::WriteSegments(nsReadSegmentFun reader, void * closure, PRUint32 count, PRUint32 *_retval) > { >+ *_retval = 0; >+ while (count > 0) { >+ PRUint32 left = PR_MIN(count, mBufferSize - mCursor); >+ if (left == 0) { >+ Flush(); what if flush fails? what if the underlying output stream is actually closed or has an error. you must not ignore those errors. they need to be propogated to the caller. also, it is usually a good idea to implement WriteFrom in terms of WriteSegments. see how nsPipeOutputStream::WriteFrom is implemented, and simply copy it. >Index: protocol/http/src/nsHttpChannel.cpp > NS_IMETHODIMP > nsHttpChannel::Open(nsIInputStream **_retval) > { >+ NS_NOTYETIMPLEMENTED("nsHttpChannel::SetContentLength"); > return NS_ERROR_NOT_IMPLEMENTED; > } wrong text in this error message.

forgot to mention that i like the fact that you are implementing WriteFrom and WriteSegments. those function should be implemented. however, that is not to say that using them is necessarily appropriate here. to answer that question you have to consider the streams involved. for example, if it is the case that ReadSegments is available... always available that is, then there would be no need (in general) to buffer the output stream if all you are doing is copying data from one stream to the other.

ok, updated per darin's comments that said, I'm not really involved in this bug. don't hold me responsible for possibly wrongly using these functions :) because I didn't write code that uses them. and, thanks for the really good idea of implementing writeFrom by using WriteSegments, I didn't think of that.

Comment on attachment 131169 [details] [diff] [review] netwerk patch v8 ew :( I managed to mixed several unrelated patches in here (I want the tree to open :) ) Please ignore changes to nsDnsService.cpp, nsMIMEInfoImpl.cpp and testdoc as well as the frst part of nsHttpChannel.cpp

Comment on attachment 131169 [details] [diff] [review] netwerk patch v8 >Index: base/src/nsBufferedStreams.cpp > NS_IMETHODIMP > nsBufferedOutputStream::WriteSegments(nsReadSegmentFun reader, void * closure, PRUint32 count, PRUint32 *_retval) > { >+ *_retval = 0; >+ while (count > 0) { >+ PRUint32 left = PR_MIN(count, mBufferSize - mCursor); >+ if (left == 0) { >+ nsresult rv = Flush(); ... >+ nsresult rv = reader(this, closure, mBuffer + mCursor, *_retval, left, &read); nit: "nsresult rv" declared twice in this function. how about moving the declaration to the top of the function and share it? ;) r=darin with that nit fixed.

Comment on attachment 131169 [details] [diff] [review] netwerk patch v8 (please see the earlier comment about the unrelated parts)

Comment on attachment 131169 [details] [diff] [review] netwerk patch v8 The comment on nsFileOutputStream::WriteFrom (and the change to the assert) should probably also be added to nsFileOutputStream::WriteSegments sr=bzbarsky, assuming you don't check in the irrelevant parts of this patch... ;)

Comment on attachment 131169 [details] [diff] [review] netwerk patch v8 Thanks for the reviews! The netwerk part is now checked in, without the unrelated parts. (one of these days, I'll make a patch that doesn't include 2 others...)

Is this bug Resolved, or is the plan to use it for the other parts of roaming?

Given that no roaming support is checked in yet (the netwerk part was just support code), the bug is left open for actual roaming support.

so it's got r=,sr=, the tree is open... whassup?

none of the real roaming patches have both review and super-review.

Sorry for the spam, but what are the plans here? The patches have been laying around for quite some time but very little has happened. Don't want to push anyone, just want to know what's going on.

Comment on attachment 127248 [details] [diff] [review] Main part, Version 6 Hi, Conrad I'm not sure whether you have time to review this huge patch, I just get message from Ben that you are interest in this feature(profile and roaming). So, could you please take a look at it and give some comments? It's already be there for several months. Currently I'm also working on this feature and based on Ben's patch to add some new staff such as LDAP support, mail filter and Junk mail data support. I will help to check in it to trunk and then we can go ahead to add more staff. Thanks a lot for your time!

Comment on attachment 125932 [details] [diff] [review] profile/ changes, version 6 Hi, bz Since Conrad has given r= on this patch, can you give sr so that we can check in it separately to make the process go ahead? This patch is not big and hope it will not spend much time of your vacation ;-) Thanks!

We should not enable roaming feature as default unless it's well tested. So, this patch just add a compiling switch that we can enable it when necessary. --enable-extensions=default,sroaming With above switch you can build roaming module

Comment on attachment 134813 [details] [diff] [review] patch for build sroaming extension by a compiling switch Hi, bz Here is abother very small patch that can enable roaming moudle by a compiling switch. It will not in the default build if the --enable-extensions switch doesn't include sroaming. Thanks

Comment on attachment 125932 [details] [diff] [review] profile/ changes, version 6 >Index: profile/public/nsISessionRoaming.idl >+%{C++ >+ >+#define NS_SESSIONROAMING_CID \ >+#define NS_SESSIONROAMING_CONTRACTID \ Please don't put those in the IDL. They'll just need to be moved out when we get close to freezing the interface, with ensuing pain. >Index: profile/resources/content/profileSelection.xul There seem to be no reasons to change this file (whitespace-only change, and not even significant). >Index: profile/src/nsProfile.cpp >+ // Roaming download >+ // Tolerate errors. Maybe the roaming extension isn't installed. >+ nsCOMPtr <nsISessionRoaming> roam = >+ do_CreateInstance(kSessionRoamingCID, &rv); >+ if (NS_SUCCEEDED(rv)) >+ roam->BeginSession(); Should that be a GetService? Does it make sense to have multiple nsISessionRoaming instances all instantiated at once? >+ // Roaming upload >+ // Tolerate errors. Maybe the roaming extension isn't installed. >+ nsCOMPtr <nsISessionRoaming> roam = >+ do_CreateInstance(kSessionRoamingCID, &rv); >+ if (NS_SUCCEEDED(rv)) >+ roam->EndSession(); Same sr=bzbarsky with those issues resolved.

Comment on attachment 134813 [details] [diff] [review] patch for build sroaming extension by a compiling switch I'm not a good reviewer for this, since I'm not quite sure how extensions and our build system hook together...

Comment on attachment 134813 [details] [diff] [review] patch for build sroaming extension by a compiling switch I don't see a reason not to enable this code. If it's not enabled by default, neither mozilla.org testers not other people using default binaries will be able to test and use it. The impact on non-users of roaming is practically 0. There is a very clear warning when you enable roaming for the first time that the code is experimental. The only thing to consider is maybe footprint. But embedding is not really an argument here: it's trivial to disable it, if you deeply care about footprint.

>>+#define NS_SESSIONROAMING_CID \ >>+#define NS_SESSIONROAMING_CONTRACTID \ >Please don't put those in the IDL. They'll just need to be moved out when we >get close to freezing the interface, with ensuing pain. So, where I can define it since nsProfile.cpp need it to get nsISessionRoaming service. I can't define it in roaming extension since profile can't depend on it.

Comment on attachment 125933 [details] [diff] [review] xpcom/ changes, version 6 bz, here is another for you. Thanks!

> So, where I can define it You have two options: 1) Don't define it. Use the contractid string where needed. 2) Define it in a CID file that contains the CIDs and contractIDs for the module (eg see nsNetCID.h)

Comment on attachment 125933 [details] [diff] [review] xpcom/ changes, version 6 sr=bzbarsky

Please, pretty please, make this extension built in to mozilla, and not require a recompile. I know that myself, my wife, a few of my friends and my boss are all awaiting this functionality. 1.6 "alpha" or a nightly release could definitely have this in it right?

Considering the cost of Windows compiler toolchains, unless this is either built in by default, or implemented entirely in an extension with no special compile time requirement, this feature will not be accessible the vast majority of users who need this functionality. As the work has already been laid out, and as we have been waiting since Netscape 4.7 to get our roaming back, please implement this now as part of the default build, and consider moving it into an extension down the road. My 2 cents -Josh

Comment on attachment 134813 [details] [diff] [review] patch for build sroaming extension by a compiling switch ok, forget this patch.

Comment on attachment 134813 [details] [diff] [review] patch for build sroaming extension by a compiling switch Please remove review requests from a patch when you do that....

Comment on attachment 125933 [details] [diff] [review] xpcom/ changes, version 6 this is in

Comment on attachment 125934 [details] [diff] [review] Default prefs, version 6 bz, this is small one to add two default roaming preference. Thanks for your time!

profile changes patch resolves bz's comments.

Comment on attachment 134896 [details] [diff] [review] profile changes bz, I'm going to check in it if you satisfy with this patch. I removed the CID definition to roaming extensions and remove unnecessary file change. Also I use do_GetService instead of createInstance

Comment on attachment 134896 [details] [diff] [review] profile changes sr=bzbarsky. Add a comment in the interface that the implementation should be a service, perhaps?

Comment on attachment 125934 [details] [diff] [review] Default prefs, version 6 Do we want to roam user.js, chrome/userChrome.css, chrome/userContent.css, etc? In any case, sr=bzbarsky for this as a first approximation...

1.6a is already out the door, so it can't be a blocker for that.

Comment on attachment 134896 [details] [diff] [review] profile changes Ben, could you review this patch? I modified something according to bz's comments

Comment on attachment 134896 [details] [diff] [review] profile changes I would personally have used a C++ string constant, to save the 2. string in the binary, but it's just a few bytes, and contract ids are wasteful anyways. r=BenB, assuming you didn't modify anything else.

Comment on attachment 134896 [details] [diff] [review] profile changes this is in with some comments

Comment on attachment 127248 [details] [diff] [review] Main part, Version 6 in this patch, there are references to "chrome://communicator/skin" it should be avoided in order to be integrated in Firebird. there are also references to "chrome://communicator/skin/profile.css" . Maybe profile roaming deserves its own css file. That would be needed if we ever decide to only use single profile. My last comment could be addressed in a followup bug, since I don't want to slow down the progress on this bug.

update milestone to 1.7b

Can somebody explain what's going on? I don't understand the process well enough. What needs to happen now before the code can go in? What are we waiting on, exactly?

Oh, sorry. Actually this is blocked by bug 227267. Upload doesn't work on trunk. I'm trying to fix that bug but don't haven't had clue yet. Without upload, roaming will not work.

This patch shows the diff between attachment 127248 [details] [diff] [review] and attachment 137283 [details] [diff] [review]. This is for Ben to look what I changed for his patch. Following things I have changed: 1) don't remove "listing.xml" after transfer complete. Because it will cause exception on windows for ACCESS_DENIED. I'm not sure why since we closed that file after transfer. Anyway we will remove it before we download it from server. 2) Fixes the moveTo problem on windows by close the stream when it complete. 3) Remove "mHavePrefs" for mozRoaming class since it is a service, we need to read the preference when we BeginSession and EndSession. So we don't need that flag to read pref from cache. 4) Clear mFiles contents when load file information. Same reason as above, we are a service. 5) Define the CID and contract ID in mozRoaming.h Ben, please look at these changes and see whether they ar OK for you. Thanks! -Pete

A ready-to-run build of Beonex Communicator 0.9 Linux (based on Mozilla 1.4.1+) with Roaming (version 6 attached here) can be downloaded at <http://download-redirect.beonex.com/communicator/0.9/beonex-comm-0.9-preview-2-linux-i686.tar.bz2>. Please test widely, you are free to distribute this URL (but please exactly this) as you please. A PGP sig is available, if you add ".sig". My Windows builds crash on startup. (Probably unrelated to roaming.) :-(

Ben, Should we file bugs for this build in this bug or create new ones? Also, any chance of a gtk2 build? I want this feature badly so I'm willing to test the heck out of it, but my eyes can only last so long with bad fonts. Thanks. ;)

> Should we file bugs for this build in this bug or create new ones? At this point, I'd prefer new bugs, so that non-fatal bugs or tester confusion don't hold off the checkin. I created tracking bug 228629 for that. Thanks. > any chance of a gtk2 build? Not from me.

So can this be committed now that bug 227267 and bug 228784 are taken care of? What has to happen now?

Comment on attachment 137284 [details] [diff] [review] diff between patch 127248 and 137283 I haven't actually tested the patch, but it looks like it's OK. It sounds wrong to me to unconditionally read the prefs in these functions, we may excessively read the prefs, if the functions are called successively, but the switch to a service may actually have introduced a bug here, yes. Maybe we should have stayed away from a service. For now, I guess it's OK, though. Thanks, r=BenB

Comment on attachment 137284 [details] [diff] [review] diff between patch 127248 and 137283 I haven't actually tested the patch, but it looks like it's OK. It sounds wrong to me to unconditionally read the prefs in these functions, we may excessively read the prefs, if the functions are called successively, but the switch to a service may actually have introduced a bug here, yes. Maybe we should have stayed away from a service. For now, I guess it's OK, though. Thanks, r=BenB

>It sounds wrong to me to unconditionally read the prefs in these functions, we >may excessively read the prefs, if the functions are called successively, but >the switch to a service may actually have introduced a bug here, yes. Maybe we >should have stayed away from a service. For now, I guess it's OK, though. Currently, It's not a problem since the ReadRoamingPrefs() only be called once when BeginSession and EndSession. In the furture, I think the IsRoaming() function should only do what it means. Just read value for roaming enabled. So that we won't excessively read the prefs.

Comment on attachment 137283 [details] [diff] [review] modified patch for sroaming extension >+ filename="mailserver/rules.dat and mailserver/msgFilterRules.dat >+ - crap" The mail filter file name depends on mail account setting. So, we only can roam filter for particular account. For example, filter for abc@mail.com is only can be roam to another profile for abc@mail.com account. Otherwise the filter will not work. I will address this in another bug. And I think we can support Junk mail data file which is training.dat in profile directory and it's not depend on specific mail account. >+ <listitem type="checkbox" >+ id="emailcerts" >+ filename="cert7.db" >+ label="" /> >+ <listitem type="checkbox" >+ id="cryptokeys" >+ filename="key3.db" >+ label="" /> >+ <listitem type="checkbox" >+ id="secmod" >+ filename="secmod.db" >+ label="" /> >+ </listbox> I'm not sure it's safe to roam these kinds of files though we leave them with false as default. >+ if (download >+ ? profileTime > remoteTime >+ : profileTime < remoteTime ) >+ { >+ //printf("conflict found!\n"); >+ conflicts.AppendCString(file); >+ } >+ else >+ copyfiles.AppendCString(file); This logic which is for file copy method has problem. Suppose user already has profiles on a location(directory on server or local). Then he setup the roaming on another Profile of Mozilla which points to the same location. Roaming module will upload profile to the location everytime it close. Because the profile will be changed everytime before we do EndSession. So the first, profile on this location will be replaced by the new profile which is just created by user and the user don't have chance to get the conflict windows to make choice. Other than above looks OK for me. I will fix above problem and looking for sr for this patch. BTW: This patch is made by Ben. I tested his patch and it works well for me. Although there are many things need to be enhanced. Like autoconfig support, LDAP support, Mail filter and Junk mail data support etc, it's the first patch which can make roaming work. Since module owner is busy and can't review, I can give review of this patch based on my understanding of roaming feature.

Comment on attachment 137283 [details] [diff] [review] modified patch for sroaming extension Ben, I'm gonna be mostly unavailable for the next 4-6 weeks, and this is a really big patch, so I don't think I'll be able to get to it in that time. You're welcome to either look for another sr, or wait, and I'll do it once I finish my current project.

Comment on attachment 137283 [details] [diff] [review] modified patch for sroaming extension Ben, I'm gonna be mostly unavailable for the next 4-6 weeks, and this is a really big patch, so I don't think I'll be able to get to it in that time. You're welcome to either look for another sr, or wait, and I'll do it once I finish my current project.

Comment on attachment 137283 [details] [diff] [review] modified patch for sroaming extension the firebird changes in this patch need to be broken out into a separate patch to be reviewed by one of the firebird peers. it will take me some time to completely review this patch. i also suggest that you separate the C++ changes from the XUL/JS changes. different folks may need to review the UI changes while others should review the backend C++ changes.

darin, note that (by your suggestion BTW :-) ) most actual networking code is implemented in JS. The UI code is in separate files, though.

ok, that makes sense. but, i think it still makes sense for the UI changes to be separated from the core changes, even though those core changes include a mix of JS and C++. sound reasonable? i just think that it might make the patch management easier since other folks (not me) need to review the UI changes.

darin, following file is about the core of roaming. Network implementation is in the transfer.js. Do you need a separate patch for these file though you can find them in patch very easy. Index: Makefile.in Index: resources/content/transfer/transfer.js Index: src/Makefile.in Index: src/mozSRoaming.cpp Index: src/mozSRoaming.h Index: src/mozSRoamingCopy.cpp Index: src/mozSRoamingCopy.h Index: src/mozSRoamingModule.cpp Index: src/mozSRoamingProtocol.h Index: src/mozSRoamingStream.cpp Index: src/mozSRoamingStream.h

More or less: Backend: Index: Makefile.in Index: README.txt Index: resources/content/transfer/conflictCheck.js Index: resources/content/transfer/transfer.js Index: resources/content/transfer/utility.js Index: src/Makefile.in Index: src/mozSRoaming.cpp Index: src/mozSRoaming.h Index: src/mozSRoamingCopy.cpp Index: src/mozSRoamingCopy.h Index: src/mozSRoamingModule.cpp Index: src/mozSRoamingProtocol.h Index: src/mozSRoamingStream.cpp Index: src/mozSRoamingStream.h Frontend: Index: jar.mn Index: resources/content/contents.rdf Index: resources/content/prefs/MANIFEST Index: resources/content/prefs/files.js Index: resources/content/prefs/files.xul Index: resources/content/prefs/firebird.js Index: resources/content/prefs/firebird.xul Index: resources/content/prefs/prefsOverlay.xul Index: resources/content/prefs/top.js Index: resources/content/prefs/top.xul Index: resources/content/transfer/MANIFEST Index: resources/content/transfer/conflictResolve.js Index: resources/content/transfer/conflictResolve.xul Index: resources/content/transfer/progressDialog.js Index: resources/content/transfer/progressDialog.xul Index: resources/locale/en-US/conflictResolve.dtd Index: resources/locale/en-US/contents.rdf Index: resources/locale/en-US/prefs.dtd Index: resources/locale/en-US/progressDialog.dtd Index: resources/locale/en-US/transfer.properties Index: resources/skin/classic/contents.rdf Index: resources/skin/classic/progressDialog.css Index: resources/skin/modern/contents.rdf Index: resources/skin/modern/progressDialog.css But e.g. utility.js is used by backend and frontend.

Ben Bucksch wrote: > More or less: > Backend: > Index: Makefile.in > Index: README.txt > Index: resources/content/transfer/conflictCheck.js > Index: resources/content/transfer/transfer.js > Index: resources/content/transfer/utility.js > Index: src/Makefile.in > Index: src/mozSRoaming.cpp > Index: src/mozSRoaming.h > Index: src/mozSRoamingCopy.cpp > Index: src/mozSRoamingCopy.h > Index: src/mozSRoamingModule.cpp > Index: src/mozSRoamingProtocol.h > Index: src/mozSRoamingStream.cpp > Index: src/mozSRoamingStream.h So many src/mozSRoaming* files scream for a src/mozSRoaming/ subdir... :)

eh... no. This *is* the dir already. It's just that, IIRC, all Mozilla C++ files (or C++ classes?) need to have a unique filename, thus the pseudo-"namespace". (Remind me again why we have the silly "ns"/"moz" prefix?)

> It's just that, IIRC, all Mozilla C++ files > (or C++ classes?) need to have a unique filename I believe that that was a codewarrior limitation and is thus not anymore relevant (as we dropped mac cfm)

yes, there's no such need to have them globally unique - nor do I think a seperate subdir is really needed. Besides, subdirs should be simple, lower-case words. In any case, I wish we could just call everything "ns" and write it off to an imperfect past. "ns" is never going away from the mozilla core codebase in any case, and I think it just makes moz* classes look shoddy.

> var prefService = Components.classes["@mozilla.org/preferences-service;1"] > .getService(Components.interfaces.nsIPrefService); > var prefs = prefService.getBranch(null); this code can be simplified to: var prefs = Components.classes["@mozilla.org/preferences-service;1"] .getService(Components.interfaces.nsIPrefBranch); > const kConflDlg = how about "kConflictDlg" instead to make it more readable? > rv = NS_NewNativeLocalFile(NS_ConvertUCS2toUTF8(remoteDirPref), PR_FALSE, this is broken. UTF8 is not necessarily the native "narrow" character encoding. use this instead: rv = NS_NewLocalFile(remoteDirPref, PR_FALSE, ... more review comments later...

hi, darin Thanks for your comments. I will fix it them after you give a full review. Do you have idea about when you can make it? I may need some time to make new patch and I'm planning it for 1.7. Thanks again!

hey pete, hopefully, i'll finish the review this week (or early next week).

Comment on attachment 137283 [details] [diff] [review] modified patch for sroaming extension minusing based on attached review comments.

Comment on attachment 141373 [details] review, p1: review comments for C++ portion of attachment 137283 [details] [diff] [review] > i would recommend using >the standard XPCOM getter style another possibility would be to return an already_AddRefed<nsIFile> >+nsresult CopyFile(nsCOMPtr<nsIFile> fromDir, >+ nsCOMPtr<nsIFile> toDir, the arguments should probably be nsIFile* (in addition to darin's comment)

woo, darin's review comment's are long. I just stumbled across EnterLastWindowClosingSurvivalArea(); ExitLastWindowClosingSurvivalArea(); That might be what's needed for this to work with Firefox (comment 95 / bug 209880).

> EnterLastWindowClosingSurvivalArea(); > ExitLastWindowClosingSurvivalArea(); > That might be what's needed for this to work with Firefox Wrong, see bug 209880. Attaching my reply to darin's partial review. Hopefully, the JS review won't be as, um, extensive. Pete Zha promised to fix any review backslash (thanks!).

> Attaching my reply to darin's partial review. Hopefully, the JS review won't be > as, um, extensive. Pete Zha promised to fix any review backslash (thanks!). I'm doing this. Will work out the patch soon this week. Sorry for delay since I have other works to do...

Comment on attachment 143917 [details] [diff] [review] patch for darin's comments Darin, Please review this patch and please refer Ben's reply for your comments. I changed items what Ben agrees. BTW: I met a compiling problem when I call target.SizeTo d:/work/cvsroot/mozilla/seamonkey/mozilla/extensions/sroaming/src/mozSRoamingSt ream.cpp(67) : error C2248: 'SizeTo' : cannot access public member declared in class 'nsVoidArray' So I didn't change it since it will not be many item to append. >+protected: >+}; >+ >+#endif Oops, I suppose to remove this "protected". Don't blam me on this, I will remove it in final checkin version.

Comment on attachment 143917 [details] [diff] [review] patch for darin's comments >Index: Makefile.in >+# The Original Code is the Platform for Privacy Preferences. >+# >+# The Initial Developer of the Original Code is International >+# Business Machines Corporation. Portions created by IBM >+# Corporation are Copyright (C) 2000 International Business >+# Machines Corporation. All Rights Reserved. >+# >+# Contributor(s): IBM Corporation. >+# Harish Dhurvasula <harishd@netscape.com> the license seems wrong to me. this looks like copy-paste from p3p. it needs to be fixed. >Index: README.txt nit: How about some line breaks in this file? >Index: resources/content/prefs/files.js >+ if (parent.firebird) i'd strongly encourage changing all "firebird" references to "firefox" don't worry, the name isn't changing again ;-) >+ var prefService = Components.classes["@mozilla.org/preferences-service;1"] >+ .getService(Components.interfaces.nsIPrefService); >+ var prefs = prefService.getBranch(null); see my comments down below about simplifying this. >Index: resources/content/prefs/firebird.js >Index: resources/content/prefs/firebird.xul s/firebird/firefox/ get one of the firefox peers to review these files. >Index: resources/content/prefs/top.js >+var _elementIDs = []; // no prefs (nsIPref) needed, see above should this comment mention nsIPrefBranch instead of nsIPref? same question applies to other instances of nsIPref. >+ var prefService = Components.classes["@mozilla.org/preferences;1"] >+ .getService(Components.interfaces.nsIPrefService); >+ var prefBranch = prefService.getBranch("roaming."); >+ try >+ { >+ showWarning = prefBranch.getBoolPref("showInitialWarning"); >+ if (!showWarning) >+ return; >+ } catch(e) {} you could also write: var prefBranch = Components.classes["@mozilla.org/preferences;1"] .getService(Components.interfaces.nsIPrefBranch); try { showWarning = prefBranch.getBoolPref("roaming.showInitialWarning"); ... this is a shortcut that avoids the call to getBranch. >Index: resources/content/transfer/conflictCheck.js >+ 2. Compare compare listing uploaded with listing.xml from server. nit: "compare" is repeated. >+ transfer.transfer(); yes! ;-) >+function makeNSIFileFromRelFilename(filename) >+{ >+ return GetIOService().newURI(gLocalDir + filename, null, null) there might be some i18n concerns here. gLocalDir (assuming it is a file:// URL string) is encoded as a %-escaped string. the underlying charset is the platform charset. however, here you are appending UCS-2 characters. if |filename| is ever non-ASCII, then you have a problem. you can avoid this problem by using nsIFile::append(). i.e., convert gLocalDir to a nsIFile, and then call the append method on it. >Index: resources/content/transfer/transfer.js >+ getSaveLogin : function() >+ { >+ if (this.saveLogin) >+ return this.savePassword ? 1 : 2; >+ else >+ return 0; >+ }, nit: no need for "else" after a "return" statement. this nit repeats. >+ onStatus : function(aRequest, aContext, aStatusCode, aStatusArg) >+ { >+ /* WORKAROUND >+ The status codes that are passed to us here in aStatusCode look like >+ nsresult error codes, but their numerical values overlap with other, >+ real errors. Darin said that real errors are never passed in here, >+ so we don't have a hard conflict. To make processing easier, I just >+ translate these status codes into made-up, unique error codes, >+ so we can later check them together with other status and error codes >+ and store them in the normal this.status field (which contains >+ normal XPCOM error codes) etc.. */ >+ if (aStatusCode == kStatusResolvingHost_Status) >+ aStatusCode = kStatusResolvingHost; ... >+ this.privateStatusChange(aStatusCode); i do not understand why you would ever try to interpret status codes from nsIProgressEventSink::onStatus and nsIRequest::onStopRequest in the same way. it makes no sense. the progress status codes only indicate state transitions of data transfer. status codes passed to nsIRequest::onStopRequest are true result codes, indicating whether or not the request succeeded or failed, and if it failed, the result code tells why. mixing progress status codes with nsIRequest status codes would likely lead to confusion. >+ /* Use this function to interpret Mozilla status/error codes and >+ update |this| appropriately. ... >+ privateStatusChange : function(aStatusCode) ... >+ this.file.setStatus(status, aStatusCode); this is what makes me nervous, but if it works for you... ok :-/ >Index: resources/content/transfer/utility.js >+function ErrorMessageForException(e) >+{ >+ if (!e) >+ return "null"; >+ else if (e.result) >+ return ErrorMessageForStatusCode(e.result); >+ else if (e.prop) >+ return GetString(e.prop); >+ else >+ return e.toString(); >+} same nit about unnecessary "else" after "return" >+function NameForStatusCode(aStatusCode) hmm... necko should use nsIErrorService :-/ so, nothing major. mostly nits. fix it up, and sr=darin on the JS parts. note: i did only a very brief review of the UI portions.

so sr=me on the entire patch when the last of my review comments are addressed. overall, i think it is looking pretty good. i'm anxious to try this out :)

Some replies to darin: >should this comment mention nsIPrefBranch instead of nsIPref? I just mean the preferences in general (prefs.js et al), so "nsIPref*", if you want. >>+ return GetIOService().newURI(gLocalDir + filename, null, null) >there might be some i18n concerns here. Thanks, need to check this. >>+ getSaveLogin : function() >>+ { >>+ if (this.saveLogin) >>+ return this.savePassword ? 1 : 2; >>+ else >>+ return 0; >>+ }, >nit: no need for "else" after a "return" statement. this nit >repeats. >[elsewhere:] it just helps unclutter code if you remove unnecessary text. I am doing stuff like this for code clarity, so that the reader can see clearly that "return 0" applies when there's no saveLogin, without having to spot the |return| under the |if|. You probably have a C++ compiler so hardwired into your brain near your eyes that it's obvious to you ;-P. > i do not understand why you would ever try to interpret status codes > from nsIProgressEventSink::onStatus and nsIRequest::onStopRequest > in the same way. When I wrote the code, I assumed that the |nsresult| that |nsIProgressEventSink::onStatus| returns is an XPCOM error/success code (nsresult can also transport success states, not just errors). Nothing in the interface suggested otherwise. I only found out about it when I noticed that the values overlap with other error codes, submitted a patch to you and you told me about my wrong assumption. By that time, I already treated them uniformly throughout the code, it was so built into the whole structure that it was hard and error-prone to change and I thought it's safest and easiest to just translate the progress sink status codes into unique XPCOM error/success codes. It still makes somewhat sense to me - "it's in progress, resolving the host" and "the host resolution failed" and "could not write to the file" are all states of the file transfer. IIRC, I actually show it in the UI exactly like that, or it was the intent to do that at a later time. > mixing progress status codes with nsIRequest status codes would likely > lead to confusion. I guess we should document that even more clearly, in the File object definition? >hmm... necko should use nsIErrorService :-/ I don't know nsIErrorService, but having to re-define all the error code constants in JS was major pain and very time-consuming, as you can imagine. Having the source module additionally also supply user-presentable strings for errors would be terrific, yes. (Necko is not alone here, same for nsIFile, IIRC.) pete's patch: > nsresult mozSRoaming::GetProfileDir(nsCOMPtr<nsIFile>& result) > { > return NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, > getter_AddRefs(result)); > } Is that correct? isn't that a double-addref, i.e. leak? > - nsresult rv = NS_OK; Is do_GetService *garanteed* to *always* set rv? I didn't want to find out the hard way that it isn't. > - else // 0=unknown, e.g. prefs not yet read, or invalid > - return 0; Please leave this in, esp. because of the comment. > nsCOMPtr<nsILocalFile> lf; // getting around dumb getter I think this is superfluous then, too, we can just set mRemoteDir directly. >if (!profileExists) { Comparing with the statements below, it looks like this is the wrong way around: !remoteExists here and !profileExists for continue; > mozSRoamingCopy() {}; > ~mozSRoamingCopy() {}; This can the be removed, too, esp. the destructor. I actually don't think the ; would compile. Same with *Stream. >nsCAutoString("$USERID") nit: That should be NS_LITERAL_CSTRING (or similar). >- rv = ioserv->NewFileURI(profiledir, getter_AddRefs(mProfileDir)); >+ rv = NS_NewFileURI(getter_AddRefs(mProfileDir), profiledir); This looks odd to be turned around. Either the API is strange or the code is wrong. Because I assume it compiles, I guess the latter. The rest looks mostly good, thanks Pete!

>nit: no need for "else" after a "return" statement. this nit >repeats. I don't fix this because Ben's comments. Anyway, it will not bother the compiler since they are in JS code. > nsresult mozSRoaming::GetProfileDir(nsCOMPtr<nsIFile>& result) > { > return NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, > getter_AddRefs(result)); > } Is that correct? isn't that a double-addref, i.e. leak? Pete:I remove the getter_AddRefs to callers and chnge to better style: nsIFile**. So it's more clear. > - nsresult rv = NS_OK; Is do_GetService *garanteed* to *always* set rv? I didn't want to find out the hard way that it isn't. Pete:I think so it suppose to. > nsCOMPtr<nsILocalFile> lf; // getting around dumb getter I think this is superfluous then, too, we can just set mRemoteDir directly. Pete: can't because mRemoteDir is nsIFile. Still to convert.

Comment on attachment 144122 [details] [diff] [review] patch for darin and Ben's comments Thanks, darin!

Please always get new license boilerplate from: http://www.mozilla.org/MPL/boilerplate-1.1/ Gerv

(In reply to comment #204) > Please always get new license boilerplate from: > http://www.mozilla.org/MPL/boilerplate-1.1/ Thanks for remaind. I did get the license from above line for the latest patch

(In reply to comment #201) > ... By that time, I > already treated them uniformly throughout the code, it was so built into > the whole structure that it was hard and error-prone to change and I > thought it's safest and easiest to just translate the progress sink > status codes into unique XPCOM error/success codes. OK > > nsresult mozSRoaming::GetProfileDir(nsCOMPtr<nsIFile>& result) > > { > > return NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, > > getter_AddRefs(result)); > > } > > Is that correct? isn't that a double-addref, i.e. leak? it is fine. there's no leak there. > > - nsresult rv = NS_OK; > > Is do_GetService *garanteed* to *always* set rv? I didn't want to find > out the hard way that it isn't. Yes, it is. > >- rv = ioserv->NewFileURI(profiledir, getter_AddRefs(mProfileDir)); > >+ rv = NS_NewFileURI(getter_AddRefs(mProfileDir), profiledir); > > This looks odd to be turned around. Either the API is strange or the > code is wrong. Because I assume it compiles, I guess the latter. the methods in nsNetUtil.h have optional parameters. as such, they list their out-params first (contrary to the way XPCOM methods are usually written). hence, it looks wrong, but it allows us to have optional parameters :-/