Add a copy or move validator to that can handle support audio and video files.

Add a copy or move validator to that can handle support audio and video files.

The validator asks a utility process to open and attempt to decode part of the file.

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

[vandebo (ex-Chrome), 2013-08-07 06:05:32]

csharp for isolate changes

This depends on https://codereview.chromium.org/20572004/

[csharp, 2013-08-07 12:49:38]

isolate change LGTM, but I'm wondering why media_file_validator_unittest.cc
isn't called media_file_validator_browsertest.cc (Because of this I thought you
were modifying the wrong isolate file).

If there isn't some reason to keep it as it, could you change the name of the
file to indicate it is a browser test?

[vandebo (ex-Chrome), 2013-08-07 15:42:36]

On 2013/08/07 12:49:38, csharp wrote:
> isolate change LGTM, but I'm wondering why media_file_validator_unittest.cc
> isn't called media_file_validator_browsertest.cc (Because of this I thought
you
> were modifying the wrong isolate file).
> 
> If there isn't some reason to keep it as it, could you change the name of the
> file to indicate it is a browser test?

It was supposed to be a unit test, but you can't use the utility process from a
unit test so I changed it to a browser test so I changed it to a browser test
and forgot to update the filename.  Fixed.

[Lei Zhang, 2013-08-08 00:08:01]

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_file_validator_browsertest.cc
(right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_validator_browsertest.cc:62:
test_file = test_file.Append(FILE_PATH_LITERAL("media"));
    test_file =
        test_file.AppendASCII("media").AppendASCII("test").AppendASCII("data");

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_validator_browsertest.cc:92:
void MoveTestFromFile(const std::string& filename,
Add a comment.

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_validator_browsertest.cc:264:
base::FilePath test_file = GetMediaTestDir().Append(
AppendASCII() is shorter than APPEND(FPL()).

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_validator_browsertest.cc:266:
ASSERT_FALSE(test_file.empty());
This will never fail because you always do Append() on a possibly empty return
result from GetMediaTestDir().

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc (right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:13: #include
"content/public/browser/child_process_data.h"
not needed?

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:35: if
(*file_closer_ && *file_closer_.get() == base::kInvalidPlatformFileValue) {
*file_closer_ can't be NULL

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/supported_audio_video_checker.cc
(right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/supported_audio_video_checker.cc:48:
static base::LazyInstance<SupportedAudioVideoExtensions>
Doesn't need to be static.

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/supported_audio_video_checker.h
(right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/supported_audio_video_checker.h:21: class
SupportedAudioVideoChecker : public fileapi::CopyOrMoveFileValidator {
Can you add a class description?

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:125:
result_callback.Run(base::PLATFORM_FILE_OK);
Need to rebase now that https://codereview.chromium.org/22441004/ has landed,
right?

[vandebo (ex-Chrome), 2013-08-08 17:34:00]

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/media_file_validator_browsertest.cc
(right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_validator_browsertest.cc:62:
test_file = test_file.Append(FILE_PATH_LITERAL("media"));
On 2013/08/08 00:08:02, Lei Zhang wrote:
>     test_file =
>        
test_file.AppendASCII("media").AppendASCII("test").AppendASCII("data");

Done.

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_validator_browsertest.cc:92:
void MoveTestFromFile(const std::string& filename,
On 2013/08/08 00:08:02, Lei Zhang wrote:
> Add a comment.

Done.

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_validator_browsertest.cc:264:
base::FilePath test_file = GetMediaTestDir().Append(
On 2013/08/08 00:08:02, Lei Zhang wrote:
> AppendASCII() is shorter than APPEND(FPL()).

Done.

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/media_file_validator_browsertest.cc:266:
ASSERT_FALSE(test_file.empty());
On 2013/08/08 00:08:02, Lei Zhang wrote:
> This will never fail because you always do Append() on a possibly empty return
> result from GetMediaTestDir().

Done.

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc (right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:13: #include
"content/public/browser/child_process_data.h"
On 2013/08/08 00:08:02, Lei Zhang wrote:
> not needed?

It is, used on line 59 on this patch set and
content/public/browser/utility_process_host.h only forward declares it.

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:35: if
(*file_closer_ && *file_closer_.get() == base::kInvalidPlatformFileValue) {
On 2013/08/08 00:08:02, Lei Zhang wrote:
> *file_closer_ can't be NULL

It can be... *file_closer_ <=> file_closer_.get(), which is a PlatformFile*. 
file_closer_.reset() will make *file_closer_ NULL

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/supported_audio_video_checker.cc
(right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/supported_audio_video_checker.cc:48:
static base::LazyInstance<SupportedAudioVideoExtensions>
On 2013/08/08 00:08:02, Lei Zhang wrote:
> Doesn't need to be static.

Done.

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/supported_audio_video_checker.h
(right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/supported_audio_video_checker.h:21: class
SupportedAudioVideoChecker : public fileapi::CopyOrMoveFileValidator {
On 2013/08/08 00:08:02, Lei Zhang wrote:
> Can you add a class description?

Done.

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc
(right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/supported_image_type_validator.cc:125:
result_callback.Run(base::PLATFORM_FILE_OK);
On 2013/08/08 00:08:02, Lei Zhang wrote:
> Need to rebase now that https://codereview.chromium.org/22441004/ has landed,
> right?

Done.

[Lei Zhang, 2013-08-08 18:18:07]

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc (right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:35: if
(*file_closer_ && *file_closer_.get() == base::kInvalidPlatformFileValue) {
On 2013/08/08 17:34:01, vandebo wrote:
> On 2013/08/08 00:08:02, Lei Zhang wrote:
> > *file_closer_ can't be NULL
> 
> It can be... *file_closer_ <=> file_closer_.get(), which is a PlatformFile*. 
> file_closer_.reset() will make *file_closer_ NULL

But nobody calls file_closer_.reset() except the dtor, right?

[vandebo (ex-Chrome), 2013-08-08 23:14:11]

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc (right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:35: if
(*file_closer_ && *file_closer_.get() == base::kInvalidPlatformFileValue) {
On 2013/08/08 18:18:07, Lei Zhang wrote:
> On 2013/08/08 17:34:01, vandebo wrote:
> > On 2013/08/08 00:08:02, Lei Zhang wrote:
> > > *file_closer_ can't be NULL
> > 
> > It can be... *file_closer_ <=> file_closer_.get(), which is a PlatformFile*.

> > file_closer_.reset() will make *file_closer_ NULL
> 
> But nobody calls file_closer_.reset() except the dtor, right?

SafeAudioVideoChecker::OnProcessStarted() calls files_closer_.release()

[Lei Zhang, 2013-08-08 23:16:21]

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc (right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:35: if
(*file_closer_ && *file_closer_.get() == base::kInvalidPlatformFileValue) {
On 2013/08/08 23:14:11, vandebo wrote:
> On 2013/08/08 18:18:07, Lei Zhang wrote:
> > On 2013/08/08 17:34:01, vandebo wrote:
> > > On 2013/08/08 00:08:02, Lei Zhang wrote:
> > > > *file_closer_ can't be NULL
> > > 
> > > It can be... *file_closer_ <=> file_closer_.get(), which is a
PlatformFile*.
> 
> > > file_closer_.reset() will make *file_closer_ NULL
> > 
> > But nobody calls file_closer_.reset() except the dtor, right?
> 
> SafeAudioVideoChecker::OnProcessStarted() calls files_closer_.release()

After that, the |state_| check makes sure we'll never get here.

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

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc (right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:35: if
(*file_closer_ && *file_closer_.get() == base::kInvalidPlatformFileValue) {
On 2013/08/08 23:16:21, Lei Zhang wrote:
> On 2013/08/08 23:14:11, vandebo wrote:
> > On 2013/08/08 18:18:07, Lei Zhang wrote:
> > > On 2013/08/08 17:34:01, vandebo wrote:
> > > > On 2013/08/08 00:08:02, Lei Zhang wrote:
> > > > > *file_closer_ can't be NULL
> > > > 
> > > > It can be... *file_closer_ <=> file_closer_.get(), which is a
> PlatformFile*.
> > 
> > > > file_closer_.reset() will make *file_closer_ NULL
> > > 
> > > But nobody calls file_closer_.reset() except the dtor, right?
> > 
> > SafeAudioVideoChecker::OnProcessStarted() calls files_closer_.release()
> 
> After that, the |state_| check makes sure we'll never get here.

In the current version of the code, yes.

[Lei Zhang, 2013-08-09 01:01:33]

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc (right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:35: if
(*file_closer_ && *file_closer_.get() == base::kInvalidPlatformFileValue) {
On 2013/08/08 23:18:24, vandebo wrote:
> In the current version of the code, yes.

Then remove the check? Adjust accordingly if the code changes in the future.

[Lei Zhang, 2013-08-09 01:01:44]

And otherwise lgtm.

[vandebo (ex-Chrome), 2013-08-09 04:11:09]

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
File chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc (right):

https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:35: if
(*file_closer_ && *file_closer_.get() == base::kInvalidPlatformFileValue) {
On 2013/08/09 01:01:33, Lei Zhang wrote:
> On 2013/08/08 23:18:24, vandebo wrote:
> > In the current version of the code, yes.
> 
> Then remove the check? Adjust accordingly if the code changes in the future.

I'd rather leave it in.  It doesn't cost much, is somewhat subtle and brings the
check to the place where it's important instead of having the check far removed
from the thing needing the check.

[Lei Zhang, 2013-08-09 04:41:20]

On 2013/08/09 04:11:09, vandebo wrote:
>
https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
> File chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc
(right):
> 
>
https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
> chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:35: if
> (*file_closer_ && *file_closer_.get() == base::kInvalidPlatformFileValue) {
> On 2013/08/09 01:01:33, Lei Zhang wrote:
> > On 2013/08/08 23:18:24, vandebo wrote:
> > > In the current version of the code, yes.
> > 
> > Then remove the check? Adjust accordingly if the code changes in the future.
> 
> I'd rather leave it in.  It doesn't cost much, is somewhat subtle and brings
the
> check to the place where it's important instead of having the check far
removed
> from the thing needing the check.

I don't care that much, but I just feel it's useless since it always evaluates
to true with the code as is.

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

On 2013/08/09 04:41:20, Lei Zhang wrote:
> On 2013/08/09 04:11:09, vandebo wrote:
> >
>
https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
> > File chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/20190002/diff/147001/chrome/browser/media_gal...
> > chrome/browser/media_galleries/fileapi/safe_audio_video_checker.cc:35: if
> > (*file_closer_ && *file_closer_.get() == base::kInvalidPlatformFileValue) {
> > On 2013/08/09 01:01:33, Lei Zhang wrote:
> > > On 2013/08/08 23:18:24, vandebo wrote:
> > > > In the current version of the code, yes.
> > > 
> > > Then remove the check? Adjust accordingly if the code changes in the
future.
> > 
> > I'd rather leave it in.  It doesn't cost much, is somewhat subtle and brings
> the
> > check to the place where it's important instead of having the check far
> removed
> > from the thing needing the check.
> 
> I don't care that much, but I just feel it's useless since it always evaluates
> to true with the code as is.

Changed to a DCHECK

[Lei Zhang, 2013-08-09 05:16:57]

lgtm++

[commit-bot: I haz the power, 2013-08-09 14:11:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/20190002/209002

[commit-bot: I haz the power, 2013-08-09 14:11:19]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2013-08-09 14:16:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/20190002/209002

[commit-bot: I haz the power, 2013-08-09 18:17:57]

Change committed as 216720