Closed Bug 24867 Opened 25 years ago Closed 21 years ago

basic UI for FTP upload (menu) not implemented

Categories

(Core Graveyard :: Networking: FTP, enhancement, P1)

Product:
Core Graveyard
Component:
Networking: FTP
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.7final

People

(Reporter: jud, Assigned: darin.moz)

References

Details

(Keywords: topembed-, verified1.7)

Attachments

(5 files, 14 obsolete files)

Workaround to get Upload functionality
5.48 KB, text/html
Details
v2.3 patch
101.77 KB, patch
neil
: review+
bryner
: superreview+
asa
: approval1.7+
Details | Diff | Splinter Review
fix for that typo
1.29 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
More file open bustage
1.36 KB, patch
darin.moz
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review
complete patch for 1.7 branch (including all followup fixes)
120.17 KB, patch
Details | Diff | Splinter Review 
I just checked in the FTP upload code for the protocol (bug 18977). You can now 
call AsyncWrite() to upload data. However, we need UI for this. 4.x allowed the 
user to drag and drop a file into a directory. To do this in our new tree widget 
world, we need some notion of directory focus (via mousover) in the widget (not 
sure if this exists, but I suspect it does).
sounds like post-beta, m15
Summary: UI for FTP upload not implemented → [FEATURE] UI for FTP upload not implemented
Target Milestone: M15
Moving all UE/UI bugs to new component: User Interface: Design Feedback
UE/UI component will be deleted.
Component: UE/UI → User Interface: Design Feedback
*** Bug 34075 has been marked as a duplicate of this bug. ***
Move to M16 for now ...
Target Milestone: M15 → M16
QA Assigning non-confidential New/Assigned User Interface: Design Feedback bugs 
to Matthew Thomas (mpt@mailandnews.com).

Matthew Thomas is now the QA owner for the User Interface: Design Feedback 
component. (Bugs that involve UI issues in the Netscape-branded Mozilla browser 
should continue be QA assigned to elig@netscape.com.)
QA Contact: elig → mpt
Target Milestone: M16 → Future
Keywords: nsbeta2
Putting on [nsbeta2-] radar. Not on the Seamonkey "in" list
Whiteboard: [nsbeta2-]
this should be an easy one for someone to take up as a weekend project-- adding 
helpwanted and to nobody@mozilla.org.
Assignee: don → nobody
Keywords: helpwanted
this should be in.
Assignee: nobody → rjc
Keywords: helpwanted, nsbeta2 → 4xp, nsbeta3
Summary: [FEATURE] UI for FTP upload not implemented → UI for FTP upload not implemented
Whiteboard: [nsbeta2-] → nsbeta3+
Target Milestone: Future → M18
Whiteboard: nsbeta3+ → [nsbeta3+]
*** Bug 48548 has been marked as a duplicate of this bug. ***
adding dogfood keyword by way of bug 48548

------ Additional Comments From Frank Tang 2000-08-10 23:57 -------
mark this as dogfood since jar ask me to put it there for all the reason I don't 
use SeaMonkey.
Keywords: dogfood
Adding JohnG, and Michael to cc: list. PDT needs input from Marketing

a. This is a feature.
b. Wasn't this deliberately cut?
Putting on [dogfood-] radar since already [nsbeta3+].  I will bring this but to 
PDT today.
Whiteboard: [nsbeta3+] → [nsbeta3+][dogfood-]
This feature is cut!  Putting on [nsbeta3-] radar.
Whiteboard: [nsbeta3+][dogfood-] → [nsbeta3-][dogfood-]
adding helpwanted keyword
Keywords: helpwanted
Question:  I heard that the back end for this was already done and all was 
needed is the UI?  Bug 18977

I was sorry to see this cut after the publishing feature was also pushed back.

maybe an outside developer will step up.
backend is there, just need the UI.
Target Milestone: M18 → Future
hmmmm... Composer was going to rely on this as an alternative for Publish...

4.x not only allow user to drag & drop a file to a ftp directory for ftp upload, 
it also show a "File:Upload File..." if you open a ftp directory listing window. 
I know the drag&drop is hard to do, but at least can we do the "File:Upload 
File..."  ?
How can this be dogfood- ever? How can release team use SeaMonkey to upload the 
bits to the ftp staging server?
remove [dogfood-]. add rtm to keyword.
Keywords: rtm
Whiteboard: [nsbeta3-][dogfood-] → [nsbeta3-]
I don't think that the browser is the ftp upload tool-of-choice at this point.
Dogfood relates to day-to-day usage, and ftp upload does not show as a dogfood
issue.  My bet (wild guess) is that the release team isn't using a browser to do
their push operations anyway.
marking dogfood-minus.
This is a futured feature request, so I'm also adding beta3 and rtm minus.
Whiteboard: [nsbeta3-] → [nsbeta3-][dogfood-][nsbeta3-][rtm-]
I'm starting work on TranZilla, a project to bring 2 levels of FTP support to 
Mozilla. Level 1 support is basic FTP support. I'll be submitting this to 
Mozilla.org to become part of Navigator.
Level 2 support will be TranZilla, a full featured Cross platform FTP client 
that will intergrate with Mozilla and be available as a seprate download. The 
homepage for TranZilla is http://tranzilla.mozdev.org
nhotta:  this is the feature I was looking for.
adding shaver to the cc list.
I'm starting to work on this, but I can't get an output stream from the FTP
channel object:

getting output stream for channel: ###!!! ASSERTION:
nsFTPChannel::OpenOutputStream: 'Not Reached', file
/s/1/mozilla/netwerk/protocol/ftp/src/nsFTPChannel.cpp, line 300
###!!! Break: at file /s/1/mozilla/netwerk/protocol/ftp/src/nsFTPChannel.cpp,
line 300
FAILED: [Exception... "Component returned failure code: 0x80004001
(NS_ERROR_NOT_IMPLEMENTED) [nsIChannel.openOutputStream]"  nsresult: "0x80004001
(NS_ERROR_NOT_IMPLEMENTED)"  location: "JS frame ::
/s/1/mozilla/netwerk/test/TestFTPUpload.js :: upload :: line 23"  data: no]

Help me, Obi-wan!  I need to get my hands on an nsIScriptableInputStream so that
I can write to the FTP channel from JS.

(Alternatively, I'd like to know how to wire things up so that an FTP
channel/URL/connection/socket is the sink for a file
channel/URL/connection/transport's source, perhaps just by matching up stream
interfaces.)

woo.  bryner showed me the light, and I'm now shipping data from files to FTP.

I think I have JS code that will upload correctly, but the lack of an event
queue in xpcshell is cramping my style.  I'll take another swing this weekend
and see if I can't get it wedged into the browser.

It's not very hard at all, now that jud has done the heavy AsyncWrite lifting.
rjc, I'll take this, unless you object, and add it to my collection. (If someone
else wants to take my upload("file:///local/file.txt",
"ftp://host/dir/remote-file.txt") code and do the wedging, just mail me.)
Assignee: rjc → shaver
Target Milestone: Future → mozilla0.9
I've got it to the point where, with 9 statements of JS (plus error handling and
debug noise), I can kick off an FTP upload.  And then it crashes my M18 in
libnecko somewhere.

I'll try more tonight from home, where there is a debug build and the like, but
in case anyone else wants to take a swing at it, you should:

- unpack that tarball in $MOZILLA_FIVE_HOME/chrome
- add
      content,install,url,resource:/chrome/ftpupload/content/
  to installed-chrome.txt
- run regchrome
- visit chrome://ftpupload/content/ (the world's ugliest XUL!)
- enter a local file URL[*] in the left box
- enter an FTP URL, like ftp://localhost/pub/incoming/remote-name.txt, in the
  right box
- click the huge freaking button
- (crash)

[*] I think any URL will actually work, which is kinda neat, but let's start
with these baby steps.
Status: NEW → ASSIGNED
I know where it's crashing now.  But I'm not smart enough to decipher netwerk
and figure out what it means:

#0  nsOnStartRequestEvent::HandleEvent (this=0x8725d08) at
/s/1/mozilla/netwerk/base/src/nsAsyncStreamListener.cpp:210

That's this line:

rv = receiver->OnStartRequest(mChannel, mContext);

and receiver is null (we get it from mListener->GetReciever()).

It looks like the nsAsyncStreamListener is never having its init() method
called, which means that it never gets a receiver set.

Should I be calling init() myself?  Where do I get a handle to the
AsyncStreamListener?  What should be the receiver?  (Perhaps the fileObserver?)

For the busy, here's the pared down version of what I'm doing:

- get the IO service
- create channels for local file: and remote ftp: URIs
- get a stream listener (``streamListener'') for the remote ftp: channel
- get an input stream (``fileInputStream'') for the local file: channel
- set the transfer count on the ftp: channel
- get a stream observer for the file: channel (``fileObserver'')
- wire them all together:
  ftpChannel.asyncWrite(fileInputStream, fileObserver, null);

valeski? gagan? can you help me figure this out?

if you set NSPR_LOG_MODULES=nsFTPProtocol:5 in your env then try running your
upload, you'll get FTP command debug output which will help determine which
codepath you're going down. FTP shouldn't prompt asyncobserver usage (it has
it's own homegrown events for this) unless there was a problem negotiating the
protocol. If the log indicates protocol error, then something has changed since
I put the STOR code in (It used to work for me) and rjc or I will need to hammer
it out. If I have a chance today, I'll debug myself.

Doesn't look like it's even attempting to store:
3076[85ef128]: nsFtpConnectionThread::Process() started for 854c320 (spec
=ftp://localhost/pub/incoming/remote-foo.txt)
OK
3076[85ef128]: 854c320 Process() - READ_BUF - read "220 loonie FTP server
(Version wu-2.6.0(1) Mon Feb 28 10:30:36 EST 2000) ready.
" (81 bytes)
3076[85ef128]: 854c320 Process() - S_USER -
3076[85ef128]: 854c320 Writing "USER anonymous
"
3076[85ef128]: SUCCEEDED
3076[85ef128]: 854c320 Process() - READ_BUF - read "331 Guest login ok, send
your complete e-mail address as password.
" (68 bytes)
3076[85ef128]: 854c320 Process() - S_PASS -
3076[85ef128]: 854c320 Writing "PASS mozilla@
"
3076[85ef128]: SUCCEEDED
3076[85ef128]: 854c320 Process() - READ_BUF - read "230 Guest login ok, access
restrictions apply.
" (48 bytes)
3076[85ef128]: 854c320 Process() - S_SYST -
3076[85ef128]: 854c320 Writing "SYST

3076[85ef128]: SUCCEEDED
3076[85ef128]: 854c320 Process() - READ_BUF - read "215 UNIX Type: L8
" (19 bytes)
3076[85ef128]: 854c320 Process() - S_PWD -
3076[85ef128]: 854c320 Writing "PWD
"
3076[85ef128]: SUCCEEDED
3076[85ef128]: 854c320 Process() - READ_BUF - read "257 "/" is current directory.
" (31 bytes)
3076[85ef128]: 854c320 Process() - S_PASV -
3076[85ef128]: 854c320 Writing "PASV
" (addr is v4)
3076[85ef128]: SUCCEEDED
3076[85ef128]: 854c320 Process() - READ_BUF - read "227 Entering Passive Mode
(127,0,0,1,16,123)
" (46 bytes)
3076[85ef128]: nsFTPChannel::GetContentType() returned text/plain
3076[85ef128]: 854c320 Process() - S_MODE -
3076[85ef128]: 854c320 Writing "TYPE A
"
3076[85ef128]: SUCCEEDED
3076[85ef128]: 854c320 Process() - READ_BUF - read "200 Type set to A.
" (20 bytes)
3076[85ef128]: 854c320 Process() - S_CWD -
3076[85ef128]: 854c320 Writing "CWD /pub/incoming/remote-foo.txt
"
3076[85ef128]: SUCCEEDED
3076[85ef128]: 854c320 Process() - READ_BUF - read "550
/pub/incoming/remote-foo.txt: No such file or directory.
" (62 bytes)
3076[85ef128]: 854c320 Process() - S_RETR -
3076[85ef128]: 854c320 Writing "RETR /pub/incoming/remote-foo.txt
"
3076[85ef128]: SUCCEEDED
3076[85ef128]: 854c320 Process() - READ_BUF - read "550
/pub/incoming/remote-foo.txt: No such file or directory.
" (62 bytes)
3076[85ef128]: 854c320 Process() - ERROR
3076[85ef128]: nsFtpConnectionThread::Process() ended for 854c320 (spec
=ftp://localhost/pub/incoming/remote-foo.txt)

(I then descend into assertion hell.)

Is the asyncWrite call not enough to make it go into STOR mode?

I feared this... AsyncWrite() has bit-rotted. you're right, we never get to a
STOR, and yes, AsyncWrite() should be enough. we're trying to download the
_uploaded_ file name :-/.

rjc, can you debug this? I can't be counted on this week for code :-/.

I've seen the enemy, and it is R_cwd.
http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp#1135

If CWD fails -- which it always will if we're specifying the complete file path
for upload, not just the directory into which to put the file -- then we
unconditionally go into S_RETR.

What I propose:

If we're doing a PUT, and CWD _succeeds_, then the user has just specified the
directory, and we need to inherit the leafName from the originating URI.  Do we
have access to that at this point?

If it fails, then we have all the information we need, and we should just
proceed to STOR /pub/incoming/remote-foo.txt.

Make sense?

Jud and I decided that we don't even need to call CWD, because we're not
interested in guessing what to do when someone tries to upload to a directory.

So, we just go straight to STOR for the PUT case, and I have a patch for that
momentarily.  With _that_ patch, though, I start to blow assertions in
nsFileChannel::OnStartRequest:

###!!! ASSERTION: wrong thread calling this routine: 'mInitiator ==
PR_CurrentThread()', file
/s/1/mozilla/netwerk/protocol/file/src/nsFileChannel.cpp, line 630
###!!! Break: at file /s/1/mozilla/netwerk/protocol/file/src/nsFileChannel.cpp,
line 630
###!!! ASSERTION: No listener...: 'mRealListener', file
/s/1/mozilla/netwerk/protocol/file/src/nsFileChannel.cpp, line 632
###!!! Break: at file /s/1/mozilla/netwerk/protocol/file/src/nsFileChannel.cpp,
line 632

Thoughts?

Turns out you don't want the fileChannel to observe the asyncWrite -- it gets
very sad when its OnRequestStart is called from the ``wrong'' thread.

Looking for r= and sr= on my FTP-protocol patch.

ftp changes look good to me, file changes look scary, check w/ warren.

(Didn't you tell me to make those fileChannel changes? =) )

Adding warren for review.

no, I suggested removing the assertion in the FTP thread that was moaning about
someone's probably not receiving data.
Ah, so.  I'll make _those_ changes instead, and report back.

Sorry about that.
OS: Windows NT → All
*** Bug 66389 has been marked as a duplicate of this bug. ***
*** Bug 66389 has been marked as a duplicate of this bug. ***
Mike, please reassign back to you if you are going to fix this?
Assignee: shaver → dougt
Status: ASSIGNED → NEW
Chaning the qa contact on these bugs to me. MPT will be moving to the 
owner of this component shortly. I would like to thank him for all his hard 
work as he moves roles in mozilla.org...Yada, Yada, Yada...
QA Contact: mpt → zach
Well, I wish I could get to this, but alas I am doomed.  Target Milesone = Future.

Help always wanted.
Keywords: 4xp, dogfood, nsbeta3, rtm
Target Milestone: mozilla0.9 → Future
Doug, the editor team was relying on this for publish, is there any way you 
could do this within the next milestone?
I got stuck in FTP assertion land, but I'll try again if I get some time soon. 
I have high hopes for dougt's networking changes.

(And it might as well languish on my buglist, eh?)
Assignee: dougt → shaver
Target Milestone: Future → mozilla1.0
beppe, FTP should allow you to upload programatically.  This bug is in regard to 
a drag and drop(?) ui.

shaver, if you have problems with uploading, let me know quickly as I want this 
feature in 0.9 really bad!  :-)
right, our anticipation is to be able to drag to the destination location
Without fixing this bug, how is it possible for one to Upload a file to a FTP 
server? Also, when did the new HTML "Apache-style" FTP UI replace the tree 
widget?
Peter, my understanding is that Bradley Baetz is fixing the dir viewer to be
faster than before. In the meantime, they have switched over to the old-style
FTP listing. (cc bbaetz)
err, no I'm not :) I'm playing with the backend (see bug 78148), but the only
changes will be API level changes. I may end up getting the directoryviewer to
use <outliner>, but since ftp uses the html interface by default now, that won't
help.

The html interface is real html, which is produced by nsIndexedToHTML. (I have
plans to make it a bit nicer, and add a stylesheet in, but I suspect that since
it looks like an ftp uri, it won't have access to anything in chrome. Thats just
something that would be Nice - I don't really have plans to do it ATM)
the patches here are pretty outdated but may still be relevant to dougt

That last patch need 92837 to be fixed.  The patches are there.

What is left is (a) confirmation dialog, (b) a upload progress dialog, and (c) a
force refresh of the content area after uploader.  

Anyone care to take it from here?
*** Bug 97384 has been marked as a duplicate of this bug. ***
Depends on: 92837
Keywords: 4xp
*** Bug 105577 has been marked as a duplicate of this bug. ***
Will drang-n-drop[tm, us reg. pat. off.] be included as part of the UI?
*** Bug 107588 has been marked as a duplicate of this bug. ***
I've picked this up in my spare time and have a new DD patch (the one from
7/31/01 no longer works.)

This is my first major mozilla work, plus I'm only doing this in my spare time,
so I'm not entirely sure how long it'll take me to implement the confirmation
dialog, download dialog, etc.

In any case, if anyone is interested in seeing what I have so far, let me know.
 Otherwise, I'll just plug away at this quietly until I have something I like.
Thanks for the note here.  I'd love to see your progress on this.  I'd especially 
like to see us get a barebones version checked in (if it's not too late for 
that).

Mike Schuette--can you attach a patch when you have a chance?

Also, I presume that the above patches are now old and should be obsoleted.  If 
anyone has any objections to obsoleting the above patches, please speak now!
Attached patch Slightly revised DD patch (obsolete) — — Splinter Review 
This patch is pretty barebones, but it works.  Log into an ftp site, DD a file
over the content area.	Answer OK to a confirm() and the file sends, but no
upload progress dialog (yet.)  After the file is done, it refreshes the content
area.

Incomplete:
- Like I said, there's no progress dialog, hence no way to cancel an upload. 
The progress dialog will take quite a bit longer than this simple revision.
- The File menu should have an "Upload File ..." menu when we're on an ftp
site.  I'm not sure of how to make the File menu update when we reach an FTP
site, so any hints there would be helpful.
- We're still unaware of dragging a file unto a directory in the ftp view. 
Files are always uploaded to the current dir.  I've heard murmurs that the ftp
view is to be rewritten, so the drop location detection can be done after that.

- Unknown if this works with a multiple-file drop ...

Obviously much work to be done, but here's barebones for ya :)
Attachment #44161 - Attachment is obsolete: true
Attachment #17344 - Attachment is obsolete: true
Attachment #17268 - Attachment is obsolete: true
Attachment #17053 - Attachment is obsolete: true
I'm not going to be able to play with this until the weekend, but you can get
the current location by getting the URL attribute form teh dropped on node. Your
curent patch will accept ftp:// files as well as directories. See the drag code
in there for how that works.

This probably isn't the correct place for it, too - it should go into directory.js
You will need my patch in bug
http://bugzilla.mozilla.org/show_bug.cgi?id=128147
Attachment #57749 - Attachment is obsolete: true
Depends on: 128147
The last patch has dialog problems.  but it is a step in the right direction. :-)
Comment on attachment 71763 [details] [diff] [review]
better patch with uses the progress dialog

* contentAreaDD.js has strings which should be localizable:
+	     if (! confirm("Upload " + url + " to " + base + " ?")) return;

* uploadURI seems to have some tabs or different spacing?

* do we need an error dialog if nothing happens in uploadURI (in or after
catch)?

remove the dumps and r=brade  :-)
this patch isn't really ready to go in.  see the depend bug.  plus the dialog is
almost functionless right now.  this is still a work in progress.  Bill has alot
of work to do to make the progress dialog work correctly for both uploading and
downloading.

Comment on attachment 71763 [details] [diff] [review]
better patch with uses the progress dialog

brade's comment said "r=brade if...", but adding localization requires the
patch to be reviewed again, in my book.  (Don't r= if there are questions
outstanding the answer to which will affect whether the patch is acceptable or
not, please!)  Marking needs-work, to amplify dougt's assessment (with which I
heartily agree).
Attachment #71763 - Flags: needs-work+
removing myself from the cc list
This bug will be much happier in the hands of dougt, who will actually love it.
Assignee: shaver → dougt
sure, hand it to me two weeks before the deadline! :-)
Target Milestone: mozilla1.0 → mozilla1.0.1
Why is the status still "New" vs. "Assigned"?  It's only 2+ years old.
Whiteboard: [nsbeta3-][dogfood-][nsbeta3-][rtm-]
Target Milestone: mozilla1.0.1 → Future
*** Bug 155612 has been marked as a duplicate of this bug. ***
Keywords: mozilla1.2
it's crazy and sad that this is now the _only_ remaining reason i have to fire
up ie from time to time... any current status on this?
It is quite sad indeed.  The backend code is complete.  We just need a download
progress dialog.  Any XUL hackers?
Adding myself to the CC:.  I'm behind a socks proxy firewall, so I would LOVE to
be able to FTP upload files; assuming it will use Moz's proxy logic to do the
lifting . . . . Just wanted to chime in with why a body might need this feature.
Component: User Interface Design → Networking: FTP
nominating for Buffy.  Doug, would you like someone on Navigator to take this?
Keywords: nsbeta1
To peter for considered for the next release.
Assignee: dougt → trudelle
->varga
Assignee: trudelle → varga
Nav triage team: nsbeta1+/adt2
Keywords: nsbeta1 → nsbeta1+
Whiteboard: [adt2]
I talked to pinkerton, he says it should put into nsContentAreadDargDrop.cpp
instead of contentAreaDD.js
I'll try to port it
Status: NEW → ASSIGNED
Attached patch patch v0.1 (obsolete) — — Splinter Review 
I have problems with uploading. File is created on FTP server, but it has zero
size.
dougt, could you take a look ?
I got this assertion in debug build:
###!!! ASSERTION: ReadSegments: 'Not Reached', file
../../../../mozilla/netwerk/base/src/nsFileStreams.cpp, line 664
Break: at file ../../../../mozilla/netwerk/base/src/nsFileStreams.cpp, line 664

ok, someone wrote a nice comment in that method, so I did what he/she suggested
I wrapped input stream with buffered input stream and now it works!
*** Bug 180458 has been marked as a duplicate of this bug. ***
So how exactly is one supposed to upload files using mozilla ? Is this not
planned ? Is any other type of upload supported, eg. HTTP upload ?

Julien Pierre (comment 85)--as the summary of this bug suggests, the UI to
permit users to upload in the browser is not done.  I am unaware of any specs on
how this might work.  The 'backend' support is all there; Composer's upload UI
supports ftp, http, https, and local files.

Jan Varga--how goes your patch?  Is it ready for reviews?
It basically works, but progress dialog needs some fixes, see bug 128147 and bug
128119
Depends on: 128119
This should at least be in the nightly build. We call it nightly because some
things (read: experimental features) are in them which may not work correctly.

Then maybe some skinner who is disgusted can write an XUL skin on the progress
dialog.. or I can open a seperate bug called "FTP progress dialog needs
skinnning support". skinners will love that one.

But when it has to do with form over function.. I think we should do function
first. Netscape doesn't have to pick it up..but Mozilla's constantly in
development.. and we need full FTP support in the GUI. (heck I would like the
browser to popup a *username* and *password* dialog *before* connecting to an
FTP site.) That would be better support than NS4 had. Of course, this would be
only from the address bar. Links (for downloading) would be assumed to be anon
ftp. But at least have basic functionality in there for now. People should be
able to assume that if browser-->then FTP. (unfortunately, IE is part of the
problem. They make FTP optional.
I forgot to add the reason for my suggestions:

there is an improved patch.

Somebody should review it.. by testing it. It should be tested on a wide basis.

Also: historical question-did NS4 have a progress dialog?
jan, can you write up exactly what it would take to fix the depend bugs?  Maybe
someone can donate their time?
Target Milestone: Future → mozilla1.4alpha
No NS4 did not show upload progress afaik.
IE5 did, in the status window.
Can I offer any help testing / light coding here?
We need a help with fixing bug 128119 and 128147.
The rest is easy.

To correct Charlie, NS4 did have upload progress UI
Whiteboard: [adt2] → [adt2] [early 1.4beta]
Target Milestone: mozilla1.4alpha → mozilla1.4beta
Keywords: mozilla1.2
Whiteboard: [adt2] [early 1.4beta] → [adt2] [UI]
note for shuehan:
The input stream must be wrapped by an nsIBufferedInputStream to get it work.
Attached patch more recent patch (obsolete) — — Splinter Review 

        Attachment #104662 -
        Attachment is obsolete: true

    
    

    
  
reassigning
Assignee: varga → shliang
Status: ASSIGNED → NEW

    
    

    
  
speaking as a driver, I don't think we want this for 1.4 beta or final.

1)  we won't have progress without #128119

2)  without progress (or even with it), are we just open up a can of worms here?
 like, what if I quit while uploading, or close the window, etc.

3)  how common is upload?  for those who really need it, there are *plenty* of
other ftp "upload" clients.  (we're already a download client).

to me, this seems like something perfect for 1.5 / firebird, as an optional add on.

comments?

    
    

    
  
"There are plenty of other upload clients?"  Well hell, there are plenty of
other download clients too -- why not remove support for FTP entirely?

MacOS X doesn't come with any convenient GUI way to upload files via FTP.  You
either have to use the shell, or you have to discover, download, and
periodically re-install nagware like Fetch.  The last time my girlfriend was
doing the "uninstall and reinstall Fetch so that it will work again" dance, I
said "just use Mozilla to do it".  Oops, this feature that Netscape had since
1.0 has been removed.  Then it turned out that she didn't have the "BSD
Subsystem" installed, so no command-line FTP either.

This is a common situation.  Most people publish to their web sites via FTP. 
Mozilla should make this easy, like it used to in the past.


    
    

    
  
I'm not saying don't add the feature, I'm saying add it once it works.

if there is no progress, how will I know when the upload is done?

also, is it hooked up properly to stop?

    
    

    
  
I thought when i handed it over to peter this feature was going go be worked on. :-/

i think the more important point is that no one is working on this right now. 
If someone had a patch ready to go, of course we would try to get it in for 1.4.  

we totally dropped the ball on the uploading functionality in mozilla.  

shliang, if you are not planning to fix this soon, please reassign it to me.   

    
    

    
  
I was in the middle of adding my comment but Jamie was quicker.

1) If 128119 can't be resolved on time for 1.4 , could a simpler progress dialog
be implemented, ie. one which just goes from 0 to 100 when completed, and allows
stopping ?

2) - if you quit while uploading, the behavior should be similar to quitting
when downloading
- if the user closes the window, mozilla should probably prompt about whether
the user intends to stop the upload

3) Yes, there are plenty of other upload clients, however Mozilla integrates
Composer and it is extremely inconvenient to have to invoke another client to
upload the file. Even if there is no progress status and there are some quirks,
I still think the feature should be implemented. I don't know what the timeframe
is for 1.5, but maybe 1.4 could implement it to the point of being barely
usable, and 1.5 can fix the quirks.


    
    

    
  
I don't want to see this go in until the UI is done.  It just seems busted
without any kind of feedback, and I'm sure that we would get a lot of bogus bug
reports as a result.  If someone finishes the progress bits, I would take this
in 1.4.  It's important functionality.

    
    

    
  
> 3) Yes, there are plenty of other upload clients, however Mozilla integrates
> Composer and it is extremely inconvenient to have to invoke another client to
> upload the file

Doesn't composer already do FTP-based publish?

(ashamed to show my face in this bug again)

    
    

    
  
bliz: what if we get it without the UI, and disable it with a hidden pref, so
that people can still bail themselves out with about:config -- and give us
feedback/bug reports, once they've leapt that barrier to entry -- but my mom
doesn't stumble into it?

    
    

    
  
> Doesn't composer already do FTP-based publish?

Yes.

In fact, if some budding XUL hacker wanted to take on the UI part of this
feature, they might find some useful code to copy in editor/ui/dialogs/.

> what if we get it without the UI, and disable it with a hidden pref

That would be awfully useful.  It would also make it easier to land the feature,
since the backend and the UI will probably need review by different sets of
people and will have different issues.

    
    

    
  
ADT: Nominating topembed
Keywords: topembed

    
    

    
  
just to clarify, shuehan showed me what the current patch does:

1)  upload, and in the status text we get:  "uploading xxx..." (it doesn't clear
when done)
2)  the stop button isn't enabled, so clicking on it wouldn't stop
3)  the page doesn't refresh when the upload finishes (so you won't know it is done)
4)  the throbber doesn't throb.
5)  no progress updates to the progress meter / status text.

    
    

    
  
note to who ever works on this.

what happens happen if you try upload a big file, and then closed the window
(since you didn't know it was done), or hit refresh or browsed somewhere else
before it was finished.

would we cancel the url (or load group)?

    
    

    
  
Can we just stick the upload-progress UI in the download manager?  I won't shed
a tear about the mildly-bogus naming, and it would make a lot of these issues
just go away.

    
  
Keywords: nsbeta1+ → nsbeta1

    
    

    
  
adt: nsbeta1-
Keywords: nsbeta1 → nsbeta1-
Whiteboard: [adt2] [UI]

    
    

    
  
topembed-
Keywords: topembed → topembed-

    
    

    
  
*** Bug 208385 has been marked as a duplicate of this bug. ***

    
    

    
  
Just got an email from someone who recently switched from 4.x to Mozilla 1.4 (as
I recommended) and who was complaining that the File Upload menu entry is
missing.  I couldn't believe it, but yes, he was right! Is someone working on
it? Could someone let me know what the current status is? Thanks.

    
    

    
  
*** Bug 217015 has been marked as a duplicate of this bug. ***

    
    

    
  
We just moved the whole institute from NS4.8 to Mozilla, and a couple of people
started to complain about the missing ftp upload menu entry (for general files,
not the composer functionality). I saw that upload is ready in the networking
code, but the UI is still not ready after more than 3 and a half year...
Is there anything hidden which can be activated by a preference ? Any patch
or optional package to try out ? This bug should be raised in priority, since
when we loose composer we also loose the limited functionality for uploading
http files. Since it seems that everything is almost there, it couldn't be that
hard to upload the patches ? Is there a major obstacle why this bug can't be fixed
now ?

    
    

    
  
the biggest hurdle is time.  :-)

the last patch seams to work for mozilla.  we are just missing a progress
dialog. However, for firebird client, it will not work - my front end changes
were in the mozilla specific xul code.  I think we need to move this kind of
functinality into the drag/drop code in gecko so that all clients can use it.  


    
    

    
  
Since there are some people who just can't wait, I decided to write my own
rudimentary piece of code to have a simple workaround until this is bug is fixed.
The upload code is already running (it currently is a html/javascript page that
calls the respective xpfe functions), I only need to put an 'Upload File' entry 
in the menu that directs the current page's URL as an argument to a js script. Can
somebody show me how to do this (a simple bookmark entry that takes the current
page as an argument will also do) ? If it works, I will publish that small tool
for the unpatient.

    
    

    
  


  
Here is a simple workaround without patching anything: Place the attached HTML
somewhere, bookmark it. Whenever you want to upload, login to the upload 
directory, then load this file via the bookmark. Please post corrections and
change what you like. I'm not a javascript or xpfe programmer, in fact, the
code
was created by copying a couple of lines from different broken scripts seen in
the newsgroups, just leaving out the broken parts...

    
  
Blocks: majorbugs

    
  
Target Milestone: mozilla1.4beta → ---

    
    

    
  
*** Bug 225670 has been marked as a duplicate of this bug. ***

    
    

    
  
It would be very wonderfull if after a ftp logon, we can see remote disk like a
local disk.
So other than drag&drop, I like using copy&paste, rename file/dirs, and upload
file and complete dirs tree.
Today is very common find a work PC (Win32 o Linux) or Sun workstation Solaris,
with Moz installed ufficially from administrator. So to have ftp complete client
integrated in Moz, maybe the only chanche to pass the company firewall and logon
on remote NON anonimous Ftp server to update Web site.
I propose this feature become blocking for 1.6
thanks, Valerio

I like to have listing of remote file property right and owner too (drwxrwxrwx)
RFE new bug?


    
  
Flags: blocking1.6?

    
  
Flags: blocking1.6? → blocking1.6-

    
    

    
  
I have some idea to share for this feature.
I think for xpfe and firebird, upload when drag and drop is enough since most
end users only use browser to browse web page and download. We don't have to do
much for everyone. For those users who want to use full feature ftp in browser,
I think comment 120 is good. We can make it as an extension for both xpfe and
firebird.
I'm working on making the drag and drop ftp upload for Mozilla. I'm going to do
it based on existing patch and enhance it. I'm not sure a process dialog is
suitable. I think the status on status could be better. Just like when loading
page, we display the process on status bar, I think it's enough for end user to
know the progress. And the stop button should work when uploading. Means when
user click "Stop", the transfer should stop.

    
    

    
  
I strongly disagree with comments #120 and #121. Why not keep things S-I-M-P-L-E
and offer the same functionality that was present in the old Netscape
Communicator 4.8 and previous versions?

That is, ONLY WHEN THE USER is already logged into a ftp:// url, display under
File->Upload File.

Once the user selects a file from the local disk the upload should begin but the
transfer status should be displayed on a detached PROGRESS DIALOG, just like
when the user downloads a file (and the Download Manager is not enabled).

Until the ftp transfer is done, the user shouldn't be able to cotinue moving
thru the ftp tree. Why? Because some FTP sites limit the number of simultaneous
connections from a single user/password or IP address. Letting the user continue
moving thru the ftp site while a ftp upload is in progress would make the
browser attempt to establish a secondary (or third, or fourth, depending on the
number of simultaneous uploads already in place) connection to the ftp site,
which more often than not will result in "you're already logged in" error
messages from the ftp server.

Of course this behaviour could be tweaked with a "max. number of simultaneous
connections for ftp uploads" preference.

Just my $0.02. I'd like to hear comments on these suggestions (just bring back
the old working Communicator 4.8 approach!).

    
    

    
  
I strongly disagree with comments #120 and #121. Why not keep things S-I-M-P-L-E
and offer the same functionality that was present in the old Netscape
Communicator 4.8 and previous versions?

That is, ONLY WHEN THE USER is already logged into a ftp:// url, display under
File->Upload File.

Once the user selects a file from the local disk the upload should begin but the
transfer status should be displayed on a detached PROGRESS DIALOG, just like
when the user downloads a file (and the Download Manager is not enabled).

Until the ftp transfer is done, the user shouldn't be able to cotinue moving
thru the ftp tree. Why? Because some FTP sites limit the number of simultaneous
connections from a single user/password or IP address. Letting the user continue
moving thru the ftp site while a ftp upload is in progress would make the
browser attempt to establish a secondary (or third, or fourth, depending on the
number of simultaneous uploads already in place) connection to the ftp site,
which more often than not will result in "you're already logged in" error
messages from the ftp server.

Of course this behaviour could be tweaked with a "max. number of simultaneous
connections for ftp uploads" preference.

Just my $0.02. I'd like to hear comments on these suggestions (just bring back
the old working Communicator 4.8 approach!).

    
    

    
  
I think comments #123 is reasonable. Before all browser is a browser. if user 
wants more complicated ftp functions, more powerful ftp clients are suggested.
So I think SIMPLE function is enough.
is uploading a directory necessary?

    
    

    
  
-> me
Assignee: shliang → darin

    
    

    
  
nominating for 1.7b, but we would need to move quickly if it is going to make
the 1.7b string change and final deadline.. 
Flags: blocking1.7b?

    
  
Severity: normal → enhancement
Status: NEW → ASSIGNED
Priority: P3 → P1
Target Milestone: --- → mozilla1.7beta

    
    

    
  
darin,
Are you working on this? Actually, dantifer is also working on this and we will
get a patch very soon. Including a progress doalog(reuse the nsIProgressDialog
to show the progress and support multiple file selection.

    
    

    
  
I'm going to put the File Upload UI into 1.7b so that we can get it for 1.7
final (need the DTD in for translation freeze)

Is there a strong opinion as to where on the Upload File menu in SeaMonkey it
should be?

I was thinking between edit page and page setup with separators around it.

Here's what 4.x looked like:

File

New
Open Page
----------
Save As
Save Frame As
----------
Send Page
Edit Page
Edit Frame
Upload File
Offline
----------
Page Setup
Print Preview
Print
----------
Close
Exit


    
    

    
  
Now i am working on ftp UI. I have got some ideas. 

1. user can upload 
  (1). single file
  (2). multiple files. 
  directory is not allowed

2. user can use three methods to upload file(s).

  (1). Menu : File -> Upload Files ...
    ...
    ----------
    Edit Page
    ----------
    Upload Files ...
    ----------
    ...
    This menu entry is dynamically enabled or disabled.

  (2). Context menu: Upload Files ...

    Upload Files...
    ----------
    Back
    ...
    This menu entry is dynamically shown or hidden.

  (3). Drap & Drop
	While browsing ftp , multiple files can be droped into contentArea.


3. a progressdialog like dialog is shown while uploading.

  +-------------------------------------------------------------+
  |Uploading : [File name] ([Current number] of [Total number]) |
  +-------------------------------------------------------------+
  |                                                             |
  |   Upload From: [Local directory]/File name]                 |
  |          To  : [Network directory]/[File name]              |
  |        Status: [Uploaded size] of [Total size] (at [Speed]) |
  |     Time left: [hh:mm:ss]                                   |
  | Time Elsapsed: [hh:mm:ss]                                   |
  |      Progress: []]]]]]]]]]]]]]]]]]]]]]]]       ] [Percent]% |
  |                                                             |
  |   +--------+                                                |
  |   | Cancel |                                                |
  |   +--------+                                                |
  +-------------------------------------------------------------+

4. after uploading , refresh

    
    

    
  
Would it be possible to add a DELE request to context menu as well?

    
    

    
  
The first version of this should be "as simple as possible."  I'm thinking "File
-> Upload File" a'la Nav4x, and that's it.

D&D could be nice, but I'm not sure I like the fact that it would change the
current D&D semantics.  I personally like being able to drag a file onto
Mozilla, and have it try to load the file into the browser window.  I'm not
saying that we shouldn't consider it, but for the first version of this patch I
think it should be excluded.

    
    

    
  
(In reply to comment #131)
> D&D could be nice, but I'm not sure I like the fact that it would change the
> current D&D semantics.

... And end up with a confusing IE like UI where the behavior changes depending
on if you're logged in or not. But then it could be user pref.
Just having menu items for Upload, Download and Delete (if reasonably possible)
would provide basic file transfer functionality for not only web publishing, but
as an alternative to sending email attachments as well.




    
    

    
  
> ... And end up with a confusing IE like UI where the behavior changes 
depending
> on if you're logged in or not. But then it could be user pref.

On the contrary, I think it is a step forward :)

    
    

    
  
(In reply to comment #132)

> ... a confusing IE like UI where the behavior changes depending
> on if you're logged in or not.

should be
... a confusing IE like UI where its behavior changes depending on if IT THINKS
you're logged in or not.

If you think you are logged in and you're not, it's nice to have a clue.


    
    

    
  
(In reply to comment #131)
>I'm not
> saying that we shouldn't consider it, but for the first version of this patch I
> think it should be excluded.
I agree, We can consider it later to add a user pref to change behavior for ftp
window.

    
    

    
  
Why not add a box somewhere within the content area where the user can drag
a file to upload it?  Is that behavior good enough for waiting for this fix
and filing a new bug?

    
    

    
  
I agree with plan A. On the contrary, with Set Access And Defaults charging us
with handling FTP, (even though we really shouldn't, and Firefox no longer
steals it), we should have basic upload functionality.  In terms of ease of use,
here are my personal priorities.
1. Emulate the IE behavior except do it better: when I type
ftp://ftp.someplace.com, ask for login information before contacting the server,
with a "I don't have one" button that logs people in anonymously.

IE will try connecting as anonymous automatically, which makes the server logs
deceptive and useless.

2. Implement wizards or "guides". see www.cnet.com's new video on the next
version of Internet Explorer. "guides" are security related information that
attaches to the navigation bar.. fully implementable in DOM. "Mozilla thinks you
need to login to this site." A guide could then be used to say "To send a file
to this server, Choose Upload File.. from the File menu."

3. Drag and Drop. I'll admit that every FTP client has it so it is weird that
Mozilla doesn't, and now that Netscape is not producing new versions, being 4xp
is irrelevant.

Once this feature is implemented, more user feedback can be gathered. But I
changed my mind--adding drag and drop first receives no objection. But from what
I understand, it isn't universally accepted by the community.

But I will point out, to a novice user, dragging and dropping to view while
browsing an ftp server, is very strange. A prompt certainly wouldn't be out of
the question.

    
    

    
  
We missed the deadline for beta freeze. I don't think this is going in.

    
    

    
  
let's shoot for 1.8a
Flags: blocking1.7b? → blocking1.7b-

    
    

    
  


  

    
    

    
  
Comment on attachment 144098 [details] [diff] [review]
Implement FTP upload UI for mozilla

darin, please take a look at this patch. Thanks!

        Attachment #144098 -
        Flags: superreview?(darin)

    
    

    
  
Comment on attachment 144098 [details] [diff] [review]
Implement FTP upload UI for mozilla

>     if (storStr.First() == '/')
>     {
>         // kill the first slash since we want to be relative to CWD.
>-        storStr.Cut(0,1);
>+//        storStr.Cut(0,1);
>     }
I made this modification for it causes problems while uploading.

we can use ftpChannel or nsIWebBrowserPersist to upload files.
the destination file is represented by a URI .
But after you login into the ftp server and you navigate to a certain 
directory, you still use the same URI will cause error.

the reason is ftpChannel or nsIWebBrowserPersist use browser connection cached,

for example:
destination file's URI is "ftp://ftp.mycom.com/incoming/test.txt"
you browser current location is "ftp://ftp.mycom.com/another/"
then you use that URI to upload file, ftpConnectionThread misunderstanding
the destination file as "ftp://ftp.mycom.com/another/incoming/test.txt"

I made this modification just for FTP uploading. But I afraid simply remove
a line will cause other problems.

    
    

    
  
i think you're supposed to use:
"ftp://ftp.mycom.com//incoming/test.txt"

    
    

    
  
Comment on attachment 144098 [details] [diff] [review]
Implement FTP upload UI for mozilla

hmm, I don't really like your nsIProgressDialog changes... Why are you
implementing nsIAuthPrompt etc on it? Why don't you set something else as the
notificationCallbacks?

bah, I had written a paragraph here how I'd like nsIProgressDialog to not care
whether its up- or downloading. then I realized it inherits from nsIDownload.

But can you rename uploadFlag to uploading?

+	     this.mUploadOperation == null;
this is a no-op. is it needed?

+	 if( this.mUploadFlag ){
+	     if( this.percent == 100 )
eww, this is such a hack. why is it needed, doesn't nsIWebProgressListener's
API offer a better way? onStateChange with STATE_STOP or something?

+    // ---------- nsIFTPEventSink methods ----------
again, why does this have to be implemented on this object?

+	 if( this.mUploadFlag )
+	     this.parent.DelayedBrowserReload(1000);
can you explain? This is in oncancel, right?

			 if ( this.target ) {
-			     this.enable( "launch" );
+			    if( ! this.mUploadFlag)

please use a single if here

+		 if ( this.mUploadFlag )
+		    this.parent.DelayedBrowserReload(1000);
why delayed? and why only if the dialog is set to auto-close?

-<!ENTITY defaultTitle	   "Saving">
+<!ENTITY defaultTitle	   "Transfer">
+<!ENTITY defaultTitleUpload	 "Uploading">

Can you use a single entity "Transferring"?

+<!ENTITY uploadingTitle      "#2&#037; of #1 Uploaded ( #3 of #4 )"> 

please remove the spaces before #3 and after #4

+<!ENTITY uploadingSource     "Upload To:"> 

"Uploading To"

+<!ENTITY uploadingTarget     "From:"> 

a _target_ has a label of "From"?

 <!ENTITY pausedMsg "Download Paused">
+<!ENTITY pausedUploadMsg "Upload Paused">

is either of these entities used?

 <!ENTITY completeMsg "Finished, #2 KB downloaded (elapsed time was #1)">
+<!ENTITY completeUploadMsg "Finished, #2 KB uploaded (elapsed time was #1)">
can you use a single entity here and use "transferred"?




(only looked at embedding/components/ui/progressDlg)

    
    

    
  
(In reply to comment #144)
> (From update of attachment 144098 [details] [diff] [review])
> hmm, I don't really like your nsIProgressDialog changes... Why are you
In fact, I agree with you. now nsIProgressDialog is really bad. I think 
it is better to create a new dialog. But what should we call the new dialog?
uploadProgressdialog? nsIProgressDialog can just be used for downloading ,he
called himself progressdialog...

> implementing nsIAuthPrompt etc on it? Why don't you set something else as the
> notificationCallbacks?
> 
> bah, I had written a paragraph here how I'd like nsIProgressDialog to not care
> whether its up- or downloading. then I realized it inherits from nsIDownload.
> 
> But can you rename uploadFlag to uploading?
> 
> +	     this.mUploadOperation == null;
> this is a no-op. is it needed?
yes, it is necessary to cancel upload.

> 
> +	 if( this.mUploadFlag ){
> +	     if( this.percent == 100 )
> eww, this is such a hack. why is it needed, doesn't nsIWebProgressListener's
> API offer a better way? onStateChange with STATE_STOP or something?
onStateChange is never used for uploading. but onStatus is used.
what make me to do such a stuid modification is: sometime, onStatus is not
alwayls called after transer is completed.(?)

> 
> +    // ---------- nsIFTPEventSink methods ----------
> again, why does this have to be implemented on this object?
> 
> +	 if( this.mUploadFlag )
> +	     this.parent.DelayedBrowserReload(1000);
> can you explain? This is in oncancel, right?
yes, this is in oncancel. you click cancel to cancel the rest uploading
jobs. but perhaps you have already uploaded some files....

> 
> 			 if ( this.target ) {
> -			     this.enable( "launch" );
> +			    if( ! this.mUploadFlag)
> 
> please use a single if here
ok, good suggest.

> 
> +		 if ( this.mUploadFlag )
> +		    this.parent.DelayedBrowserReload(1000);
> why delayed? and why only if the dialog is set to auto-close?
you can not understand this only if you tested the code yourself.
ftp connection needs time to clean up. if the ftp server limits its
connection number from one ip, then you got the error information:
"you can not login more than one connection." that's the reason.

> 
> -<!ENTITY defaultTitle	   "Saving">
> +<!ENTITY defaultTitle	   "Transfer">
> +<!ENTITY defaultTitleUpload	 "Uploading">
> 
> Can you use a single entity "Transferring"?
> 
when we don't know the exact direction , to use transfering is the only
choice. but after you know the direction, why not use 'saving'/'uploading' ?


> +<!ENTITY uploadingTitle      "#2&#037; of #1 Uploaded ( #3 of #4 )"> 
> 
> please remove the spaces before #3 and after #4
> 
ok.

> +<!ENTITY uploadingSource     "Upload To:"> 
> 
> "Uploading To"
ok.

> 
> +<!ENTITY uploadingTarget     "From:"> 
> 
> a _target_ has a label of "From"?
ok.

> 
>  <!ENTITY pausedMsg "Download Paused">
> +<!ENTITY pausedUploadMsg "Upload Paused">
> 
> is either of these entities used?
> 
i admit it is my mistake. i should remove pauseUploadMSg for it is never used.

>  <!ENTITY completeMsg "Finished, #2 KB downloaded (elapsed time was #1)">
> +<!ENTITY completeUploadMsg "Finished, #2 KB uploaded (elapsed time was #1)">
> can you use a single entity here and use "transferred"?
> 
same as "transfering"

> 
> 
> 
> (only looked at embedding/components/ui/progressDlg)
> 

now I feel it is really bad to try to reuse nsIProgressDialog. the same feeling
with you :)

    
    

    
  
> 
> +<!ENTITY uploadingTarget     "From:"> 
> 
> a _target_ has a label of "From"?

I have not explained this clearly.

target for uploading should be the URI, source for uploading should be URI or 
nsIlocalFile.

but nsIProgressDialog is quite amazing. the target is nsILocalFile, if you use
URI instead, crash ...

So I exchange them. I pass source(nsIlocalFIle) as target(pass) and target(URI)
as source(pass), labels are also changed.

aha, what a funny job.

    
    

    
  
darin, have you seen the patch? should I use a new progressdialog instead of
reusing the old one? 
waitting to get your suggestion ...

    
    

    
  
(In reply to comment #145)
> > +	     this.mUploadOperation == null;
> > this is a no-op. is it needed?
> yes, it is necessary to cancel upload.

I find that hard to believe. As written, that line does _nothing_ _at_ _all_.
(I think you want a single equals sign)

> > why delayed? and why only if the dialog is set to auto-close?
> you can not understand this only if you tested the code yourself.
> ftp connection needs time to clean up. if the ftp server limits its
> connection number from one ip, then you got the error information:
> "you can not login more than one connection." that's the reason.

That rather seems like a bug in the FTP implementation of mozilla, it would be
good to report that bug.

> > -<!ENTITY defaultTitle	   "Saving">
> > +<!ENTITY defaultTitle	   "Transfer">
> > +<!ENTITY defaultTitleUpload	 "Uploading">
> > 
> > Can you use a single entity "Transferring"?
> > 
> when we don't know the exact direction , to use transfering is the only
> choice. but after you know the direction, why not use 'saving'/'uploading' ?

I don't understand. Why are you changing "Saving" to "Transfer", then?

> now I feel it is really bad to try to reuse nsIProgressDialog. the same feeling
> with you :)

well, _my_ feeling is really that the nsIProgressDialog API should be more
generic, and the impl should suck less. ah well :)

I wish you had commented on why nsIAuthPrompt needs to implemented by the
progressdialog.


    
    

    
  
> I find that hard to believe. As written, that line does _nothing_ _at_ _all_.
> (I think you want a single equals sign)
:) I have to admit that I was wrong here.
> 
> > > why delayed? and why only if the dialog is set to auto-close?
> > you can not understand this only if you tested the code yourself.
> > ftp connection needs time to clean up. if the ftp server limits its
> > connection number from one ip, then you got the error information:
> > "you can not login more than one connection." that's the reason.
> 
> That rather seems like a bug in the FTP implementation of mozilla, it would be
> good to report that bug.
OK, I will do it right now.
> 
> > > -<!ENTITY defaultTitle	   "Saving">
> > > +<!ENTITY defaultTitle	   "Transfer">
> > > +<!ENTITY defaultTitleUpload	 "Uploading">
> > > 
> > > Can you use a single entity "Transferring"?
> > > 
> > when we don't know the exact direction , to use transfering is the only
> > choice. but after you know the direction, why not use 'saving'/'uploading' ?
> 
> I don't understand. Why are you changing "Saving" to "Transfer", then?
> 
> > now I feel it is really bad to try to reuse nsIProgressDialog. the same 
feeling
> > with you :)
> 
> well, _my_ feeling is really that the nsIProgressDialog API should be more
> generic, and the impl should suck less. ah well :)
OK, we have wasted too much time here. Now I am waiting for comments from
darin, and preparing to implement a new progressdialog.

> 
> I wish you had commented on why nsIAuthPrompt needs to implemented by the
> progressdialog.
> 
object for that callback requires this interface.
is there anyway to avoid this? I am a newcomer here, just want to do someting 
for mozilla, I don't know much...


    
    

    
  
> object for that callback requires this interface.

the object that you set as the notificationCallbacks on the channel needs to
implement this. correct. However, I'd rather a differnet object be set as that.

See, the main reason why I dislike the progressdialog implementing this is that
for downloads, it is not implemented here either. I'm not sure if anything
implementes it for the "save link target as" case, but it wouldn't be on the
progressdialog anyway because if dl manager is used, the progress dialog is not
involved at all.

    
    

    
  
> See, the main reason why I dislike the progressdialog implementing this is 
that
> for downloads, it is not implemented here either. I'm not sure if anything
> implementes it for the "save link target as" case, but it wouldn't be on the
> progressdialog anyway because if dl manager is used, the progress dialog is 
not
> involved at all.
OK, perhaps we should use a new progressdialog.


    
  

        Attachment #144098 -
        Attachment is obsolete: true

    
  
Target Milestone: mozilla1.7beta → mozilla1.7final

    
    

    
  


  
according to Darin's response, I modify the above patch. This uploading part
can be used to upload files to webdav server, too.

        Attachment #145411 -
        Attachment is obsolete: true

    
  

        Attachment #144098 -
        Flags: superreview?(darin)

    
    

    
  
Comment on attachment 145537 [details] [diff] [review]
modify the uploading method and dialog. now it can upload files both to ftp server and webdav server.

Darin, please take time to have a look.

        Attachment #145537 -
        Flags: superreview?

    
    

    
  
Comment on attachment 145537 [details] [diff] [review]
modify the uploading method and dialog. now it can upload files both to ftp server and webdav server.

Darin, please take time to have a look.

        Attachment #145537 -
        Flags: superreview?(darin)

    
  

        Attachment #145537 -
        Flags: superreview?

    
    

    
  
your change to netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp will break
some servers.

    
    

    
  
ZF.Dang:  FYI, I am currently working on a revised patch for this bug.  I will
be posting something here shortly.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v2 patch (obsolete)
        — 
          — Splinter Review
      

    


  
This patch moves most of the methods from nsIDownload onto a new interface
called nsITransfer.  nsIDownload now inherits from nsITransfer instead, and
this allows the nsIProgressDialog to inherit from nsITransfer instead of
nsIDownload.  As a result the progress dialog code can be reused with only a
few modifications.

Unlike the current nsIDownload, nsITransfer uses nsIURIs for both the source
and target of the transfer.  In the download case the transfer is to a file URI
(or some other URI scheme that maps to nsIFile).  nsWebBrowserPersist is
already able to accept a nsIURI for the target, so there is not much of a
paradigm change here.

Much of the changes to this patch result from the nsIDownload interface change.


The result of this patch is a new file menu, labeled "Upload File", that is
enabled whenever a FTP directory listing is selected into the browser frame.

It would be good to add drag-n-drop and/or a context menu in the future, but
this is at least a start.

        Attachment #71763 -
        Attachment is obsolete: true

        Attachment #122399 -
        Attachment is obsolete: true

        Attachment #145537 -
        Attachment is obsolete: true

    
    

    
  
I have tested this patch, and found two problems.

1.the first is uploading path. And that is the reason why I modify 
nsFtpConnectionThread.cpp file.
in my test,I open ftp://xxx/incoming/, refresh it, then upload yuquan03.mp3.
it failed to upload. log from ftp server is:
--refresh
[2] Thu 08Apr04 16:34:53 - (000052) CWD /incoming/
[6] Thu 08Apr04 16:34:53 - (000052) 250 Directory changed to /incoming
[2] Thu 08Apr04 16:34:53 - (000052) LIST
[6] Thu 08Apr04 16:34:53 - (000052) 150 Opening ASCII mode data connection 
for /bin/ls.
[6] Thu 08Apr04 16:34:53 - (000052) 226 Transfer complete.
--connection remains.
--uploading
[2] Thu 08Apr04 16:35:04 - (000052) PASV
[6] Thu 08Apr04 16:35:04 - (000052) 227 Entering Passive Mode 
(129,158,217,140,42,211)
[2] Thu 08Apr04 16:35:04 - (000052) STOR incoming/yuquan02.mp3
[6] Thu 08Apr04 16:35:04 - (000052) 550 yuquan02.mp3: Cannot create file.
--closing connection
[5] Thu 08Apr04 16:35:05 - (000052) Closing connection for user MOZILLA 
(00:01:01 connected)

this patch try to upload files to ftp://xxx/incoming/incoming/yuquan02.mp3
But after this failure, I upload the file again, it succeeded. For after the 
above upload operation, ftp connection is closed;mozilla create a new thread 
for uploading.

2. second. nsWebbrowserPersist does not pass correct uploading progress to 
progress dialog. actually, progress dialog shows two progress, the first is 
very fast, so I think perhaps it indicates speed of reading disk file. If you 
upload big files, you can see this problem.

I have files these two bugs before: bugid are 238847 & 238848. you can get 
detailed information there.

and what is more, we can delay other more convenient ways, such as DD && 
context menu. But I think the limitation to upload only one file is hard to 
accept. who will use this crippled uploading function? 

    
    

    
  
Should we updated the this bug by adding this bug depend on 238847 & 238848 at
the top as stated from the recent comments?

    
  
Depends on: 238848

    
  
Depends on: 238847

    
    

    
  
ZF.Dang:

thank you for pointing out those problems.  i can reproduce the URL generation
problem on my end.  as for the progress dialog problem, i think i have a
solution that will work for that.  when the progress dialog believes it is
uploading, it can simply ignore status/progress messages related to anything
other than the target URI.  see my comments in the other bugs.  marking those as
blocking this bug.

    
    

    
  
> and what is more, we can delay other more convenient ways, such as DD && 
> context menu. But I think the limitation to upload only one file is hard to 
> accept. who will use this crippled uploading function? 

I see this patch as only the first step in providing uploading capabilities in
the browser.  (I personally don't believe that this will be any kind of killer
feature for the product.  I don't think many users need to upload anything, and
those that do probably have other tools they like to use.)

In my mind, this patch is about improving the Mozilla platform as much as it is
about providing Netscape 4x parity.  This patch will help us if we ever want to
use GNOME 2.6's new file selection dialog, which enables the user to select
gnome-vfs locations (smb://, sftp://, etc.) as the destination to save a file
to.  So, uploading is a natural piece of that puzzle :-)

    
    

    
  
(In reply to comment #161)
> thank you for pointing out those problems.  i can reproduce the URL generation
> problem on my end.  as for the progress dialog problem, i think i have a
> solution that will work for that.  when the progress dialog believes it is
> uploading, it can simply ignore status/progress messages related to anything
> other than the target URI.  see my comments in the other bugs.  marking those as
> blocking this bug.
At first, I use nsFtpChannel to upload files. But I find it can not work with
the current progress dialog. Then I try to use nsWebbrowserPersist. But I can
not solve the problem of wrong progress. Finallly, I pick up nsFtpChannel again
and modify progress dialog a lot. That is the reason.

yeah, D&D, context menu, multiple files are up to you.

BTW:Now my patch can show correct progress, context menu,multiple files
selecting, D&D (I remove it before posting), URI (this is obviously not enough,
for I simply remove a line in nsFtpConnectionThread.cpp)


    
    

    
  

      
      
      
      

        Attached patch
          
          
        v2.1 patch (obsolete)
        — 
          — Splinter Review
      

    


  
This patch addresses the progress problem by making nsWebBrowserPersist only
report upload progress if the target file is non-local.

        Attachment #145639 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 145940 [details] [diff] [review]
v2.1 patch

notes on this patch:

- nsITransfer introduced as a superclass of nsIDownload.  nsIProgressDialog now
inherits from nsITransfer instead of nsIDownload.  this allows
nsIProgressDialog to be used more easily to represent upload progress.

- "target" property in nsIDownload::init (now nsITarget::init) is now a nsIURI
instead of a nsIFile.

- nsIDownload provides a nsILocalFile "targetFile" readonly attribute, so that
the local file can be accessed easily when the transfer is really a download.

- my plan is to break nsITransfer out into its own IDL file.  i will probably
ask Leaf to copy nsIDownload.idl,v to nsITransfer.idl,v so i can preserve CVS
history for the bulk of the interface.

- nsWebBrowserPersist needed to be tweaked to properly report either download
or upload progress but not both mixed together.

- most of the nsDownloadManager changes are straightforward to deal with
nsIDownload interface change.

- navigator changes exist to hook up UI.

- filepicker bug is fixed so that the initial filterIndex can be remembered in
a pref and set before showing the filepicker.  fixed this in both xpfe and
toolkit

- nsFTPChannel was not reporting total progress properly.  it was always using
mContentLength, but that value is normally not set to anything other than -1. 
better to use aProgressMax which is determined by the code in
nsFtpConnectionThread.cpp

- i went to great lengths in navigator.js to get the nsIURI for the currently
focused content frame, so that that directory would be the target of the
upload.  actually, the frame could be any FTP file, and the directory of that
FTP file would be used as the target directory of the upload.

        Attachment #145940 -
        Flags: review?(cbiesinger)

    
    

    
  
Comment on attachment 145940 [details] [diff] [review]
v2.1 patch

+  var targetURI = IOS.newURI(fileURL.file.leafName, null, targetBaseURI);

you don't need a charset here? it seems you can use
contentFrame.document.characterSet...

+  // display name property is unused by the progress dialog implementation,
and
+  // it's not obvious what we should pass for that argument

hm, I would just pass the leafName of the file I guess

why not pass Date.now()*1000 as start time? (*1000 because it is msec while
PRTime is usec...)

+function BrowserUploadFile()

hm, this function looks almost identical to BrowserOpenFileWindow... it would
be nice if they could share the code... maybe a function that shows a
filepicker, taking a filter index pref argument, return a uri?


you are using spaces in the if ( foo ) statements in nsProgressDialog.js
inconsistently...

+			 if ( this.targetFile ) {

above, you used != null

 nsFTPChannel::OnProgress(nsIRequest *request, nsISupports* aContext,
			  PRUint32 aProgress, PRUint32 aProgressMax)

-				   aProgress, (PRUint32) mContentLength);
+				   aProgress, (PRUint32) aProgressMax);

it seems the cast is no longer necessary

nsIDownload.idl
+     * @param aTarget The target (nsIURI) of the transfer.

"The target URI of the transfer", maybe?

xpfe/browser/resources/content/nsBrowserStatusHandler.js
+	   // Enable/disable menu entries for file upload...
+	   if (channel.URI.schemeIs("ftp"))
+	     this.canUploadFile.removeAttribute('disabled');

ok... what this will do, I think, is enable the file item when showing a file
on an ftp server... and uploading it would replace that file...
is that desired? (did ns4 do that?)


would setting .enabled = false/true be better? ask some frontend person, I
guess..


you'll also need to change
xpfe/components/download-manager/src/nsDownloadProgressListener.js I think

xpfe/components/download-manager/src/nsDownloadManager.cpp
   // the persistent descriptor of the target is the unique identifier we use

hm, looks like actually the path is used.
while you're touching this file, could you change this to "the path of the
target ..."

r=me, if you get moa for the xpfe changes from neil or jag; and you need a
firebird peer to review the toolkit/ changes which I didn't look at.

        Attachment #145940 -
        Flags: review?(cbiesinger) → review+

    
    

    
  
Comment on attachment 145940 [details] [diff] [review]
v2.1 patch

oh, one more thing...
+    this.mReqCount    = 0;
it seems you're never using this variable

    
    

    
  
(In reply to comment #166)
>(From update of attachment 145940 [details] [diff] [review])
>>xpfe/browser/resources/content/nsBrowserStatusHandler.js
>>+	   // Enable/disable menu entries for file upload...
>>+	   if (channel.URI.schemeIs("ftp"))
>>+	     this.canUploadFile.removeAttribute('disabled');
> 
>ok... what this will do, I think, is enable the file item when showing a
>file on an ftp server... and uploading it would replace that file...
>is that desired? (did ns4 do that?)
> 
>would setting .enabled = false/true be better? ask some frontend person, I
>guess..
No, menus are magic, because of the Mac, so you can't use xbl properties.

    
    

    
  
Comment on attachment 145940 [details] [diff] [review]
v2.1 patch

>+    set target(newval) { 
>+        // If newval references a file on the local filesystem, then
>+        // grab a reference to its corresponding nsIFile.
>+        try {
>+            this.mTargetFile = newval.QueryInterface(nsIFileURL).file;
>+        } catch (e) {
>+            this.mTargetFile = null;
>+        }
>+        return this.mTarget = newval;
>+    },
try/catch is not nice for expected QI exceptions, use this:
this.mTargetFile = newval instanceof nsIFileURL ? newval.file : null;

>+            // otherwise, wait for STATE_STOP with aRequest corresponding to
>+            // our target.  XXX redirects might screw up this logic.
>+            try {
>+                var chan = aRequest.QueryInterface(nsIChannel);
>+                if (chan.URI.equals(this.target)) {
>+                    // we are done transfering...
>+                    this.completed = true;
>+                }
>+            }
>+            catch (e) {
>+            }
Now this case I understand that aRequest should be a channel, so in this case
your use of try/catch is OK.

>     QueryInterface: function (iid) {
>         if (!iid.equals(Components.interfaces.nsIProgressDialog) &&
>             !iid.equals(Components.interfaces.nsIDownload) && 
>             !iid.equals(Components.interfaces.nsIWebProgressListener) &&
>             !iid.equals(Components.interfaces.nsIObserver) &&
>+            !iid.equals(Components.interfaces.nsIInterfaceRequestor) &&
>             !iid.equals(Components.interfaces.nsISupports)) {
>             throw Components.results.NS_ERROR_NO_INTERFACE;
>         }
>         return this;
>     },
You forgot to add nsITransfer.

>     // Get filename from target file.
>     fileName: function() {
>-        return this.target ? this.target.leafName : "";
>+        if (this.targetFile)
>+            return this.targetFile.leafName;
>+        if (this.target) {
>+            try {
>+                var url = this.target.QueryInterface(nsIURL); 
>+                if (url)
>+                    return url.fileName;
>+            } catch (e) { /* fall through */ }
>+        }
>+        return "";
>     },
Don't use if to test the return value of QueryInterface, it never returns null.
(Too much C++ programming!)
Hmm... don't you need to unescape the url's fileName? Or don't you have enough
information to do that?

>                 string = this.replaceInsert( string,
>                                              2,
>-                                             this.target.fileSize >> 10 );
>+                                             this.targetFile.fileSize >> 10 );
[OT: I hate this bit, it takes up far too much room]

>     // Hide a given dialog field.
>     hide: function( field ) {
>-        this.dialogElement( field ).setAttribute( "style", "display: none;" );
>-        // Hide the associated separator, too.
>-        this.dialogElement( field+"Separator" ).setAttribute( "style", "display: none;" );
>+        try {
>+            this.dialogElement( field ).setAttribute( "style", "display: none;" );
>+            // Hide the associated separator, too.
>+            this.dialogElement( field+"Separator" ).setAttribute( "style", "display: none;" );
>+        } catch (e) {}
>     },
Eww... I'd prefer to see a null check rather than a try/catch. Also please
change these to use .hidden = true; to hide the XUL elements.

>     // Return input in hex, prepended with "0x" and leading zeros (to 8 digits).
>     hex: function( x ) {
>          var hex = Number(x).toString(16);
>          return "0x" + ( "00000000" + hex ).substring( hex.length );
>     },
[OT: "0x" + ("0000000" + Number(x).toString(16)).slice(-8);]

>+    const filterIndexPref = "browser.open-file.picker-filter-index";
It would be neat if you could save the dir too, as per browser.download.dir or
mail.compose.attach.dir :-)
Personally I don't like that pref name, browser.open.filterIndex would do for
me, or picker_filter_index if you must...

>+          // Enable/disable menu entries for file upload...
>+          if (channel.URI.schemeIs("ftp"))
>+            this.canUploadFile.removeAttribute('disabled');
>+          else
>+            this.canUploadFile.setAttribute('disabled', 'true');
I don't think this is the best place to do this, what happens if you load an
ftp dir listing into one frame and then focus another frame before doing
file/upload? If necessary append the check to the menu_FilePopup onpopupshowing
code.

>       // Create the local directory into which to save associated files. 
>       const lfContractID = "@mozilla.org/file/local;1";
>       const lfIID = Components.interfaces.nsILocalFile;
>       filesFolder = Components .classes[lfContractID].createInstance(lfIID);
>-      filesFolder.initWithPath(persistArgs.target.path);
>+      filesFolder.initWithPath(fp.file.path);
[OT?] filesFolder = fp.file.clone();?

>+/**
>+ * This function extracts the local file path corresponding to the given URI.
>+ */
[OT] Yeah, and someone made the mistake of handing the file path off to
RDF.GetResource :-( With any luck we'll be able to standardize on URLs
throughout.

> public:
>   NS_DECL_NSIWEBPROGRESSLISTENER
>+  NS_DECL_NSITRANSFER
>   NS_DECL_NSIDOWNLOAD
>   NS_DECL_ISUPPORTS
I don't see the equivalent change to NS_IMPL_ISUPPORTS...

        Attachment #145940 -
        Flags: review+ → review-

    
    

    
  
biesi, neil: thank you both for your careful reviews.  new patch coming up....

    
    

    
  
(In reply to comment #166)
> (From update of attachment 145940 [details] [diff] [review])
> +  var targetURI = IOS.newURI(fileURL.file.leafName, null, targetBaseURI);
> 
> you don't need a charset here? it seems you can use
> contentFrame.document.characterSet...

No need.  The charset is inherited from targetBaseURI.

    
    

    
  
> this.mTargetFile = newval instanceof nsIFileURL ? newval.file : null;

i figured this required the object to implement nsIClassInfo, but now i see that
xpconnect is actually able to remember that instanceof returned true, and then
treat newval as if it had all the attributes and properties of nsIFileURL. 
that's very neat! ;-)

    
    

    
  
> Hmm... don't you need to unescape the url's fileName? Or don't you have enough
> information to do that?

right, i was thinking that i don't have enough information to unescape it. 
however, hmm... i could maybe use nsITextToSubURI::unEscapeURIForUI for that. 
it employs an aglo to carefully unescape only stuff that can be interpreted
relative to a particular charset, which is pretty much the best you can hope for.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v2.2 patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #145940 -
        Attachment is obsolete: true

    
  

        Attachment #145537 -
        Flags: superreview?(darin)

    
  

        Attachment #146152 -
        Flags: superreview?(bryner)

        Attachment #146152 -
        Flags: review?(neil.parkwaycc.co.uk)

    
    

    
  
fwiw that's not what classinfo means :). classinfo means js doesn't need to QI
at all for any interface listed in classinfo, they're automatically reflected.

    
    

    
  
Comment on attachment 146152 [details] [diff] [review]
v2.2 patch

+    this.mReqCount    = 0;

this is still unused...

bug 239115 bitrotted the (seamonkey) downloadmanager changes somewhat, sorry
about that

+  // display name property is unused by the progress dialog implementation,
and
+  // it's not obvious what we should pass for that argument, so just leave it
+  // empty for now.
+  dialog.init(fileURL, targetURI, leafName, null, Date.now()*1000, persist);

the comment doesn't seem to match the code anymore

    
    

    
  
> +    this.mReqCount    = 0;
> this is still unused...
...
> the comment doesn't seem to match the code anymore

oops.. thanks biesi!  new patch coming up.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v2.3 patch
        — 
          — Splinter Review
      

    


  
revised patch per biesi's last comments

        Attachment #146152 -
        Attachment is obsolete: true

    
  

        Attachment #146152 -
        Flags: superreview?(bryner)

        Attachment #146152 -
        Flags: review?(neil.parkwaycc.co.uk)

    
  

        Attachment #146202 -
        Flags: superreview?(bryner)

        Attachment #146202 -
        Flags: review?(neil.parkwaycc.co.uk)

    
    

    
  
Comment on attachment 146202 [details] [diff] [review]
v2.3 patch

r=me if you can find a way of fixing the size of the upload progress dialog,
it's too small as it stands, probably because the buttons are hidden. Maybe
calling sizeToContent before hiding the buttons would work, or you could just
disable the buttons instead.

When can we expect a multiple upload ;-)

        Attachment #146202 -
        Flags: review?(neil.parkwaycc.co.uk) → review+

    
    

    
  
Neil, I should have commented that I solved the dialog sizing problem with some
help from Ben Goodger.  His suggestion was to add a little bit of CSS to
nsProgressDialog.xul to specify a default width.  It seems that I have not
included that in the v2.3 patch :-(

I'll attach a complete patch shortly.

    
    

    
  
Comment on attachment 146202 [details] [diff] [review]
v2.3 patch

>--- xpfe/browser/resources/content/navigator.js	10 Apr 2004 09:41:33 -0000	1.535
>+++ xpfe/browser/resources/content/navigator.js	15 Apr 2004 17:38:03 -0000
>+function selectFileToOpen(label, prefRoot)
> {
>+  var fileURL = null;
>+
>   // Get filepicker component.
>+  const nsIFilePicker = Components.interfaces.nsIFilePicker;
>+  var fp = Components.classes["@mozilla.org/filepicker;1"].createInstance(nsIFilePicker);
>+  fp.init(window, gNavigatorBundle.getString(label), nsIFilePicker.modeOpen);
>+  fp.appendFilters(nsIFilePicker.filterAll | nsIFilePicker.filterText | nsIFilePicker.filterImages |
>+                   nsIFilePicker.filterXML | nsIFilePicker.filterHTML);
>+

This will now propagate an exception if one of these methods throws one, is
that what you wanted?

>+  dump("updateFileUploadItem: canUpload=" + canUpload + "\n");

I assume you'll remove this.

sr=bryner with those addressed.

        Attachment #146202 -
        Flags: superreview?(bryner) → superreview+

    
    

    
  
(In reply to comment #182)
> This will now propagate an exception if one of these methods throws one, is
> that what you wanted?

Yes, each of the callsites traps exceptions.


> >+  dump("updateFileUploadItem: canUpload=" + canUpload + "\n");
> 
> I assume you'll remove this.

oops!  thanks for catching that.  i'll remove it before checking in.

Neil: multi-file upload will have to wait.  the current patch is something i'm
going to try to land for 1.7, so it needs to be kept minimal (as if the current
patch is somehow small)

    
  

        Attachment #146202 -
        Flags: approval1.7?

    
    

    
  
so it looks like this was checked in:
136     set target(newval) { 
137         // If newval references a file on the local filesystem, then grab a
138         // reference to its corresponding nsIFile.
139         this.mTargetFile = newval instanceof nsIFileURL ? newfile.file : null;

note that newfile is not defined in line 139 - it should be newval... supposedly
this breaks progress reporting in the dialog

    
    

    
  
Comment on attachment 146387 [details] [diff] [review]
fix for that typo

r+sr=bzbarsky

        Attachment #146387 -
        Flags: superreview+

        Attachment #146387 -
        Flags: review+

    
    

    
  
typo fix checked in
Checking in nsProgressDialog.js;
/cvsroot/mozilla/embedding/components/ui/progressDlg/nsProgressDialog.js,v  <--
 nsProgressDialog.js
new revision: 1.36; previous revision: 1.35
done

this may fix bug 240832

    
    

    
  
It sounds like that checkin was intended to fix bug 240835.  Did it also fix bug
240811?

    
    

    
  
(In reply to comment #188)
> It sounds like that checkin was intended to fix bug 240835.  Did it also fix bug
> 240811?

possible... I am not quite sure how firefox's download manager works, and
whether it uses this file. I find it somewhat unlikely, actually.

    
    

    
  


  
Two issues:
1. Some platforms don't like a null defaultDirectory
2. openTopWin wants a string, selectFileToOpen returns a file URL.

    
    

    
  
Comment on attachment 146495 [details] [diff] [review]
More file open bustage

thanks neil!  my bad for apparently not testing the last round of changes :-(

        Attachment #146495 -
        Flags: superreview+

        Attachment #146495 -
        Flags: review+
Blocks: 240997

    
    

    
  
-                        
fp.file.parent.QueryInterface(Components.interfaces.nsILocalFile));
+                         fp.file.parent.nsILocalFile);

What's with that change exactly?

    
    

    
  
> What's with that change exactly?

yeah, i forgot to mention that i spoke to neil about that over IRC.  it turns
out that that change is not needed.  it is valid code, but it is not needed.  in
fact, the patch that i'm going to check in will change that to |fp.file.parent|
since xpconnect will perform the QI automagically based on typelib info.

    
    

    
  
(In reply to comment #190)
> More file open bustage

So I take it this bug is what's causing Mozilla (20040418/WXP) to crash on File
Open?  I noticed that this morning.

    
    

    
  
> So I take it this bug is what's causing Mozilla (20040418/WXP) to crash on File
> Open?  I noticed that this morning.

yes, it is.  i'll be checking neil's patch today.

    
    

    
  
Comment on attachment 146495 [details] [diff] [review]
More file open bustage

patch checked in with "fp.file.parent.nsILocalFile" changed to "fp.file.parent"

    
    

    
  
marking FIXED ... doesn't look like drivers are going to approve this change for
the 1.7 branch.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED

    
    

    
  
Is it allowed for this bug to be resolved as fix if there are existing
unresolved dependance bugs already?

    
    

    
  
Yes, if it's fixed.  Just because there's a dependency doesn't mean this bug
can't be fixed till that one is.  Usually it just means someone thought fixing
that one would make fixing this one easier.

    
    

    
  
I think that you're thinking of bug 24496, which was checked in on March 17th. 
However, it may not be hardcoded (it could be a preference) - and, even if it is
hardcoded, I doubt that b.m.o. is using the version of Mozilla to which this was
applied.

FWIW: I completely agree that a bug should be prevented from being marked FIXED
if there are still unresolved dependencies.  (Not blocking bug, but dependent
bugs.)  If it's to be marked fixed, and dependents are still unresolved, then
those dependencies should be removed... (Since then they are, obviously, not
dependencies after all.)

    
    

    
  
You're using a far narrower definition of "dependency" than most developers do...

    
    

    
  
OK, no more dependency "problem" =)

    
    

    
  
At least, bugzilla got less open bugs today.  :-)  Darin, thank you for all you
do in making the FTP UI and the FTP Upload possible.  It is worth it.

    
    

    
  
I don't see any other bug, or note about it...

Will this patch be ported back to the 1.7 branch for Firefox 1.0?  Or will this
be a post 1.0 Firefox feature?

    
    

    
  
>>When can we expect a multiple upload ;-)
Any bugs like this in bugzilla, so I can CC's myself to it?  I saw none.

    
    

    
  
Zook: I reopened bug 208385, which was marked as a dup of this one.  Its summary
is "Drag and drop from Windows Explorer to Mozilla on an ftp site does not
result in an upload".

    
    

    
  
Do you know if there is a bug/rfe reported for multiple file uploads?

    
    

    
  
You mean like a directory upload? In the bug, it's very clear that the aim is to
support drag and drop uploads. I think "Upload File.." does what it has to do
minimally. It would of course be expected that eventually Upload File.. would
support a shift-click to select multiple files, and would queue them.

The thing is this shift-click functionality is probably a different API call..
(not a programmer, but some dialogs support shift-click and some don't.)

Does the current patch / backend subsystem use and anticipate a queue? Because
if it does, we've got part of the implementation already.

--Sam

    
    

    
  
>>I don't see any other bug, or note about it...
>>Will this patch be ported back to the 1.7 branch for 
>>Firefox 1.0?  Or will this be a post 1.0 Firefox feature?
It is now confirmed from the April 19th, mozilla.org staff meeting report.  What
is confirmed is that this FTP Upload feature will be put into the Mozilla 1.7
Release...   So, look like you got your wish for Firefox 1.0 after all.

--snip--
- Release candidate shooting for tomorrow or Wed
- We need to land a few biggish changes we want to take on the 1.7
   branch first
   - Changes to prevent URL bar spoofing/phishing
   - Find in textareas
   - FTP upload
- These all landed on trunk last week
- As soon as these are in, off we go
- We've scheduled plenty of time on the branch for 1.7.
--snip--



    
    

    
  
Comment on attachment 146202 [details] [diff] [review]
v2.3 patch

a=asa (on behalf of drivers) for checkin to 1.7

        Attachment #146202 -
        Flags: approval1.7? → approval1.7+

    
    

    
  
note, bug 241199's patch will also need to be checked in when this lands on branch

    
    

    
  
aiie, you guys are killing me! ;)

so when darin landed this on the trunk, he also landed a bunch of stuff in
camino (causing bug 241199) to "fix" it. Where does that code live? Won't
landing this break the camino branch?

camino's branch is called MOZILLA_1_7_BRANCH, but is just mozilla/camino, not
the same branch as the rest of seamonkey.

    
    

    
  
when will this land on the branch?

    
    

    
  
Mike: No worries.  I will do my best to ensure that all related changes go in on
the branch when this lands.

    
    

    
  


  
This patch has been checked in on the 1.7 branch.

    
    

    
  
fixed1.7
Keywords: fixed1.7

    
    

    
  
Mozilla trunk build 2004050509 WinNT4
Hmmm ... If I login to my webspace at ftp://USERNAME:PWD@home.isp.net/ and go
into a subdirectory like ftp://USERNAME:PWD@home.isp.net/example/ and then
upload a file the transfer succeeds but the file lands in the "root" of my ftp
space, not in the example directory. I am not sure if this is a Mozilla bug or a
proxy/ISP issue. I can't find an existing, should I file a new one?

    
    

    
  
Ack, that problem was supposed to have been fixed by the patch for bug 238847! 
Please comment in that bug that you can still reproduce the problem, and I'll
try to investigate it today.

    
    

    
  
did the appropriate things land on the camino branch (also called
MOZILLA_1_7_BRANCH)?

    
    

    
  
i hear it did, thanks ;)

    
    

    
  
(In reply to comment #217)
> Mozilla trunk build 2004050509 WinNT4

I just tested today's 1.7 branch build on Linux, and could not reproduce this
problem.  It sounds like the problem you are seeing might be specific to your
ISP's FTP server.  If you could send me more information, like a network trace,
or FTP log, that would be great.  Just set these environment variables before
running Mozilla:

  c:\> set NSPR_LOG_MODULES=nsFTPProtocol:5
  c:\> set NSPR_LOG_FILE=c:\log.txt

Thanks!  (Be sure to inspect the log file for your password... you don't need to
send that to me!)

    
    

    
  
darin: hm.,.. that patch seems to be missing the nsDownloadProgressListener.js
changes (xpfe)?

    
    

    
  
(In reply to comment #222)
> darin: hm.,.. that patch seems to be missing the nsDownloadProgressListener.js
> changes (xpfe)?

biesi: thank you for catching that error.  i have checked in the changes to the
1.7 branch.  can you spot any other changes that i forgot?  i spent a lot of
time going through bonsai to make sure that i got everything, but for some
reason i missed this change to nsDownloadProgressListener.js! :(

    
    

    
  
(In reply to comment #223)
> biesi: thank you for catching that error.  i have checked in the changes to the
> 1.7 branch.  can you spot any other changes that i forgot? 

it looks like you got all the other changes.

    
  
QA Contact: zach → benc
Summary: UI for FTP upload not implemented → basic UI for FTP upload (menu) not implemented

    
    

    
  
V branch.
Keywords: fixed1.7, 
          
            helpwanted, 
          
            nsbeta1- → verified1.7

    
  
Blocks: 245908

    
    

    
  
(In reply to comment #209)
> It is now confirmed from the April 19th, mozilla.org staff meeting report.  What
> is confirmed is that this FTP Upload feature will be put into the Mozilla 1.7
> Release...
> --snip--
> - We need to land a few biggish changes we want to take on the 1.7
>    branch first
>    - FTP upload
there and in whatsnew:
http://www.mozilla.org/releases/mozilla1.7/README.html#new
you speak about FTP upload feature implemented.
Is implemented also the UI?

But where is this new feature???
How can I upload a file?
If I drag a file in Moz opened on a ftp://user:pass@ftp.domain.tld it open then
dialog "what to do with this file?"
For me this bug is also open or it is a hidden feature.
:-))


    
    

    
  
(In reply to comment #226)
> But where is this new feature???

Same place it's always been, under File

Note the bug Summary "basic UI for FTP upload (menu)"

    
    

    
  
Remember if you're on the ftp site(s) in the url toolbar then the file upload
link in the File menu would show up, if you on the http site(s) then you
wouldn't find it there.

    
    

    
  
Ok, I found it, tried it, and it works well (only for file not directory).
I havent see it because I use localized Mozilla, but (I dont know why) I'm
looked for Upload in File menu, and the traslation dont help.

To ask the delete/rename file capability on an ftp server I must open a new
enhancement bug?
These dont require GUI advanced functionality but little interaction (like right
click on file name).

    
  
No longer blocks: majorbugs

    
    

    
  
I just went to an FTP site(1) with the latest Firefox trunk nightly build(2). I
could see *no UI* for FTP upload in the File menu (or any other menu). Am I
overlooking something? Should this "Core" bug be reopened?

(1) ftp://ftp.netscape.com/pub/netscape8/

(2) Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20050920
Firefox/1.6a1

    
    

    
  
In SeaMonkey 1.0a is Upload File command available in File submenu. Maybe you
should file/find bug in Firefox product.

    
    

    
  
This bug has only fixed the suite/seamonkey. The corresponding firefox bugs are
bug 215673 and bug 208385.
Depends on: 391855

    
  
Product: Core → Core Graveyard

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: