I'd like to enable saving of remote files in nsWebBrowserPersist.cpp I will attach a preliminary patch with some of my suggested changes.

This patch hasn't been thoroughly tested and I'm sure there are some problems. I was able to successfully edit ftp://bubblegum.mcom.com/cmanske/sss.html and see the results on the server. (I haven't tried http or a file with an image yet). One problem is that I can successfully upload but the data doesn't appear until I quit the application which did the publishing. Please let me know if you see something obviously wrong. Also, if you want to test in Composer, you'll need a small patch.

change qa contact to Dharma. Web Browser Persist is his area.

i haven't looked carefully over the entire patch, but you seem to be using nsIStorageStream correctly.

I've looked at the patch and it seems reasonable but I am concerned that the persist code is getting unecessarily complicated and monolithic I still prefer the idea of a seperate upload object, with persistence being a two stage process - persist into a temporary folder and then upload, rather than mixing download, fixup and upload mixed in a single object. The real complication comes when considering authentication and progress - upload and download will both need authentication and progress notifications and I can see it becoming pretty nasty. So while I welcome small changes to make upload possible at a pinch via webrowserpersist, I much prefer putting the effort into an seperate upload object unencumbered by issues of walking DOMs, fixup, download etc. I don't see too much difficulty in the form this object would take, that is to say it would have an API to allow the caller to upload a directory or file, flags to control directory recursion and callbacks for progress and authentication. Naturally the webbrowserpersist object would have to be able to output content ready to run from its final destination but this should be relatively straightforward to do.

Adam and I have come to this agreement: no authentication code will be in nsWebBrowserPersist callers should implement progress notifications no protocol-specific checking (except file?) will be in nsWebBrowserPersist (persist code shouldn't have to read headers) callers are responsible for checking if files/directories exist on a server callers are responsible for optimizations (uploading only the needed files) there are some valid scenarios where the "temp saving" approach wouldn't work persist code will provide an API for caller to specify outputstream for uri (as part of another bug) I will make a few minor changes/fixes to the above patch before I'm ready for reviews.

Comment on attachment 67255 [details] [diff] [review] updated patch taking some of Adam's feedback into account Some issues: At 260,5 Deleting the NS_ENSURE_SUCCESS doesn't look right to me. 260,5 & 1089,30 we should probably start to use the proper flags instead of -1 in file creation args. 404,4 should be able to pass contentType.get() into StartUpload. No need to convert to newContentType. 404,4 StartUpload return value should be checked. If an error then abort the download 1742,12 SaveDocumentWithFixup should probably take a const char * instead of nsString & for the content type to avoid all this converting the type back and forth between various string classes. 1742,12 Remove the #ifdef 0 stuff 1836,5 Put the assignment to nsnull back in in case url->GetFileName returns an error and the param contains whatever garbage value it went in with

Here is my latest patch. The strategy in this patch is to listen to the input streams for files which are being uploaded. These items are tracked in mUploadList. The saving process won't actually end now (in EndUpload) when all of these conditions are met: * no known files left to read (mOutputMap is empty) * no known streams left for upload (mUploadList is empty) * no known documents left to upload (mDocList is empty) It seems like "EndUpload" isn't a good name since all output goes through there (not just uploading but also saving). I'd like a better name if someone could suggest it. "Finale()"? feedback/comments please! :-)

Comment on attachment 67979 [details] [diff] [review] updated patch which doesn't use inputstream listeners r=adamlock The code looks sane, but we'll have to keep a close eye on it to ensure it doesn't regress the existing save to file:// behaviour.

Comment on attachment 67979 [details] [diff] [review] updated patch which doesn't use inputstream listeners >Index: src/nsWebBrowserPersist.cpp >+struct UploadData >+{ >+ ~UploadData() >+ { >+ } nit: empty destructor seems like noise to me. >+ { >+ // see where the paths match up >+ // mCurrentDataPathIsRelative = PR_FALSE; >+ // mCurrentDataPath = aDataPath; >+ // mCurrentRelativePathToData = ""; >+ >+ // if protocol is "file" then make it relative >+ } should there be some code in between these brackets? >+ nsCOMPtr<nsILocalFile> localFile; >+ GetLocalFileFromURI(aURI, getter_AddRefs(localFile)); should this happen closer to where localFile is actually used? here: >+ if (localFile) >+ { >+ localFile->SetLeafName(newFileName.get()); >+ PRUint32 segsize = 8192; >+ PRUint32 maxsize = 0xFFFFFFFF; or PRUint32(-1), so you don't have to be careful about counting out 8 F's ;-) >+nsWebBrowserPersist::EnumCalcUploadProgress(nsHashKey *aKey, void *aData, void* closure) >+{ >+ nsWebBrowserPersist *pthis = (nsWebBrowserPersist *) closure; >+ UploadData *data = (UploadData *) aData; any need to protect against null |pthis| or null |data|? > newValue.AssignWithConversion(nsEscape(rawPathURL.get(), url_Path)); nsEscape mallocs its return value, and AssignWithConversion only copies, so this line leaks the result of nsEscape. you might try using NS_EscapeURLPart instead.. like so: nsCAutoString buf; if (NS_EscapeURLPart(rawPathURL.get(), rawPathURL.Length(), url_Path, buf)) newValue.AssignWithConversion(buf); else newValue.AssignWithConversion(rawPathURL); this code avoids the extra malloc unless it is absolutely necessary. >+ nsCAutoString newContentType(ToNewCString(aFormatType)); this leaks the return value of ToNewCString(). how about this instead: NS_ConvertUCS2toASCII newContentType(aFormatType); >Index: src/nsWebBrowserPersist.h > PRBool mJustStartedLoading; > PRBool mCompleted; >+ PRBool mStartSaving; PRPackedBool is better for member variables.

Responding to Darin's feedback: * destructor removed * commented code fixed and uncommented * localFile is moved closer but it's used in 2 places so it's as good as it gets for now (2nd usage is in a different if block not seen in this diff) * I don't like to use negative numbers for unsigned ints but I changed it since that is the standard in most of the networking code I've seen :-) * I added the null checks even though they'd be highly unlikely * nsEscape fixed as suggested * I reworked the parameters to take const char * so there is less string manipulation for content type * I moved around some PRBool's and made them all PRPackedBool's

Comment on attachment 68400 [details] [diff] [review] updated patch based on darin's feedback r=adamlock

I mispoke about one thing: NS_EscapeURLPart should use esc_Directory | esc_FileName | esc_FileExtension instead of url_Path, since url_Path is exclusively for the nsEscape function.

Comment on attachment 68400 [details] [diff] [review] updated patch based on darin's feedback sr=darin with the correction to NS_EscapeURLPart

checked in (with modifications requested by reviewers); THANKS EVERYONE! :-)