[Downloads] Move client guid for AV scanning of downloaded files to chrome/

content/browser/download/base_file_win.cc

Index: content/browser/download/base_file_win.cc
diff --git a/content/browser/download/base_file_win.cc b/content/browser/download/base_file_win.cc
index 398e5fc214ff4c80201048dbe376435d9afe0fcb..f31216019c2a7dd7281dea80725ef518b39c1fef 100644
--- a/content/browser/download/base_file_win.cc
+++ b/content/browser/download/base_file_win.cc
@@ -4,16 +4,18 @@
 
 #include "content/browser/download/base_file.h"
 
+#include <objbase.h>
 #include <windows.h>

[Lei Zhang, 2013/08/07 01:03:05]

nit: I think <windows.h> usually goes first?

[Greg Billock, 2013/08/07 17:00:56]

we got some of both. I'll change it here though.

+#include <cguid.h>
 #include <shellapi.h>
 
 #include "base/file_util.h"
+#include "base/guid.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "content/browser/download/download_interrupt_reasons_impl.h"
 #include "content/browser/download/download_stats.h"
-#include "content/browser/safe_util_win.h"
 #include "content/public/browser/browser_thread.h"
 
 namespace content {
@@ -326,7 +328,14 @@ DownloadInterruptReason BaseFile::AnnotateWithSourceInformation() {
 
   bound_net_log_.BeginEvent(net::NetLog::TYPE_DOWNLOAD_FILE_ANNOTATED);
   DownloadInterruptReason result = DOWNLOAD_INTERRUPT_REASON_NONE;
-  HRESULT hr = ScanAndSaveDownloadedFile(full_path_, source_url_);
+  GUID guid = GUID_NULL;
+  if (!client_guid_.empty() && base::IsValidGUID(client_guid_)) {

[asanka, 2013/08/07 16:42:36]

Skip this validation step and just DCHECK the return value from
CLSIDFromString() or UUIDFromString()? You'd still need to check if the
client_guid_ isn't empty since the base DownloadManagerDelegate implementation
passes an empty string.

[Greg Billock, 2013/08/07 17:42:56]

I wasn't sure what bad thing CLSIDFromString might do if the string is
malformatted. It looks to me like it might interpret it as some kind of
classname and consult the registry, which is not what I want.

+    HRESULT hr = CLSIDFromString(

[asanka, 2013/08/07 16:42:36]

Have you looked at using UuidFromString() instead?

The documentation for CLSIDFromString() says that it can return
REGDB_E_CLASSNOTREG (CLSID corresponding to the class string was not found in
the registry) and REGDB_E_READREGDB (the registry could not be opened for
reading). These might only apply if you pass a class string instead of a GUID
string. Still, UuidFromString() seems to have less baggage.

[Greg Billock, 2013/08/07 17:42:56]

I saw UuidFromString(), but it isn't used elsewhere in Chrome, which made me
suspicious, and I found Stuff On The Internet that CLSIDFromString is preferred.
I'm happy to change it, though -- you probably know more about it than I do. :-)

On 2013/08/07 16:42:36, asanka wrote:

[asanka, 2013/08/07 19:56:12]

I don't consider myself to be an expert on this, so I'm not going to strongly
advocate for either. But from my reading of the documentation, it seemed
UuidFromString was a more straightforward API.

CLSIDFromString is preferred over GUIDFromString since the latter appears to be
deprecated. The documentation for UuidFromString doesn't have such a disclaimer.

FWIW, I checked briefly and noticed that IE9 doesn't call
IIAttachmentExecute::SetClientGuid() (or in IE's case,
ieframe!CAttachmentServices::SetClientGuid).

[Greg Billock, 2013/08/07 23:10:31]

Tried UuidFromString. This is part of the RPC library, which we don't link. We'd
have to bring in another dependency for it.

A wrinkle is that CLSIDFromString wants braces around the string. Sigh. I'm
thinking maybe using ifdefs here is actually better. :-(

+        base::UTF8ToUTF16(client_guid_).c_str(), &guid);
+    if (FAILED(hr))
+      guid = GUID_NULL;
+  }
+  HRESULT hr = base::AVScanFile(full_path_, source_url_.spec(), guid);
 
   // If the download file is missing after the call, then treat this as an
   // interrupted download.