301367 - Tag Files with Source URL

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, enhancement)

Description

[Rich Walsh]

This enhancement tags saved content with the URI that originated it.  Complete
files identify the URL used to fetch them.  Fragments such as text & URLs
dragged from pages identify the URL that contained them.  URLs that identify the
source as chrome:// are supressed (e.g. URLs dragged from the address bar).

Files in four areas are affected:
* nsExternalHelperAppService.cpp - Downloads
* nsWebBrowserPersist.cpp - Save as
* nsDragService.cpp - Drag & Drop
* nsLocalFileOS2.cpp - .SUBJECT extended attribute handling

While implementing this feature, I also moved .TYPE EA handling from
nsDragService to nsLocalFileOS2 and revised nsDragService's file-creation
routines to better comply with standard practises.

This patch has been tested with Firefox, Thunderbird, and Seamonkey.  In most
cases, the URLs generated by Thunderbird & Messenger are mailbox:// URIs and
aren't very useful.  If I can work around build issues, I plan to convert them
to mailto: URIs in a future patch.

Comment 1

[Rich Walsh]

Attachment: patch to tag files with source url

Comment 2

[OstGote!]

Is this "tag" an equivalent to the Win32 Bug 267369 comment 1?

Comment 3

[Mike Kaply [:mkaply]]

(In reply to comment #2)
> Is this "tag" an equivalent to the Win32 Bug 267369 comment 1?

Very similar.

Comment 4

[Rich Walsh]

(In reply to comment #2)
> Is this "tag" an equivalent to the Win32 Bug 267369 comment 1?

Probably - I'm not familiar enough with those filesystems to comment
knowledgably.  All of OS/2's native filesystems (including FAT) are capable of
associating arbitrary data with a file.  Unlike the resource fork of a Mac file,
the data (known as an "extended attribute") is linked to the file at the
directory level, not embedded in the file itself.  Its disk space is allocated
independently & is not transferred when a file is copied to a filesystem that
doesn't support EAs (e.g. CDFS).

FYI... this is simply a reimplementation of a feature Mike Kaply added to the
OS/2 port of Netscape 2.02.

Comment 5

[OstGote!]

Thanks for the answers, that is what I wanted to know. Yes, for Windows on NTFS 
you can do the same with Alternate Data Streams as meta info for files (the 
data are not included in the file itself, only associated). You will lost this 
info if you copy the file to a non-NTFS filesystem. WinXP SP2 saves for example 
the zone information for downloaded files in such ADS.

Comment 6

[Mike Kaply [:mkaply]]

Rich, it is interesting that other platforms want this as well.

Maybe we should figure out a way to do it in nsLocalFile so other platforms get
it as well?

Comment 7

[Rich Walsh]

(In reply to comment #6)
> Maybe we should figure out a way to do it in nsLocalFile so other platforms
> get it as well?

nsILocalFile is a frozen interface, so any new method has to be added to the
platform's subclass.  Plus, only the tiniest fraction of all localfile objects
will ever use this facility.  Adding anything to localfile beyond a new method
to support it would be a major waste of space.

If other platforms want to implement this, it should be fairly simple:
- add a "SetFileSource()" method to their nsLocalFileXXX
- add an #ifdef to my code to QI the generic file to their platform's flavor
- use the rest of my (very minimal) code as-is.

Comment 8

[Mike Kaply [:mkaply]]

Comment on attachment 189829 [details] [diff] [review]
patch to tag files with source url

r=mkaply

Comment 9

[Darin Fisher]

Comment on attachment 189829 [details] [diff] [review]
patch to tag files with source url

Whenever you change an interface definition, you MUST change the corresponding
"uuid" property of the interface.  Otherwise, existing XPCOM components may
crash when they attempt to use the old interface.  By changing the uuid, you
make it so that QueryInterface can be used properly to test for the existance
of a particular version of the interface.

Also, a style nit:
"SaveFileSource( mCurrentBaseURI, aFile);"
why the extra whitespace?  is that how the rest of the source file is coded? 
if not, then please change your style to match that of the surrounding code,
thx!

Comment 10

[timeless]

erm. please no. please no. please no.

please coordinate with bug 90918

Comment 11

[Rich Walsh]

FYI...  I'm coordinating my efforts with those working on Bug #90918.  When that
bug's status is clearer, I'll submit another patch containing either the entire
revision or just the OS/2-specific parts.  Meanwhile, OS/2 builders can continue
to use this patch if they want its features.

Comment 12

[Peter Weilbacher]

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

Comment 13

[Rich Walsh]

Attachment: revised patch to tag files with source url

This is an improved version of the previous patch.  It only supports OS/2 and is intended primarily for those who create private OS/2 builds.

Comment 14

[Peter Weilbacher]

Comment on attachment 204417 [details] [diff] [review]
revised patch to tag files with source url

As the cross-platform coordination in bug 90918 has failed I think it makes sense to try the OS/2 only approach again, hence I ask for reviews again.

This patch attends to darin's comments about the first version.

Comment 15

[Peter Weilbacher]

It's Rich's patch so he should be assignee.

Comment 16

[Darin Fisher]

Comment on attachment 204417 [details] [diff] [review]
revised patch to tag files with source url

I don't really like the ifdef'ing in cross-platform code, but ok.

sr=darin assuming mkaply is okay with the stuff inside the ifdefs ;-)

Comment 17

[Andy Willis (abwillis)]

Adding Mike to CC list, review reminder.

Comment 18

[Peter Weilbacher]

Comment on attachment 204417 [details] [diff] [review]
revised patch to tag files with source url

As this is OS/2 only I think I am within my boundaries to "steal" this review from Mike.

This patch is by now well tested in my unofficial builds and I went through the code again and am happy with it.

Rich, if you haven't had great ideas how to improve it since you made the original patch I am going to check it in soon.

Comment 19

[Peter Weilbacher]

Patch checked into trunk.

Comment 20

[Peter Weilbacher]

Comment on attachment 204417 [details] [diff] [review]
revised patch to tag files with source url

Users really want this OS/2 only feature also on branch. While it changes an (OS/2 only) IDL file, it only adds functions to it, so it doesn't break the API freeze...

(I could add that I have been shipping with this patch in my unofficial 1.8.x builds and it never caused a problem.)

Comment 21

[Daniel Veditz [:dveditz]]

Adding functions breaks the API freeze. May not matter to JavaScript callers, but can break any C++ code that calculates the sizes of vtables. However, the rules might be more lax for OS/2 since we're not shipping it from MoCo -- what rules do you want to apply?

Given this is all OS/2 only I can leave it up to you and approve the ifdef'd stuff in shared Gecko files.

approved for 1.8.1.8, a=dveditz for release-drivers

Comment 22

[Peter Weilbacher]

mkaply, what's your take on this?
Rich, Andy, any other opinions?

Comment 23

[Steve Wendt]

(In reply to comment #21)
> but can break any C++ code that calculates the sizes of vtables.

What is the likelihood that such code exists?  To my knowledge, no one has encountered such a problem due to this patch.

(In reply to comment #22)
> any other opinions?

Peter, since you'll be the one hearing about it and fixing it if there is a problem, I'd say check it in.

Comment 24

[Mike Kaply [:mkaply]]

I say we go for it. I'd love to get as many of these "Peter W only" patches into the branch as possible.

Comment 25

[Peter Weilbacher]

OK, great, so we all agree. :-) Landed the patch on 1.8 branch now.

Comment 26

[Daniel Veditz [:dveditz]]

(In reply to comment #23)
> (In reply to comment #21)
> > but can break any C++ code that calculates the sizes of vtables.
> 
> What is the likelihood that such code exists?  To my knowledge, no one has
> encountered such a problem due to this patch.

That's not the point. Were this a file subject to the 1.8-branch API freeze--and it is not IMO--then this would not be allowed, period. We're not allowed to guess whether it is or isn't likely someone's using a particular interface. Some recent patches that alter a previously altered interface have even had to create yet another subclass in the form of _MOZILLA_1_8_BRANCH2 -- it's ugly, but it's the rule. It's easier to follow the rule than to waste time proving that your interface doesn't need to. It's far better and easier to follow the rule than to break stuff and have to re-release and lose user's trust when their extensions break.

OS/2 releases live in their own universe, no problems. comment 21 was merely intended clarify the "no API change" rule should some future patch need to change a shared interface.

Comment 27

[Peter Weilbacher]

Verified as fixed with the branch nightly
   Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8.1.8pre)
   Gecko/20071003 BonEcho/2.0.0.8pre