Add quarantine metadata to downloads on the Mac.

Add quarantine metadata to downloads on the Mac.

BUG=10853
Checked in r14657.

[smorgan, 2009-04-24 22:53:43]

I created mac_safe_util.* as an analog to win_safe_util.*, which contains the
semi-equivalent Windows function, but it seemed pretty arbitrary. Suggestions
for a better home for the function are welcome.

[Avi (use Gerrit), 2009-04-27 16:43:57]

http://codereview.chromium.org/99005/diff/1010/1016
File chrome/common/mac_safe_util.mm (right):

http://codereview.chromium.org/99005/diff/1010/1016#newcode25
Line 25: NSDictionary* quarantine_properties = nil;
If you're making a mutable copy on line 34 and using setValue:forKey: several
times subsequently, you should declare this as an NSMutableDictionary*. (Why
didn't gcc complain?)

[smorgan, 2009-04-27 16:54:36]

http://codereview.chromium.org/99005/diff/1010/1016
File chrome/common/mac_safe_util.mm (right):

http://codereview.chromium.org/99005/diff/1010/1016#newcode25
Line 25: NSDictionary* quarantine_properties = nil;
On 2009/04/27 16:43:57, Avi wrote:
> If you're making a mutable copy on line 34 and using setValue:forKey: several
> times subsequently, you should declare this as an NSMutableDictionary*. (Why
> didn't gcc complain?)

Fixed, good catch.

Looks like there was no warning because setValue:forKey: is part of the KVO
protocol, which is declared as a category on NSObject.

[Mark Mentovai, 2009-04-27 17:04:53]

lg otherwise

http://codereview.chromium.org/99005/diff/1010/1013
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/99005/diff/1010/1013#newcode30
Line 30: #include "chrome/common/mac_safe_util.h"
Does "safe" have a specific meaning on Windows?  If so, that would
contraindicate use of this name.

Long-term, I prefer for things like this to generally follow the
whatever_platform pattern.

http://codereview.chromium.org/99005/diff/1010/1013#newcode131
Line 131: bool DownloadFile::Open(const char* open_mode) {
This is called at the beginning of a download, correct?

http://codereview.chromium.org/99005/diff/1010/1015
File chrome/common/mac_safe_util.h (right):

http://codereview.chromium.org/99005/diff/1010/1015#newcode11
Line 11: namespace mac_util {
Not so sure about the choice of namespace.  Do other things in chrome/common use
mac_util as a namespace?

http://codereview.chromium.org/99005/diff/1010/1016
File chrome/common/mac_safe_util.mm (right):

http://codereview.chromium.org/99005/diff/1010/1016#newcode7
Line 7: #import <Cocoa/Cocoa.h>
#import <Foundation/Foundation.h> instead, I don't think you use any Cocoa
outside of Foundation in this file.

http://codereview.chromium.org/99005/diff/1010/1016#newcode19
Line 19: void AddQuarantineMetadataToFile(const FilePath& file, const GURL&
source,
This looks familiar!  :)

http://codereview.chromium.org/99005/diff/1010/1016#newcode27
Line 27: if (::LSCopyItemAttribute(&file_ref, kLSRolesAll,
kLSItemQuarantineProperties,
#include <ApplicationServices/ApplicationServices.h>

http://codereview.chromium.org/99005/diff/1010/1016#newcode29
Line 29: if (::CFGetTypeID(quarantine_properties_base) ==
#include <CoreFoundation/CoreFoundation.h>

http://codereview.chromium.org/99005/diff/1010/1016#newcode34
Line 34: [[(NSDictionary*)quarantine_properties_base mutableCopy] autorelease];
Continuation line gets an extra 4, not 2, spaces of indentation.

http://codereview.chromium.org/99005/diff/1010/1016#newcode35
Line 35: }
else log about weirdness?

http://codereview.chromium.org/99005/diff/1010/1016#newcode42
Line 42: return;
This is surprising.  Shouldn't we only hit this code for files that we
absolutely want to quarantine anyway?  What's your thinking here?

http://codereview.chromium.org/99005/diff/1010/1016#newcode44
Line 44: 
Is the comment from the original code about kLSQuarantineAgentNameKey,
kLSQuarantineAgentBundleIdentifierKey, and kLSQuarantineTimeStampKey not valid
here?  Seems like it is.  Should probabl include something that explains it...

http://codereview.chromium.org/99005/diff/1010/1016#newcode46
Line 46: NSString* type = (source.SchemeIs("http") || source.SchemeIs("https"))
Why don't you make |type| a CFStringRef and then put the cast over to NSString*
where you set kLSQuarantineTypeKey?

http://codereview.chromium.org/99005/diff/1010/1016#newcode57
Line 57: [NSString stringWithUTF8String:referrer.spec().c_str()];
continuation line

http://codereview.chromium.org/99005/diff/1010/1016#newcode65
Line 65: [NSString stringWithUTF8String:source.spec().c_str()];
continuation line

http://codereview.chromium.org/99005/diff/1010/1016#newcode70
Line 70: ::LSSetItemAttribute(&file_ref, kLSRolesAll,
kLSItemQuarantineProperties,
else log about weirdness here too?

[smorgan, 2009-04-27 20:15:09]

http://codereview.chromium.org/99005/diff/1010/1013
File chrome/browser/download/download_file.cc (right):

http://codereview.chromium.org/99005/diff/1010/1013#newcode30
Line 30: #include "chrome/common/mac_safe_util.h"
On 2009/04/27 17:04:53, Mark Mentovai wrote:
> Does "safe" have a specific meaning on Windows?  If so, that would
> contraindicate use of this name.
> 
> Long-term, I prefer for things like this to generally follow the
> whatever_platform pattern.

Renamed file and namespace to quarantine_mac

http://codereview.chromium.org/99005/diff/1010/1013#newcode131
Line 131: bool DownloadFile::Open(const char* open_mode) {
On 2009/04/27 17:04:53, Mark Mentovai wrote:
> This is called at the beginning of a download, correct?

Correct (and potentially again when it's renamed). Quarantine early, quarantine
often :)

http://codereview.chromium.org/99005/diff/1010/1016
File chrome/common/mac_safe_util.mm (right):

http://codereview.chromium.org/99005/diff/1010/1016#newcode7
Line 7: #import <Cocoa/Cocoa.h>
On 2009/04/27 17:04:53, Mark Mentovai wrote:
> #import <Foundation/Foundation.h> instead, I don't think you use any Cocoa
> outside of Foundation in this file.

Done.

http://codereview.chromium.org/99005/diff/1010/1016#newcode27
Line 27: if (::LSCopyItemAttribute(&file_ref, kLSRolesAll,
kLSItemQuarantineProperties,
On 2009/04/27 17:04:53, Mark Mentovai wrote:
> #include <ApplicationServices/ApplicationServices.h>

Done.

http://codereview.chromium.org/99005/diff/1010/1016#newcode29
Line 29: if (::CFGetTypeID(quarantine_properties_base) ==
On 2009/04/27 17:04:53, Mark Mentovai wrote:
> #include <CoreFoundation/CoreFoundation.h>

It's part of the Foundation umbrella, so no need.

http://codereview.chromium.org/99005/diff/1010/1016#newcode34
Line 34: [[(NSDictionary*)quarantine_properties_base mutableCopy] autorelease];
On 2009/04/27 17:04:53, Mark Mentovai wrote:
> Continuation line gets an extra 4, not 2, spaces of indentation.

Done.

http://codereview.chromium.org/99005/diff/1010/1016#newcode35
Line 35: }
On 2009/04/27 17:04:53, Mark Mentovai wrote:
> else log about weirdness?

Done.

http://codereview.chromium.org/99005/diff/1010/1016#newcode42
Line 42: return;
On 2009/04/27 17:04:53, Mark Mentovai wrote:
> This is surprising.  Shouldn't we only hit this code for files that we
> absolutely want to quarantine anyway?  What's your thinking here?

This is a case that Camino ran into. Users can (via reverse-engineered files
used by the OS) opt certain file types out of quarantine. Unconditionally adding
the metadata here causes those files to be quarantined despite the user having
gone well out of their way to try to prevent it.

I've tried to clarify the comment; let me know if it needs more explanation.

http://codereview.chromium.org/99005/diff/1010/1016#newcode44
Line 44: 
On 2009/04/27 17:04:53, Mark Mentovai wrote:
> Is the comment from the original code about kLSQuarantineAgentNameKey,
> kLSQuarantineAgentBundleIdentifierKey, and kLSQuarantineTimeStampKey not valid
> here?  Seems like it is.  Should probabl include something that explains it...

That behavior is explicitly documented in the LSQuarantine header, so it didn't
seem important, but I've added a note.

http://codereview.chromium.org/99005/diff/1010/1016#newcode46
Line 46: NSString* type = (source.SchemeIs("http") || source.SchemeIs("https"))
On 2009/04/27 17:04:53, Mark Mentovai wrote:
> Why don't you make |type| a CFStringRef and then put the cast over to
NSString*
> where you set kLSQuarantineTypeKey?

Done.

http://codereview.chromium.org/99005/diff/1010/1016#newcode57
Line 57: [NSString stringWithUTF8String:referrer.spec().c_str()];
On 2009/04/27 17:04:53, Mark Mentovai wrote:
> continuation line

Done.

http://codereview.chromium.org/99005/diff/1010/1016#newcode65
Line 65: [NSString stringWithUTF8String:source.spec().c_str()];
On 2009/04/27 17:04:53, Mark Mentovai wrote:
> continuation line

Done.

http://codereview.chromium.org/99005/diff/1010/1016#newcode70
Line 70: ::LSSetItemAttribute(&file_ref, kLSRolesAll,
kLSItemQuarantineProperties,
On 2009/04/27 17:04:53, Mark Mentovai wrote:
> else log about weirdness here too?

Done.

[Mark Mentovai, 2009-04-27 20:27:36]

LGTM