The default bug view has changed. See this FAQ.

Tag Files with Source URL

VERIFIED FIXED

Status

Core Graveyard
File Handling
--
enhancement
VERIFIED FIXED
12 years ago
9 months ago

People

(Reporter: Rich Walsh, Assigned: Rich Walsh)

Tracking

({verified1.8.1.8})

Trunk
x86
OS/2
verified1.8.1.8

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 years ago
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.
(Assignee)

Comment 1

12 years ago
Created attachment 189829 [details] [diff] [review]
patch to tag files with source url
Attachment #189829 - Flags: review?(mozilla)

Comment 2

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

Comment 3

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

Very similar.
(Assignee)

Comment 4

12 years ago
(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

12 years ago
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

12 years ago
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?
(Assignee)

Comment 7

12 years ago
(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

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

r=mkaply
Attachment #189829 - Flags: superreview?(darin)
Attachment #189829 - Flags: review?(mozilla)
Attachment #189829 - Flags: review+

Comment 9

12 years ago
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!
Attachment #189829 - Flags: superreview?(darin) → superreview-

Comment 10

12 years ago
erm. please no. please no. please no.

please coordinate with bug 90918
Depends on: 90918
(Assignee)

Comment 11

12 years ago
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

12 years ago
*** Bug 272001 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 13

12 years ago
Created attachment 204417 [details] [diff] [review]
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.
Attachment #189829 - Attachment is obsolete: true

Comment 14

12 years ago
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.
Attachment #204417 - Flags: superreview?(darin)
Attachment #204417 - Flags: review?(mozilla)

Comment 15

12 years ago
It's Rich's patch so he should be assignee.
Assignee: mozilla → dragtext
No longer depends on: 90918

Comment 16

11 years ago
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 ;-)
Attachment #204417 - Flags: superreview?(darin) → superreview+
Adding Mike to CC list, review reminder.

Comment 18

11 years ago
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.
Attachment #204417 - Flags: review?(mozilla) → review+

Comment 19

11 years ago
Patch checked into trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 20

10 years ago
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.)
Attachment #204417 - Flags: approval1.8.1.8?
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
Attachment #204417 - Flags: approval1.8.1.8? → approval1.8.1.8+

Comment 22

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

Comment 23

10 years ago
(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

10 years ago
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

10 years ago
OK, great, so we all agree. :-) Landed the patch on 1.8 branch now.
Keywords: fixed1.8.1.8
(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

10 years ago
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.8 → verified1.8.1.8
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.