Implement FTP del, mkdir and rmdir for Nvu

RESOLVED INCOMPLETE

Status

()

--
enhancement
RESOLVED INCOMPLETE
14 years ago
3 years ago

People

(Reporter: glazou, Assigned: glazou)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

14 years ago
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.
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

14 years ago
Created attachment 155678 [details] [diff] [review]
fix #1

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!
(Assignee)

Updated

14 years ago
Attachment #155678 - Flags: superreview?(darin)
Attachment #155678 - Flags: review?(dougt)

Comment 2

14 years ago
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 3

14 years ago
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 4

14 years ago
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?
Attachment #155678 - Flags: superreview?(darin) → superreview-
(Assignee)

Comment 5

14 years ago
> 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.
(Assignee)

Comment 6

14 years ago
Created attachment 158974 [details] [diff] [review]
fix #2

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

Updated

14 years ago
Attachment #155678 - Attachment is obsolete: true
(Assignee)

Comment 7

14 years ago
Comment on attachment 158974 [details] [diff] [review]
fix #2

darin, can you sr please ?
Attachment #158974 - Flags: superreview?(darin)

Comment 8

14 years ago
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.
Attachment #158974 - Flags: superreview?(darin) → superreview-

Comment 9

14 years ago
Comment on attachment 155678 [details] [diff] [review]
fix #1

removing from queue.


any chance on posting a new patch?
Attachment #155678 - Flags: review?(dougt) → review-
(Assignee)

Comment 10

10 years ago
Created attachment 353684 [details] [diff] [review]
fix #3, in anwer to dougt's comment and for mozilla-central trunk
Attachment #158974 - Attachment is obsolete: true
Attachment #353684 - Flags: review?(doug.turner)

Comment 11

10 years ago
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.
Attachment #353684 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 12

10 years ago
(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= ?
(Assignee)

Comment 13

10 years ago
(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
(Assignee)

Comment 14

10 years ago
Created attachment 353790 [details] [diff] [review]
final patch

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|.
Attachment #353684 - Attachment is obsolete: true
Attachment #353790 - Flags: review?(doug.turner)
(Assignee)

Updated

10 years ago
Attachment #353790 - Attachment is obsolete: true
Attachment #353790 - Flags: review?(doug.turner)
(Assignee)

Comment 15

10 years ago
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?
(Assignee)

Comment 16

10 years ago
Created attachment 357116 [details] [diff] [review]
fix #4

Final patch. Extensively tested with BlueGriffon, works fine including with
non-ascii chars.
Attachment #357116 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #357116 - Flags: review? → review?(doug.turner)
(Assignee)

Comment 17

10 years ago
Comment on attachment 357116 [details] [diff] [review]
fix #4

Doug, can you review please ?

Comment 18

10 years ago
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=
Attachment #357116 - Flags: review?(doug.turner) → review+
-etimedout comment 18
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.