110135 - New flags and properties on nsIWebBrowserPersist for editor

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Adam Lock]

From bug 99642:

1. outputEncodingFlags (get/set; defaults to 0)
  Can you explain what encoding flags you mean?
2. outputPreferredWidth (get/set; defaults to 72)
  Preferred width would only be possible for documents, not URIs
3. contentType (get/set; defaults to "text/html")
  What do you mean here?
4. doReplace (get/set; defaults to true)
  Could be a flag but editor will still have to ask if they wish to
  override a file before calling webbrowserpersist.

Comment 1

[Kathleen :Brade]

outputEncodingFlags come from nsIDocumentEncoder.idl (iirc)

Why would preferred width only apply in some cases (local files and not remote
files?)?  I don't understand.

The contentType would be useful so we can save a file as "text/html" or
"text/plain" or similar.  This is useful if the user wants to save an html page
as a text file (remove html tags).

Akkana--can you help with this bug?  We should get rid of all saving in
nsIEditor and just call nsIWebBrowserPersist instead (will be in JS when I
finish my current bug).

If Akkana can't help out, I'll volunteer to come up with this patch.

Comment 2

[Adam Lock]

You'll have to say what output flags you need. As to why flags and wrapping only
works locally, it because when you call saveURI, the data is just dumped out raw
to disk - no parsing & no encoding. If you call saveDocument, you provide a DOM
document which is fed through the encoder. 

I can see how specifying the document content type would be useful. It should
default to whatever is most appropriate to the provided DOM document type.

BTW nsIDocumentEncoder is not defined in IDL but as a header file, so there is
no chance that I can expose anything from it directly. 

Comment 3

[Kathleen :Brade]

The specific area that we need fixed up for editor is:
http://lxr.mozilla.org/seamonkey/source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp#653

contentType needs to be settable (and default appropriately in embedding case)
replaceFlag needs to be settable also (and default appropriately)
outputFlags need to be settable (you use 0 for embedding)
saveCopyFlag probably needs to be settable also
wrapColumn needs to be settable

My goal is to get rid of nsEditor::SaveFile and have one set of saving code:
http://lxr.mozilla.org/seamonkey/source/editor/libeditor/base/nsEditor.cpp#978

Comment 4

[Akkana Peck]

I can help out with this bug.

nsIDocumentEncoder was supposed to be IDL, but it missed getting converted when
layout/content switched over to allowing xpidl instead of DOM idl.  There's a
separate bug tracking the idl-ization (which would go away if we replace the
interface with this class).

We need the mime type (or equivalent, but mime type is best because it's
standard and extensible) because we need to be able to save as html, unicode, or
ascii (at a minimum, and as xml or other types for some pages, and perhaps is
image/jpeg or similar if we're pointing only at an image).

We need the flags defined in nsIDocumentEncoder.h because different clients who
need to serialize dom output have different needs.  It doesn't need to be flags,
and they don't need to be exactly the same as in nsIDocumentEncoder, if you
think there's a better way of expressing these output modes.

We need to be able to specify width first because plain text output usually
needs to be wrapped (e.g. output from plaintext mail compose), but also because
html source needs to be wrapped (it's nice for readability) and not everyone
agrees on the right wrap column setting (we default to 72 but many people prefer
wider or narrower).

Comment 5

[Adam Lock]

aSaveCopy's only purpose is to call ResetModificationCount after saving. You
should be able to call this yourself from the editor?

The other stuff can probably added as extra args to the saveDocument method.
I'll submit a diff to the interface demonstrating the changes.

Comment 6

[Adam Lock]

Attachment: Something like this?

Comment 7

[Akkana Peck]

The changes look fine, except that Kathy and I discussed the aFile/aDataPath
part -- we're hoping to use this API for publishing as well as saving to local
files, so we may need a URI instead of something that can only use a local file.

Comment 8

[Kathleen :Brade]

nsILocalFile should be nsIURI in SaveDocument.
It might be clearer if aDataPath was renamed to aBaseURI or something similar.

Comment 9

[Adam Lock]

Attachment: Work in progress

This diff is still work in progress but gives an indication of how I intend to
proceed. The nsIWebBrowserPersist is much like last time, except that the file
and data path parameters are now nsISupports instead of nsILocalFile. You can
supply an nsILocalFile or nsIURI object here and both should work.

I'm not quite done making the transition to nsIURI but my next patch should
work while saving files locally. Making it save files remotely could be much
harder. See the parts marked TODO-IURI-fixup to see what else needs doing.

Comment 10

[Adam Lock]

Attachment: Work in progress 2

This version completes the transition from nsILocalFile to nsIURI. Saving to
disk via local file objects or file:// URLs works but any other kind of URL
WON'T work.

Comment 11

[Adam Lock]

Some notes:

1. The most important thing at the moment is that the changes to
nsIWebBrowserPersist satisfy what the editor people need it to do. After it's
frozen it will be too late.
2. Forget trying to get the impl to save to non file:// URIs for the moment.
3. Add some code to assert if caller tries to save to a non file:// URI
4. Raise bug (after this one is checked in) to remove the aSaveCopy parameter
from nsIDiskDocument::SaveFile. The caller should remove the dirty flag on the
document for themselves.

Comment 12

[Adam Lock]

Attachment: Final patch

This patch should be ready for review. Kathy can you try it out to make sure it
does everything you need?

Comment 13

[Adam Lock]

Attachment: Updated patch

The last patch was missing some minor changes to
mozilla/embedding/browser/webBrowser.

Note the patch doesn't contain diffs for the test apps because of some other
stuff I have pending sr but if they break just append "nsnull, 0, 0" to their
calls to saveDocument and they will build.

Comment 14

[Adam Lock]

Attachment: Updated patch

This patch fixes a silly bug where saveDocument does nothing if it's told to
save and fixup a document which has no images, js etc.

I've also included patches for the embedding test apps

Comment 15

[Michael Dunn]

Adam, What is the customer impact without this fix? Should we consider this for
cs (094)?

Comment 16

[Adam Lock]

Attachment: Updated patch

New patch with extra return value checks and a few typo fixes.

Comment 17

[Adam Lock]

Michael, it's likely the customer will want the 6 line fix for the silly bug I
mentioned but I'm not sure about the other stuff. I'll ask some people tomorrow.

Comment 18

[Kathleen :Brade]

Comment on attachment 59569 [details] [diff] [review]
Updated patch

r=brade

Comment 19

[Simon Fraser [no longer active]]

Comments:

+   * @param aFile      Target local file which may either be an nsILocalFile
+   *                   or nsIURI object.
    *
    * @return NS_OK Operation has been started.
    * @return NS_ERROR_INVALID_ARG One or more arguments was invalid.
    */
-  void saveURI(in nsIURI aURI, in nsIInputStream aPostData, in nsILocalFile 
aFile);
+  void saveURI(in nsIURI aURI, in nsIInputStream aPostData, in nsISupports 
aFile);

I'm not sure I like this ambiguity in the 'aFile' parameter. After all, if the 
caller has an nsILocalFile, then can make an nsIFileURI out of that, and thus 
pass a URI. Why not force all callers to just pass nsIURIs? The same goes for the 
other methods that have been changed to use nsISupports for the data path.

Comment 20

[Adam Lock]

The principle reason I accept either is because it avoids a lot of ugly code on
the client side. For example, whereas before someone would:

1. Display a file picker to the user to get a filename & path
2. Create local file objects
3. Call saveDocument

They would now have to:

1. Display a file picker
2. Create localfile objects
3. Call GetURL on them to get the text file:// URL
4. Create uri objects
5. Call saveDocument

Seeing as every client would have to do these extra steps (to turn the file path
into a file spec) I believe it is more convenient to simply put the code behind
the interface and accept either. 

Comment 21

[Simon Fraser [no longer active]]

Getting a fileURL from the file picker is easier than that. From the file picker 
you get an nsIFile. CreateInstance() an nsIFileURL. Call it's SetFile method.

Comment 22

[David Epstein]

changed qa contact to Dharma. WebBrowserPersist is his area.

Comment 23

[Adam Lock]

Sorry when I said file picker I was speaking generically. Embedders are likely
to use their own file pickers or system native ones. Most of these will return a
path as a string which has to be converted the way I described.

I'll investigate further to see the work involved.

It occurred to me yesterday that I make the assumption that the nsIURI object
they provide will have certain other interfaces (e.g. nsIFileURL) when it may
not, for instance if they're passing their own object that implements nsIURI. So
even if the parameter is an nsIURI, I will have to create a new URI from the
input URI's spec to be sure that it's a kind of object I can deal with. 

Comment 24

[Adam Lock]

Attachment: Before and after on mfcEmbed

This patch demonstrates the effect of changing the args to be nsIURI only for
the mfcEmbed sample. It doubles the amount of code that has to be written.

There is little change to the amount of work the webbrowserpersist object has
to do.

Comment 25

[Adam Lock]

Attachment: Patch only accepts nsIURI files

This patch makes the nsIWebBrowserPersist take nsIURI files and datapaths. I
have to say I really hate this way simply because it adds about 10 extra lines
of code to each client and drags in nsNetUtil.h.

The PPEmbed has not been updated for this patch but it should be self evident
what changes will be needed from looking at mfcEmbed.

Comment 26

[Simon Fraser [no longer active]]

+        nsXPIDLCString fileURL;
+        file->GetURL(getter_Copies(fileURL));
+        nsCOMPtr<nsIURI> fileURI;
+        NS_NewURI(getter_AddRefs(fileURI), fileURL.get());

I think this is the wrong pattern for making a URI from a file path. I think it 
should be more like:

make a new nsILocalFile
call initWithPath()
call NS_NewFileURI() with that nsILocalFile

If you call this in more than one place, just factor it into a helper method.


+    nsCOMPtr<nsIURI> fileAsURI;
+    nsXPIDLCString fileURL;
+    PRBool isFile = PR_FALSE;
+
+    nsresult rv = aFileURI->GetSpec(getter_Copies(fileURL));
+    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
+    NS_NewURI(getter_AddRefs(fileAsURI), fileURL.get());
+    fileAsURI->SchemeIs("file", &isFile);
+    NS_ENSURE_TRUE(isFile, NS_ERROR_INVALID_ARG);

This code confuses me too. It look like it's checking whether the nsIURI
is actually a file URI, but it also reconstructs a new 'fileAsURI' that's 
equivalent to the old one, going via a path. This seems expensive, and bad on 
platforms where going to/from paths loses volume information (Mac).

Why not try to QI the nsIURI to a nsIFileURL to test for file-ness, or just call 
GetLocalFileFromURI()? And if you need to clone the nsIURI, just call Clone().

This stuff happens again twice in SaveDocument() as well.

+nsresult nsWebBrowserPersist::AppendPathToURI(nsIURI *aURI, const nsAString & 
aPath) const

I'm a little surprised that there are not URI APIs for this, which contain the / 
logic.

+            // TODO infer the other content type - from the DOM document maybe?
+            contentType.AssignWithConversion("text/html");

Maybe look at the file extension? We have MIME APIs for extension -> mime type 
mappings.

+    // We have to do this to ensure any changes to the file path (extension 
appended etc.)
+    // are propogated to the URI for fixup purposes.
+    nsXPIDLCString newURL;
+    rv = localFile->GetURL(getter_Copies(newURL));
+    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
+    rv = aFile->SetSpec(newURL.get());
+    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);

I'm not sure that localFile->GetURL() is implemented on all platforms. It might 
be better to make a new nsIURI from the nsIFile, and then get its descriptor, and 
set that on aFile.

I think rpotts should sr this.

Comment 27

[Adam Lock]

The reason for all the wasteful duplication instead of Clone'ing, is that I have
no idea if the nsIURI object passed to saveDocument/saveURI by an embedder is
implemented on one of our nsStandardURL objects. Conceivably they could
implement nsIURI on one of their own objects. Even it implemented nsIFileURL, it
might not return an nsIFile via GetFile. 

GetURL is now implemented on all platforms so it should be safe to use.

Even with the aid of helper methods, I hope the new patch illustrates that
changing the interface to accept only nsIURI instead of nsIURI or nsILocalFile
makes life a lot harder for the embedder.

Comment 28

[Adam Lock]

Rick, what do you think is the best patch to land?

Comment 29

[rpotts (gone)]

hey adam,

i think that having the nsIWebBrowserPersist methods should take an nsIURI. 
However, i totally agree that currently this is a pain for callers !!

so, i've talked with darin, and we've agreed that we should add some facility
which  can translate a platform-specific filepath into a file:// URL.  ideally
this translation should not involve creating an intermediate nsLocalFile or
nsStdUrl objects ;-)

but, if callers had a simple way of translating their 'file path' into a file://
url then it would get rid of all of the messy code that is currently necessary
at the call sites...

it's sort of funny, because currently there is nsIIOService::newFileURI(...)
which makes *no* sense in the current world... so, maybe we should tweak the
IOService and add a file translation routine...  or just change newFileURI to
take a string 'path' instead of an nsIFile...

i'm going to file a bug about this - and who knows i might just fix it while i'm
at it...

does this approach sound ok with you?  it should eliminate the extra work on the
call sites.

-- rick

Comment 30

[Adam Lock]

Okay, this sounds like a reasonable compromise to me. The next question, is this
bug blocked until this new magic NS_NewFileURI helper method appears?

Comment 31

[rpotts (gone)]

hey adam,

i've just created bug #114456 to track the NS_NewFileURI(...) issue...

-- rick

Comment 32

[Adam Lock]

Attachment: Final patch (nsIURI)

Can I have reviews on this patch please? All args are now nsIURI only, patch
also updates save-as stuff which uses webbrowserpersist these days.

Comment 33

[rpotts (gone)]

Comment on attachment 63835 [details] [diff] [review]
Final patch (nsIURI)

looks good.

if we resolve bug #114456, i'll see if i can use the new APIs to simplify the
conversion between string and nsIFile ;-)

-- rick

Comment 34

[Kathleen :Brade]

I don't think the most recent patch is adequate.  It introduces serious bugs
(dataloss) on Macintosh because callers are creating nsIURIs instead of nsIFileURLs.

nsIURI's are lossy on Mac unless the nsIURI can be QI'd to an nsIFileURL from
which nsIFileURL::file can be retrieved and used for saving.

There are two strategies here:
 (1) use nsIURI for the destination parameter and force callers to create
nsIFileURLs that have the nsIFile (for local files).
 (2) use nsISupports for the destination parameter and callers can pass in
nsIFile, nsIURI, or nsIOutputStream or other types which we could support.  The
implementation would be responsible for all conversions or other details instead
of the numerous clients (who can pass in the object of most convenience).

Comment 35

[Adam Lock]

Attachment: The other final patch :) (nsISupports)

This is the patch which uses nsISupports that I believe should be used to
resolve the issues raised by Kathy.

Internally the webbrowserpersist object is identical but allowing the API to
accept nsIURI or nsIFile (or if we wanted at some time other objects suchas 
nsIOutputStream) makes the client side code much, much cleaner. There is no
thrashing about in the clients converting about between nsIURI & nsIFile which
could lead to incorrect behaviour on the Mac if some client forgot to account
for the lossy nature of file URIs on that platform.

Comment 36

[Ben Goodger (use ben at mozilla dot org for email)]

*makes rustling noises*

rpotts, can you review the newer patch? ;)

I'm eagerly awaiting this to land as it'll let me fix several of my .9.8 bugs. 

Comment 37

[Kathleen :Brade]

Comment on attachment 63835 [details] [diff] [review]
Final patch (nsIURI)

For the record:  This patch is not Adam's preferred patch.
I sent feedback to Adam in private e-mail but I am reviewing this patch in case 
others some day want to use parts of it elsewhere.

>Index: mozilla/embedding/browser/activex/src/control/MozillaBrowser.cpp
>===================================================================
>         nsCOMPtr<nsILocalFile> file;
>         NS_NewLocalFile(T2A(szFile), TRUE, getter_AddRefs(file));
>+        nsXPIDLCString fileURL;
>+        file->GetURL(getter_Copies(fileURL));
>+        nsCOMPtr<nsIURI> fileURI;
>+        NS_NewURI(getter_AddRefs(fileURI), fileURL.get());

I would expect this could would look more like this:

  >	    nsCOMPtr<nsILocalFile> file;
  >	    NS_NewLocalFile(T2A(szFile), TRUE, getter_AddRefs(file));
  >+	    nsCOMPtr<nsIURI> fileURI;
  >+	    nsresult rv = NS_NewFileURI(getter_AddRefs(fileURI), file);
  >+	    NS_ENSURE_SUCCESS(rv, rv);

note: less lines of code; no string needed; and checking of result code


>         nsCOMPtr<nsILocalFile> dataPath;
>         NS_NewLocalFile(szDataPath, TRUE, getter_AddRefs(dataPath));
>+        nsXPIDLCString datapathURL;
>+        dataPath->GetURL(getter_Copies(datapathURL));
>+        nsCOMPtr<nsIURI> datapathURI;
>+        NS_NewURI(getter_AddRefs(datapathURI), datapathURL.get());

same as above; I expect to see:
  >+	    nsCOMPtr<nsIURI> datapathURI;
  >+	    nsresult rv = NS_NewFileURI(getter_AddRefs(datapathURI), dataPath);
  >+	    NS_ENSURE_SUCCESS(rv, rv);


>Index: mozilla/embedding/browser/powerplant/source/CBrowserShell.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/embedding/browser/powerplant/source/CBrowserShell.cpp,v
>retrieving revision 1.32
>diff -u -r1.32 CBrowserShell.cpp
>--- mozilla/embedding/browser/powerplant/source/CBrowserShell.cpp	7 Jan 2002 14:34:14 -0000	1.32
>+++ mozilla/embedding/browser/powerplant/source/CBrowserShell.cpp	7 Jan 2002 18:37:13 -0000
>@@ -706,7 +706,19 @@
>     nsCOMPtr<nsILocalFile> parentDirAsLocal(do_QueryInterface(parentDir, &rv));
>     if (NS_FAILED(rv)) return rv;
> 

>+    nsCOMPtr<nsIURI> fileURI;
>+    nsXPIDLCString fileURL;
>+    localFile->GetURL(getter_Copies(fileURL));
>+    rv = NS_NewURI(getter_AddRefs(fileURI), fileURL.get());
>+    if (NS_FAILED(rv)) return rv;
>+
>+    nsCOMPtr<nsIURI> datapathURI;
>+    nsXPIDLCString dataPathURL;
>+    parentDirAsLocal->GetURL(getter_Copies(dataPathURL));
>+    NS_NewURI(getter_AddRefs(datapathURI), dataPathURL.get());
>+    if (NS_FAILED(rv)) return rv;

especially because this is lossy and your introducing a bug, please change as
above:
  >+	    nsCOMPtr<nsIURI> fileURI;
  >+	    nsresult rv = NS_NewFileURI(getter_AddRefs(fileURI), localFile);
  >+	    NS_ENSURE_SUCCESS(rv, rv);
  >+
  >+	    nsCOMPtr<nsIURI> datapathURI;
  >+	    rv = NS_NewFileURI(getter_AddRefs(datapathURI), parentDirAsLocal);
  >+	    NS_ENSURE_SUCCESS(rv, rv);


>@@ -723,7 +735,13 @@
>+    nsCOMPtr<nsIURI> fileURI;
>+    nsXPIDLCString fileURL;
>+    localFile->GetURL(getter_Copies(fileURL));
>+    NS_NewURI(getter_AddRefs(fileURI), fileURL.get());
>+    if (NS_FAILED(rv)) return rv;

same as above:
  >+	    nsCOMPtr<nsIURI> fileURI;
  >+	    nsresult rv = NS_NewFileURI(getter_AddRefs(fileURI), localFile);
  >+	    NS_ENSURE_SUCCESS(rv, rv);



>Index: mozilla/embedding/components/ui/progressDlg/nsProgressDlg.js
>===================================================================
>RCS file: /cvsroot/mozilla/embedding/components/ui/progressDlg/nsProgressDlg.js,v
>retrieving revision 1.3
>@@ -554,4 +558,13 @@
>+function makeURIFromFile(aFile)
>+{
>+  const stdURLContractID = "@mozilla.org/network/standard-url;1";
>+  const stdURLIID = Components.interfaces.nsIURI;
>+  var uri = Components.classes[stdURLContractID].createInstance(stdURLIID);
>+  uri.spec = aFile.URL;
>+  return uri;
> }

I think the above function is also buggy; it'd be better to do something like:
  var fileurl = uri.QueryInterface(Components.interfaces.nsIFileURL);
  if (fileurl)
    fileurl.file = aFile;
  else
  {
    uri.spec = aFile.URL;
    dump("possibly loss of data here when converting from file to string\n");
  }
Can you see if that would work?


>Index: mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp,v
>retrieving revision 1.9
>@@ -220,20 +228,66 @@
> 
> /* void saveURI (in nsIURI aURI, in string aFileName); */
> NS_IMETHODIMP nsWebBrowserPersist::SaveURI(
>-    nsIURI *aURI, nsIInputStream *aPostData, nsILocalFile *aFile)
>+    nsIURI *aURI, nsIInputStream *aPostData, nsIURI *aFileURI)
> {
>     NS_ENSURE_TRUE(mFirstAndOnlyUse, NS_ERROR_FAILURE);
>     mFirstAndOnlyUse = PR_FALSE; // Stop people from reusing this object!
>-    return SaveURIInternal(aURI, aPostData, aFile, PR_FALSE);
>+
>+    nsCOMPtr<nsIURI> fileAsURI;
>+    nsXPIDLCString fileURL;
>+    PRBool isFile = PR_FALSE;
>+
>+    nsresult rv = aFileURI->GetSpec(getter_Copies(fileURL));
>+    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
>+    NS_NewURI(getter_AddRefs(fileAsURI), fileURL.get());

Is there some reason you don't want to return NS_ERROR_FAILURE instead of rv?
I'm not clear on why you are creating an nsIURI from the nsIURI passed in.
If you can't use the parameter passed in, is there a way you can clone the
nsIURI
as well as any nsIFile data that may be available if the caller passed in an
nsIFileURL?



> NS_IMETHODIMP nsWebBrowserPersist::SaveDocument(
>-    nsIDOMDocument *aDocument, nsILocalFile *aFile, nsILocalFile *aDataPath)
>+    nsIDOMDocument *aDocument, nsIURI *aFileURI, nsIURI *aDataPathURI,
>+    const char *aOutputContentType, PRUint32 aEncodingFlags, PRUint32 aWrapColumn)
> {
>     NS_ENSURE_TRUE(mFirstAndOnlyUse, NS_ERROR_FAILURE);
>     mFirstAndOnlyUse = PR_FALSE; // Stop people from reusing this object!
>-    return SaveDocumentInternal(aDocument, aFile, aDataPath);
>+
>+    nsCOMPtr<nsIURI> fileAsURI;
>+    nsCOMPtr<nsIURI> datapathAsURI;
>+
>+    nsXPIDLCString fileURL;
>+    PRBool isFile = PR_FALSE;
>+
>+    nsresult rv = aFileURI->GetSpec(getter_Copies(fileURL));
>+    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
>+    NS_NewURI(getter_AddRefs(fileAsURI), fileURL.get());
>+    fileAsURI->SchemeIs("file", &isFile);
>+    NS_ENSURE_TRUE(isFile, NS_ERROR_INVALID_ARG);

Again, I'm not clear why you are creating an nsIURI instead of using the one
passed
in or cloning the one you have.  The above behavior (uri->string->uri) will
introduce 
bugs on the Macintosh.



>+    if (aDataPathURI)
>+    {
>+        nsXPIDLCString datapathURL;
>+        rv = aDataPathURI->GetSpec(getter_Copies(datapathURL));
>+        NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
>+        rv = NS_NewURI(getter_AddRefs(datapathAsURI), datapathURL.get());
>+        NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
>+        datapathAsURI->SchemeIs("file", &isFile);
>+        NS_ENSURE_TRUE(isFile, NS_ERROR_INVALID_ARG);
>+    }

Again, this is lossy on Mac...


>@@ -739,13 +891,36 @@
> 
> nsresult
> nsWebBrowserPersist::MakeOutputStream(
...
>+    nsresult rv = MakeOutputStreamFromFile(localFile, aCalcFileExt, aChannel, aOutputStream);
>+    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
>+
>+    // We have to do this to ensure any changes to the file path (extension appended etc.)
>+    // are propogated to the URI for fixup purposes.
>+    nsXPIDLCString newURL;
>+    rv = localFile->GetURL(getter_Copies(newURL));
>+    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
>+    rv = aFile->SetSpec(newURL.get());
>+    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);

The above is potentially lossy; is there a better way to do this?


>Index: mozilla/embedding/tests/mfcembed/BrowserView.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/embedding/tests/mfcembed/BrowserView.cpp,v
>retrieving revision 1.25
>diff -u -r1.25 BrowserView.cpp
>--- mozilla/embedding/tests/mfcembed/BrowserView.cpp	1 Jan 2002 12:58:27 -0000	1.25
>+++ mozilla/embedding/tests/mfcembed/BrowserView.cpp	7 Jan 2002 18:37:20 -0000
>@@ -689,23 +689,32 @@
>+            nsCOMPtr<nsIURI> fileURI;
>+            nsCOMPtr<nsIURI> datapathURI;
>+
>             nsCOMPtr<nsILocalFile> file;
>             NS_NewLocalFile(T2A(pStrFullPath), TRUE, getter_AddRefs(file));
>+            nsXPIDLCString fileURL;
>+            file->GetURL(getter_Copies(fileURL));
>+            NS_NewURI(getter_AddRefs(fileURI), fileURL.get());

lossy... (see above examples)


>@@ -838,11 +847,15 @@
>         nsCOMPtr<nsIWebBrowserPersist> persist(do_QueryInterface(mWebBrowser));
> 		if(persist)
>         {
>+            nsCOMPtr<nsIURI> fileURI;
>             nsCOMPtr<nsILocalFile> file;
>             NS_NewLocalFile(strFullPath.GetBuffer(0), TRUE, getter_AddRefs(file));
>-			persist->SaveURI(linkURI, nsnull, file);
>+            nsXPIDLCString fileURL;
>+            file->GetURL(getter_Copies(fileURL));
>+            NS_NewURI(getter_AddRefs(fileURI), fileURL.get());

lossy; see above examples...


> void CBrowserView::OnSaveImageAs()
>@@ -884,11 +897,15 @@
>         nsCOMPtr<nsIWebBrowserPersist> persist(do_QueryInterface(mWebBrowser));
> 		if(persist)
>         {
>+            nsCOMPtr<nsIURI> fileURI;
>             nsCOMPtr<nsILocalFile> file;
>             NS_NewLocalFile(strFullPath.GetBuffer(0), TRUE, getter_AddRefs(file));
>-			persist->SaveURI(linkURI, nsnull, file);
>+            nsXPIDLCString fileURL;
>+            file->GetURL(getter_Copies(fileURL));
>+            NS_NewURI(getter_AddRefs(fileURI), fileURL.get());

lossy; see above examples...


>Index: mozilla/embedding/tests/winEmbed/winEmbed.cpp
>===================================================================
>@@ -420,17 +421,26 @@
>         // Save away
>         nsCOMPtr<nsIWebBrowserPersist> persist(do_QueryInterface(aWebBrowser));
> 
>+        nsCOMPtr<nsIURI> fileURI;
>+        nsCOMPtr<nsIURI> datapathURI;
>+
>         nsCOMPtr<nsILocalFile> file;
>         NS_NewLocalFile(szFile, TRUE, getter_AddRefs(file));
>+        nsXPIDLCString fileURL;
>+        file->GetURL(getter_Copies(fileURL));
>+        NS_NewURI(getter_AddRefs(fileURI), fileURL.get());

lossy; see above examples...


>Index: mozilla/xpfe/communicator/resources/content/contentAreaUtils.js
>===================================================================
>@@ -359,6 +361,15 @@
>+function makeURIFromFile(aFile)
>+{
>+  const stdURLContractID = "@mozilla.org/network/standard-url;1";
>+  const stdURLIID = Components.interfaces.nsIURI;
>+  var uri = Components.classes[stdURLContractID].createInstance(stdURLIID);
>+  uri.spec = aFile.URL;
>+  return uri;
> }

see JS comment above for how to fix this function.


>Index: mozilla/xpfe/components/ucth/resources/helperAppDldProgress.js
>===================================================================
>+function makeURIFromFile(aFile)
>+{
>+  const stdURLContractID = "@mozilla.org/network/standard-url;1";
>+  const stdURLIID = Components.interfaces.nsIURI;
>+  var uri = Components.classes[stdURLContractID].createInstance(stdURLIID);
>+  uri.spec = aFile.URL;
>+  return uri;
>+}
>+

see JS comment above for how to fix this function.
Can we instead put this function in a better JS file where more places can use
it
(without duplicating the code)?

Comment 38

[Adam Lock]

Rick can you look at the "other final patch".

My preference is for that version because it is much cleaner on the embedding
side, relieving embedders of the inconvenient and easy to screw up task of
converting a file into a uri, especially when it can be lossy on the mac.

Comment 39

[Kathleen :Brade]

Comment on attachment 63984 [details] [diff] [review]
The other final patch :) (nsISupports)

The following files are all fine with me (r=brade):
>Index: mozilla/embedding/browser/powerplant/source/CBrowserShell.cpp
>Index: mozilla/embedding/browser/webBrowser/nsWebBrowser.cpp
>Index: mozilla/embedding/components/webbrowserpersist/public/nsIWebBrowserPersist.idl
>Index: mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.h
>Index: mozilla/xpfe/components/ucth/resources/helperAppDldProgress.js


These changes are also r=brade assuming the points I make below are addressed
in 
whatever is checked in:

>Index: mozilla/embedding/browser/activex/src/control/MozillaBrowser.cpp
>===================================================================
>+#include "nsNetUtil.h"

Is this include still needed?


>Index: mozilla/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
>===================================================================
>+nsresult nsWebBrowserPersist::GetLocalFileFromURI(nsIURI *aURI, nsILocalFile **aLocalFile) const
>+{
...
>+    nsCOMPtr<nsIFileURL> fileURL = do_QueryInterface(aURI, &rv);

I think we need to check isScheme("file") before we QI to get nsIFileURL.


> nsresult
> nsWebBrowserPersist::MakeOutputStream(
>+    nsIURI *aFile, PRBool aCalcFileExt,
>     nsIChannel *aChannel, nsIOutputStream **aOutputStream)
> {
...
>+    // We have to do this to ensure any changes to the file path (extension appended etc.)
>+    // are propogated to the URI for fixup purposes.
>+    nsXPIDLCString newURL;
>+    rv = localFile->GetURL(getter_Copies(newURL));
>+    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
>+    rv = aFile->SetSpec(newURL.get());

Can you explain what scenario would require this "fixup"?  
Is there any way we could do the fixup somewhere else where it wouldn't be
lossy?


>Index: mozilla/embedding/tests/mfcembed/BrowserView.cpp
>===================================================================
>@@ -689,8 +689,11 @@
> 
>         // Save the file
>         nsCOMPtr<nsIWebBrowserPersist> persist(do_QueryInterface(mWebBrowser));
>+        if(persist)
>         {
>+            nsCOMPtr<nsIURI> fileURI;
>+            nsCOMPtr<nsIURI> datapathURI;

Are these still necessary or can these 2 lines be removed?



>Index: mozilla/embedding/tests/winEmbed/winEmbed.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/embedding/tests/winEmbed/winEmbed.cpp,v
>retrieving revision 1.48
>diff -u -r1.48 winEmbed.cpp
>--- mozilla/embedding/tests/winEmbed/winEmbed.cpp	1 Jan 2002 12:58:32 -0000	1.48
>+++ mozilla/embedding/tests/winEmbed/winEmbed.cpp	8 Jan 2002 18:29:35 -0000
>@@ -50,6 +50,7 @@
> // Mozilla header files
> #include "nsEmbedAPI.h"
> #include "nsWeakReference.h"
>+#include "nsNetUtil.h"
> #include "nsIClipboardCommands.h"
> #include "nsXPIDLString.h"
> #include "nsIWebBrowserPersist.h"
>@@ -420,6 +421,9 @@
>         // Save away
>         nsCOMPtr<nsIWebBrowserPersist> persist(do_QueryInterface(aWebBrowser));
> 
>+        nsCOMPtr<nsIURI> fileURI;
>+        nsCOMPtr<nsIURI> datapathURI;

Are these still needed?  
Do you still need the #include you add above?

Comment 40

[Adam Lock]

The issue with lossy MakeOutputStream has been solved by Darin's suggestion in
bug 118651 to just call SetFile to resync the URI to the file - no need to get
the spec from the file and set it on the uri.

+    // Resync the URI with the file after the extension has been appended
+    
+    nsCOMPtr<nsIFileURL> fileURL = do_QueryInterface(aURI, &rv);
+    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
+    fileURL->SetFile(localFile);
+

The unused variables in the embedding samples has been fixed too.

Comment 41

[rpotts (gone)]

Comment on attachment 63984 [details] [diff] [review]
The other final patch :) (nsISupports)

sr=[e-mail]

Comment 42

[Adam Lock]

Fix is checked in. Thanks everyone!

Comment 43

[Greg K.]

Removing dependency on unresolved bug 114456 since this resolved bug obviously
doesn't depend on it.