Last Comment Bug 467524 - [SeaMonkey] FTP file upload not working
: [SeaMonkey] FTP file upload not working
Status: RESOLVED FIXED
[See comment 2 and comment 5]
: fixed-seamonkey2.0.4
Product: SeaMonkey
Classification: Client Software
Component: Download & File Handling (show other bugs)
: Trunk
: All All
: -- major (vote)
: seamonkey2.1a1
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on: 495291
Blocks: 495583
  Show dependency treegraph
 
Reported: 2008-12-02 04:07 PST by Stefan A. Möller
Modified: 2010-04-05 17:15 PDT (History)
5 users (show)
mbeltzner: blocking1.9.2-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix for seamonkey repository [See bug 495291] (1.16 KB, patch)
2009-06-17 18:37 PDT, Michal Novotny (:michal)
no flags Details | Diff | Splinter Review
Branch patch [Checkin: Comment 15] (4.41 KB, patch)
2010-02-27 09:49 PST, neil@parkwaycc.co.uk
kairo: review+
kairo: approval‑seamonkey2.0.4+
Details | Diff | Splinter Review
WIP#1 (13.72 KB, patch)
2010-02-28 14:38 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
WIP#2 (18.33 KB, patch)
2010-03-01 09:58 PST, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review
Proposed patch (14.84 KB, patch)
2010-03-07 16:28 PST, neil@parkwaycc.co.uk
kairo: ui‑review+
Details | Diff | Splinter Review
With extra goodness [Checkin: Comment 30] (14.52 KB, patch)
2010-04-01 08:51 PDT, neil@parkwaycc.co.uk
iann_bugzilla: review+
Details | Diff | Splinter Review

Description Stefan A. Möller 2008-12-02 04:07:14 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b3pre) Gecko/20081202 SeaMonkey/2.0a2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b3pre) Gecko/20081202 SeaMonkey/2.0a2pre

With SM 2.0a I cannot upload files to a FTP server any more.

Reproducible: Always

Steps to Reproduce:
1. Open your FTP location and log in.
2. Menu "File -> Upload file"
3. Choose some file in the file picker.
4. Click the "Open" button.
Actual Results:  
File picker closes, but otherwise nothing happens. There is no network activity.

Expected Results:  
File should be uploaded to the server.
Comment 1 Serge Gautherie (:sgautherie) 2009-01-28 10:27:39 PST
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090127 SeaMonkey/2.0a3pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/11e029835944
 +http://hg.mozilla.org/comm-central/rev/daa8b76471bf)

Confirming.
Nothing in the Error Console either.

(Firefox does not have this feature.)
Comment 2 Michal Novotny (:michal) 2009-06-17 18:27:57 PDT
File upload is broken in comm-central repository because of missing interface nsIProgressDialog. In seamonkey repository and mozilla CVS it is located in xpfe/components/download-manager/public/nsIProgressDialog.idl. It seems that this file was never imported to mozilla-central or comm-central repository. There are still some references to this interface in mozilla-central http://mxr.mozilla.org/mozilla-central/search?string=nsIProgressDialog so I'm not sure if this file was omitted intentionally. If yes then we probably need to define the interface somewhere in the comm-central. Someone more familiar with seamonkey should decide...
Comment 3 Michal Novotny (:michal) 2009-06-17 18:37:06 PDT
Created attachment 383841 [details] [diff] [review]
fix for seamonkey repository
[See bug 495291]

Upload is broken in seamonkey repository as well, but for another reason. This patch fixes it.
Comment 4 Serge Gautherie (:sgautherie) 2009-06-17 18:57:09 PDT
Comment on attachment 383841 [details] [diff] [review]
fix for seamonkey repository
[See bug 495291]

How did you create this patch?
Neil already fixed this 3 weeks ago in bug 495291.
Comment 5 Serge Gautherie (:sgautherie) 2009-06-17 20:09:33 PDT
(In reply to comment #2)

Indeed!

Current CVS (still) has:
http://mxr.mozilla.org/mozilla/source/xpfe/components/download-manager/public/
Makefile.in  	1k  	Aug 18 2007
nsIDownload.idl 	4k 	Aug 17 2007
nsIDownloadManager.idl 	8k 	Jul 7 2005
nsIDownloadProgressListener.idl 	3k 	Apr 6 2005
nsIProgressDialog.idl 	3k 	Aug 14 2007

mozilla-central rev 2 had only:
http://hg.mozilla.org/mozilla-central/file/10cab3c34c28/xpfe/components/download-manager/public/
2007-03-22 16:01 -0700	1943	Makefile.in
2007-03-22 16:01 -0700	8274	nsIDownloadManager.idl
2007-03-22 16:01 -0700	3916	nsIDownloadProgressListener.idl

The missing IDL was added by
http://hg.mozilla.org/mozilla-central/rev/fd843d9a910b
and recently removed by bug 495583.

I don't think this can explain the current bug report,
but I think the current (IDL) situation is odd (= additional regression) and should be looked at here or (rather) in bug 495291 or (probably) a dedicated bug.
Comment 6 Serge Gautherie (:sgautherie) 2009-06-17 20:18:27 PDT
(In reply to comment #4)
> How did you create this patch?

Ah! This is a cvs patch actually:
if this is actually needed on 1.8.1 branch too (which I'm not sure of), you should just ask for approval in bug 495291.
Comment 7 Robert Kaiser 2009-06-18 04:21:38 PDT
xpfe/components/download-manager/ isn't built on current SeaMonkey any more, so it doesn't really matter if a file is in there or not.

That said, we won't block beta for FTP upload.
Comment 8 Serge Gautherie (:sgautherie) 2009-08-19 10:01:56 PDT
(In reply to comment #7)
> xpfe/components/download-manager/ isn't built on current SeaMonkey any more, so
> it doesn't really matter if a file is in there or not.

The point is not the location, but that a file seems to be missing in the tree!

Moving this bug to Toolkit.
Comment 9 Mike Beltzner [:beltzner, not reading bugmail] 2009-09-08 14:53:00 PDT
Not blocking as per Kairo's comment 7
Comment 10 neil@parkwaycc.co.uk 2010-02-27 09:21:39 PST
I don't think we need toolkit help for this.
Comment 11 neil@parkwaycc.co.uk 2010-02-27 09:49:43 PST
Created attachment 429343 [details] [diff] [review]
Branch patch
[Checkin: Comment 15]

This is just a copy of the 1.8 branch XPFE nsIProgressDialog.idl which is enough to get the dialog to work. I couldn't find a better place to put it.
Comment 12 neil@parkwaycc.co.uk 2010-02-27 09:51:36 PST
Comment on attachment 429343 [details] [diff] [review]
Branch patch
[Checkin: Comment 15]

D'oh, KaiRo isn't unique :-(
Comment 13 Robert Kaiser 2010-02-27 11:09:00 PST
Comment on attachment 429343 [details] [diff] [review]
Branch patch
[Checkin: Comment 15]

r+a=me for 1.9.1 only.

For trunk, we need to have a larger patch, no matter if we need the IDL or not, we should have everything reviewed at once.
Comment 14 Robert Kaiser 2010-02-27 11:30:41 PST
(In reply to comment #12)
> (From update of attachment 429343 [details] [diff] [review])
> D'oh, KaiRo isn't unique :-(

kairo@ is unique actually :)
Comment 15 neil@parkwaycc.co.uk 2010-02-28 07:56:11 PST
Pushed changeset c0d669d37469 to releases/comm-1.9.1
Comment 16 neil@parkwaycc.co.uk 2010-02-28 14:38:21 PST
Created attachment 429439 [details] [diff] [review]
WIP#1

This is a straight port of the upload progress dialog, but with less cruft, so although it works, it's still ugly. At the very least I need to use the same time and size formatting as the download progress dialog (speed too but I can't see where that is handled right now). I could probably change the cancel button into a stop icon but I was too lazy (if you look carefully you might notice that I haven't even localised it yet). I'm not sure what to do with the source and target though, as the options available for downloads don't make sense here.
Comment 17 Robert Kaiser 2010-02-28 14:53:04 PST
Comment on attachment 429439 [details] [diff] [review]
WIP#1

Not sure if I'm the right person to review this patch, I don't really that deeply into code. Also, I'm not sure how much value it is to review unlocalizable code.
Comment 18 Robert Kaiser 2010-02-28 15:08:14 PST
Comment on attachment 429439 [details] [diff] [review]
WIP#1

hmm, any chance we can put it in common/downloads so it's found in one location with the similar download progress stuff?
Also, can we name it uploadProgress.xul/.js to state that it's upload only?
Comment 19 neil@parkwaycc.co.uk 2010-03-01 09:58:02 PST
Created attachment 429540 [details] [diff] [review]
WIP#2

Moving it into common/downloads made the l10n much easier.
Comment 20 Robert Kaiser 2010-03-04 13:14:21 PST
Comment on attachment 429540 [details] [diff] [review]
WIP#2

Hrm, I really would have liked UI that is more like the download progress windows we have on trunk, but I won't insist on that as I don't want to get into another flamewar about anything like this.
Comment 21 neil@parkwaycc.co.uk 2010-03-07 16:28:47 PST
Created attachment 431010 [details] [diff] [review]
Proposed patch
Comment 22 Robert Kaiser 2010-03-11 08:43:59 PST
Comment on attachment 431010 [details] [diff] [review]
Proposed patch

IMHO, the "From" isn't really needed, as I'd colloquially say I'm "uploading foo.zip to example.org" and we could use exactly that wording in the dialog, but else it looks good to me.
Comment 23 Ian Neal 2010-03-23 06:37:02 PDT
Comment on attachment 431010 [details] [diff] [review]
Proposed patch

Could DownloadUtils getDownloadStatus be made use of?
Is it worth having globals for the status and size elements?
Comment 24 neil@parkwaycc.co.uk 2010-03-23 07:13:20 PDT
(In reply to comment #23)
> (From update of attachment 431010 [details] [diff] [review])
> Could DownloadUtils getDownloadStatus be made use of?
I probably didn't bother because we don't use it for downloads either, but I can look into it, as updates.js seems to use it in a similar way.

> Is it worth having globals for the status and size elements?
Yeah, I could do that.
Comment 25 neil@parkwaycc.co.uk 2010-03-23 10:39:46 PDT
getDownloadStatus doesn't format in the way we want, it gives us
[remaining] - [transferred] of [total] ([rate])
but we want
[transferred] of [total] ([rate])
[elapsed]
[remaining]
as separate fields.
Comment 26 neil@parkwaycc.co.uk 2010-03-23 10:41:39 PDT
(In reply to comment #23)
> (From update of attachment 431010 [details] [diff] [review])
> Is it worth having globals for the status and size elements?
Also the elapsed, percent and progressmeter elements, no?
Comment 27 Ian Neal 2010-03-24 09:00:55 PDT
(In reply to comment #26)
> (In reply to comment #23)
> > (From update of attachment 431010 [details] [diff] [review])
> > Is it worth having globals for the status and size elements?
> Also the elapsed, percent and progressmeter elements, no?

I was looking at ones used in multiple locations but yes, those as well.
Comment 28 neil@parkwaycc.co.uk 2010-03-24 09:18:44 PDT
(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #23)
> > > (From update of attachment 431010 [details] [diff] [review])
> > > Is it worth having globals for the status and size elements?
> > Also the elapsed, percent and progressmeter elements, no?
> I was looking at ones used in multiple locations but yes, those as well.
Well, I don't see how centralising all the progressmeter updates in setPercent absolves it from using a global variable, but I guess I'm already inconsistent with my use of gBundle, so I might as well make them all into variables.
Comment 29 neil@parkwaycc.co.uk 2010-04-01 08:51:54 PDT
Created attachment 436485 [details] [diff] [review]
With extra goodness
[Checkin: Comment 30]

Not only with extra variables, but uses Services.jsm too :-)
Comment 30 neil@parkwaycc.co.uk 2010-04-05 14:17:13 PDT
Pushed changeset 2668ddc4da4b to comm-central.

Note You need to log in before you can comment on or make changes to this bug.