Last Comment Bug 407215 - [10.5] Better quarantining support
: [10.5] Better quarantining support
Status: RESOLVED FIXED
: fixed1.8.1.12
Product: Camino Graveyard
Classification: Graveyard
Component: Downloading (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: Camino1.6
Assigned To: Mark Mentovai
:
:
Mentors:
Depends on: 393515
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-06 15:01 PST by Mark Mentovai
Modified: 2007-12-13 17:59 PST (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
10.5-only patch (10.41 KB, patch)
2007-12-06 15:03 PST, Mark Mentovai
no flags Details | Diff | Splinter Review
Quarantine dialog (20.69 KB, image/png)
2007-12-06 15:14 PST, Mark Mentovai
no flags Details
With SDK and DT goodness (14.56 KB, patch)
2007-12-07 11:38 PST, Mark Mentovai
stuart.morgan+bugzilla: review+
Details | Diff | Splinter Review
Without leak (14.57 KB, patch)
2007-12-07 14:39 PST, Mark Mentovai
mark: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review
1.8 branch patch for Camino 1.6b1 (17.28 KB, patch)
2007-12-13 16:53 PST, Mark Mentovai
no flags Details | Diff | Splinter Review

Description Mark Mentovai 2007-12-06 15:01:50 PST
We can set metadata on downloads, so that the system can track the file's origin URL and its referrer.
Comment 1 Mark Mentovai 2007-12-06 15:03:46 PST
Created attachment 291961 [details] [diff] [review]
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) 2007-12-06 15:05:57 PST
Will this accomplish bug 401748 for us, then?
Comment 3 Mark Mentovai 2007-12-06 15:09:13 PST
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 2007-12-06 15:14:43 PST
Created attachment 291963 [details]
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 2007-12-07 11:38:43 PST
Created attachment 292108 [details] [diff] [review]
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 2007-12-07 13:59:38 PST
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 2007-12-07 14:39:11 PST
Created attachment 292154 [details] [diff] [review]
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 2007-12-07 15:58:20 PST
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) 2007-12-07 16:07:22 PST
(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) 2007-12-13 14:34:35 PST
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 2007-12-13 16:53:30 PST
Created attachment 293037 [details] [diff] [review]
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 2007-12-13 17:59:44 PST
Checked in on the trunk and MOZILLA_1_8_BRANCH for 1.6b1.

Note You need to log in before you can comment on or make changes to this bug.