407215 - [10.5] Better quarantining support

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[Mark Mentovai]

We can set metadata on downloads, so that the system can track the file's origin URL and its referrer.

Comment 1

[Mark Mentovai]

Attachment: 10.5-only patch

A lot of this stuff is 10.5-only, so this will need a little work to make it build with earlier SDKs, and to only try to do the quarantining stuff if it's available at runtime.  To try this out, build with the 10.5 SDK.

Comment 2

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

Will this accomplish bug 401748 for us, then?

Comment 3

[Mark Mentovai]

Yeah, but not for core.  I couldn't think of a good place in core for this to go.  On the other hand, very similar code could go into core's mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp, with the Objective-C Cocoa Foundation stuff turned into regular C Core Foundation stuff.

Comment 4

[Mark Mentovai]

Attachment: Quarantine dialog

Note the addition of the hostname that offered the download, and that the button that offers to show the disk image now does something a little more useful and shows the referring web page.

Comment 5

[Mark Mentovai]

Attachment: With SDK and DT goodness

It would be hot if someone could run with this on 10.4 and make sure that it's really a no-op as it should be, and that it doesn't, say, set your house on fire.

Comment 6

[Stuart Morgan]

Comment on attachment 292108 [details] [diff] [review]
With SDK and DT goodness

Compiles and runs fine on 10.4.

>+#if MAC_OS_X_VERSION_MAX_ALLOWED <= MAC_OS_X_VERSION_10_4  // SDK
>+// This is a helper used by QuarantineDownload to look up strings at runtime.
>+static const CFStringRef GetCFStringFromBundle(CFBundleRef bundle,
>+                                               CFStringRef symbol) {

Should be stuff this in a utility file for general use next time we need to play extreme SDK sports?

>+  if (::LSCopyItemAttribute(&tempFSRef, kLSRolesAll,
>+                            lsItemQuarantineProperties,
>+                            &quarantinePropertiesBase) == noErr) {

This is 10.4+ according to the docs, so this will need weakification for branch.

>+      quarantineProperties =
>+          [(NSDictionary*)quarantinePropertiesBase mutableCopy];

Drip drip!

r=me with the leak fixed.

Comment 7

[Mark Mentovai]

Attachment: Without leak

>Should be stuff this in a utility file for general use next time we need to
>play extreme SDK sports?

This was the first time I needed to do runtime lookups of string constants, since that's how CFStringRef constants are, unlike simple header declarations.  I don't know if we'll need to do this again soon.  If we do, that'd probably be a better time to move this function to a more general home.

Comment 8

[Mark Mentovai]

Ooh, we can set kMDItemWhereFroms too, see the proof-of-concept patch I just stuck on bug 337051.

Comment 9

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

(In reply to comment #8)
> Ooh, we can set kMDItemWhereFroms too, see the proof-of-concept patch I just
> stuck on bug 337051.

That's bug 325873, so please attach your actual-concept patch there ;)

Comment 10

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 292154 [details] [diff] [review]
Without leak

sr=pink

+void nsDownloadListener::QuarantineDownload() {

would be great to have a comment about what this method actually does.

Comment 11

[Mark Mentovai]

Attachment: 1.8 branch patch for Camino 1.6b1

NS_GetReferrerFromChannel doesn't exist on the branch, so this contains a copy from netwerk/base/public/nsNetUtil.h.

This version also does the symbol lookup stuff for LSCopyItemAttribute.

Finally, it's got the description of QuarantineDownload that pink asked for.  That'll be checked in on the trunk, too.

Comment 12

[Mark Mentovai]

Checked in on the trunk and MOZILLA_1_8_BRANCH for 1.6b1.