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

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

This client app should not be used by all clients of content/. Moving to chrome so it can be managed there and re-used if needed.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216843

[Greg Billock, 2013-08-01 19:25:42]

Lei. Want to have a look?

John, I've put this code in base. Look right?

[Lei Zhang, 2013-08-01 19:35:39]

https://codereview.chromium.org/21355004/diff/7001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/21355004/diff/7001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:137:
base::ScanAndSaveDownloadedFile(dest_platform_path, std::string());
Don't you need to punt this off to the FILE thread (pool)?

[vandebo (ex-Chrome), 2013-08-01 19:41:06]

https://codereview.chromium.org/21355004/diff/7001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/21355004/diff/7001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:142: //
TODO(gbillock): Insert AV call here in the right validator.
Remove TODO.

I think we want a base class that does the this (but leaves PreWrite validation
pure virtual), so that multiple validators can easily do it.

[Greg Billock, 2013-08-01 21:55:47]

https://codereview.chromium.org/21355004/diff/7001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/21355004/diff/7001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:137:
base::ScanAndSaveDownloadedFile(dest_platform_path, std::string());
On 2013/08/01 19:35:39, Lei Zhang wrote:
> Don't you need to punt this off to the FILE thread (pool)?

Yeah, I think so. We're on the IO thread, but we should probably bounce...

https://codereview.chromium.org/21355004/diff/7001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:142: //
TODO(gbillock): Insert AV call here in the right validator.
On 2013/08/01 19:41:06, vandebo wrote:
> Remove TODO.
> 
> I think we want a base class that does the this (but leaves PreWrite
validation
> pure virtual), so that multiple validators can easily do it

Done. That'll be a fairly easy refactor when we get more validators.

[vandebo (ex-Chrome), 2013-08-01 22:07:12]

https://codereview.chromium.org/21355004/diff/7001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/21355004/diff/7001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:142: //
TODO(gbillock): Insert AV call here in the right validator.
On 2013/08/01 21:55:47, Greg Billock wrote:
> On 2013/08/01 19:41:06, vandebo wrote:
> > Remove TODO.
> > 
> > I think we want a base class that does the this (but leaves PreWrite
> validation
> > pure virtual), so that multiple validators can easily do it
> 
> Done. That'll be a fairly easy refactor when we get more validators.

I have another one in the works, so if you want to do it now, it won't be wasted
work.

https://codereview.chromium.org/21355004/diff/12001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/21355004/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:129:
BrowserThread::PostTask(
Post over only for windows?

[jam, 2013-08-02 05:45:25]

lgtm

[Greg Billock, 2013-08-02 15:26:10]

https://codereview.chromium.org/21355004/diff/7001/chrome/browser/media_galle...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/21355004/diff/7001/chrome/browser/media_galle...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:142: //
TODO(gbillock): Insert AV call here in the right validator.
OK, I'll probably do this.

On 2013/08/01 22:07:12, vandebo wrote:
> On 2013/08/01 21:55:47, Greg Billock wrote:
> > On 2013/08/01 19:41:06, vandebo wrote:
> > > Remove TODO.
> > > 
> > > I think we want a base class that does the this (but leaves PreWrite
> > validation
> > > pure virtual), so that multiple validators can easily do it
> > 
> > Done. That'll be a fairly easy refactor when we get more validators.
> 
> I have another one in the works, so if you want to do it now, it won't be
wasted
> work.

https://codereview.chromium.org/21355004/diff/12001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/21355004/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:129:
BrowserThread::PostTask(
On 2013/08/01 22:07:12, vandebo wrote:
> Post over only for windows?

I had it that way, but this is a bit less code. Not a big deal though.

[Greg Billock, 2013-08-02 17:57:58]

On 2013/08/02 15:26:10, Greg Billock wrote:
>
https://codereview.chromium.org/21355004/diff/7001/chrome/browser/media_galle...
> File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
> (right):
> 
>
https://codereview.chromium.org/21355004/diff/7001/chrome/browser/media_galle...
> chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:142:
//
> TODO(gbillock): Insert AV call here in the right validator.
> OK, I'll probably do this.
> 
> On 2013/08/01 22:07:12, vandebo wrote:
> > On 2013/08/01 21:55:47, Greg Billock wrote:
> > > On 2013/08/01 19:41:06, vandebo wrote:
> > > > Remove TODO.
> > > > 
> > > > I think we want a base class that does the this (but leaves PreWrite
> > > validation
> > > > pure virtual), so that multiple validators can easily do it
> > > 
> > > Done. That'll be a fairly easy refactor when we get more validators.
> > 
> > I have another one in the works, so if you want to do it now, it won't be
> wasted
> > work.
> 
>
https://codereview.chromium.org/21355004/diff/12001/chrome/browser/media_gall...
> File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
> (right):
> 
>
https://codereview.chromium.org/21355004/diff/12001/chrome/browser/media_gall...
> chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:129:
> BrowserThread::PostTask(
> On 2013/08/01 22:07:12, vandebo wrote:
> > Post over only for windows?
> 
> I had it that way, but this is a bit less code. Not a big deal though.

+brettw for base review

[Lei Zhang, 2013-08-02 19:59:47]

https://codereview.chromium.org/21355004/diff/12001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/21355004/diff/12001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:129:
BrowserThread::PostTask(
On 2013/08/01 22:07:12, vandebo wrote:
> Post over only for windows?

+1. As is, it's just extra bouncing between threads on Mac/Linux.

https://codereview.chromium.org/21355004/diff/19001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/21355004/diff/19001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:129:
BrowserThread::PostTask(
Can you use PostTaskAndReplyWithResults() and get rid of |post_write_callback_|
?

[Greg Billock, 2013-08-02 23:39:39]

On 2013/08/02 19:59:47, Lei Zhang wrote:
>
https://codereview.chromium.org/21355004/diff/12001/chrome/browser/media_gall...
> File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
> (right):
> 
>
https://codereview.chromium.org/21355004/diff/12001/chrome/browser/media_gall...
> chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:129:
> BrowserThread::PostTask(
> On 2013/08/01 22:07:12, vandebo wrote:
> > Post over only for windows?
> 
> +1. As is, it's just extra bouncing between threads on Mac/Linux.
> 
>
https://codereview.chromium.org/21355004/diff/19001/chrome/browser/media_gall...
> File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
> (right):
> 
>
https://codereview.chromium.org/21355004/diff/19001/chrome/browser/media_gall...
> chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:129:
> BrowserThread::PostTask(
> Can you use PostTaskAndReplyWithResults() and get rid of
|post_write_callback_|
> ?

Steve, added a base class. Like it?

[Lei Zhang, 2013-08-03 00:28:26]

https://codereview.chromium.org/21355004/diff/26001/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/21355004/diff/26001/base/file_util.h#newcode111
base/file_util.h:111: #endif
Since this has gotten a bit more separated from the matching #if, add //
defined(OS_WIN)

https://codereview.chromium.org/21355004/diff/26001/base/file_util_win.cc
File base/file_util_win.cc (right):

https://codereview.chromium.org/21355004/diff/26001/base/file_util_win.cc#new...
base/file_util_win.cc:38: static const GUID kClientID = { 0x2676a9a2, 0xd919,
0x4fee,
no need to be static.

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.h (right):

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:14: class
ImageDecoder;
not used

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
File content/browser/safe_util_win.cc (right):

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
content/browser/safe_util_win.cc:12: #include "base/path_service.h"
remove

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
content/browser/safe_util_win.cc:13: #include "base/strings/string_util.h"
remove

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
content/browser/safe_util_win.cc:14: #include
"base/strings/utf_string_conversions.h"
remove

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
content/browser/safe_util_win.cc:17: #include "url/gurl.h"
remove

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
content/browser/safe_util_win.cc:25: static const GUID kClientID = { 0x2676a9a2,
0xd919, 0x4fee,
Do we want to expose the copy in base and get rid of this?

Also, it doesn't need to be static.

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
File content/browser/safe_util_win.h (right):

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
content/browser/safe_util_win.h:11: class GURL;
remove

[Lei Zhang, 2013-08-03 00:33:07]

Actually, where is ScanningFileValidator instantiated? It look like a copy of
SupportedImageTypeValidator.

[Lei Zhang, 2013-08-03 00:42:39]

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:93:
const base::FilePath& dest_platform_path) {
fits on previous line.

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:155:
BrowserThread::PostTask(
You are already on the IO thread, just do:

result_callback.Run(base::PLATFORM_FILE_OK);

[Greg Billock, 2013-08-05 16:25:44]

On 2013/08/03 00:33:07, Lei Zhang wrote:
> Actually, where is ScanningFileValidator instantiated? It look like a copy of
> SupportedImageTypeValidator.

It isn't. If the base class works for Steve I'll move over
SupportedImageTypeValidator.

[vandebo (ex-Chrome), 2013-08-05 18:05:30]

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.cc (right):

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:121:
BrowserThread::PostTask(
no need to post task here, just run the callback.

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.h (right):

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:19: class
ScanningFileValidator : public fileapi::CopyOrMoveFileValidator {
Scanning file validator is not a very descriptive name.  SystemAVScanner ?

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:23: static bool
SupportsFileType(const base::FilePath& path);
No need for this method.

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:26: virtual
void StartPreWriteValidation(
I did not intend for this to be a fully functional validator, just something
that other validators could inherit from.

Omit (leaving it pure virtual).

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:92:
base::PlatformFileError ScanFile(
Why aren't you using the base class?

[Greg Billock, 2013-08-05 21:42:30]

https://codereview.chromium.org/21355004/diff/26001/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/21355004/diff/26001/base/file_util.h#newcode111
base/file_util.h:111: #endif
On 2013/08/03 00:28:26, Lei Zhang wrote:
> Since this has gotten a bit more separated from the matching #if, add //
> defined(OS_WIN)

Done.

https://codereview.chromium.org/21355004/diff/26001/base/file_util_win.cc
File base/file_util_win.cc (right):

https://codereview.chromium.org/21355004/diff/26001/base/file_util_win.cc#new...
base/file_util_win.cc:38: static const GUID kClientID = { 0x2676a9a2, 0xd919,
0x4fee,
On 2013/08/03 00:28:26, Lei Zhang wrote:
> no need to be static.

Done.

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.cc (right):

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:121:
BrowserThread::PostTask(
On 2013/08/05 18:05:30, vandebo wrote:
> no need to post task here, just run the callback.

Done.

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.h (right):

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:14: class
ImageDecoder;
On 2013/08/03 00:28:26, Lei Zhang wrote:
> not used

Done.

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:19: class
ScanningFileValidator : public fileapi::CopyOrMoveFileValidator {
On 2013/08/05 18:05:30, vandebo wrote:
> Scanning file validator is not a very descriptive name.  SystemAVScanner ?

I meant like "FileValidator that does Validation by Scanning" but I see it came
out a bit awkward. I'm not sure I like your name any better though. How about
"AVScannerValidatorBase"

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:23: static bool
SupportsFileType(const base::FilePath& path);
On 2013/08/05 18:05:30, vandebo wrote:
> No need for this method.

Done.

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:26: virtual
void StartPreWriteValidation(
On 2013/08/05 18:05:30, vandebo wrote:
> I did not intend for this to be a fully functional validator, just something
> that other validators could inherit from.
> 
> Omit (leaving it pure virtual).

OK. I wasn't sure whether reading the whole file would do you any good for yours
or not. I'll just leave it virtual.

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:92:
base::PlatformFileError ScanFile(
On 2013/08/05 18:05:30, vandebo wrote:
> Why aren't you using the base class?

Checking if the base class would satisfy the thing you needed for MP3s. I'm
going to switch to it in the next patchset.

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:93:
const base::FilePath& dest_platform_path) {
On 2013/08/03 00:42:39, Lei Zhang wrote:
> fits on previous line.

Done.

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:155:
BrowserThread::PostTask(
On 2013/08/03 00:42:39, Lei Zhang wrote:
> You are already on the IO thread, just do:
> 
> result_callback.Run(base::PLATFORM_FILE_OK);

Done. This changes semantics a little, but it looks like its fine.

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
File content/browser/safe_util_win.cc (right):

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
content/browser/safe_util_win.cc:12: #include "base/path_service.h"
On 2013/08/03 00:28:26, Lei Zhang wrote:
> remove

Done.

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
content/browser/safe_util_win.cc:13: #include "base/strings/string_util.h"
On 2013/08/03 00:28:26, Lei Zhang wrote:
> remove

Done.

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
content/browser/safe_util_win.cc:14: #include
"base/strings/utf_string_conversions.h"
On 2013/08/03 00:28:26, Lei Zhang wrote:
> remove

Done.

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
content/browser/safe_util_win.cc:17: #include "url/gurl.h"
On 2013/08/03 00:28:26, Lei Zhang wrote:
> remove

Done.

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
content/browser/safe_util_win.cc:25: static const GUID kClientID = { 0x2676a9a2,
0xd919, 0x4fee,
On 2013/08/03 00:28:26, Lei Zhang wrote:
> Do we want to expose the copy in base and get rid of this?
> 
> Also, it doesn't need to be static.

Done.

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
File content/browser/safe_util_win.h (right):

https://codereview.chromium.org/21355004/diff/26001/content/browser/safe_util...
content/browser/safe_util_win.h:11: class GURL;
On 2013/08/03 00:28:26, Lei Zhang wrote:
> remove

Done.

[Lei Zhang, 2013-08-05 22:42:39]

https://codereview.chromium.org/21355004/diff/42001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.cc (right):

https://codereview.chromium.org/21355004/diff/42001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:79:
SupportedImageTypeValidator::~SupportedImageTypeValidator() {}
This isn't right. Shouldn't it be ScanningFileValidator here and elsewhere in
this file?

[Greg Billock, 2013-08-05 23:06:12]

https://codereview.chromium.org/21355004/diff/42001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.cc (right):

https://codereview.chromium.org/21355004/diff/42001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:79:
SupportedImageTypeValidator::~SupportedImageTypeValidator() {}
On 2013/08/05 22:42:40, Lei Zhang wrote:
> This isn't right. Shouldn't it be ScanningFileValidator here and elsewhere in
> this file?

Yes. I'm updating it in the next patchset. This was a copy-paste to make sure it
did what Steve needed for the new class.

[vandebo (ex-Chrome), 2013-08-05 23:21:12]

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/21355004/diff/26001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:92:
base::PlatformFileError ScanFile(
On 2013/08/05 21:42:30, Greg Billock wrote:
> On 2013/08/05 18:05:30, vandebo wrote:
> > Why aren't you using the base class?
> 
> Checking if the base class would satisfy the thing you needed for MP3s. I'm
> going to switch to it in the next patchset.

Yes, I think a base class that implements the PostWriteValidation step will
solve the problem...

[brettw, 2013-08-05 23:22:17]

https://codereview.chromium.org/21355004/diff/42001/base/file_util_win.cc
File base/file_util_win.cc (right):

https://codereview.chromium.org/21355004/diff/42001/base/file_util_win.cc#new...
base/file_util_win.cc:38: const GUID kClientID = { 0x2676a9a2, 0xd919, 0x4fee,
I don't get what this is. Is this the GUID of the requesting app that we
generated, or some MS thing? If it's an app ID, it doesn't belong in base.

And this is copy-and-pasted from another file. This kind of thing should not be
duplicated.

https://codereview.chromium.org/21355004/diff/42001/base/file_util_win.cc#new...
base/file_util_win.cc:187: HRESULT ScanAndSaveDownloadedFile(const
base::FilePath& full_path,
Why is this file called "Save" downloaded file? It looks like it doesn't save
anything. Well, it calls "Save" on the attachment services, but that just saves
the flags you set.

Don't use "base::" here since you're in the base namespace. Same in header.

[Greg Billock, 2013-08-06 15:10:24]

https://codereview.chromium.org/21355004/diff/42001/base/file_util_win.cc
File base/file_util_win.cc (right):

https://codereview.chromium.org/21355004/diff/42001/base/file_util_win.cc#new...
base/file_util_win.cc:38: const GUID kClientID = { 0x2676a9a2, 0xd919, 0x4fee,
On 2013/08/05 23:22:18, brettw wrote:
> I don't get what this is. Is this the GUID of the requesting app that we
> generated, or some MS thing? If it's an app ID, it doesn't belong in base.
> 
> And this is copy-and-pasted from another file. This kind of thing should not
be
> duplicated.

Yeah, I'm not sure what to do with this. It was in the initial repo checkin 5
years ago. I don't know where it came from or how. I could make it an argument.
Would that be better?

The docs at http://msdn.microsoft.com/en-us/library/ms647048.aspx say it should
be "created using uuidgen.exe and defined internally."

There's various GUIDs sitting around the codebase, but I don't know whether this
is more like a type flag or more like an app-specific thing that shouldn't be
duplicated. Any ideas on who to ask?

https://codereview.chromium.org/21355004/diff/42001/base/file_util_win.cc#new...
base/file_util_win.cc:187: HRESULT ScanAndSaveDownloadedFile(const
base::FilePath& full_path,
On 2013/08/05 23:22:18, brettw wrote:
> Why is this file called "Save" downloaded file? It looks like it doesn't save
> anything. Well, it calls "Save" on the attachment services, but that just
saves
> the flags you set.

This is copy-pasted from the code in content/. I'm happy to change it, but at
least in the initial patchset, I wanted to make it clear what was going on. My
guess is that in the MS docs, the active ingredient for IAttachmentExecute is
called "Save". Not totally accurate, though, I agree.

> Don't use "base::" here since you're in the base namespace. Same in header.

done

[Greg Billock, 2013-08-06 15:26:29]

On 2013/08/06 15:10:24, Greg Billock wrote:
> https://codereview.chromium.org/21355004/diff/42001/base/file_util_win.cc
> File base/file_util_win.cc (right):
> 
>
https://codereview.chromium.org/21355004/diff/42001/base/file_util_win.cc#new...
> base/file_util_win.cc:38: const GUID kClientID = { 0x2676a9a2, 0xd919, 0x4fee,
> On 2013/08/05 23:22:18, brettw wrote:
> > I don't get what this is. Is this the GUID of the requesting app that we
> > generated, or some MS thing? If it's an app ID, it doesn't belong in base.
> > 
> > And this is copy-and-pasted from another file. This kind of thing should not
> be
> > duplicated.
> 
> Yeah, I'm not sure what to do with this. It was in the initial repo checkin 5
> years ago. I don't know where it came from or how. I could make it an
argument.
> Would that be better?
> 
> The docs at http://msdn.microsoft.com/en-us/library/ms647048.aspx say it
should
> be "created using uuidgen.exe and defined internally."
> 
> There's various GUIDs sitting around the codebase, but I don't know whether
this
> is more like a type flag or more like an app-specific thing that shouldn't be
> duplicated. Any ideas on who to ask?
> 
>
https://codereview.chromium.org/21355004/diff/42001/base/file_util_win.cc#new...
> base/file_util_win.cc:187: HRESULT ScanAndSaveDownloadedFile(const
> base::FilePath& full_path,
> On 2013/08/05 23:22:18, brettw wrote:
> > Why is this file called "Save" downloaded file? It looks like it doesn't
save
> > anything. Well, it calls "Save" on the attachment services, but that just
> saves
> > the flags you set.
> 
> This is copy-pasted from the code in content/. I'm happy to change it, but at
> least in the initial patchset, I wanted to make it clear what was going on. My
> guess is that in the MS docs, the active ingredient for IAttachmentExecute is
> called "Save". Not totally accurate, though, I agree.
> 
> > Don't use "base::" here since you're in the base namespace. Same in header.
> 
> done

I think cpu originally added this ID. Carlos, do you recollect the backstory of
this client ID GUID?

[jam, 2013-08-06 17:32:30]

https://codereview.chromium.org/21355004/diff/47002/content/browser/safe_util...
File content/browser/safe_util_win.cc (right):

https://codereview.chromium.org/21355004/diff/47002/content/browser/safe_util...
content/browser/safe_util_win.cc:31: bool SaferOpenItemViaShell(HWND hwnd, const
std::wstring& window_title,
btw this isn't used anywhere. so just delete this file, and that solves the
duplication concern that Brett raised.

Regarding the guid, I agree that it seems like something that should be provided
by the chrome layer. so how about the base method takes in the guid. it can be
stored in a header somewhere in chrome/browser. Media gallery code can access it
fine. the download code can ask DownloadManagerDelegate for that guid.

[brettw, 2013-08-06 17:32:33]

https://codereview.chromium.org/21355004/diff/47002/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/21355004/diff/47002/base/file_util.h#newcode110
base/file_util.h:110: const std::string& source_url);
Style nit: since it fits, put the first arg on the previous line and align the
next argo to it.

I think we should be passing in the GUID to base. Looking at the header, this
should be an application ID, which means that it should be defined in Chrome
rather than shared between all apps that use Content or base. So we should add
the GUID as a parameter here, and then hook up some delegate thingy (John should
probably suggest how) for the content code to get it from Chrome.

[Greg Billock, 2013-08-06 22:07:16]

OK, how does this look? I tried a stab at using GUID and ifdefs, and then at
putting the guid string in the BaseFile constructor where it probably really
wants to be. Both were really unsatisfactory, so I ended up with a more brittle
but less invasive approach using strings.

This code pathing hasn't changed that much in five years, so I think this is
probably the right direction. Let me know if its what you had in mind.

https://codereview.chromium.org/21355004/diff/47002/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/21355004/diff/47002/base/file_util.h#newcode110
base/file_util.h:110: const std::string& source_url);
On 2013/08/06 17:32:33, brettw wrote:
> Style nit: since it fits, put the first arg on the previous line and align the
> next argo to it.
> 
> I think we should be passing in the GUID to base. Looking at the header, this
> should be an application ID, which means that it should be defined in Chrome
> rather than shared between all apps that use Content or base. So we should add
> the GUID as a parameter here, and then hook up some delegate thingy (John
should
> probably suggest how) for the content code to get it from Chrome.

OK, heading in that direction. base should be in final form now.

https://codereview.chromium.org/21355004/diff/47002/content/browser/safe_util...
File content/browser/safe_util_win.cc (right):

https://codereview.chromium.org/21355004/diff/47002/content/browser/safe_util...
content/browser/safe_util_win.cc:31: bool SaferOpenItemViaShell(HWND hwnd, const
std::wstring& window_title,
On 2013/08/06 17:32:31, jam wrote:
> btw this isn't used anywhere. so just delete this file, and that solves the
> duplication concern that Brett raised.

OK, sounds good.

> Regarding the guid, I agree that it seems like something that should be
provided
> by the chrome layer. so how about the base method takes in the guid. it can be
> stored in a header somewhere in chrome/browser. Media gallery code can access
it
> fine. the download code can ask DownloadManagerDelegate for that guid.

OK, I'll put the ID available there.

[Greg Billock, 2013-08-06 23:34:04]

On 2013/08/06 22:07:16, Greg Billock wrote:
> OK, how does this look? I tried a stab at using GUID and ifdefs, and then at
> putting the guid string in the BaseFile constructor where it probably really
> wants to be. Both were really unsatisfactory, so I ended up with a more
brittle
> but less invasive approach using strings.
> 
> This code pathing hasn't changed that much in five years, so I think this is
> probably the right direction. Let me know if its what you had in mind.
> 
> https://codereview.chromium.org/21355004/diff/47002/base/file_util.h
> File base/file_util.h (right):
> 
>
https://codereview.chromium.org/21355004/diff/47002/base/file_util.h#newcode110
> base/file_util.h:110: const std::string& source_url);
> On 2013/08/06 17:32:33, brettw wrote:
> > Style nit: since it fits, put the first arg on the previous line and align
the
> > next argo to it.
> > 
> > I think we should be passing in the GUID to base. Looking at the header,
this
> > should be an application ID, which means that it should be defined in Chrome
> > rather than shared between all apps that use Content or base. So we should
add
> > the GUID as a parameter here, and then hook up some delegate thingy (John
> should
> > probably suggest how) for the content code to get it from Chrome.
> 
> OK, heading in that direction. base should be in final form now.
> 
>
https://codereview.chromium.org/21355004/diff/47002/content/browser/safe_util...
> File content/browser/safe_util_win.cc (right):
> 
>
https://codereview.chromium.org/21355004/diff/47002/content/browser/safe_util...
> content/browser/safe_util_win.cc:31: bool SaferOpenItemViaShell(HWND hwnd,
const
> std::wstring& window_title,
> On 2013/08/06 17:32:31, jam wrote:
> > btw this isn't used anywhere. so just delete this file, and that solves the
> > duplication concern that Brett raised.
> 
> OK, sounds good.
> 
> > Regarding the guid, I agree that it seems like something that should be
> provided
> > by the chrome layer. so how about the base method takes in the guid. it can
be
> > stored in a header somewhere in chrome/browser. Media gallery code can
access
> it
> > fine. the download code can ask DownloadManagerDelegate for that guid.
> 
> OK, I'll put the ID available there.

Added Asanka to look at the downloads stuff.

Think of the change as three sections:

1. Move the AV invocation to base. I think we have that straightened around.
brettw and jam looking at that.
2. Move the app GUID into chrome (this should probably have happened anyway).
asanka and jam to look at that.
3. Use the new base method from media galleries, along with the new base class.
vandeobo to look at that.

[Lei Zhang, 2013-08-07 01:03:04]

Media Galleries bits lgtm with a bunch of nits all around.

https://codereview.chromium.org/21355004/diff/68002/base/file_util_win.cc
File base/file_util_win.cc (right):

https://codereview.chromium.org/21355004/diff/68002/base/file_util_win.cc#new...
base/file_util_win.cc:82: bool SetInternetZoneIdentifierDirectly(const
base::FilePath& full_path) {
nit: no need for base:: instead namespace base.

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:407:
ChromeDownloadManagerDelegate::ApplicationClientIdForFileScanning() const {
nit: indentation.

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.cc (right):

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:45: if
(!client_guid.empty() && base::IsValidGUID(client_guid)) {
I don't think you need this, the constant should always be valid here. Just
DCHECK the result from CLSIDFromString().

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:55: HRESULT
scan_result = base::AVScanFile(dest_platform_path, std::string(),
nit: I'd prefer to wrap after the equal sign.
HRESULT scan_result =
    base::AVScanFile(dest_platform_path, std::string(), guid);

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.h (right):

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:9: #include
"base/files/file_path.h"
nit: forward-decl

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:10: #include
"base/memory/scoped_ptr.h"
nit: remove

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:30: explicit
ScanningFileValidator();
nit: not explicit

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:19: #if
defined(OS_WIN)
nit: not needed.

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
File content/browser/download/base_file_win.cc (right):

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
content/browser/download/base_file_win.cc:8: #include <windows.h>
nit: I think <windows.h> usually goes first?

https://codereview.chromium.org/21355004/diff/68002/content/public/browser/do...
File content/public/browser/download_manager_delegate.cc (right):

https://codereview.chromium.org/21355004/diff/68002/content/public/browser/do...
content/public/browser/download_manager_delegate.cc:42:
DownloadManagerDelegate::ApplicationClientIdForFileScanning() const {
nit: indentation.

[vandebo (ex-Chrome), 2013-08-07 16:13:13]

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.cc (right):

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:8: #include
<objbase.h>
Are these needed?

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:14: #include
"base/files/scoped_platform_file_closer.h"
Lots of unused headers.  At least:

scoped_platform_file_closer
scoped_ptr
weak_ptr
stl_util
utf_string_conversions

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:40:
base::PlatformFileError result = base::PLATFORM_FILE_OK;
I don't think you need this.

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:58: result =
base::PLATFORM_FILE_ERROR_SECURITY;
Just return here.

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.h (right):

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:16: class
ScanningFileValidator : public fileapi::CopyOrMoveFileValidator {
nit: AVScanningFileValidator ?

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:20: virtual
void StartPreWriteValidation(
You don't need this, remove.

[asanka, 2013-08-07 16:42:35]

https://codereview.chromium.org/21355004/diff/68002/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/21355004/diff/68002/base/file_util.h#newcode109
base/file_util.h:109: //                If GUID.Empty is passed, no client GUID
is set.
GUID_NULL. Or as suggested elsewhere, take a const GUID* and allow it to be
NULL.

https://codereview.chromium.org/21355004/diff/68002/base/file_util_win.cc
File base/file_util_win.cc (right):

https://codereview.chromium.org/21355004/diff/68002/base/file_util_win.cc#new...
base/file_util_win.cc:198: if (client_guid != GUID_NULL) {
IsEqualGUID(). Or take a const GUID* and check if it's not NULL.

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
File content/browser/download/base_file_win.cc (right):

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
content/browser/download/base_file_win.cc:332: if (!client_guid_.empty() &&
base::IsValidGUID(client_guid_)) {
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.

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
content/browser/download/base_file_win.cc:333: HRESULT hr = CLSIDFromString(
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.

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
File content/browser/download/download_file_impl.h (right):

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
content/browser/download/download_file_impl.h:70: virtual void
SetClientGuid(const std::string& guid) OVERRIDE;
Could you remind me why a SetClientGuid() method was added instead of adding a
parameter to the constructor?

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

https://codereview.chromium.org/21355004/diff/68002/base/file_util_win.cc
File base/file_util_win.cc (right):

https://codereview.chromium.org/21355004/diff/68002/base/file_util_win.cc#new...
base/file_util_win.cc:82: bool SetInternetZoneIdentifierDirectly(const
base::FilePath& full_path) {
On 2013/08/07 01:03:05, Lei Zhang wrote:
> nit: no need for base:: instead namespace base.

Done.

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/download/c...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/download/c...
chrome/browser/download/chrome_download_manager_delegate.cc:407:
ChromeDownloadManagerDelegate::ApplicationClientIdForFileScanning() const {
On 2013/08/07 01:03:05, Lei Zhang wrote:
> nit: indentation.

Done.

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.cc (right):

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:8: #include
<objbase.h>
On 2013/08/07 16:13:14, vandebo wrote:
> Are these needed?

yes

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:14: #include
"base/files/scoped_platform_file_closer.h"
On 2013/08/07 16:13:14, vandebo wrote:
> Lots of unused headers.  At least:
> 
> scoped_platform_file_closer
> scoped_ptr
> weak_ptr
> stl_util
> utf_string_conversions

Done.

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:40:
base::PlatformFileError result = base::PLATFORM_FILE_OK;
On 2013/08/07 16:13:14, vandebo wrote:
> I don't think you need this.

Moved lower

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:45: if
(!client_guid.empty() && base::IsValidGUID(client_guid)) {
On 2013/08/07 01:03:05, Lei Zhang wrote:
> I don't think you need this, the constant should always be valid here. Just
> DCHECK the result from CLSIDFromString().

Done.

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:55: HRESULT
scan_result = base::AVScanFile(dest_platform_path, std::string(),
On 2013/08/07 01:03:05, Lei Zhang wrote:
> nit: I'd prefer to wrap after the equal sign.
> HRESULT scan_result =
>     base::AVScanFile(dest_platform_path, std::string(), guid);

Done.

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:58: result =
base::PLATFORM_FILE_ERROR_SECURITY;
On 2013/08/07 16:13:14, vandebo wrote:
> Just return here.

Done.

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.h (right):

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:9: #include
"base/files/file_path.h"
On 2013/08/07 01:03:05, Lei Zhang wrote:
> nit: forward-decl

Done.

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:10: #include
"base/memory/scoped_ptr.h"
On 2013/08/07 01:03:05, Lei Zhang wrote:
> nit: remove

Done.

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:20: virtual
void StartPreWriteValidation(
On 2013/08/07 16:13:14, vandebo wrote:
> You don't need this, remove.

Done.

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:30: explicit
ScanningFileValidator();
On 2013/08/07 01:03:05, Lei Zhang wrote:
> nit: not explicit

Done.

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:19: #if
defined(OS_WIN)
On 2013/08/07 01:03:05, Lei Zhang wrote:
> nit: not needed.

Done.

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
File content/browser/download/base_file_win.cc (right):

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
content/browser/download/base_file_win.cc:8: #include <windows.h>
On 2013/08/07 01:03:05, Lei Zhang wrote:
> nit: I think <windows.h> usually goes first?

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

https://codereview.chromium.org/21355004/diff/68002/content/public/browser/do...
File content/public/browser/download_manager_delegate.cc (right):

https://codereview.chromium.org/21355004/diff/68002/content/public/browser/do...
content/public/browser/download_manager_delegate.cc:42:
DownloadManagerDelegate::ApplicationClientIdForFileScanning() const {
On 2013/08/07 01:03:05, Lei Zhang wrote:
> nit: indentation.

Done.

[vandebo (ex-Chrome), 2013-08-07 17:11:11]

LGTM

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.h (right):

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:16: class
ScanningFileValidator : public fileapi::CopyOrMoveFileValidator {
On 2013/08/07 16:13:14, vandebo wrote:
> nit: AVScanningFileValidator ?

Comment missed.

https://codereview.chromium.org/21355004/diff/83001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.cc (right):

https://codereview.chromium.org/21355004/diff/83001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:47: if
(scan_result != S_OK)
nit: flip the case of this to fail safely in the face of future change.  If
result is ok, return ok, default return fail.

[Greg Billock, 2013-08-07 17:42:55]

https://codereview.chromium.org/21355004/diff/68002/base/file_util.h
File base/file_util.h (right):

https://codereview.chromium.org/21355004/diff/68002/base/file_util.h#newcode109
base/file_util.h:109: //                If GUID.Empty is passed, no client GUID
is set.
On 2013/08/07 16:42:36, asanka wrote:
> GUID_NULL. Or as suggested elsewhere, take a const GUID* and allow it to be
> NULL.

Done.

https://codereview.chromium.org/21355004/diff/68002/base/file_util_win.cc
File base/file_util_win.cc (right):

https://codereview.chromium.org/21355004/diff/68002/base/file_util_win.cc#new...
base/file_util_win.cc:198: if (client_guid != GUID_NULL) {
On 2013/08/07 16:42:36, asanka wrote:
> IsEqualGUID(). Or take a const GUID* and check if it's not NULL.

Done.

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.h (right):

https://codereview.chromium.org/21355004/diff/68002/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.h:16: class
ScanningFileValidator : public fileapi::CopyOrMoveFileValidator {
On 2013/08/07 17:11:11, vandebo wrote:
> On 2013/08/07 16:13:14, vandebo wrote:
> > nit: AVScanningFileValidator ?
> 
> Comment missed.

Looks good, but not done yet -- that'll mean rerunning gyp_chromium and a full
rebuild on windows. :-(

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
File content/browser/download/base_file_win.cc (right):

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
content/browser/download/base_file_win.cc:332: if (!client_guid_.empty() &&
base::IsValidGUID(client_guid_)) {
On 2013/08/07 16:42:36, asanka wrote:
> 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.

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.

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
content/browser/download/base_file_win.cc:333: HRESULT hr = CLSIDFromString(
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:
> 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.

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
File content/browser/download/download_file_impl.h (right):

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
content/browser/download/download_file_impl.h:70: virtual void
SetClientGuid(const std::string& guid) OVERRIDE;
On 2013/08/07 16:42:36, asanka wrote:
> Could you remind me why a SetClientGuid() method was added instead of adding a
> parameter to the constructor?

I went that route at first. It quickly explodes, though -- the SaveFile needs
one, and DownloadFileImpl, and then of course an arg to the factory, and all of
the associated tests, mocks, subclasses, etc. It ends up being an invasive
change across like 20-25 files which mainly introduces noise.

There's a lot to like about it, but I ended up concluding that this narrow use
was OK to have a setter.

https://codereview.chromium.org/21355004/diff/83001/chrome/browser/media_gall...
File chrome/browser/media_galleries/fileapi/scanning_file_validator.cc (right):

https://codereview.chromium.org/21355004/diff/83001/chrome/browser/media_gall...
chrome/browser/media_galleries/fileapi/scanning_file_validator.cc:47: if
(scan_result != S_OK)
On 2013/08/07 17:11:12, vandebo wrote:
> nit: flip the case of this to fail safely in the face of future change.  If
> result is ok, return ok, default return fail.

Done.

[asanka, 2013-08-07 19:56:11]

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
File content/browser/download/base_file_win.cc (right):

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
content/browser/download/base_file_win.cc:333: HRESULT hr = CLSIDFromString(
On 2013/08/07 17:42:56, Greg Billock wrote:
> 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.
:-)

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).

> On 2013/08/07 16:42:36, asanka wrote:
> > 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.

https://codereview.chromium.org/21355004/diff/103001/content/browser/download...
File content/browser/download/mock_download_file.h (right):

https://codereview.chromium.org/21355004/diff/103001/content/browser/download...
content/browser/download/mock_download_file.h:52: virtual void
SetClientGuid(const std::string& guid) OVERRIDE {}
Can you make this a mock? You'll probably have to update expectations in
unittests, but that would ensure that we are calling SetClientGuid() when we
should.

[Greg Billock, 2013-08-07 20:37:23]

On 2013/08/07 19:56:11, asanka wrote:
>
https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
> File content/browser/download/base_file_win.cc (right):
> 
>
https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
> content/browser/download/base_file_win.cc:333: HRESULT hr = CLSIDFromString(
> On 2013/08/07 17:42:56, Greg Billock wrote:
> > 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.
> :-)
> 
> 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).
> 
> > On 2013/08/07 16:42:36, asanka wrote:
> > > 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.
> 
>
https://codereview.chromium.org/21355004/diff/103001/content/browser/download...
> File content/browser/download/mock_download_file.h (right):
> 
>
https://codereview.chromium.org/21355004/diff/103001/content/browser/download...
> content/browser/download/mock_download_file.h:52: virtual void
> SetClientGuid(const std::string& guid) OVERRIDE {}
> Can you make this a mock? You'll probably have to update expectations in
> unittests, but that would ensure that we are calling SetClientGuid() when we
> should.

That sounds good. I'm going to split this pig of a CL up into a couple chunks.
I've sent Steve the chunk establishing the base class for media galleries. I'll
send you and jam a CL to pull the constant out into chrome/ where it belongs and
possibly do the constructor refactoring, etc. Then we can pull the AV call into
base as a refactor instead of mashing this all together.

[Greg Billock, 2013-08-07 21:15:27]

On 2013/08/07 20:37:23, Greg Billock wrote:
> On 2013/08/07 19:56:11, asanka wrote:
> >
>
https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
> > File content/browser/download/base_file_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
> > content/browser/download/base_file_win.cc:333: HRESULT hr = CLSIDFromString(
> > On 2013/08/07 17:42:56, Greg Billock wrote:
> > > 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.
> > :-)
> > 
> > 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).
> > 
> > > On 2013/08/07 16:42:36, asanka wrote:
> > > > 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.
> > 
> >
>
https://codereview.chromium.org/21355004/diff/103001/content/browser/download...
> > File content/browser/download/mock_download_file.h (right):
> > 
> >
>
https://codereview.chromium.org/21355004/diff/103001/content/browser/download...
> > content/browser/download/mock_download_file.h:52: virtual void
> > SetClientGuid(const std::string& guid) OVERRIDE {}
> > Can you make this a mock? You'll probably have to update expectations in
> > unittests, but that would ensure that we are calling SetClientGuid() when we
> > should.
> 
> That sounds good. I'm going to split this pig of a CL up into a couple chunks.
> I've sent Steve the chunk establishing the base class for media galleries.
I'll
> send you and jam a CL to pull the constant out into chrome/ where it belongs
and
> possibly do the constructor refactoring, etc. Then we can pull the AV call
into
> base as a refactor instead of mashing this all together.

OK, this change is much more focused now. Asanka, let's sort out whether we want
to put this in the constructor. John, do you think having this constant be a
string in the public API makes sense?

[jam, 2013-08-07 22:44:42]

lgtm with nits

https://codereview.chromium.org/21355004/diff/120001/content/browser/download...
File content/browser/download/download_file_impl.h (right):

https://codereview.chromium.org/21355004/diff/120001/content/browser/download...
content/browser/download/download_file_impl.h:69: // before RenameAndAnnotate()
to take effect.
nit: don't duplicate comment in impl. no line above also (see the above methods)

https://codereview.chromium.org/21355004/diff/120001/content/browser/download...
File content/browser/download/mock_download_file.h (right):

https://codereview.chromium.org/21355004/diff/120001/content/browser/download...
content/browser/download/mock_download_file.h:52: virtual void
SetClientGuid(const std::string& guid) OVERRIDE {}
why not use mock macros like all the above methods?

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

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
File content/browser/download/base_file_win.cc (right):

https://codereview.chromium.org/21355004/diff/68002/content/browser/download/...
content/browser/download/base_file_win.cc:333: HRESULT hr = CLSIDFromString(
On 2013/08/07 19:56:12, asanka wrote:
> On 2013/08/07 17:42:56, Greg Billock wrote:
> > 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.
> :-)
> 
> 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).
> 
> > On 2013/08/07 16:42:36, asanka wrote:
> > > 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.

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. :-(

https://codereview.chromium.org/21355004/diff/103001/content/browser/download...
File content/browser/download/mock_download_file.h (right):

https://codereview.chromium.org/21355004/diff/103001/content/browser/download...
content/browser/download/mock_download_file.h:52: virtual void
SetClientGuid(const std::string& guid) OVERRIDE {}
On 2013/08/07 19:56:12, asanka wrote:
> Can you make this a mock? You'll probably have to update expectations in
> unittests, but that would ensure that we are calling SetClientGuid() when we
> should.

Done.

[Greg Billock, 2013-08-08 20:07:46]

https://codereview.chromium.org/21355004/diff/120001/content/browser/download...
File content/browser/download/download_file_impl.h (right):

https://codereview.chromium.org/21355004/diff/120001/content/browser/download...
content/browser/download/download_file_impl.h:69: // before RenameAndAnnotate()
to take effect.
On 2013/08/07 22:44:43, jam wrote:
> nit: don't duplicate comment in impl. no line above also (see the above
methods)

Done.

https://codereview.chromium.org/21355004/diff/120001/content/browser/download...
File content/browser/download/mock_download_file.h (right):

https://codereview.chromium.org/21355004/diff/120001/content/browser/download...
content/browser/download/mock_download_file.h:52: virtual void
SetClientGuid(const std::string& guid) OVERRIDE {}
On 2013/08/07 22:44:43, jam wrote:
> why not use mock macros like all the above methods?

Filled this out now -- I'm saving this for the test to check for.

[asanka, 2013-08-08 21:16:00]

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
File content/browser/download/base_file_win.cc (right):

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
content/browser/download/base_file_win.cc:334: if (!client_guid_.empty() &&
base::IsValidGUID(client_guid_)) {
Nit: Don't need the client_guid_.empty() call if you are calling IsValidGUID().

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
File content/browser/download/download_manager_impl_unittest.cc (right):

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
content/browser/download/download_manager_impl_unittest.cc:201: virtual
std::string ApplicationClientIdForFileScanning() const {
Use a mock and set expectations so this is consistent with the rest of the test.

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
content/browser/download/download_manager_impl_unittest.cc:609: stream.get(), _,
_));
Create a MockDownloadFile here, set its expectations to receive the client ID,
and then return it from the MockCreateFile call.

Something like:

MockDownloadFile* mock_file = new MockDownloadFile;
EXPECT_CALL(*mock_file, SetClientGuid(std::string("client-id")));

EXPECT_CALL(..., MockCreateFile(...))
  .WillOnce(Return(mock_file));

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
content/browser/download/download_manager_impl_unittest.cc:613: EXPECT_TRUE(NULL
!= download_manager_->GetDownload(local_id));
Is this change necessary?

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
File content/browser/download/mock_download_file.h (right):

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
content/browser/download/mock_download_file.h:52: void SetClientGuid(const
std::string& guid) OVERRIDE {
MOCK_METHOD1(...)

You can later use EXPECT_CALL(...) and friends to check if the expected value
was seen.

[Greg Billock, 2013-08-08 23:55:45]

Figured out the root cause -- mocking const method silently fails if you don't
do MOCK_CONST_METHOD. :-(

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
File content/browser/download/base_file_win.cc (right):

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
content/browser/download/base_file_win.cc:334: if (!client_guid_.empty() &&
base::IsValidGUID(client_guid_)) {
On 2013/08/08 21:16:01, asanka wrote:
> Nit: Don't need the client_guid_.empty() call if you are calling
IsValidGUID().

Done.

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
File content/browser/download/download_manager_impl_unittest.cc (right):

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
content/browser/download/download_manager_impl_unittest.cc:201: virtual
std::string ApplicationClientIdForFileScanning() const {
On 2013/08/08 21:16:01, asanka wrote:
> Use a mock and set expectations so this is consistent with the rest of the
test.

Done.

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
content/browser/download/download_manager_impl_unittest.cc:609: stream.get(), _,
_));
On 2013/08/08 21:16:01, asanka wrote:
> Create a MockDownloadFile here, set its expectations to receive the client ID,
> and then return it from the MockCreateFile call.
> 
> Something like:
> 
> MockDownloadFile* mock_file = new MockDownloadFile;
> EXPECT_CALL(*mock_file, SetClientGuid(std::string("client-id")));
> 
> EXPECT_CALL(..., MockCreateFile(...))
>   .WillOnce(Return(mock_file));

Done, but as an aside I think these tests would be improved by less use of
gmock.

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
content/browser/download/download_manager_impl_unittest.cc:613: EXPECT_TRUE(NULL
!= download_manager_->GetDownload(local_id));
On 2013/08/08 21:16:01, asanka wrote:
> Is this change necessary?

I'll put it back.

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
File content/browser/download/mock_download_file.h (right):

https://codereview.chromium.org/21355004/diff/126001/content/browser/download...
content/browser/download/mock_download_file.h:52: void SetClientGuid(const
std::string& guid) OVERRIDE {
On 2013/08/08 21:16:01, asanka wrote:
> MOCK_METHOD1(...)
> 
> You can later use EXPECT_CALL(...) and friends to check if the expected value
> was seen.

Done.

[asanka, 2013-08-09 15:05:16]

LGTM. Thanks!

https://codereview.chromium.org/21355004/diff/141001/content/browser/download...
File content/browser/download/download_manager_impl_unittest.cc (right):

https://codereview.chromium.org/21355004/diff/141001/content/browser/download...
content/browser/download/download_manager_impl_unittest.cc:601:
.WillOnce(Return(ByRef(mock_file)));
Nit: ByRef() isn't required here.

[Greg Billock, 2013-08-09 17:50:20]

https://codereview.chromium.org/21355004/diff/141001/content/browser/download...
File content/browser/download/download_manager_impl_unittest.cc (right):

https://codereview.chromium.org/21355004/diff/141001/content/browser/download...
content/browser/download/download_manager_impl_unittest.cc:601:
.WillOnce(Return(ByRef(mock_file)));
On 2013/08/09 15:05:17, asanka wrote:
> Nit: ByRef() isn't required here.

Done.

[commit-bot: I haz the power, 2013-08-09 17:52:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/21355004/145001

[commit-bot: I haz the power, 2013-08-09 21:52:28]

Retried try job too often on win7_aura for step(s) content_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-08-09 21:53:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/21355004/127017

[commit-bot: I haz the power, 2013-08-10 01:00:05]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-08-10 02:17:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/21355004/179001

[commit-bot: I haz the power, 2013-08-10 13:49:59]

Change committed as 216843