Make MediaFileUtil run on thread pool.

Move {Native,Device}MediaFileUtil from FILE thread to SequencedWorkerPool to avoid it blocks other I/O.

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

[tzik, 2012-08-02 18:03:12]

Could you take a look?

[kinuko, 2012-08-02 21:12:46]

Are adding some guard in FileUtilHelper not to perform cross-filleutil operation
which use different TRs?

http://codereview.chromium.org/10829147/diff/6001/content/browser/fileapi/bro...
File content/browser/fileapi/browser_file_system_helper.cc (right):

http://codereview.chromium.org/10829147/diff/6001/content/browser/fileapi/bro...
content/browser/fileapi/browser_file_system_helper.cc:52:
pool->GetSequencedTaskRunner(media_sequence_token),
Having this as the first argument makes me feel that this task runner is the
most important TR while it would not.

http://codereview.chromium.org/10829147/diff/6001/content/browser/fileapi/bro...
content/browser/fileapi/browser_file_system_helper.cc:54:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO),
When we have more and more task runners this wouldn't scale well.  It could be
in a different CL but maybe we could have a separate class which returns
appropriate task runners for different purposes (Media/File/IO/etc)?
Some FileUtil/FileSystemOperation might need to handle multiple task runners and
would be nice to have some neat interface.
Mock implementation of the class just return current message loop.  Chrome
implementation could keep multiple sequence tokens and return appropriate TRs.

http://codereview.chromium.org/10829147/diff/2003/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context.cc (right):

http://codereview.chromium.org/10829147/diff/2003/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.cc:64: isolated_provider_(new
IsolatedMountPointProvider(media_task_runner)) {
I may prefer keeping this task_runner as a field of FileSystemContext and have a
getter for the TR as we do so for other TRs.

http://codereview.chromium.org/10829147/diff/2003/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation_context.h (right):

http://codereview.chromium.org/10829147/diff/2003/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation_context.h:42: void
SetTaskRunner(base::SequencedTaskRunner* task_runner);
Please add comment which task runner we use if it's not called.

[tzik, 2012-08-03 22:29:18]

http://codereview.chromium.org/10829147/diff/6001/content/browser/fileapi/bro...
File content/browser/fileapi/browser_file_system_helper.cc (right):

http://codereview.chromium.org/10829147/diff/6001/content/browser/fileapi/bro...
content/browser/fileapi/browser_file_system_helper.cc:52:
pool->GetSequencedTaskRunner(media_sequence_token),
On 2012/08/02 21:12:46, kinuko wrote:
> Having this as the first argument makes me feel that this task runner is the
> most important TR while it would not.

Done.

http://codereview.chromium.org/10829147/diff/6001/content/browser/fileapi/bro...
content/browser/fileapi/browser_file_system_helper.cc:54:
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO),
On 2012/08/02 21:12:46, kinuko wrote:
> When we have more and more task runners this wouldn't scale well.  It could be
> in a different CL but maybe we could have a separate class which returns
> appropriate task runners for different purposes (Media/File/IO/etc)?
> Some FileUtil/FileSystemOperation might need to handle multiple task runners
and
> would be nice to have some neat interface.
> Mock implementation of the class just return current message loop.  Chrome
> implementation could keep multiple sequence tokens and return appropriate TRs.

Done.

http://codereview.chromium.org/10829147/diff/2003/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_context.cc (right):

http://codereview.chromium.org/10829147/diff/2003/webkit/fileapi/file_system_...
webkit/fileapi/file_system_context.cc:64: isolated_provider_(new
IsolatedMountPointProvider(media_task_runner)) {
On 2012/08/02 21:12:46, kinuko wrote:
> I may prefer keeping this task_runner as a field of FileSystemContext and have
a
> getter for the TR as we do so for other TRs.

Done. Moved to FileSystemTaskRunners.

http://codereview.chromium.org/10829147/diff/2003/webkit/fileapi/file_system_...
File webkit/fileapi/file_system_operation_context.h (right):

http://codereview.chromium.org/10829147/diff/2003/webkit/fileapi/file_system_...
webkit/fileapi/file_system_operation_context.h:42: void
SetTaskRunner(base::SequencedTaskRunner* task_runner);
On 2012/08/02 21:12:46, kinuko wrote:
> Please add comment which task runner we use if it's not called.

Done.

[kinuko, 2012-08-03 23:13:39]

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
webkit/fileapi/file_system_context.h:51: // |file_task_runner| is used for all
file operations and file related
nit: can you update the comment as well?

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
File webkit/fileapi/file_system_operation_context.h (right):

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
webkit/fileapi/file_system_operation_context.h:59: // FILE thread is set as
default value.
nit: "file_system_context_->task_runners()->file_task_runner() is used
otherwise."

This method is not inline but it's a still simplest setter so I'd feel this
should be named set_task_runner()... though not 100% sure.

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
File webkit/fileapi/file_system_task_runners.cc (right):

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
webkit/fileapi/file_system_task_runners.cc:16: base::SequencedTaskRunner*
media_task_runner)
indent is off?

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
File webkit/fileapi/file_system_task_runners.h (right):

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
webkit/fileapi/file_system_task_runners.h:25: FileSystemTaskRunners();
Please add some comments, or add a static method like
CreateMockFileSystemTaskRunners() so that we can make sure we do not misuse this
class outside test code.  (I prefer the latter)

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
webkit/fileapi/file_system_task_runners.h:44: 
nit: please remove this empty line

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/local_file_...
File webkit/fileapi/local_file_system_operation.cc (right):

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/local_file_...
webkit/fileapi/local_file_system_operation.cc:37: base::PlatformFileError
IsCompatibleFileSystemPair(FileSystemType src_type,
nit: IsCrossOperationAllowed(src, dest) ...?  Also if this method is named
'IsFoo' I think this should just return boolean.

Please add comments.
"If two types are supposed to run on different task runners we should not allow
cross FileUtil operations at this layer."

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/local_file_...
webkit/fileapi/local_file_system_operation.cc:42: 
nit: extra empty line

[tzik, 2012-08-04 00:24:21]

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
File webkit/fileapi/file_system_context.h (right):

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
webkit/fileapi/file_system_context.h:51: // |file_task_runner| is used for all
file operations and file related
On 2012/08/03 23:13:39, kinuko wrote:
> nit: can you update the comment as well?

Done.

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
File webkit/fileapi/file_system_operation_context.h (right):

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
webkit/fileapi/file_system_operation_context.h:59: // FILE thread is set as
default value.
On 2012/08/03 23:13:39, kinuko wrote:
> nit: "file_system_context_->task_runners()->file_task_runner() is used
> otherwise."
> 
> This method is not inline but it's a still simplest setter so I'd feel this
> should be named set_task_runner()... though not 100% sure.

Done.

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
File webkit/fileapi/file_system_task_runners.cc (right):

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
webkit/fileapi/file_system_task_runners.cc:16: base::SequencedTaskRunner*
media_task_runner)
On 2012/08/03 23:13:39, kinuko wrote:
> indent is off?

Done.

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
File webkit/fileapi/file_system_task_runners.h (right):

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
webkit/fileapi/file_system_task_runners.h:25: FileSystemTaskRunners();
On 2012/08/03 23:13:39, kinuko wrote:
> Please add some comments, or add a static method like
> CreateMockFileSystemTaskRunners() so that we can make sure we do not misuse
this
> class outside test code.  (I prefer the latter)

Done.

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/file_system...
webkit/fileapi/file_system_task_runners.h:44: 
On 2012/08/03 23:13:39, kinuko wrote:
> nit: please remove this empty line

Done.

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/local_file_...
File webkit/fileapi/local_file_system_operation.cc (right):

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/local_file_...
webkit/fileapi/local_file_system_operation.cc:37: base::PlatformFileError
IsCompatibleFileSystemPair(FileSystemType src_type,
On 2012/08/03 23:13:39, kinuko wrote:
> nit: IsCrossOperationAllowed(src, dest) ...?  Also if this method is named
> 'IsFoo' I think this should just return boolean.
> 
> Please add comments.
> "If two types are supposed to run on different task runners we should not
allow
> cross FileUtil operations at this layer."

Done.

http://codereview.chromium.org/10829147/diff/13002/webkit/fileapi/local_file_...
webkit/fileapi/local_file_system_operation.cc:42: 
On 2012/08/03 23:13:39, kinuko wrote:
> nit: extra empty line

Done.

[kinuko, 2012-08-06 19:00:14]

almost good

http://codereview.chromium.org/10829147/diff/14030/webkit/fileapi/file_system...
File webkit/fileapi/file_system_task_runners.h (right):

http://codereview.chromium.org/10829147/diff/14030/webkit/fileapi/file_system...
webkit/fileapi/file_system_task_runners.h:19: class FILEAPI_EXPORT
FileSystemTaskRunners {
nit: a brief comment please?

http://codereview.chromium.org/10829147/diff/14030/webkit/fileapi/file_system...
webkit/fileapi/file_system_task_runners.h:26: static
scoped_ptr<FileSystemTaskRunners> CreateMockTaskRunners();
nit: please put this after the public dtor

http://codereview.chromium.org/10829147/diff/14030/webkit/fileapi/local_file_...
File webkit/fileapi/local_file_system_operation.cc (right):

http://codereview.chromium.org/10829147/diff/14030/webkit/fileapi/local_file_...
webkit/fileapi/local_file_system_operation.cc:39: base::PlatformFileError
AllowCrossOperation(FileSystemType src_type,
This function name doesn't sound like a check function.  I vote for
IsCrossOperationAllowed (returns bool) or maybe CheckIfCrossOperationAllowed
(returns error code)?

[tzik, 2012-08-06 20:09:01]

http://codereview.chromium.org/10829147/diff/14030/webkit/fileapi/file_system...
File webkit/fileapi/file_system_task_runners.h (right):

http://codereview.chromium.org/10829147/diff/14030/webkit/fileapi/file_system...
webkit/fileapi/file_system_task_runners.h:19: class FILEAPI_EXPORT
FileSystemTaskRunners {
On 2012/08/06 19:00:14, kinuko wrote:
> nit: a brief comment please?

Done.

http://codereview.chromium.org/10829147/diff/14030/webkit/fileapi/file_system...
webkit/fileapi/file_system_task_runners.h:26: static
scoped_ptr<FileSystemTaskRunners> CreateMockTaskRunners();
On 2012/08/06 19:00:14, kinuko wrote:
> nit: please put this after the public dtor

Done.

http://codereview.chromium.org/10829147/diff/14030/webkit/fileapi/local_file_...
File webkit/fileapi/local_file_system_operation.cc (right):

http://codereview.chromium.org/10829147/diff/14030/webkit/fileapi/local_file_...
webkit/fileapi/local_file_system_operation.cc:39: base::PlatformFileError
AllowCrossOperation(FileSystemType src_type,
On 2012/08/06 19:00:14, kinuko wrote:
> This function name doesn't sound like a check function.  I vote for
> IsCrossOperationAllowed (returns bool) or maybe CheckIfCrossOperationAllowed
> (returns error code)?

Done.

[kinuko, 2012-08-06 20:39:52]

lgtm

[commit-bot: I haz the power, 2012-08-06 21:50:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/10829147/7053

[commit-bot: I haz the power, 2012-08-06 21:51:03]

Presubmit check for 10829147-7053 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    webkit

Presubmit checks took 5.0s to calculate.

[tzik, 2012-08-06 22:05:39]

I forgot ask OWNERS to review.

Tony, could you take a look to webkit/tools/test_shell/simple_file_system.cc?

[tony, 2012-08-06 23:56:34]

webkit/tools LGTM

[tzik, 2012-08-06 23:58:45]

On 2012/08/06 23:56:34, tony wrote:
> webkit/tools LGTM

Thanks!

[commit-bot: I haz the power, 2012-08-06 23:59:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/10829147/7053

[commit-bot: I haz the power, 2012-08-07 01:25:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/10829147/6032