Closed Bug 124029 (4xRoaming) Opened 22 years ago Closed 20 years ago

Roaming - 4.x-HTTP-compatible

Categories

(Core Graveyard :: Profile: Roaming, enhancement, P2)

enhancement

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.8alpha1

People

(Reporter: BenB, Assigned: BenB)

References

Details

(Whiteboard: Regressions in bug 228629)

Attachments

(6 files, 24 obsolete files)

19.94 KB, text/plain
Details
21.33 KB, text/html
Details
11.79 KB, text/html
Details
15.42 KB, text/plain
Details
261.42 KB, patch
Details | Diff | Splinter Review
1.88 KB, patch
Details | Diff | Splinter Review
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
Assignee: asa → ben.bucksch
Severity: normal → enhancement
Depends on: 124026
OS: Linux → All
QA Contact: doronr → ben.bucksch
Hardware: PC → All
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. 
Target Milestone: --- → mozilla1.1
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.
Status: NEW → ASSIGNED
s/cconrad/Conrad Carlen/

Conrad, please see above, if that's OK with you.
Component: Browser-General → Profile Manager BackEnd
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.
Priority: -- → P1
Attached file Current state. Do not touch. (obsolete) —
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.
Attached file Current state. Do not touch. (obsolete) —
Forgot some parts.
Same disclaimers.
Attachment #88601 - Attachment is obsolete: true
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).
Alias: 4xRoaming
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?
Attached patch Current state (obsolete) — Splinter Review
Current state of work. For status, see updates above. Work in progress.
Attachment #88606 - Attachment is obsolete: true
Attached patch Netwerk part, version 1 (obsolete) — Splinter Review
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.
Target Milestone: mozilla1.1alpha → mozilla1.3beta
Depends on: 186016
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).
Attachment #96277 - Attachment is obsolete: true
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 :)
Keywords: 4xp
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!).
Blocks: 136492
Target Milestone: mozilla1.3beta → mozilla1.4alpha
Blocks: majorbugs
> 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.
No longer depends on: 186016
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.
Attached patch Netwerk part, version 5 (obsolete) — Splinter Review
Attachment #96278 - Attachment is obsolete: true
Attached patch profile/ changes, version 5 (obsolete) — Splinter Review
Attachment #110411 - Attachment is obsolete: true
Attachment #115494 - Flags: review?(darin)
Attachment #115496 - Flags: review?(ccarlen)
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"
Attachment #115494 - Flags: review?(darin) → review-
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.
Depends on: 163129
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?
Attached patch Netwerk part, version 6 (obsolete) — Splinter Review
Attachment #115494 - Attachment is obsolete: true
Attachment #115497 - Attachment is obsolete: true
Attached patch profile/ changes, version 6 (obsolete) — Splinter Review
Attachment #115496 - Attachment is obsolete: true
Attached patch xpcom/ changes, version 6 (obsolete) — Splinter Review
Attached patch Default prefs, version 6 (obsolete) — Splinter Review
Attachment #125931 - Flags: review?(ccarlen)
Attachment #125932 - Flags: review?(ccarlen)
Attachment #125927 - Flags: review?(darin)
Attachment #125934 - Flags: review?(ccarlen)
Attachment #125933 - Flags: review?(darin)
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
Attachment #125933 - Flags: review?(darin) → review+
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.
Attachment #125927 - Flags: review?(darin) → review-
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
Attachment #125931 - Flags: review?(ccarlen) → review+
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.
Attachment #125932 - Flags: review?(ccarlen) → review+
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
Attachment #125934 - Flags: review?(ccarlen) → review+
> 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)?
Blocks: 210870
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?
Attached patch Main part, Version 6 (obsolete) — Splinter Review
As patch, for ccarlen

> looks good - just a few comments

Great :-). I was already scared :).
Changing milestone as 1.4a is history.
Target Milestone: mozilla1.4alpha → ---
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.)
Attached patch netwerk patch, v7 (obsolete) — Splinter Review
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.
Attachment #131104 - Flags: review?(darin)
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.
Attachment #131104 - Flags: review?(darin) → review-
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.
Attached patch netwerk patch v8 (obsolete) — Splinter Review
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
Attachment #131169 - Flags: review?(darin)
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.
Attachment #131169 - Flags: review?(darin) → review+
Comment on attachment 131169 [details] [diff] [review]
netwerk patch v8

(please see the earlier comment about the unrelated parts)
Attachment #131169 - Flags: superreview?(bz-vacation)
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...
;)
Attachment #131169 - Flags: superreview?(bz-vacation) → superreview+
Attachment #115496 - Flags: review?(ccarlen)
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...)
Attachment #131169 - Attachment is obsolete: true
Flags: blocking1.6a?
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!
Attachment #127248 - Flags: review?(ccarlen)
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!
Attachment #125932 - Flags: superreview?(bz-vacation)
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
Attachment #134813 - Flags: superreview?(bz-vacation)
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.
Attachment #125932 - Flags: superreview?(bz-vacation) → superreview+
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!
Attachment #125933 - Flags: superreview?(bz-vacation)
> 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
Attachment #125933 - Flags: superreview?(bz-vacation) → superreview+
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.
Attachment #134813 - Attachment is obsolete: true
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....
Attachment #134813 - Flags: superreview?(bz-vacation)
Comment on attachment 125933 [details] [diff] [review]
xpcom/ changes, version 6

this is in
Attachment #125933 - Attachment is obsolete: true
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!
Attachment #125934 - Flags: superreview?(bz-vacation)
Attached patch profile changes (obsolete) — Splinter Review
profile changes patch resolves bz's comments.
Attachment #125932 - Attachment is obsolete: true
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
Attachment #134896 - Flags: superreview?(bz-vacation)
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?
Attachment #134896 - Flags: superreview?(bz-vacation) → superreview+
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...
Attachment #125934 - Flags: superreview?(bz-vacation) → superreview+
Flags: blocking1.6b?
Flags: blocking1.6a?
1.6a is already out the door, so it can't be a blocker for that.
Flags: blocking1.6a?
Comment on attachment 134896 [details] [diff] [review]
profile changes

Ben, could you review this patch? I modified something according to bz's
comments
Attachment #134896 - Flags: review?(ben.bucksch)
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.
Attachment #134896 - Flags: review?(ben.bucksch) → review+
Flags: blocking1.6b? → blocking1.6b-
Comment on attachment 134896 [details] [diff] [review]
profile changes

this is in with some comments
Attachment #134896 - Attachment is obsolete: true
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
Target Milestone: --- → mozilla1.7beta
Attachment #125934 - Attachment is obsolete: true
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? 
Whiteboard: Waiting for review
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.
Depends on: 227267
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.
Depends on: 228784
Priority: P1 → P3
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
Attachment #137284 - Flags: review+
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.
Attachment #137283 - Flags: superreview?(dmose)
Attachment #137283 - Flags: review+
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.
Attachment #137283 - Flags: superreview?(dmose) → superreview?(darin)
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.
Attachment #137283 - Flags: superreview?(darin) → superreview-
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!).
Attachment #141373 - Attachment is obsolete: true
> 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...
Attached patch patch for darin's comments (obsolete) — Splinter Review
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.
Attachment #143917 - Flags: superreview?(darin)
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.
Attachment #143917 - Flags: superreview?(darin) → superreview-
Attachment #144107 - Attachment mime type: text/plain → text/html
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.
Attachment #143917 - Attachment is obsolete: true
Comment on attachment 144122 [details] [diff] [review]
patch for darin and Ben's comments

Thanks, darin!
Attachment #144122 - Flags: superreview?(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 :-/
Comment on attachment 144122 [details] [diff] [review]
patch for darin and Ben's comments

sr=darin (i thought i already marked the patch)
Attachment #144122 - Flags: superreview?(darin) → superreview+
Finally, we're getting close to a version usable for corporate roll-outs!  But
now I have two questions:

1. Will this be included in 1.7-final?  Will it be in any version of Firefox? I
don't know how to interpret the Target Milestones and Flags on this bug.

2. Is there a usable version of the Client Customization Kit?  All I can find is
this link to the Netscape 7.0-specific version,
http://channels.netscape.com/ns/browsers/cck.jsp
(In reply to comment #208)
> Finally, we're getting close to a version usable for corporate roll-outs!  But
> now I have two questions:
> 
> 1. Will this be included in 1.7-final?  Will it be in any version of Firefox? I
> don't know how to interpret the Target Milestones and Flags on this bug.
> 
It's impossible to be in 1.7 final since 1.7 has already been feature freezed.
> 2. Is there a usable version of the Client Customization Kit?  All I can find is
> this link to the Netscape 7.0-specific version,
> http://channels.netscape.com/ns/browsers/cck.jsp
I'm not sure it can work with Client Customization Kit since our setting is in
the registry instead of preference. We will think about to move the setting to
preference.
There is no CCK for Mozilla but you can use this here : 
http://www.alain.knaff.lu/howto/MozillaCustomization/

(which will not work if Roaming use the registry)
surely roaming uses appreg.dat/registry.dat (or what it's called), not the
windows registry?
Yes.
jag, could you please sr the UI parts of the latest patch (2004-03-17 06:50)?
See comment 182 for a list of relevant files.
(Not marking request on patch, because it would undo darin's +.)

Pete, I just see in comment 184 ff that it would be better to use shorter
filesnames for the C++ files, namely:
Core.cpp/h (instead of mozSRoaming.cpp/)
Copy.cpp/h
Module.cpp
Protocol.h
Stream.cpp/h
Would you do like to do that still at the next chance, or should we leave things
as-is? I don't care much, but we can't really change it after checkin due to CVS.

I also found a few more bugs when looking again over your changes. The
profileExists stuff is *still* not right, I think, it should be different on
upload or download. That's entirely my fault, I didn't realize that, and I will
fix that myself before checkin (I haven't even determined yet how the code
*should* be), so no need for you to worry about it. I also saw a few other small
bugs:
- prefBranch.setBoolPref("showInitialWarning", false);
  must now be
  prefBranch.setBoolPref("roaming.showInitialWarning", false);
  because you changed the namespace to root.
- mIsRoaming(false) should be mIsRoaming(PR_FALSE), I think
  (IIRC one OS/2 compiler was broken about that)
- mHavePrefs is never used anymore, you can remove that (not important)
- Any reason why you removed the "remove(kListingTransferFilename);" statement?
> Pete, I just see in comment 184 ff that it would be better to use shorter
> filesnames for the C++ files, namely:
> Core.cpp/h (instead of mozSRoaming.cpp/)
> Copy.cpp/h
> Module.cpp
> Protocol.h
> Stream.cpp/h
> Would you do like to do that still at the next chance, or should we leave things
I suggest to use "ns" as prefix. And why you remove "SRoaming" to Core? I think
"SRoaming" is better to understand.
> as-is? I don't care much, but we can't really change it after checkin due to CVS.
Actually, we can change it after check in, only bad thing is it will clear the
cvs log.
> 
> I also found a few more bugs when looking again over your changes. The
Do you want me to make a new patch to fix these problem?
>- Any reason why you removed the "remove(kListingTransferFilename);" statement?
I don't understand... This statement is in the latest patch. What do you mean I
remove it?
Per darin's request, I looked up the IRC discussion where I and darin (and
brade and cmanske) talked about the threading issues. Attaching the part which
darin attended. Some excerpts:
Jul 25 23:59:08 <darin> then all you need is a modal progress dialog, right?
Jul 25 23:59:25 <darin> and after launching that dialog you can issue all the
normal async downloads
Jul 25 23:59:58 <darin> right... since you _are_ wanting a dialog, you get the
event queue stuff for free
Jul 26 00:08:53 <BenB> darin: it's just good to knwo that I have to implement
the download in the JS of the dialog!
Jul 26 00:09:16 <darin> right, or you could have a C++ component that the JS
invokes to do the work
Jul 26 00:11:36 <darin> put the guts of your code in some module that has
nothing to do with UI, but invoke it after loading the progress dialog
> I suggest to use "ns" as prefix.

It's pointless by now, if we don't need unique filenames anymore. Note that some
other new modules, esp. in extensions/, don't use any prefixes anymore either or
a custom prefix.

> And why you remove "SRoaming" to Core?
> I think "SRoaming" is better to understand.

Because the module (and dir) is already called sroaming, so it's redundant, and
"Core" makes the purposes clearer IMHO, but SRoaming would be OK with me as well.

> Do you want me to make a new patch to fix these problem?

No, you can wait with that until you have to make a new one anyways, saves work
for you.

> > - Any reason why you removed the "remove(kListingTransferFilename);"
> >   statement?
> 
> I don't understand... This statement is in the latest patch. What do you
> mean I remove it?

Oh, right. But it used to be *also* under "Step 7", see patch version 6.
(In reply to comment #214)
> Actually, we can change it after check in, only bad thing is it will clear the
> cvs log.

You could ask someone to copy the file on the cvs server, so the log would not
be lost.
Comment on attachment 144122 [details] [diff] [review]
patch for darin and Ben's comments

+  if (parent.firefox)

That should probably be:

  if ("firefox" in parent)

possible followed by a null check.

Perhaps you should do s/firebird/firefox/g

+	 <listitem type="checkbox"
+	   id="bookmarks"
+	   filename="bookmarks.html"
+	   label="" />

To keep in style with the rest of the file, line the rest of the attributes up
with the first one.

+    var bundle = Components.classes["@mozilla.org/intl/stringbundle;1"]
+		  .getService()
+		  .QueryInterface(Components.interfaces.nsIStringBundleService)
+		  .createBundle("chrome://sroaming/locale/prefs.properties");

Instead of doing that it might be cleaner to use a <xul:stringbundle/>

Oh, and for future reference you can merge the getInterface and QI into one
statement; getInterface with one arg will QI to that arg.

Would you count conflictCheck.js as part of the "UI"? If so, I'll still have to
review that file. All the UI files above it look fine to me.
> Would you count conflictCheck.js as part of the "UI"?

No, that's logic. See comment 182.

> All the UI files above it look fine to me.

Thanks for the superreview!
Attached patch patch for jag and Ben's comments (obsolete) — Splinter Review
Changes:
Address Ben's comment 213
Address Jag's comment 218
Change file/class name. Change file name of firebird to firefox.
> Instead of doing that it might be cleaner to use a <xul:stringbundle/>
> 
> Oh, and for future reference you can merge the getInterface and QI into one
> statement; getInterface with one arg will QI to that arg.
I'm not changing to use <xul:stringbundle/> because it seems have bug and I
don't see a good example of using it in Mozilla code base. Maybe consider it
later. But I do merge the getService and QI into one statement.
Comment on attachment 148638 [details] [diff] [review]
patch for jag and Ben's comments

Thanks Pete!

jag, is this OK with you?
Attachment #148638 - Flags: superreview?(jag)
Attachment #148638 - Flags: review+
(In reply to comment #221)
> > Instead of doing that it might be cleaner to use a <xul:stringbundle/>
>
> I'm not changing to use <xul:stringbundle/> because it seems have bug and I
> don't see a good example of using it in Mozilla code base. Maybe consider it
> later.

Interesting, what's the bug you're seeing with stringbundle? We're using it in a
lot of places. A lot of navigator's UI uses <stringbundle>, for example, and we
use it in the <tabbrowser> XBL component:

http://lxr.mozilla.org/mozilla/source/xpfe/browser/resources/content/navigator.xul#105
http://lxr.mozilla.org/mozilla/source/xpfe/global/resources/content/bindings/tabbrowser.xml#56
Attached patch use <stringbundle> (obsolete) — Splinter Review
jag, thanks for your example. Here is the patch. Can you take a look and give
sr?
Attachment #148651 - Flags: superreview?(aagrajag)
Attachment #148651 - Flags: superreview?(jag)
Attachment #148651 - Flags: superreview?(aagrajag)
Is there a chance for 1.8? Sorry for spamming, but can't wait for this...
Comment on attachment 148651 [details] [diff] [review]
use <stringbundle>

I don't believe the MANIFEST files are still needed, I believe they are
left-overs from the old Mac build system.

Looks good, though I wonder if you needed to put all three stringbundles in all
those xul files. Do all of those XUL files actually use JS which needs access
to e.g. filedescr.properties, or do only one or two of those xul files? We
lazily load stringbundles, and once loaded are cached and shared, so if some of
those "includes" are unnecessary it's no big deal, other than perhaps confusing
others as to why they're there.

sr=jag on the UI part if you could take a look at whether you can cull any
<stringbundle> "includes". No need to attach a new patch just for that though.
Attachment #148651 - Flags: superreview?(jag) → superreview+
Comment on attachment 148651 [details] [diff] [review]
use <stringbundle>

Actually I just remembered one thing I saw:

+  if (!gStringBundleFiledescr)
+  {
+    try {
+      gStringBundleFiledescr =
document.getElementById("bundle_roaming_filedescr");
+    } catch (e) {
+      return null;
+    }
+  }
+  try {
+    return gStringBundleFiledescr.getString(filename);
+  } catch (e) {
+    try {
+      return gStringBundleFiledescr.getString(id);
+    } catch (e) {
+      if (id)
+	 return id;
+      return filename;
+    }
+  }

You don't need to try/catch getElementById. At worst the element doesn't exist
(but then why did this function get called?) and it'll return null, so you
could do a null check there. In fact, the code as it stands would in that case
leave gStringBundleFileDescr null and the getString call on it would fail
(triggering an exception), probably not exactly what you were expecting.

Also, I believe you may get a JS "variable redeclared" warning for having the
nested |e| variables in that second and third try/catch there.

+function GetString(name)
+{
+  if (!gStringBundle)
+  {
+    try {
+      gStringBundle = document.getElementById("bundle_roaming_transfer");
+    } catch (e) {dump(e);
+      return null;
+    }
+  }
+  try {
+    return gStringBundle.getString(name);
+  } catch (e) {dump(e);
+    return null;
+  }
+}

Same here, and remove or comment out those (and any other) |dump()s|.
(In reply to comment #227)
> Same here, and remove or comment out those (and any other) |dump()s|.
Sorry for the dump, It was used to debug the stringbundle

Ben, Is this patch ok for you to check in? I think we can address jag's comments
when check in. If you need a new patch, please let me know. Thanks!
> Is this patch ok for you to check in?

Yes. I'll fix the remaining few problems mentioned, mail drivers, and then check
in. Many thanks, Pete, for your help on reviews!
Comment on attachment 148651 [details] [diff] [review]
use <stringbundle>

Asking about alpha1 approval. See mail to drivers.
Attachment #148651 - Flags: approval1.8a1?
Attachment #144122 - Attachment is obsolete: true
Attachment #137283 - Attachment is obsolete: true
Attachment #148638 - Attachment is obsolete: true
Attachment #137284 - Attachment is obsolete: true
Attachment #141373 - Attachment description: review comments for C++ portion of attachment 137283 → review, p1: review comments for C++ portion of attachment 137283
Attachment #141373 - Attachment is obsolete: false
Attachment #142913 - Attachment description: BenB: Re review → review, p2: BenB: Re review
Attachment #144107 - Attachment description: My response to BenB's response to my review comments on the C++ part → review, p3: My response to BenB's response to my review comments on the C++ part
Attachment #148651 - Attachment description: use stringbundle → use <stringbundle>
Here is the latest patch. Includes
- Pete's review changes (last patch)
- jag's review comments fixed
- fix for the bug in Copy method I mentioned above
- minor changes to license/credits (URL, year, whitespace)
Attachment #127248 - Attachment is obsolete: true
Attachment #148651 - Attachment is obsolete: true
Still waiting for reaction by drivers.
Priority: P3 → P2
Whiteboard: Waiting for review → Waiting for driver approval
Target Milestone: mozilla1.7beta → mozilla1.8alpha
(Trivial update to fix conflicts.)
Attachment #125931 - Attachment is obsolete: true
We have driver approval for 1.8 alpha checkin. I'll make final tests and then
check in.
Checked in. Yay, finally. :-) I guess I hold the record for the longest time
from start of work to checkin. Many thanks for your incredible patience.

Again, many thanks to Matt Perry and Dave Caplinger for the very generous
donations and all the others who donated (see bug 124026).
Also many thanks to Pete Zha of Sun for driving this through reviews and fixing
the reviewer complaints.

Tracking bug for regressions: bug 228629.
For now, serious bugs should be added as dependency to that bug, too.

Wheeee.
Whiteboard: Waiting for driver approval
Marking FIXED :)

FYI, this should appear in 1.8 alpha2, but not in 1.7 final.

BTW: Thanks to everyone for following comment 11 and not spamming this bug. It
is long enough with just the development-related discussion, it would have been
unusable and annoying when mixed with other comments.
Any kinds of complaints or requests are to be directed to your politician (also
called /dev/null on Unix) ;-P.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Whiteboard: Regressions in bug 228629
Are there some instructions how to set it up etc.?
Not yet. It should be quite similar to 4.x' roaming with HTTP in setup. HTTPS
and plain FTP (assuming login with upload rights, of course) is now possible,
too. Maybe just try around, esp. FTP should be straight-forward. I plan to write
a howto later.

References about 4.x roaming with HTTP:
<http://www.klomp.org/mod_roaming/>
  (HTTP MOVE is not used by my implementation)
<http://web.archive.org/web/20020605164606/http://help.netscape.com/products/client/communicator/manual_roaming2.html>
For those who are interested in the inner workings:
<http://lxr.mozilla.org/seamonkey/source/extensions/sroaming/README.txt>
<http://lxr.mozilla.org/seamonkey/source/extensions/sroaming/resources/content/transfer/conflictCheck.js#40>
<http://lxr.mozilla.org/seamonkey/source/extensions/sroaming/plan.txt>


> Maybe just try around, esp. FTP should be straight-forward.

Maybe not. OK, very short:

Roaming urls:
 <ftp://user@ftp-server//path/to/dir/>
 <http://user@http-server/path/to/dir/>
 Mind the number of slashes, it may not work otherwise.
 Servers used: wu-FTPd and Apache with mod_roaming
 You can leave out user@, the username in the corresponding textfield
 will be used.

Sorry for spamming you and that I don't have an howto ready now, I realize you
are itching to try this out and it may *not* be obvious how.
I'm trying this out in the nightly and I'm seeing a few problems with it:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a2) Gecko/20040529

Must manually create listing.xml file on server to enable.

Passwords - will not stay checked

Fill Form data - will not stay checked

Helper App Settings - blocks upload/download (freezes on Roaming
profile conflicts)

Private Keys - blocks upload/download (freezes on Roaming profile
conflicts)

Security Module Databae - blocks upload/download (freezes on Roaming
profile conflicts)

If both Private Keys and Security Module Database are checked then you
get: Transfer error with file "undefined": NS_ERROR_ILLEGAL_VALUE

Window doesn't close on Roaming Transfer upload - 'Cancel' is a bit
confusing as the open to close the window.
*** Bug 18043 has been marked as a duplicate of this bug. ***
I've read in the README that changed files are only uploaded when you shutdown
the application. What about adding a "Sync Now" button somewhere to do it
manually or a timer to do it in configurable intervals during the session?
Should be far easier to implement than a sync on every bookmark or preferences
change, shouldn't it?
> "Sync Now"

I filed bug 245985 about that.
Attachment #127248 - Flags: review?(ccarlen)
Attachment #148651 - Flags: approval1.8a1?
Attachment #148638 - Flags: superreview?(jag)
Just a heads-up: I just checked in fixes for several major bugs. Windows
installer builds are still broken.
Component: Profile: BackEnd → Profile: Roaming
QA Contact: ben.bucksch → core.profile-roaming
Blocks: 31766
Any chance of getting this into Thunderbird? Does it work yet in the Suite?
It doesn't really work in the suite yet. It garbles the cookie file.
(In reply to comment #246)
> It garbles the cookie file.

That's bug 244793, testers/investigation needed, help welcome
this entry is opened at 2002.02 and roaminng is still not in neither mozilla 
nor firefox. can we ask to include it even it's not 100% perfect. i use it 
since netscape 4.x and realy would like to use again.
PLEASE add to both firefox and mozilla!!!
It is tremendously disappointing for me to have paid $100 bounty on this bug
nearly three years ago and still have nothing I can use.  To see this bug marked
"RESOLVED/FIXED" is just insulting.
once they figure this is a big blocker for enterprises, somebody will implement
it for firefox :P
(In reply to comment #249)
> It is tremendously disappointing for me to have paid $100 bounty on this bug
> nearly three years ago and still have nothing I can use.

Why can't you use it? Just download Mozilla 1.8.x and set up your roaming
profile on an FTP server. It most definitely works, I can tell you that.

> To see this bug marked "RESOLVED/FIXED" is just insulting.

Well, it is FIXED. If you have any additional requests/suggestion, you should
file a new bug.

Prog.
Status: RESOLVED → VERIFIED
Mozilla does not have the same roaming functionality as Netscape 4, despite the
4xp keyword this bug has.  The user is not prompted for their *username* and
password when Mozilla starts, and therefore you can't plug the username into an
environment variable in Mozilla's "Roaming URL" preference to support multiple
roaming users under one Mozilla profile.  Sure you can set up multiple Mozilla
profiles, each with their own distinct Roaming URL, but then every other
computer they could possibly use needs to have the same profiles created, which
defeats the purpose of roaming, at least for the enterprise IMHO.  The current
implementation of roaming in Mozilla is great for a single user who wants a
centralized profile, but does nothing for companies with hundreds of users, a
lot of whom use multiple computers at multiple branch offices.
(In reply to comment #251)
> Why can't you use it? Just download Mozilla 1.8.x and set up your roaming
> profile on an FTP server. It most definitely works, I can tell you that.

It definitely does NOT work. It does NOT work with WebDAV, it does NOT work with
https, and the download dialog has serious bugs.
Oh, and it does garble the cookie file.

> 
> > To see this bug marked "RESOLVED/FIXED" is just insulting.
> 
> Well, it is FIXED. If you have any additional requests/suggestion, you should
> file a new bug.

It certainly is NOT fixed. In the status this is in, it is never going to be in
the release versions.
Even in the 1.8 versions, it is only in the nightly builds, not in the officials
alphas (unless they put it in a4, which I haven't used.)

-Joe
Joachim -- note the title of this bug. Netscape Communicator didn't support
https properly, and I don't think it supported WebDAV either. There are other
entries here -- Bug #17917 is one -- for making roaming _better_, but that's not
_this_ bug, which is done. Complaining here isn't helpful.
(In reply to comment #254)
> Joachim -- note the title of this bug. Netscape Communicator didn't support
> https properly, and I don't think it supported WebDAV either. There are other
> entries here -- Bug #17917 is one -- for making roaming _better_, but that's not
> _this_ bug, which is done.

Well, granted. But even then, the garbled Cookie file is part of this bug, since
that worked in Netscape 4.
Please file bugs with this implementation as separate bugs in Bugzilla. Thanks.
(In reply to comment #256)
> Please file bugs with this implementation as separate bugs in Bugzilla. Thanks.

Joachim - when you do make the new makes make them blocked by this bug so we can
spot them and vote for it.  I know some folks DO pay attention to what's getting
lots of votes.  If 85 folks took their votes from this bug and put it on the new
bugs somebody might notice.  :)
(In reply to comment #256)
> Please file bugs with this implementation as separate bugs in Bugzilla. Thanks.

This particular bug was long ago entered as bug 244793. And I in fact have voted
for it.
> make them blocked by this bug

Please don't. We now have a category "Browser | Profile: Roaming" where you
should be able to find them.
*** Bug 187513 has been marked as a duplicate of this bug. ***
No longer blocks: majorbugs
This bug is marked *Core* and *Trunk* and was "fixed" way back in 2004. So where is this feature in Thunderbird and Firefox? Should this bug be reassigned to "Product: Mozilla Application Suite" to avoid ambiguity? Thanks.
> where is this feature in Thunderbird and Firefox?

Just because something is in Core does not mean it's appearing in Firefox or Thunderbird. Both can use it, but they need work, for UI and hookup in startup/shutdown. Thunderbird is bug 310158, Firefox is bug 249343.

> Should this bug be reassigned to "Product: Mozilla Application Suite"

No, because it even has its own Component. And it's not (inherently) limited to Seamonkey.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.