90918 - RFE: Add url in "comments" field of the "get info" window when download file.

Status: NEW

(Firefox :: File Handling, enhancement, P4)

Description

[Benjamin Mucci]

Add the url from where files are downloaded in the "Comments" field of the "Get
info" window in Finder... as in NS.

Comment 1

[timeless]

ok so there are two halves here, right?
a. need to be able to stick strings into comments field [current assignment]
b. need to do so [can split to new bug, or handle later, would be networking 
http or xpapps]

Comment 2

[Doug Turner (:dougt)]

apps team has everything they need from necko.

They will need some platform specific code to do the comment handling, but that 
is not necko.

Comment 3

[sairuh (rarely reading bugmail)]

spam: over to File Handling. i have not changed the assigned developer [or the
other fields for that matter], so if anyone realizes that a bug should have a
more appropriate owner, go ahead and change it. :)

Comment 4

[sairuh (rarely reading bugmail)]

would be nice, marking helpwanted, 4xp, p4, and future

Comment 5

[Andy Lyttle]

Do any other platforms have someplace to store comments or other attributes
about a file?  It's remarkably useful...

Comment 6

[Asa Dotzler [:asa]]

->default owner

Comment 7

[Bill Law]

restoring target milestone

Comment 8

[Andrew Hagen]

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

Comment 9

[Andrew Hagen]

Moving over the changes from duplicate bug 112671, including the target
milestone. Not sure that is right, though.

Comment 10

[Greg K.]

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

Comment 11

[timeless]

ntfs supports a comment field (and other arbitrary fields)

do we really have api access to this?

Comment 12

[Bill McGonigle (not currently reading bugmail; please contact directly)]

I think these are the calls to use:

PBDTSetComment:
http://developer.apple.com/techpubs/mac/MoreToolbox/MoreToolbox-502.html
Carbon:
http://developer.apple.com/techpubs/macosx/Carbon/Files/FileManager/File_Manager/Functions/Adding_to_th_top_Database.html#//apple_ref/c/func/PBDTSetCommentSync
async version:
http://developer.apple.com/techpubs/macosx/Carbon/Files/FileManager/File_Manager/Functions/Adding_to_th_top_Database.html#//apple_ref/c/func/PBDTSetCommentAsync

In response to comment #5, you could probably get any BeOS user to wax nostalgic
about the filesystem as a database.

Comment 13

[Jo Hermans]

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

Comment 14

[Steve Dagley]

As an FYI - the Mac OS X Finder doesn't use the same mechanism as Mac OS 9 and
earlier for file comments.  Currently the only way to set file comments for the
OS X Finder is via sending an AppleEvent.  Where that's documented I'm not sure.

Comment 15

[Andy Lyttle]

10.2 fixes this; the Finder now displays comments in the Info window.  You have
to flip down a triangle.

Comment 16

[Jo Hermans]

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

Comment 17

[Vedran Miletic]

retargeting

Comment 18

[Boris Zbarsky [:bzbarsky]]

.

Comment 19

[guanxi]

See also:
bug 154580 -- a very similar RFE for Mac
bug 125729 -- a similar RFE for all platforms

Comment 20

[Boris Zbarsky [:bzbarsky]]

> bug 154580 -- a very similar RFE for Mac

For a different browser, which doesn't share much of the relevant code with
Mozilla...

Comment 21

[Bill McGonigle (not currently reading bugmail; please contact directly)]

The URL I gave above changed:
http://developer.apple.com/documentation/Carbon/Reference/File_Manager/index.html#//apple_ref/c/func/PBDTSetCommentAsync

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Not be flippant, but could someone ask the author of the Safari plugin
DownloadComment http://www.ecamm.com/mac/free/ what calls/API he/she uses (if
that's the problem here, rather than time or priority) to get this to work?  Or
better yet, the author of iCab http://www.icab.de/ since iCab actually stores
the URLs in the comments 100% of the time and the Safari plugin barely manages 50%.

It's sad that Netscape pioneered this feature on the Mac but none of NS's
descendants have it anymore :(

Comment 23

[Boris Zbarsky [:bzbarsky]]

The problem is not so much lack of API knowledge in particular but lack of
developers with Mac access in general (needed to write, test, etc the patch).

Comment 24

[Jo Hermans]

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

Comment 25

[timeless]

i don't know why i didn't mention it earlier, but of course befs also has slots
for comment fields ...

Comment 26

[Torben]

I have posted a patch <https://bugzilla.mozilla.org/attachment.cgi?id=179614>
for doing this for Camino in bug 154580. This can probably be modified to work
for Mozilla/Firefox too.

Comment 27

[Torben]

Attachment: Saves the URL as a Finder comment for drag and drops

This will add the URL as a Finder comment for images draged and dropped. Most
of the code should probably be moved somewhere else (any hints where?) so it
can be reused for the other file save operations . Also need to find out where
this is done...

Comment 28

[Torben]

BTW, the above patch is only tested with Camino.

Comment 29

[Simon Fraser [no longer active]]

Comment on attachment 179991 [details] [diff] [review]
Saves the URL as a Finder comment for drag and drops

I'd like to see a version of this patch without Hungarian notation, which
follows the Mozilla coding style more closely. Please also rename the "LessAE"
methods.

Also, explain:

+// This need to be an independent function or the compiler will fail

Comment 30

[Torben]

Attachment: Cleaned up patch, moving review requests

> I'd like to see a version of this patch without Hungarian notation, which
> follows the Mozilla coding style more closely.

Ok, I've tried.

>Please also rename the "LessAE" methods.

Apple's original code
<http://developer.apple.com/samplecode/MoreAppleEvents/MoreAppleEvents.html>
uses More* so I used Less* for my trimmed-down version. Renamed to Do* in lack
of better ideas.

> Also, explain:
> +// This need to be an independent function or the compiler will fail

If I try to do

someDesc->descriptorType = typeNull;
someDesc->dataHandle = nil;

in the main SetComment function, the compiler throws an error (invalid
conversion from (AEDesc *) to (AEDesc), or something similar), even with
&someDesc or (AEDesc *)someDesc. Moving this into a separate function makes
everything work well.

Comment 31

[Mike Pinkerton (not reading bugmail)]

+pascal OSErr DoFESetCommentCFString(const CFURLRef aFile, const CFStringRef
aComment)
+pascal void DoAEDisposeDesc(AEDesc* aDesc)

why are these declared |pascal|? Who is calling them besides our C++ code?

+  CFStringRef mFile = CFURLCopyFileSystemPath(aFile, kCFURLHFSPathStyle);
+
+  if (!mFile) return coreFoundationUnknownErr;

the "m" prefix is used for member variables in mozilla, not locals. please
rename these.

+  if ((nsnull == aComment) && (nil == &theComment)) return paramErr;

nit, how we normally write this in mozilla is:

if (!aComment && !&theComment)
  return paramError;

+  if (nil == data)

same as above

if (!data)
 ...

+  DisposePtr((Ptr) data);

nit, on toolbox calls, we try to avoid picking up a scoped c++ methods by using
the global scoping operator (::). So this instance would be:

 ::DisposePtr((Ptr)data));

looks reasonable otherwise. does this only happen when dragging files? what
about when downloading them via normal means? shouldn't this AE code live
somewhere more generic so it can be reused by the download code as well?

Comment 32

[Torben]

Attachment: patch with pinkerton's comments addresed

Comment 33

[Torben]

Comment on attachment 181275 [details] [diff] [review]
patch with pinkerton's comments addresed

> does this only happen when dragging files?
> what about when downloading them via normal means?

For the moment this is drag and drop only, still need to figure out where the
other file saving code lives
(uriloader/exthandler/nsExternalHelperAppService.cpp seems most promissing,
will test some more).

> shouldn't this AE code live somewhere more generic
> so it can be reused by the download code as well?

Yes, but where would that be? Maybe inside nsILocalFileMac would work?

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

> Yes, but where would that be? Maybe inside nsILocalFileMac would work?

a generic interface would be nice, so that other OSes could implement that too
(windows, beos come to mind)

Comment 35

[Torben]

Attachment: Work in progress

Put most of the Apple-specific stuff into LocalFileOSX

> still need to figure out where the other file saving code lives
> (uriloader/exthandler/nsExternalHelperAppService.cpp seems most promissing,

This works for click-to-save (some problems when downloading multiple files at
the same time), but not for Save as..., the download manager is probably a
better place. Unfortuanely this, AFAICT all the save as... code, is
application-specific so I need to figure out how to make all three
(Camino/FF/Moz) coexist nicely before I can fix this :-( 

> a generic interface would be nice, so that other OSes could implement that
too

Any pointers/examples for how I do this?

Comment 36

[Torben]

Attachment: Patch ver. 2, works for all file saves

This now works for all kinds of file saves and in both Firefox and Mozilla, and
with the patch in bug 154580 for Camino.

The actual code for setting the comment is in LocalFilesMac, for drag & drop
this is called from nsDragService (which is mac-specific), for Save as... and
click-to-save this is done through a service in
nsExternalHelperAppService/nsOSHelperAppService called from the download
manager code. Code for other platforms can be added in the relevant
nsOSHelperAppService, the default is to do nothing.

Comment 37

[timeless]

Comment on attachment 182481 [details] [diff] [review]
Patch ver. 2, works for all file saves

>Index: toolkit/components/downloads/src/nsDownloadManager.cpp
>@@ -300,6 +301,9 @@
>     AssertProgressInfoFor(aPath);
>     
>     nsDownload* download = NS_STATIC_CAST(nsDownload*, mCurrDownloads.Get(&key));
this really shouldn't be ifdef'd:
>+#if defined (XP_MACOSX)
>+    download->SetURLAsFileComment();
>+#endif

>@@ -2272,3 +2276,15 @@

ns_imethodimp pairs w/ ns_imethod not with nsresult. should this be virtual?
this function is confusingly named relative to the interace method which takes
2 parameters.
>+NS_IMETHODIMP
>+nsDownload::SetURLAsFileComment()

>Index: toolkit/components/downloads/src/nsDownloadManager.h
>@@ -226,6 +226,7 @@
>+  nsresult SetURLAsFileComment();

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp
>@@ -1018,6 +1018,12 @@
>+NS_IMETHODIMP nsExternalHelperAppService::SetURLAsFileComment(nsIURI* aFile, nsIURI* aURL)
>+{
afaik you aren't checking the rv anyway. perhaps you should return not
implemented and comment at the call site that you're intentionally ignoring the
result?
>+  // this method may be implemented by a OS specific implementation of this service, else do nothing.
>+  return NS_OK; // NS_ERROR_NOT_IMPLEMENTED ?

>Index: uriloader/exthandler/nsIExternalHelperAppService.idl
>@@ -70,6 +70,16 @@
>+   * Sets the originating URL as a OS-specific comment to the file, not implemented
>+   * for all OSes.
add @throws explaining what you expect people to do if they don't implement it
:)
>+   */
>+  void setURLAsFileComment(in nsIURI aFile, in nsIURI aURL);

>Index: uriloader/exthandler/mac/nsOSHelperAppService.cpp
>@@ -187,6 +215,30 @@
>+NS_IMETHODIMP nsOSHelperAppService::SetURLAsFileComment(nsIURI* aFile, nsIURI* aURL)
for public methods (NS_IMETHODIMP) you need to NS_ENSURE_ARG_POINTER() before
you dereference.

>Index: xpcom/io/nsILocalFileMac.idl
>@@ -198,6 +198,16 @@
>+    * setFinderComment
i'd rather this be a method on nsILocalFile2 and it was setComment, so that we
can share among all platforms...

>+    * @param   aComment   A UTF8 encoded nsACString containing the comment.
use AUTF8String ?
>+    void setFinderComment(in ACString aComment);

>Index: xpcom/io/nsLocalFileOSX.cpp
>@@ -1663,6 +1663,100 @@
>+/* void setFinderComment(in ACString aComment); */
>+NS_IMETHODIMP nsLocalFile::SetFinderComment(const nsACString& aComment)
>+{
How do i clear a file's comment?
>+  if (aComment.IsEmpty())
>+    return NS_ERROR_INVALID_ARG;

>+  StAEDesc theComment;
>+  if ((!comment) || (!&theComment))

please excuse my ignorance, what does !&theComment mean?

>Index: xpfe/components/download-manager/src/nsDownloadManager.cpp
>@@ -219,6 +220,9 @@
>   nsDownload* dl = mCurrDownloads.GetWeak(aTargetPath);
>   if (dl) {
>     AssertProgressInfoFor(aTargetPath);
this shouldn't be ifdef'd

>+#if defined (XP_MACOSX)
>+    dl->SetURLAsFileComment();
>+#endif

>@@ -1394,3 +1398,16 @@
>+NS_IMETHODIMP
>+nsDownload::SetURLAsFileComment()

ns_imethodimp pairs w/ ns_imethod not with nsresult. should this be virtual?

>Index: xpfe/components/download-manager/src/nsDownloadManager.h
>@@ -120,6 +120,7 @@
>+  nsresult SetURLAsFileComment();

Comment 38

[Mike Pinkerton (not reading bugmail)]

it sounds like bug 154580 should be rewritten to use this, no?

Comment 39

[Torben]

Attachment: nsILocalFile2 - take 1

timeless:
> i'd rather this be a method on nsILocalFile2 and it was setComment, so that
we
> can share among all platforms...

Would something like this be OK?

pinkerton:
> it sounds like bug 154580 should be rewritten to use this, no?

I prefer the AppleScript-solution, but you're the boss :-)

Comment 40

[timeless]

Comment on attachment 182724 [details] [diff] [review]
nsILocalFile2 - take 1

this would be fine with me. now we just need to convince doug or darin :).

Comment 41

[Bill McGonigle (not currently reading bugmail; please contact directly)]

(In reply to comment #39)
>
> I prefer the AppleScript-solution, but you're the boss :-)

I've heard Apple folk say, "don't count on the Finder running," though I'm not sure under which 
circumstances the Finder wouldn't be running.

Comment 42

[Andy Lyttle]

Currently, under normal circumstances, the Finder should always be running; there's no UI to quit it, 
and it will be relaunched automatically if it crashes or you Force Quit.  However, you can quit the Finder 
by sending it an AppleEvent quit message (e.g. osascript -e 'tell application "Finder" to quit').  I expect 
this to hold true for quite awhile longer, but what if Apple one day decides to replace the Finder with 
some new alternative kind of shell?  Or, what if a third party decided to offer a Finder replacement (sort 
of like LiteStep is an Explorer replacement for Windows)?  It's a good idea for applications to not rely on 
the Finder being available if there's a better way.

Comment 43

[Torben]

Attachment: nsILocalFile2 - take 2

Implementation of nsILocalFile2 for all platforms. nsILocalFile<OS> now
inherits from nsILocalFile2 (which inherits from nsILocalFile) and not directly
from nsILocalFile.

Note: only tested on mac (which is the only platform I have access to).

Updated patch for actually using this comming up soon (after I have finnished
building and testing).

Comment 44

[Christian :Biesinger (don't email me, ping me on IRC)]

why implement this on platforms that do not have any concept of extended
attributes (like comment)?

Comment 45

[Torben]

Re comment 41 and 42:
Under OS X the _only_ way to set a comment is through an AppleEvent (an
AppleScript is just a way of letting the OS create the event) to the Finder. If
the Finder isn't running, you don't get the comment, period.

In fact, the comment is stored in the Finder's database and not in the file (as
under Mac classic), you can easily test this by creating a file (foo.bar),
giving this a comment and then delete the file (and empty the trash if you
like). Then create a new file with the same name and do "Get info" - it will
have the same comment as the deleted file.

Re comment 44:
> why implement this on platforms that do not have any concept of
> extended attributes (like comment)?

Ask timeless, I'm perfectly happy with this being a mac-only thing :-)
Alternatively I can go with the nsExternalHelperAppService-approach (maybe even
move the code from nsLocalFileOSX to nsOSHelperAppService), then if someone
wants this for an other OS they will have to create a suitable method in the
relevant nsOSHelperAppService (and nsLocalFile<OS> if needed/wanted).

Comment 46

[Christian :Biesinger (don't email me, ping me on IRC)]

I liked that you put this on nsILocalFile2 :-) But I think that the nsLocalFile
impls for the other platforms should only implement this interface if and when
they actually implement this method.

Comment 47

[Torben]

Attachment: Patch, ver.3 adressing timeless' and biesi's comments

nsILocalFile2 now only implemented an mac (warning added), no ifdef'ing in
nsDownloadManager, SetURLAsFileComment() renamed to SetCommentForDownload()
(maybe just SetComment() would do?),  etc. Also moved some of the code to fit
better with the surrounding code and removed some possible leaks from
SetComment(const nsACString& aComment) if something goes wrong.

>>+  StAEDesc theComment;
>>+  if ((!comment) || (!&theComment))

> please excuse my ignorance, what does !&theComment mean?

Copyed from Apple's code, they don't tell why this is needed but I guess bad
things can happen if theComment is not initialized. This should not be possible
since the StAEDesc-class always will be initialized, but better safe than
sorry...

Comment 48

[Torben]

Attachment: Patch ver. 4, correcting error

Oops, there was one error with the last patch. This is the right one.

Comment 49

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 182858 [details] [diff] [review]
Patch ver. 4, correcting error

r=pink

Comment 50

[Torben]

Comment on attachment 182858 [details] [diff] [review]
Patch ver. 4, correcting error

asking sfraser for sr and doug for r/sr? for the LocalFile2 change.

Comment 51

[Doug Turner (:dougt)]

Comment on attachment 182858 [details] [diff] [review]
Patch ver. 4, correcting error

I am not ready for a nsILocalFile2.  Hang this off of the mac interface if you
must.

Comment 52

[Torben]

Attachment: Patch w/o LocalFile2

Removed all traces of nsILocalFile2 (sorry timeless/biesi), moved the
declaration of SetComment to nsILocalFileMac else as the last patch. Other
OS'es may still hook up through the ExtAppHandlerService.
Moving request for sr (I'm assuming pinkerton's r still holds).

Comment 53

[Doug Turner (:dougt)]

timeless says he wants to implement this for every other platform.  well maybe
he would like to if he had the time.  In any case, we might want to think of a
more general solution.  one idea is to make nsLocalFile impl nsIProperties.  Or
make nsILocalFile2 -- but not freeze it until we have more methods to justify
its existence.  others?

Comment 54

[timeless]

i sure don't think anyone was proposing freezing nsILocalFile2 anytime soon...

Comment 55

[Doug Turner (:dougt)]

you know how these things can happen...  Is there any better solution then
introducing a new interface for file comment information?

Comment 56

[Andy Lyttle]

I certainly agree that we should be thinking cross-platform, with platform-specific implementations.  But 
I'm not a coder. :-)

Comment 57

[Kevin Brosnan]

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

Comment 58

[Mike Kaply [:mkaply]]

Since only certain platforms will do this, why not put the interface in the
platform IDL and #ifdef it?

Comment 59

[OstGote!]

(In reply to comment #5)
> Do any other platforms have someplace to store comments or other attributes
> about a file?  It's remarkably useful...

For Windows (2000, XP, 2003 with NTFS) or Gnome see Bug 267369 which wants the
same for these platforms.

Comment 60

[Rich Walsh]

Unaware of this bug, I independently implemented all of its functionality for
OS/2 (see Bug #301367).  I believe my patch is simpler, slightly more
comprehensive, and easier to implement across platforms (while writing it, I was
thinking "this would be easy for the Mac people to implement").

Rather than messing with 2 toolkit's download managers, my code saves the URL at
exactly the same point in nsExternalHelperAppService that the Mac type & creator
fields are written.  For 'Save as', it taps into nsWebBrowserPersist at two
points to ensure both single & queued multiple requests are tagged.

Like this bug's patch, it modifies native d&d code, and adds a new method to
nsILocalFileXXX that actually saves the URL.  Since the URL isn't a "comment" on
OS/2, the new method has the more generic name 'SetFileSource()'.

Since I expected this would be OS/2-only, there are a fair number of #ifdef's in
the two common files.  A cross-platform implementation would eliminate them and
only require #ifdef's where localfile is QI'd to the appropriate platform's
flavor.  When I complete this overly-long comment, I'll conjure up an X-P patch
for your perusal.

BTW...  I've tested this in Firefox, Thunderbird, and Seamonkey.

Comment 61

[Rich Walsh]

Attachment: alternate cross-platform patch

This patch contains changes to cross-platform code & the nsLocalFileXXX IDLs
needed to implement this feature on Mac & OS/2.  OS-specific code can be
extracted from existing patches and used virtually as-is.

Comment 62

[Torben]

Index: embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp
Re attachment (id=190216)
I'm not a reviewer but this is something I noted:

-#ifdef XP_MAC
+#if defined(XP_MAC)

XP_MAC means mac classic (OS 9 and below), which we don't care about anymore,
use XP_MACOSX instead.

+// tag the file with its source URI
+void
+nsWebBrowserPersist::SaveFileSource(nsIURI *aSrc, nsIURI *aFile)
+{
+#if defined(XP_OS2) || defined(XP_MAC)
.
.
.
+#endif // supported platforms
+    return;

if !defined(XP_OS2) && !defined(XP_MACOSX)
  return;

+
+    if (!aSrc || !aFile)
+        return;

NS_ENSURE_ARG_POINTER(aSrc, aFile);


+    if (platformFile)
+    {
+        nsCString uriString;

nsCAutoString uriString;

+        aSrc->GetSpec(uriString);

Do we know what character encoding the uri has?


Index: uriloader/exthandler/nsExternalHelperAppService.cpp
===================================================================

+// tag the file with its source URI
+static void SaveFileSource(nsIURI *aSrc, nsIFile *aFile)

Why do you need to repeat the function here? What happens if someone updates one
of the functions but not the other?

I will try to get time to test the patch on mac during the weekend.

Comment 63

[Torben]

> Do we know what character encoding the uri has?

AIUI (from nsIURI.idl) this will be UTF8, if so we probably want
    void setFileSource( in AUTF8String aURI );
in the nsILocalFileXXX.idl

Comment 64

[Rich Walsh]

(In reply to comment #62)
> +void
> +nsWebBrowserPersist::SaveFileSource(nsIURI *aSrc, nsIURI *aFile)
> +{
> +#if defined(XP_OS2) || defined(XP_MAC)
> ....
> +#endif // supported platforms
> +    return;
> 
> if !defined(XP_OS2) && !defined(XP_MACOSX)
>   return;

Are saying "negate the test at the top & put the return there"?  That leaves a
bunch of meaningless code in place for other platforms that probably won't
compile.  It would have to be bracketed with #else...#endif to achieve the same
result.  A matter of style, I suppose.

> +    if (!aSrc || !aFile)
> +        return;
> 
> NS_ENSURE_ARG_POINTER(aSrc, aFile);

Doesn't this require that the method have a return value?  I used a void return
because a failure here is of no real consequence.  Plus, IIRC, in one of the two
files a URI may not be available - it's not an "error", just "circumstances".

> +    if (platformFile)
> +    {
> +        nsCString uriString;
> 
> nsCAutoString uriString;

Maybe.  Is the typical URI for saved content gt or lt 64 bytes?

> +        aSrc->GetSpec(uriString);
> 
> Do we know what character encoding the uri has?

The URI is UTF-8 with some escaped characters.  Based on the comments at the top
of nsIURI.idl, I decided to leave it alone.

> Index: uriloader/exthandler/nsExternalHelperAppService.cpp
> ===================================================================
> 
> +// tag the file with its source URI
> +static void SaveFileSource(nsIURI *aSrc, nsIFile *aFile)
> 
> Why do you need to repeat the function here? What happens if someone updates
> one of the functions but not the other?

This is a pretty trivial function that was originally in-line code.  I split it
out of SetUpTempFile() which calls it to reduce complexity there & concentrate
the #ifdefs in one place.  Also, note that its signature is different than the
(private) method in nsWebBrowserPersist.  Having the same name helps ensure that
an LXR query will point future updaters to both.

> I will try to get time to test the patch on mac during the weekend.

Great!

Comment 65

[Rich Walsh]

(In reply to comment #63)
> > Do we know what character encoding the uri has?
> 
> AIUI (from nsIURI.idl) this will be UTF8, if so we probably want
>     void setFileSource( in AUTF8String aURI );
> in the nsILocalFileXXX.idl

Sounds good.

Comment 66

[Christian :Biesinger (don't email me, ping me on IRC)]

why not use a cross-platform interface that all platforms can implement, that
has a function like setMetaData(PRUint32 type, AString value)? that avoids
having to have ifdefs in consumers, and means that platforms can just implement
it and things work.

Comment 67

[Rich Walsh]

(In reply to comment #66)
> why not use a cross-platform interface that all platforms can implement

My initial reaction was "where does it go that it will be available to
embedding, uriloader, and possibly widget?".  I see that IDLs for
uriloader/exthandler are processed first.  I suppose a new interface could be
put in that directory (darned if I know what to call it).  It would be
implemented by nsExternalHelperAppService.cpp as an NS_ERROR_NOT_IMPLEMENTED,
and could be overridden by nsOSHelperAppService.cpp on those platforms that want
to support the feature.  Comments?

Comment 68

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #67)
> (In reply to comment #66)
> > why not use a cross-platform interface that all platforms can implement
> 
> My initial reaction was "where does it go that it will be available to
> embedding, uriloader, and possibly widget?".

xpcom/io/nsILocalFile2.idl (or somesuch)

> It would be
> implemented by nsExternalHelperAppService.cpp as an NS_ERROR_NOT_IMPLEMENTED,
> and could be overridden by nsOSHelperAppService.cpp on those platforms that want
> to support the feature.  Comments?

I guess. why not implement it on the nsLocalFile impls, though?

Comment 69

[Rich Walsh]

(In reply to comment #68)
> I guess. why not implement it on the nsLocalFile impls, though?

If this could be added to nsIFile or nsILocalFile, we'd all be enjoying this
feature by now.  And, there is no nsILocalFile2 AFAICT.  Declaring
SetFileSource() in nsLocalFile.h and implementing it as a no-op in
nsLocalFileCommon.cpp would certainly be expedient - but would that conform to
"accepted practises"?

Comment 70

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #69)
> And, there is no nsILocalFile2 AFAICT.

Indeed. Why not create one?

Comment 71

[Rich Walsh]

Attachment: hybrid cross-platform patch

Combines the best aspects of the original & alternate patches:	a new method
added to nsIExternalHelperAppService, minimal changes to other XP code, and no
#ifdefs.  This patch contains only cross-platform code.

Comment 72

[Rich Walsh]

Attachment: Mac-specific patch

Companion to "hybrid cross-platform patch".  Contains Mac-specific code taken
from the original patch with only minimal changes.

Comment 73

[Torben]

Re attachment (id=190295) 

This works good except for one major flaw, it doesn't set the comment for
click-to-save (ie when you don't get a save dialogue) :-( Save As... and
drag-and-drop works fine (tested with Camino and Firefox). 
You also calls helperAppService->SaveFileSource from three different places, are
we sure we don't end up setting the comment several times? This will not cause
any problems AFAICT but would probably be nice to avoid anyway.

It's good you have been able to get this into the back-end code, I'll see if I
can fix the above problems, if Rich doesn't beat me to it :-)

Re the mac-specific patch, since <nsIURI>->GetSpec() seems to be UTF8-encoded we
can probably remove the UnescapeFragment()-functions. We probably also want to
use the same name for the nsILocalFileXXX-functions, this will make it easier if
we ever should implement LocalFile2. I can take care of the mac-specific parts.

>> And, there is no nsILocalFile2 AFAICT.
> Indeed. Why not create one?

Could the powers that be (dougt, darin ?) make a decision here?

Comment 74

[Rich Walsh]

(In reply to comment #73)
> Re attachment (id=190295) [edit] 
> 
> This works good except for one major flaw, it doesn't set the comment for
> click-to-save (ie when you don't get a save dialogue)

I'm not seeing this - everything I save is tagged.  (I assume "click to save"
means that you had previously checked "Do this automatically..." in the Download
dialog so that specific file types are saved without prompting.)  IIRC, these
files should get tagged at line 1584 in nsExternalHelperAppService.cpp after the
temp d/l file has been created - and right after Mac type & creator are set.

> You also calls helperAppService->SaveFileSource from three different places,
> are we sure we don't end up setting the comment several times? This will not
> cause any problems AFAICT but would probably be nice to avoid anyway.

There are several different paths thru WebBrowserPersist which I found by trial
and error.  E.g. Save as "Web page, complete" & "Web page, html only" take
different routes.  I'll add some logging to my copy to confirm SaveFileSource()
only gets called once for each file.

> Re the mac-specific patch, [...] We probably also want to use the same name
> for the nsILocalFileXXX-functions, this will make it easier if we ever should
> implement LocalFile2. I can take care of the mac-specific parts.

I was trying to impose as few changes on your work as possible.  If you do
change it, please use "setFileSource" as it is more generic.

> >> And, there is no nsILocalFile2 AFAICT.
> > Indeed. Why not create one?
> 
> Could the powers that be (dougt, darin ?) make a decision here?

This was shot down once, why pursue it?  Creating a major new interface to
support a single, rather trivial method isn't very compelling (IMHO).

Comment 75

[Rich Walsh]

Attachment: OS/2-specific patch

Code needed to implement source-tagging on OS/2.  Also contains ancillary
changes to nsLocalFileOS2 & nsDragService(OS2) described in Bug #301367.

Comment 76

[Rich Walsh]

Attachment: OS/2-specific patch

previous patch was missing some #includes

Comment 77

[Torben]

(In reply to comment #74)

> > This works good except for one major flaw, it doesn't set the comment for
> > click-to-save (ie when you don't get a save dialogue)

> I'm not seeing this - everything I save is tagged. [snip correct assuming] 
> these files should get tagged at line 1584 in nsExternalHelperAppService.cpp 
> after the temp d/l file has been created - and right after Mac type & creator
> are set.

The problem is that we are tagging the temporary file, and on OS X this info
will not be copied to the final file. And we can't tagg the final file here
since it doesn't exist yet :-( The first time I tried to add this I found where
to do this, looking through the code to try to find this again... (BTW, I don't
think type & creator are set on OS X, these are just dummy functions.)
 
> > Re the mac-specific patch, [...] We probably also want to use the same name
> > for the nsILocalFileXXX-functions

> please use "setFileSource" as it is more generic.

Will do.
 
Re nsILocalFile2 
> This was shot down once, why pursue it?  Creating a major new interface to
> support a single, rather trivial method isn't very compelling (IMHO).

I don't really care either way - a nsILocalFile2-implementation might be cleaner
if we ever want to be able to create comments for more than downloads (ie from
composer) - but a final decission would be nice (timeless and biesi seems to
really want this, dougt didn't really say no in comment 53). But until further
notice, I think we should keep working on the exthandler-implementation.

Comment 78

[Rich Walsh]

(In reply to comment #77)
> 
> > > This works good except for one major flaw, it doesn't set the comment for
> > > click-to-save (ie when you don't get a save dialogue)
> 
> The problem is that we are tagging the temporary file, and on OS X this info
> will not be copied to the final file.

I've got a handle on this - I'll fix it.

Comment 79

[Rich Walsh]

Attachment: revised cross-platform patch

defers tagging downloads until d/l is complete & file has been moved or renamed

Comment 80

[Torben]

Re attachment (id=190361)

> defers tagging downloads until d/l is complete & file has been moved or renamed

I now remember why I moved this to the front-end code, this placement gives
really strange behaviour: if you download more than one file at the same time
the source will not be set (and on Firefox no comment after this, Camino seems
to recover fine). More curriously evry second time you launch FF/Camino the
comment will not be set at all even for single downloads.

Comment 81

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 190361 [details] [diff] [review]
revised cross-platform patch

+  void saveFileSource(in nsIURI aFile, in nsIURI aSource);

why's this an nsIURI instead of a nsIFile or nsILocalFile? (for aFile)

Comment 82

[Rich Walsh]

(In reply to comment #81)
> (From update of attachment 190361 [details] [diff] [review] [edit])
> +  void saveFileSource(in nsIURI aFile, in nsIURI aSource);
> 
> why's this an nsIURI instead of a nsIFile or nsILocalFile? (for aFile)

There's one call in nsExternalHelperAppService where the data is an nsIFile, but
three such calls in nsWebBrowserPersist where it's an nsIURI.  The latter
generates more invocations (e.g. save page, complete) to a method that may not
even be implemented.  Why create a bunch of nsIFiles that may never be used?

Comment 83

[Rich Walsh]

(In reply to comment #80)
> Re attachment (id=190361) [edit]
> 
> if you download more than one file at the same time the source will not be
> set (and on Firefox no comment after this, Camino seems to recover fine).
> More curriously evry second time you launch FF/Camino the comment will not
> be set at all even for single downloads.

It's 100% reliable here - I'd look at platform-specific code.  Do you have any
Mac apps that also attach comments to their files - do they have reliability
issues?  Also, do settings like "ask before saving" and "show download manager"
affect this behavior?

Comment 84

[Christian :Biesinger (don't email me, ping me on IRC)]

most platforms will implement that method (windows, os/2, beos, mac). it makes
no sense for that method to take an nsIURI. it can only save stuff in local files.

Comment 85

[Torben]

Attachment: Example code

> (From update of attachment 190361 [details] [diff] [review])
> +  void saveFileSource(in nsIURI aFile, in nsIURI aSource);
> 
> why's this an nsIURI instead of a nsIFile or nsILocalFile? (for aFile)

We probably should declare aFile as nsISupports, and then do the neccesary
conversion within the function, see the attached example (this works fine,
except for the alreay mentioned problems). This saves us the
nsIFile->nsIURI->nsIFile conversion.

You probably could simplyfy the code slightly by moving the
SaveFileSource-calls from nsWebBrowserPersist::SaveDocument and
nsWebBrowserPersist::SaveDocuments into
nsWebBrowserPersist::SaveDocumentWithFixup, it gets called just before your
changes in both circumstances.

[strange mac behaviour]
> It's 100% reliable here - I'd look at platform-specific code. 

The strangest thing is that it doesn't give any errors, everything seems to go
as expected (except that the comment doesn't exist afterwards). And doing the
exact same thing from the front-end always works. Will examine further though.

> do settings like "ask before saving" and "show download manager"
> affect this behavior?

AFAICT no.

Comment 86

[Torben]

> Re attachment (id=190361) [edit]

> this placement gives really strange behaviour

The problem seems to be that we try to set the comment before the Finder knows
that the file exist, this can be fixed by forcing the Finder to update first
(why this only happens in specific cases, I don't understand but never mind).
Will attach updated mac-patch after I have cleaned it up.

I have tested the changes I suggested in comment 85 and they work fine.

Comment 87

[Rich Walsh]

(In reply to comment #86)

After thinking about it, I have to agree that SaveFileSource() should take an
nsILocalFile (the fact that SaveDocumentWithFixup() creates one that we can
reuse helped).  Plus, this change certainly makes the OS-override a lot simpler.
 Hold off on the Mac update a bit longer (my apologies for making you do extra
work).  I'll put together yet another XP patch that changes SaveFileSource's
signature, and moves its invocation into SaveDocumentWithFixup (along with
whatever else needs updating).

Comment 88

[Doug Turner (:dougt)]

could someone summarize where we are with the fix to this bug?

Comment 89

[Rich Walsh]

Attachment: final(?) cross-platform patch

Comment 90

[Rich Walsh]

Attachment: OS/2-specific patch

Comment 91

[Rich Walsh]

(In reply to comment #88)
> could someone summarize where we are with the fix to this bug?

I _think_ we're done.

Changes to cross-platform code are pretty minimal.  A new method has been added
to nsIExternalHelperAppService ('SaveFileSource') which is called from
nsWebBrowserPersist & nsExternalHelperAppService.  It enables all files and
folders created by downloads and 'Save as' menuitems to be tagged.  The default
implementation is a no-op.

Platforms that want to add this feature can override the method in
nsOSHelperAppService to provide an OS-specific implementation.  On Mac & OS/2,
we've added a new method to nsILocalFileXXX named 'SetFileSource' to handle
writing the URI to file. Platforms that want to tag files created via d&d can do
so from their OS-specific versions of nsDragService.

Comment 92

[Doug Turner (:dougt)]

One of the problems that was brought up to me was that in having "common"
functionality in a platform specific interface is going to make for muddy usage:

nsCOMPtr<nsILocalFile> lp = ...

#ifdef XP_WIN
 nsCOMPtr<nsILocalFileWin> nativeLP = do_QueryInterface(lp);
 nativeLP->SetComment(somestring);
#elif MAC
 nsCOMPtr<nsILocalFileMac> nativeLP = do_QueryInterface(lp);
 nativeLP->SetComment(somestring);
.
.
.
#endif

Although I do not see a need for a nsILocalFile2 as there really isn't enough
stuff to warrant such a move, I do not want to have to write any #ifdef when
using nsIFile/nsILocalFile.

I suggested using nsIProperties, but that was passed on.

We could also have a service that takes a nsILocalFile and sets a comment to it.
 In this way, we have do not add any size to the localfile class.  Even this
service approach is better then the muddy code example.

after thinking about this, maybe we should create a nsILocalFile2 and keep it
mutable until we figure out what else should go on it.  sigh.

Comment 93

[Rich Walsh]

(In reply to comment #92)
> One of the problems that was brought up to me was that in having "common"
> functionality in a platform specific interface is going to make for muddy
> usage

See my previous comment.  This has been resolved without the need for nsILocalFile2.

Comment 94

[Torben]

Minor nits

Cross-platform patch (attachment id=190656)
Index: uriloader/exthandler/nsIExternalHelperAppService.idl
===================================================================

+interface nsILocalFile;

+  void saveFileSource(in nsILocalFile aFile, in nsIURI aSource);

To avoid adding the extra interface this could use nsIFile instead, we still can
pass it an LocalFile and do conversion to LocalFileXXX in one step.

OS/2-specific patch (attachment id=190657)
Index: uriloader/exthandler/os2/nsOSHelperAppService.cpp
===================================================================

+#include "nsIFileURL.h"

I don't think you need this anymore.

Mac-specific patch coming up later today.

Comment 95

[Torben]

(In reply to comment #92)
> One of the problems that was brought up to me was that in having "common"
> functionality in a platform specific interface is going to make for muddy
> usage:

> #ifdef XP_WIN
> #elif MAC
> #endif

> [...] I do not want to have to write any #ifdef when using
> nsIFile/nsILocalFile.

As the patch is written, we don't need any #ifdef's, this is handled through the
helperAppService (which if a OS-specific implementation doesn't exist returns
NS_ERROR_NOT_IMPLEMENTED).

I've also changed my mind about renaming the LocalFileMac-function, it's still
SetComment (since it can be used for setting other comments than just the
source), so there isn't any change of confussion with the OS/2-function
(SetFileSource). I suggest that implementations for other platform is given
specific names too.

> I suggested using nsIProperties, but that was passed on.

Would't this make changes to the frozen nsIFile or nsILocalFile interfaces (I
don't understand nsIProperties very well)?

Comment 96

[Torben]

Attachment: Mac-specific patch

The main difference between this and the earlier patch is a new (protected)
function in nsLocalFileOSX UpdateInFinder which forces the Finder to notice the
new file(s), this avoids the problems mentioned in comment 80. I have also
removed the UnescapeFragment-functions from nsOSHelperAppService since the URL
will be UTF8 anyway.

Comment 97

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #93)
> See my previous comment.  This has been resolved without the need for
nsILocalFile2.

Yes, instead of doing that, you added a method to each nsILocalFilePlatform
interface (with differing names), and _additionally_ you added a method to
nsIExternalHelperAppService (which I'm not sure is a good place for this).


> Would't this make changes to the frozen nsIFile or nsILocalFile interfaces (I
> don't understand nsIProperties very well)?

no, you'd qi the nsIFile to nsIProperties. Why not do that?

>removed the UnescapeFragment-functions from nsOSHelperAppService since the URL
>will be UTF8 anyway.

but it may still contain escaped characters...

Comment 98

[timeless]

if you're going to do it this way, can you change save...() to set...().

note that the interface you've chosen to adulturate really doesn't fit at all.

Comment 99

[Torben]

Re attachment 190656 [details] [diff] [review], attachment 190657 [details] [diff] [review], attachment id=190697

OK, noone seems very satisfied with this.

Would a nsILocalFile2-implementation be acceptable (see attachment id=182802,
the function would be renamed to SetFileSource and return
NS_ERROR_NOT_IMPLEMENTED on unsupported platforms)? Then we would need something
like

  nsCOMPtr<nsILocalFile2> localFile2 = do_QueryInterface(aFile);
  localFile2->SetFileSource(aSourceURI);

whereever we want to set this.

>> Would't this make changes to the frozen nsIFile or nsILocalFile interfaces
>> (I don't understand nsIProperties very well)?

> no, you'd qi the nsIFile to nsIProperties. Why not do that?

Because I can't find any documentation or examples on how to implement
nsIProperties :-(

Comment 100

[Torben]

Attachment: Implements nsIProperties for Mac v. 0.1

> I can't find any documentation or examples on how to implement
> nsIProperties

I was able to figure this out at last, but I don't like this solution. My main
objection is that nsIProperties declares five functions of which IMHO only one
makes sense to implement for files.

I suggest we go for a nsILocalFile2-implementation.

Comment 101

[Rich Walsh]

(In reply to comment #98)
> note that the interface you've chosen to adulturate really doesn't fit at all.

That's odd - this was an integral part of all the patches submitted in May. No
one offered any objections at that time, so I carried that implementation
forward into my patches.

Regardless, I'm not married to any specific back-end implementation.  It will
take me about 20 minutes to change my most recent patch to either nsILocalFile2
 or nsIProperties.  I'll defer to Torben on which it should be.  Either way, the
final result should be a lot simpler than anything seen so far.

Comment 102

[Rich Walsh]

Attachment: xp patch using nsILocalFile2

Comment 103

[Rich Walsh]

Attachment: OS/2 patch using nsILocalFile2

Comment 104

[Torben]

Attachment: Mac patch using nsILocalFile2

Comment 105

[Torben]

Attachment: nsILocalFile2 for other OS'es

To avoid #ifdef's we nsILocalFile2 need to exist for all platforms.

Comment 106

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #105)
> To avoid #ifdef's we nsILocalFile2 need to exist for all platforms.

I don't understand, why not just check for QI failure?

Comment 107

[Rich Walsh]

(In reply to comment #106)
> > To avoid #ifdef's we nsILocalFile2 need to exist for all platforms.
> I don't understand, why not just check for QI failure?

That's exactly what the xp code does - no #ifdefs needed.

BTW... who's qualified/willing to review this?

Comment 108

[Christian :Biesinger (don't email me, ping me on IRC)]

I can review the exthandler and wbp changes, at least the xp ones. darin or
dougt should approve nsILocalFile2.

Comment 109

[Torben]

Comment on attachment 190934 [details] [diff] [review]
nsILocalFile2 for other OS'es

> I can review the exthandler and wbp changes, at least the xp ones.

if you (biesi) are OK with these...

> darin or
dougt should approve nsILocalFile2.

... and dougt with these, 
then I'll add review request for the OS-specific implementation

Comment 110

[monty]

If it is easy to fix while you are in the download code, would you please also
fix the bug - save original timestamp (last-modified date) on file downloads -
https://bugzilla.mozilla.org/show_bug.cgi?id=178506

thanks

Comment 111

[Torben]

Comment on attachment 190934 [details] [diff] [review]
nsILocalFile2 for other OS'es

We don't need this, my bad.

Comment 112

[Doug Turner (:dougt)]

Comment on attachment 190918 [details] [diff] [review]
xp patch using nsILocalFile2

nsILocalFile2 should go into a new file.

Also, this interface _SHOULD_ be implemented on all platforms even if the
method just returns not implemented.

Also, I have no idea what the comment means if I don't read the context of this
bug report.

Comment 113

[Rich Walsh]

Attachment: nsILocalFile2.idl

Comment 114

[Rich Walsh]

Attachment: patch to xpcom/io for nsILocalFile2

updates nsILocalFileXXX.idl for Mac, OS2, and Win;
default implementions of nsILocalFile2 for Mac Classic, Unix, and Win;

Comment 115

[Rich Walsh]

Attachment: patches to other xp code

patches to nsWebBrowserPersist and nsExternalHelperAppService

Comment 116

[Rich Walsh]

Attachment: revised patch for Mac OS X

a copy of the previous Mac OS x patch, minus changes to nsILocalFileMac.idl

Comment 117

[Rich Walsh]

Attachment: revised patch for OS/2

a copy of the previous OS/2 patch, minus changes to nsILocalFileOS2.idl

Comment 118

[Doug Turner (:dougt)]

Comment on attachment 191087 [details]
nsILocalFile2.idl

> It is called after the download is complete and the file has been closed.

I think think interface should return some documented failure code when the
localfile isn't in a state where setFileSource couldn't be set.

Also, this should just be attribute so that you can both get and set it.

>Platforms which save files via drag & drop should call this method from their native code to tag those files as well.

I am not sure why we need to state this.

Comment 119

[Rich Walsh]

(In reply to comment #118)
> (From update of attachment 191087 [details] [edit])
> 
> I think think interface should return some documented failure code when the
> localfile isn't in a state where setFileSource couldn't be set.

I'm not sure I understand:  a specific error code when it does fail or one when
it is guaranteed to fail every time?  Honestly, from the calling code's
point-of-view, who cares?  The code has already saved the file, and prior to
exiting, it tacks on the URI as an afterthought.  A failure has no effect on
further processing.  OTOH, if you're suggesting a "don't even try" flag, I would
hope that platform implementers would have the good sense to maintain such a
flag in their nsLocalFileXXX.

> Also, this should just be attribute so that you can both get and set it.

What current or planned feature of any mozapp would ever use this?  Every
comment I've ever seen suggests that users expect to access this info via native
facilities.  Regardless, I'll change it if you wish.

> > Platforms which save files via drag & drop should call this method from
> > their native code to tag those files as well.
>
> I am not sure why we need to state this.

Uniformity of features:  it would be considered peculiar if some saved content
provided this info while others didn't.  IOW, just more bug reports :-)

Comment 120

[Torben]

Comment on attachment 191087 [details]
nsILocalFile2.idl


 * Contributor(s):
 *   Torben <[e-mail]>

Please use Torben Lüders <[e-mail]>


> > Also, this should just be attribute so that you can both get and set it.
> 
> What current or planned feature of any mozapp would ever use this?

And there is no way to tell if this would return the original URI and not some
random text set by the user or an other application.

Comment 121

[Doug Turner (:dougt)]

if this can just be be random text set by the user, we shouldn't call it uri
anything, we should call it what it is "comment" or something similar. 

Comment 122

[Rich Walsh]

(In reply to comment #121)
> if this can just be be random text set by the user, we shouldn't call it uri
> anything, we should call it what it is "comment" or something similar. 

The code delivers a URI, not an ill-defined "comment".  Where that info ends up
is entirely platform dependent.  Mac may be forced to put it into a metadata
grab-bag called "Comments", but OS/2 (and probably Win) will use a more
appropriate metadata tag & reserve its "comments" tag for bona-fide free-form
comments.

In any case, isn't this discussion awfully wide of the mark?  This feature isn't
being added because mozapps have any need for this info.  It's being added
because users on most platforms have been asking for it for the last 4 years. 
It's a remarkably trivial feature - why attach undue significance to it?

Comment 123

[Rich Walsh]

Would it help if SetFileSource() were renamed SaveFileSource() to avoid implying
that this is an attribute with a corresponding Get method?

Comment 124

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #122)
> In any case, isn't this discussion awfully wide of the mark?  This feature isn't
> being added because mozapps have any need for this info.  It's being added
> because users on most platforms have been asking for it for the last 4 years. 
> It's a remarkably trivial feature - why attach undue significance to it?

because it's an api change. I think you are focussing way too much on the
specific callers you added, especially in the IDL comment. functions should be
general where possible.

>Uniformity of features:  it would be considered peculiar if some saved content
>provided this info while others didn't.  IOW, just more bug reports :-)

but why does this need to be stated in an IDL comment? What if another way to
save files gets added, do you want to name it here too?

>> Also, this should just be attribute so that you can both get and set it.
>What current or planned feature of any mozapp would ever use this?

Directory listings could use it. Or extensions for whatever purpose.

Comment 125

[Christian :Biesinger (don't email me, ping me on IRC)]

hmm... to avoid the "could be comment or uri" on some platforms, maybe two
attributes would be better?
  attribute AUTF8String comment;
  attribute AUTF8String source;
where the latter would always return an error on platforms not supporting it,
and the caller would have to fallback?

just a thought...

Comment 126

[Torben]

> ... maybe two attributes would be better?
>   attribute AUTF8String comment;
>   attribute AUTF8String source;
> where the latter would always return an error on platforms not supporting it,
> and the caller would have to fallback?

And then we need
   attribute AUTF8String notes;
   attribute AUTF8String summary;
for Gnome and Windows (bug 267369)...

I don't see why the xp-code should be concerned with how the OS stores the
source, a catch-all-possibilities will just add unnecessary bloat.

>>> Also, this should just be attribute so that you can both get and set it.
>Directory listings could use it. Or extensions for whatever purpose.

OK, then let make this an attribute but let GetFileSource return
NS_ERROR_NOT_IMPLEMENTED until someone actually needs it. (On mac we can get the
comment and parse it to see of it is a valid URI, if so we'll assume it is the
orginating source)

Comment 127

[Christian :Biesinger (don't email me, ping me on IRC)]

>And then we need
>   attribute AUTF8String notes;
>   attribute AUTF8String summary;
>for Gnome and Windows (bug 267369)...

? those could use "comments" just fine.

Comment 128

[Torben]

> >   attribute AUTF8String notes;
> >   attribute AUTF8String summary;
> >for Gnome and Windows (bug 267369)...
> 
> ? those could use "comments" just fine.

Then why can't we use source/comment/whatever for every OS?

>> > if this can just be be random text set by the user,...
> ... but OS/2 (and probably Win) will use a more appropriate metadata tag

But we still can't guarantee that this has not been changed to something that we
can't use.

> why not use [...] a function like setMetaData(PRUint32 type, AString value)?

Sigh, since a simple solution won't do maybe we should do this, then we can have
a arbitary number of metadata for platforms that support it. Maybe also add
getMetaData, but it would be nice if we could keep it simpler than nsIProperties...

Comment 129

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 191091 [details] [diff] [review]
patches to other xp code

+	 nsCOMPtr<nsIURI> uriSource = data->mOriginalLocation;

I'd probably add an assertion that this is non-null.

can you diff with more context in the future?

The second wbp change is to set the source on the foo_files directory, I
assume?

@@ -3554,6 +3576,18 @@ nsWebBrowserPersist::SaveDocumentWithFix
	     NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
	 }
     }
+    else
+    {
+	 // close the stream, then tag the file it created with its source URI

please change this so that it's
  if (localFile) {
    // your new code
  } else {
    // the current code
  }

double negatives are not so nice

similar in the exthandler file

sorry for taking so long.

Comment 130

[Ulrik Sverdrup]

Putting a comment of mine from another bug here:

We should save the URL as metadata on OS X v10.4, using the metadata api
Safari 2.0 sets kMDItemWhereFroms to the download URL, we should do the same. 

Comment 131

[Andy Lyttle]

The comments on this page may be helpful:
http://www.macosxhints.com/article.php?story=20050517161414507

Comment 132

[Louise]

we're not going to see this for Firefox 3?

Comment 133

[Doug Turner (:dougt)]

Comment on attachment 191087 [details]
nsILocalFile2.idl

I really do not want a nsILocalFile2.

How about nsIFileSource, or nsILocalFileSource.

Comment 134

[Doug Turner (:dougt)]

Comment on attachment 191088 [details] [diff] [review]
patch to xpcom/io for nsILocalFile2

since I r-'ed the patch for nsILocalFile2, this one also is minused.

also, we should really have setters and getters for this attribute.  I should be able to ask any nsILocalFile "where did you come from" and have it tell me something if possible.