104883 - rewrite nsIDiskDocument.idl (and related) to use nsIURI

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[Kathleen :Brade]

nsIDiskDocument needs to be rewritten to use nsIURI in the parameters/API.  The 
implementation can QueryInterface to nsIFileURI if it needs the local file.

Comment 1

[Simon Fraser [no longer active]]

Subsume into bug 101001?

Comment 2

[Akkana Peck]

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.

Comment 3

[Kathleen :Brade]

Attachment: patch1

Comment 4

[Adam Lock]

a/r=adamlock for the webBrowser diff

Comment 5

[Simon Fraser [no longer active]]

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?

Comment 6

[Akkana Peck]

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).

Comment 7

[Kathleen :Brade]

Attachment: patch #2

Comment 8

[Kathleen :Brade]

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.

Comment 9

[Simon Fraser [no longer active]]

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

Comment 10

[Kathleen :Brade]

The constructor does set the mod count to 0.
aSaveCopy is still used to determine whether to reset mod count or not.

Comment 11

[Akkana Peck]

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

Comment 12

[Simon Fraser [no longer active]]

> 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.

Comment 13

[Akkana Peck]

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[4]: *** [nsEditorRegistration.o] Error 1
gmake[4]: 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?)

Comment 14

[Simon Fraser [no longer active]]

It seems that depending on necko for nsIURI is hard to avoid.

Comment 15

[Akkana Peck]

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.

Comment 16

[Kathleen :Brade]

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

Comment 17

[sujay]

Shrir and Michael, if you can help me test some of these...se brade's last
comment...thanks...

Comment 18

[sujay]

verified in 11/7 build.