This is part of Nvu landing into the trunk. Nvu has a site manager allowing to browse a remote FTP site and delete a file on such a site, create a directory and remove a directory. But del, mkdir and rmdir are not implemented in the FTP stack.

This is a first fix for this bug, implementing DEL, MKDIR and RMDIR. Of course, since Nvu is the only app using it, this is ok to encapsulate everything in ifdefs on MOZ_STANDALONE_COMPOSER if you want it. darin, dougt: could you please r/sr ? thanks a lot!

I don't think this needs to be #ifdef'd for MOZ_STANDALONE_COMPOSER. I really want to avoid forking the code like that ;-) This looks like good stuff to make available to other Mozilla based apps or extensions, so I'm all for it.

Comment on attachment 155678 [details] [diff] [review] fix #1 >Index: netwerk/protocol/ftp/public/nsIFTPChannel.idl > [scriptable, uuid(3476df52-1dd2-11b2-b928-925d89b33bc0)] > interface nsIFTPChannel : nsIChannel > { >+ void deleteFile(); >+ void createDirectory(); >+ void removeDirectory(); any interface change must be accompanied by a UUID change. otherwise, it is impossible to build extensions to Mozilla that fail gracefully when the interface changes yet again. please document how these methods work. i.e., say that they flag the channel to perform the specified operation once opened (via nsIChannel::open or nsIChannel::asyncOpen). >Index: netwerk/protocol/ftp/src/nsFTPChannel.cpp >+ mScheduledForDELE(PR_FALSE), >+ mScheduledForMKD(PR_FALSE), >+ mScheduledForRMD(PR_FALSE) mScheduledForDEL instead? (consistent with other 3-letter acronyms) >+ } else if (mCallbacks != this) { > return mCallbacks ? mCallbacks->GetInterface(anIID, aResult) : NS_ERROR_NO_INTERFACE; > } >+ else >+ return NS_ERROR_NO_INTERFACE; > } Is that last |else| really necessary? Also, what is the purpose of the additional "else if" branch? >Index: netwerk/protocol/ftp/src/nsFTPChannel.h >+ PRPackedBool mScheduledForDELE; >+ PRPackedBool mScheduledForMKD; >+ PRPackedBool mScheduledForRMD; >+ > PRBool mIsPending; maybe mIsPending should become a PRPackedBool as well, so that all 4 of these can be packed into a single DWORD? >Index: netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp had to stop there... will continue later.

Comment on attachment 155678 [details] [diff] [review] fix #1 >Index: netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp > nsFtpState::S_user() { >+ > // some servers on connect send us a 421 or 521. (84525) (141784) ... > nsFtpState::S_pass() { >+ > nsresult rv; nit: additional newlines here is not consistent with other methods in this file. or maybe the file is just not consistent with itself. at any rate, better to be consistent or not change anything ;-) >+ if (mAction == DEL) >+ return FTP_S_DELE; >+ else if (mAction == MKDIR) >+ return FTP_S_MKD; >+ else if (mAction == RMDIR) >+ return FTP_S_RMD; else after return is not needed. >+void >+nsFtpState::ScheduleForFileDeletion(PRBool aEnabled) >+{ >+ if (aEnabled) >+ mAction = DEL; >+} so, what happens if some consumer of nsIFTPChannel tries to configure the channel to DEL and MKDIR or something like that? should we have some sanity checks someplace? >Index: netwerk/protocol/ftp/src/nsFtpConnectionThread.h >+typedef enum _FTP_ACTION {GET, PUT, DEL, MKDIR, RMDIR} FTP_ACTION; in some places you use "DEL" and others "DELE" .. >+ void ScheduleForFileDeletion(PRBool aEnabled); >+ void ScheduleForDirCreation(PRBool aEnabled); >+ void ScheduleForDirRemoval(PRBool aEnabled); maybe implement these inline?

> any interface change must be accompanied by a UUID change. otherwise, > it is impossible to build extensions to Mozilla that fail gracefully > when the interface changes yet again. Sure. > please document how these methods work. i.e., say that they flag the > channel to perform the specified operation once opened (via > nsIChannel::open or nsIChannel::asyncOpen). Ok, will do. > >Index: netwerk/protocol/ftp/src/nsFTPChannel.cpp > > >+ mScheduledForDELE(PR_FALSE), > >+ mScheduledForMKD(PR_FALSE), > >+ mScheduledForRMD(PR_FALSE) > > mScheduledForDEL instead? (consistent with other 3-letter acronyms) I preferred consistency with RFC 959. See a few lines below. > >+ } else if (mCallbacks != this) { > > return mCallbacks ? mCallbacks->GetInterface(anIID, aResult) : NS_ERROR_NO_INTERFACE; > > } > >+ else > >+ return NS_ERROR_NO_INTERFACE; > > } > > Is that last |else| really necessary? Also, what is the purpose > of the additional "else if" branch? Hmmm.... Why did I add that?-) Let me rethink about it a bit more. > >+ PRPackedBool mScheduledForDELE; > >+ PRPackedBool mScheduledForMKD; > >+ PRPackedBool mScheduledForRMD; > >+ > > PRBool mIsPending; > > maybe mIsPending should become a PRPackedBool as well, so that > all 4 of these can be packed into a single DWORD? Sure. > nit: additional newlines here is not consistent with other methods > in this file. or maybe the file is just not consistent with itself. > at any rate, better to be consistent or not change anything ;-) Yeah, thos empty lines are only here because I remove some ifdef DEBUG_glazman code... Will remove them. > else after return is not needed. Yep. > so, what happens if some consumer of nsIFTPChannel > tries to configure the channel to DEL and MKDIR or > something like that? should we have some sanity > checks someplace? I did nothing because I did not know what to do. a) fire an error b) cancel all previous orders and keep the last one c) do nothing, supposing that the embedder knows what he/she is doing (yeah, I can be anive from time to time ;-) > in some places you use "DEL" and others "DELE" .. Yes, that's consistent with RMD/RMDIR, MKD/MKDIR: GET, PUT, DEL, MKDIR, RMDIR are ftp actions, the ones you type in your ftp command-line client RETR, STOR, DELE, MKD, RMD are FTP commands (RFC 959) > >+ void ScheduleForFileDeletion(PRBool aEnabled); > >+ void ScheduleForDirCreation(PRBool aEnabled); > >+ void ScheduleForDirRemoval(PRBool aEnabled); > > maybe implement these inline? Ok. I'll submit a new patch ASAP. It will perhaps also contain implementation for RNFR and RNTO in order to rename a file.

In response to darin's comments. Includes support for RENAME (RNFR+RNTO).

Comment on attachment 158974 [details] [diff] [review] fix #2 darin, can you sr please ?

Comment on attachment 158974 [details] [diff] [review] fix #2 >Index: ftp/public/nsIFTPChannel.idl >+ * This method prepares the channel for renaming. The renaming itself >+ * will be will be performed when the channel is opened, >+ * synchronously or not. >+ */ >+ void renameTo(in AString aNewName); typo: "will be will be performed..." is AString the right choice here? what about AUTF8String to match nsIURL::fileName? >Index: ftp/src/nsFTPChannel.h >+ nsAutoString mScheduledNewName; This should not be a auto string. That increases the size of the containing object by 128 bytes, but in many cases those are wasted bytes since we are rarely doing a rename. Instead, it should be heap allocated separately (i.e., use nsString instead). >Index: ftp/src/nsFtpConnectionThread.cpp >+ return FTP_S_RNFR; >+ default: > return FTP_S_LIST; > } >+ } looks like there is some funky indentation going on here. please tidy it up. >+nsresult >+nsFtpState::SingleAbsolutePathCommand(FTP_ACTION aAction) { open bracket on newline for consistency. >+ inline void ScheduleForRenaming(PRBool aEnabled, const nsAutoString & aNewPath) { >+ if (aEnabled) >+ { >+ mAction = RENAME; >+ mNewPath = ToNewCString(aNewPath); >+ } >+ } no need to pass nsAutoString reference. use |const nsString&| instead, or |const nsSubstring&| if the function doesn't need the string to be null terminated, which is the case here. also, this ToNewCString call destroys (truncates) any non-ASCII unicode values. i think you need to convert aNewPath to the same charset as mURL->GetOriginCharset() returns. in other words, this won't work on systems that use non-ASCII filenames w/ a funky character encoding. it might be best to change your API to accept a nsIURI for the new file location. then you can just call nsIURI::GetAsciiSpec to return a version of the URL that has already been translated into the right charset, with %-escaping where needed. This is probably also good for consumers of the FTP channel, since they already know how to work with URIs in a i18n way.

Comment on attachment 155678 [details] [diff] [review] fix #1 removing from queue. any chance on posting a new patch?

Comment on attachment 353684 [details] [diff] [review] fix #3, in anwer to dougt's comment and for mozilla-central trunk cool. just some comments -- it isn't clear from looking at the interface if you can call more than one method. For example: ftpChannel.renameTo("blah") ftpChannel.deleteFile(); I do not understand the cut here: + + rv = aURL->GetFilePath(delStr); + delStr.Cut(0,1); do you want to change the URI of the channel after the move? For example, return mNewURI when someone calls channel.URI Other than that, this looks fine. r= with answers to the above quesitons.

(In reply to comment #11) > cool. Thx for reviewing Doug ! > just some comments -- > > it isn't clear from looking at the interface if you can call more than one > method. For example: > > ftpChannel.renameTo("blah") > ftpChannel.deleteFile(); No you can't. The second line above will nix the |mScheduledForRNFR| member calling |ClearSchedules()|. Do you think I should make the second line above throw a warning or an error? I can do that. > I do not understand the cut here: > > + > + rv = aURL->GetFilePath(delStr); > + delStr.Cut(0,1); I have to check my old code. I remember this was absolutely needed because the filePath had an extra char but I don't remember which one. Let me investigate and come back to you ASAP. > do you want to change the URI of the channel after the move? For example, > return mNewURI when someone calls channel.URI Excellent suggestion. Doing. > Other than that, this looks fine. r= with answers to the above quesitons. Can I suppose a new patch in answer to your questions above will carry forward that r= ?

(In reply to comment #11) > I do not understand the cut here: > > + > + rv = aURL->GetFilePath(delStr); > + delStr.Cut(0,1); Ok, got it. It's because your $HOME dir in the ftp server may not be the / of your ftp server... In that case, absolute paths make the ftp command fail so we always need to remove the leading slash. I will change the last line above into the following, for sanity reasons: if (delStr.First() == '/') delStr.Cut(0,1); Please note that's something nsFtpConnectionThread.cpp must already do in another location for the same reason: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp#1213

trivial changes to address dougt's questions and suggestions + optimization making |S_stor| rely on the new |SingleAbsolutePathCommand| just like new |S_dele|, |S_mkdir|, |S_rmdir| and |S_rnfr|.

Comment on attachment 353790 [details] [diff] [review] final patch >+nsresult >+nsFtpState::S_rnto() { >+ nsCAutoString sizeBuf; >+ mNewURI->GetAsciiSpec(sizeBuf); Ah wait :-( The above (suggestion from comment #8) is wrong. It's not the asciiSpec we need but the path... Doug, is it ok to just call GetPath() here or are there magic charset weirdnesses again?

Final patch. Extensively tested with BlueGriffon, works fine including with non-ascii chars.

Comment on attachment 357116 [details] [diff] [review] fix #4 Doug, can you review please ?

Comment on attachment 357116 [details] [diff] [review] fix #4 do you have to test for a dir seperator: + str.Append(mPwd); + str.Append(mPath); if that or comment that you don't need to, and r=

-etimedout comment 18