The default bug view has changed. See this FAQ.

[10.5] Better quarantining support

RESOLVED FIXED in Camino1.6

Status

Camino Graveyard
Downloading
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Mark Mentovai, Assigned: Mark Mentovai)

Tracking

({fixed1.8.1.12})

Trunk
Camino1.6
All
Mac OS X
fixed1.8.1.12

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
We can set metadata on downloads, so that the system can track the file's origin URL and its referrer.
(Assignee)

Comment 1

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

Updated

9 years ago
Hardware: PC → All
Will this accomplish bug 401748 for us, then?
Summary: Better quarantining support [10.5] → [10.5] Better quarantining support
(Assignee)

Comment 3

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

Comment 4

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

Comment 5

9 years ago
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.
Attachment #291961 - Attachment is obsolete: true
Attachment #292108 - Flags: review?(stuart.morgan)

Comment 6

9 years ago
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.
Attachment #292108 - Flags: review?(stuart.morgan) → review+
(Assignee)

Comment 7

9 years ago
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.
Attachment #292108 - Attachment is obsolete: true
Attachment #292154 - Flags: superreview?(mikepinkerton)
Attachment #292154 - Flags: review+
(Assignee)

Comment 8

9 years ago
Ooh, we can set kMDItemWhereFroms too, see the proof-of-concept patch I just stuck on bug 337051.
(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 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.
Attachment #292154 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 11

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

Comment 12

9 years ago
Checked in on the trunk and MOZILLA_1_8_BRANCH for 1.6b1.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Component: General → Downloading
Keywords: fixed1.8.1.12
Resolution: --- → FIXED
Target Milestone: --- → Camino1.6
You need to log in before you can comment on or make changes to this bug.