Closed Bug 467524 Opened 16 years ago Closed 14 years ago

[SeaMonkey] FTP file upload not working

Categories

(SeaMonkey :: Download & File Handling, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1a1

People

(Reporter: s.a.moeller, Assigned: neil)

References

Details

(Keywords: fixed-seamonkey2.0.4, Whiteboard: [See comment 2 and comment 5])

Attachments

(2 files, 4 obsolete files)

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.
Keywords: regression
Version: unspecified → Trunk
[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.)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-seamonkey2?
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...
Upload is broken in seamonkey repository as well, but for another reason. This patch fixes it.
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.
Attachment #383841 - Attachment description: fix for seamonkey repository → fix for seamonkey repository [See bug 495291]
Attachment #383841 - Attachment is obsolete: true
(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.
Blocks: 495583
Depends on: 495291
Flags: blocking-seamonkey2.0b1?
(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.
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.
Flags: blocking-seamonkey2.0b1? → blocking-seamonkey2.0b1-
(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.
status1.9.1: --- → ?
Component: Download & File Handling → Download Manager
Flags: blocking1.9.2?
Flags: blocking-seamonkey2?
Flags: blocking-seamonkey2.0b1-
OS: Windows 2000 → All
Product: SeaMonkey → Toolkit
QA Contact: download → download.manager
Hardware: x86 → All
Summary: FTP file upload not working → FTP file upload not working (Toolkit part)
Whiteboard: [See comment 2 and comment 5]
Summary: FTP file upload not working (Toolkit part) → [SeaMonkey] FTP file upload not working (Toolkit part)
Not blocking as per Kairo's comment 7
Flags: blocking1.9.2? → blocking1.9.2-
I don't think we need toolkit help for this.
Component: Download Manager → Download & File Handling
Product: Toolkit → SeaMonkey
QA Contact: download.manager → download
Summary: [SeaMonkey] FTP file upload not working (Toolkit part) → [SeaMonkey] FTP file upload not working
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #429343 - Flags: review?
Attachment #429343 - Flags: approval-seamonkey2.0.4?
Comment on attachment 429343 [details] [diff] [review]
Branch patch
[Checkin: Comment 15]

D'oh, KaiRo isn't unique :-(
Attachment #429343 - Flags: review? → review?(kairo)
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.
Attachment #429343 - Flags: review?(kairo)
Attachment #429343 - Flags: review+
Attachment #429343 - Flags: approval-seamonkey2.0.4?
Attachment #429343 - Flags: approval-seamonkey2.0.4+
(In reply to comment #12)
> (From update of attachment 429343 [details] [diff] [review])
> D'oh, KaiRo isn't unique :-(

kairo@ is unique actually :)
Pushed changeset c0d669d37469 to releases/comm-1.9.1
Attached patch WIP#1 (obsolete) — Splinter Review
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.
Attachment #429439 - Flags: review?(kairo)
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 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?
Attached patch WIP#2 (obsolete) — Splinter Review
Moving it into common/downloads made the l10n much easier.
Attachment #429439 - Attachment is obsolete: true
Attachment #429540 - Flags: ui-review?(kairo)
Attachment #429439 - Flags: review?(kairo)
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.
Attached patch Proposed patch (obsolete) — Splinter Review
Attachment #429540 - Attachment is obsolete: true
Attachment #431010 - Flags: ui-review?(kairo)
Attachment #429540 - Flags: ui-review?(kairo)
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.
Attachment #431010 - Flags: ui-review?(kairo) → ui-review+
Attachment #431010 - Attachment description: WIP#3 → Proposed patch
Attachment #431010 - Flags: review?(iann_bugzilla)
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?
(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.
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.
(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?
(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.
(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.
Not only with extra variables, but uses Services.jsm too :-)
Attachment #431010 - Attachment is obsolete: true
Attachment #436485 - Flags: review?(iann_bugzilla)
Attachment #431010 - Flags: review?(iann_bugzilla)
Attachment #436485 - Flags: review?(iann_bugzilla) → review+
Pushed changeset 2668ddc4da4b to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Attachment #429343 - Attachment description: Branch patch → Branch patch [Checkin: Comment 15]
Attachment #436485 - Attachment description: With extra goodness → With extra goodness [Checkin: Comment 30]
status1.9.1: ? → ---
Target Milestone: --- → seamonkey2.1a1
You need to log in before you can comment on or make changes to this bug.