All users were logged out of Bugzilla on October 13th, 2018
nsIDiskDocument needs to be rewritten to use nsIURI in the parameters/API. The implementation can QueryInterface to nsIFileURI if it needs the local file.
Subsume into bug 101001?
Funny, I was going to mark this dependant on bug 101001 since part of that bug is to figure out what we're doing about nsIDiskDocument and related interfaces. They're definitely intertwined.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.6
a/r=adamlock for the webBrowser diff
Comments: + readonly attribute nsIURI fileSpec; Is "fileSpec" a good description for this? I also question whether nsIDiskDocument needs one at all; perhaps we should look into using using the nsIURI that the document was loaded with. + * @param aURI location to which to stream the document. * @param aReplaceExisting true if replacing an existing file, otherwise false. - * If false and aFile exists, SaveFile returns an error. + * If false and aURI exists, SaveFile returns an error. Whacky spacing. - nsCOMPtr<nsIFile> mFileSpec; + nsCOMPtr<nsIURI> mFileSpec; Probably want to rename 'mFileSpec' too. + // this does a simple string compare but isn't really checking for aliases + // or ".." in path which might actually cause this method to fail to match + // even though the same file is being edited + PRBool isSameFile; + if (NS_SUCCEEDED(docFileSpec->Equals(newFileLocation, &isSameFile)) && isSameFile) You should probably get an nsIFile from each one here, and use nsIFile::Equals(). +nsresult +nsEditorShell::PromptAndSetTitleIfNone(nsIDOMHTMLDocument *aHTMLDoc, PRBool * aTitleChanged, PRBool *retVal) Is this really part of this change? +nsresult +nsEditorShell::ShowSaveFilePicker(PRBool aDoSaveAsText, nsIURI *aDocumentURI, + nsIDOMHTMLDocument *ahtmlDocument, const char *aMIMEType, + PRInt16 *aDialogResult, nsIFile ** aResultingLocation) Ditto. [Aside: _please_ don't put any more methods into nsEditorShell; it's going away sooner or later. Anything that poses UI should go into JS.] + // find out if the doc already has a fileSpec associated with it. + nsCOMPtr<nsIURI> docFile; + PRBool noFileSpec = (diskDoc->GetFileSpec(getter_AddRefs(docFile)) == NS_ERROR_NOT_INITIALIZED); This would change if we rely on the document's URI, rather than storing one in nsIDiskDocument. Index: mozilla/editor/libeditor/html/nsHTMLEditRules.cpp Index: mozilla/editor/libeditor/text/nsTextEditRules.cpp Why did these change?
Re whether we rely on the document's existing URI: might we ever need a way to associate a url somewhere out on the net with a local file? Composer probably wouldn't, but the browser might (going on the assumption that we're going to integrate nsIDiskDocument with nsIWebBrowserPerist and have both browser and editor use the same interface).
New patch address sfraser's comments. It also removes the InitDiskDocumentCall and the mFileSpec altogether since the document uri can be obtained from the document (no need to store it twice). I removed the tabs from nsIDiskDocument.idl. I also added comments above some methods in nsEditorShell.cpp which I think we should be moving into JS. There is another bug assigned to me to move stuff into JS The edit rules changes seem to be needed because nsIDiskDocument now has nsIURI instead of nsIFile.
You removed nsDocument::InitDiskDocument(), which sets mModCount = 0;. Does the ctor set this to 0? + void SaveFile(in nsIURI aFile, in boolean aReplaceExisting, in boolean aSaveCopy, in wstring aFileType, in wstring aFileCharset, in unsigned long aSaveFlags, in unsigned long aWrapColumn); You can remove aSaveCopy from this method, since it's never used here any more. The rest looks good, sr=sfraser
The constructor does set the mod count to 0. aSaveCopy is still used to determine whether to reset mod count or not.
I'm getting compile errors on linux, though the patch applied cleanly. This tree is a couple days old; I wonder if that's why? I'll try it in a newer tree. nsDocument.h:602: `nsIFile' was not declared in this scope nsDocument.h:602: template argument 1 is invalid nsDocument.h:602: warning: ISO C++ forbids declaration of `mFileSpec' with no type In file included from nsDocument.cpp:66: nsContentList.h:69: warning: `nsBaseContentList::IndexOf (nsIContent *, PRInt32 &)' was hidden nsContentList.h:125: warning: by `nsContentList::IndexOf (nsIContent *, PRInt32 &, int)' Is nsIServiceManager really the correct file to include to get nsIURI? Shouldn't we be able to just include nsIURI.h? nsWebBrowserPersist.cpp: you save the return value of GetURL but then you don't check it. nsEditorShell.cpp line 1759: if NS_FAILED(rv) we return NS_ERROR_NULL_POINTER instead of rv. Is this intentional? I really hate having all this text-and-html logic in the editor shell (re SaveDocument). Shouldn't we be moving toward relying less on the editor shell, and putting logic in the editor classes themselves? Though, since the comment says the method should move into JS, I guess this is just temporary anyway? None of these issues is important and none of them should block getting it in -- it looks fine aside from the questions I had (assuming it does compile, which I assume was a problem with my old tree -- still building on my newer tree). r=akkana
> Shouldn't we be moving toward relying less on the editor shell, > and putting logic in the editor classes themselves? I think it should move out of editorShell, and probably into the composer DLL. libeditor certainly should _not_ have APIs for saving to files.
Yes, Simon has a point -- libeditor shouldn't own this code either. Point taken. In my tree updated this morning, applying the patch then rebuilding from the top, I get a bit farther, but it still doesn't build: ...config-defs.h -Wp,-MD,.deps/nsEditorRegistration.pp nsEditorRegistration.cpp In file included from ../../../editor/libeditor/base/nsEditor.h:49, from nsEditorRegistration.cpp:42: ../../../dist/include/content/nsIDiskDocument.h:14:20: nsIURI.h: No such file or directory gmake: *** [nsEditorRegistration.o] Error 1 gmake: Leaving directory `/builds/vanilla/mozilla/editor/libeditor/build' Looks like the problem is that we didn't previously have a REQUIRES dependency on necko, and this adds one. If I add necko to the REQUIRES list, the build finishes. (Windows will presumably need this too, assuming that we decide that we do indeed want to add this dependency -- Simon? Opinion?)
It seems that depending on necko for nsIURI is hard to avoid.
If we agree that switching to nsIURI is the right thing, then that automatically implies a libnecko dependance. So as long as that's all right, my r=akkana stands (provided you add necko to the REQUIRES for linux and windows makefiles). The rest of the build completed correctly with that change.
I checked this in today; only 1 linux makefile was missing the necko requires line (that I found). I'm not sure why mozilla/editor/libeditor/build/makefile.win and Makefile.in are so different from each other. I'll file a separate bug on that issue. Sujay--for verifying this bug, test saving in these ways (and any other variations): save new page (about:blank) save existing local file save as existing local file to new location save as existing local file writing over another file location save remote file to new local location save remote file over existing local location cancel prompt for title with above variations cancel save as dialog with above variations
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Shrir and Michael, if you can help me test some of these...se brade's last comment...thanks...
verified in 11/7 build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.