Support filesystem files from BlobURLRequestJob

Support filesystem files from BlobURLRequestJob

BlobURLRequestJob's ctor takes FileSystemContext to create FileSystemFileStreamReader.

BUG=141835
TEST=content_unittests --gtest_filter="BlobURLRequestJobTest.*"

TBR=jam@chromium.org for content change

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163245

[hashimoto, 2012-10-18 10:39:34]

Please review

[kinuko, 2012-10-18 11:57:53]

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job.cc:26: #include
"webkit/fileapi/file_system_file_stream_reader.h"
not needed?

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job.cc:315: switch (item.type()) {
In this way there may not be much value to use switch/case here.

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:35: #include
"webkit/fileapi/sandbox_mount_point_provider.h"
I think we can avoid including this file. (See comments below)

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:36: #include
"webkit/quota/mock_special_storage_policy.h"
ditto

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:60: return GURL(url_string);
Can we use GetFileSystemRootURI()? something like:

GURL(fileapi::GetFileSystemRootURI(GURL(kFileSystemURLOrigin),
fileapi::kFileSystemTypeTemporary).spec() + filename)

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:199:
sandbox_provider()->GetFileUtil(fileapi::kFileSystemTypeTemporary);
file_system_context_->GetFileUtil(type) should work.

It might be nicer to define some constant for fileapi::kFileSystemTypeTemporary
so that we can easily change testing fs type if necessary (e.g. we can instead
use kFileSystemTypeTest for simpler tests) & save typing.

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:234: // Prepare file system.
It feels a bit heavy to run this setup for every test.  Can we only setup
filesystem env for the tests that use FileSystemURL?

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:238: new
quota::MockSpecialStoragePolicy,
special_storage_policy_ ?

Actually I think you can pass NULL here.

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:244: true /* create */,
base::Bind(&OnValidateFileSystem));
Can we instead call file_system_context_->OpenFileSystem() here?

[hashimoto, 2012-10-18 13:38:13]

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job.cc:26: #include
"webkit/fileapi/file_system_file_stream_reader.h"
On 2012/10/18 11:57:54, kinuko wrote:
> not needed?

Done.

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job.cc:315: switch (item.type()) {
On 2012/10/18 11:57:54, kinuko wrote:
> In this way there may not be much value to use switch/case here.

Done.

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:35: #include
"webkit/fileapi/sandbox_mount_point_provider.h"
On 2012/10/18 11:57:54, kinuko wrote:
> I think we can avoid including this file. (See comments below)

Done.

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:36: #include
"webkit/quota/mock_special_storage_policy.h"
On 2012/10/18 11:57:54, kinuko wrote:
> ditto

Done.

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:60: return GURL(url_string);
On 2012/10/18 11:57:54, kinuko wrote:
> Can we use GetFileSystemRootURI()? something like:
> 
> GURL(fileapi::GetFileSystemRootURI(GURL(kFileSystemURLOrigin),
> fileapi::kFileSystemTypeTemporary).spec() + filename)

Done.

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:199:
sandbox_provider()->GetFileUtil(fileapi::kFileSystemTypeTemporary);
On 2012/10/18 11:57:54, kinuko wrote:
> file_system_context_->GetFileUtil(type) should work.
Done.

Here I was mimicking file_system_url_request_job_unittest.cc, and
file_system_file_stream_reader_unitteest.cc is also doing the same thing.
Should I also fix them?
> 
> It might be nicer to define some constant for
fileapi::kFileSystemTypeTemporary
> so that we can easily change testing fs type if necessary (e.g. we can instead
> use kFileSystemTypeTest for simpler tests) & save typing.

Done.

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:234: // Prepare file system.
On 2012/10/18 11:57:54, kinuko wrote:
> It feels a bit heavy to run this setup for every test.  Can we only setup
> filesystem env for the tests that use FileSystemURL?
Sorry, it looks my understanding of webkit/fileapi code is not so good.
You mean there is a light weight utility which can be used to provide
FileSystemContext for tests?

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:238: new
quota::MockSpecialStoragePolicy,
On 2012/10/18 11:57:54, kinuko wrote:
> special_storage_policy_ ?
Sorry, something went wrong during merge.
> 
> Actually I think you can pass NULL here.
Done.

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:244: true /* create */,
base::Bind(&OnValidateFileSystem));
On 2012/10/18 11:57:54, kinuko wrote:
> Can we instead call file_system_context_->OpenFileSystem() here?

Done.

[kinuko, 2012-10-19 04:07:19]

looking good

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:199:
sandbox_provider()->GetFileUtil(fileapi::kFileSystemTypeTemporary);
On 2012/10/18 13:38:13, hashimoto wrote:
> On 2012/10/18 11:57:54, kinuko wrote:
> > file_system_context_->GetFileUtil(type) should work.
> Done.
> 
> Here I was mimicking file_system_url_request_job_unittest.cc, and
> file_system_file_stream_reader_unitteest.cc is also doing the same thing.
> Should I also fix them?

I see... could you fix them as well in a separate patch?  Directly using
sandbox_provider() is not very portable unless we're specifically interested in
testing the SandboxMountPointProvider.

> > 
> > It might be nicer to define some constant for
> fileapi::kFileSystemTypeTemporary
> > so that we can easily change testing fs type if necessary (e.g. we can
instead
> > use kFileSystemTypeTest for simpler tests) & save typing.
> 
> Done.

http://codereview.chromium.org/11103027/diff/15001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:234: // Prepare file system.
On 2012/10/18 13:38:13, hashimoto wrote:
> On 2012/10/18 11:57:54, kinuko wrote:
> > It feels a bit heavy to run this setup for every test.  Can we only setup
> > filesystem env for the tests that use FileSystemURL?
> Sorry, it looks my understanding of webkit/fileapi code is not so good.
> You mean there is a light weight utility which can be used to provide
> FileSystemContext for tests?

As we chatted I meant we shouldn't always do this but can put this part in a
separate function and call it only in tests that are related to FileSystemURL.

http://codereview.chromium.org/11103027/diff/25001/webkit/blob/blob_url_reque...
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/11103027/diff/25001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:57: return GURL(root_uri.spec() +
filename);
Btw this can be also implemented using root URL returned by OpenFileSystem (you
can refer it in OnValidateFileSystem). You can remember the root and make this
function a member method.

http://codereview.chromium.org/11103027/diff/25001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:249: const char
kFileSystemFilename1[] = "FileSystemFile1.dat";
I'd use shorter names if we only use this variable in a few lines below.

http://codereview.chromium.org/11103027/diff/25001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_util.h (right):

http://codereview.chromium.org/11103027/diff/25001/webkit/fileapi/file_system...
webkit/fileapi/file_system_util.h:26: WEBKIT_STORAGE_EXPORT extern const char
kTestDir[];
This change is not necessary now?

[hashimoto, 2012-10-19 07:07:45]

New patch set which replaces the existing hand-made task scheduling system with
base::RunLoop.
PTAL.

http://codereview.chromium.org/11103027/diff/25001/webkit/blob/blob_url_reque...
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/11103027/diff/25001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:57: return GURL(root_uri.spec() +
filename);
On 2012/10/19 04:07:19, kinuko wrote:
> Btw this can be also implemented using root URL returned by OpenFileSystem
(you
> can refer it in OnValidateFileSystem). You can remember the root and make this
> function a member method.

Done.

http://codereview.chromium.org/11103027/diff/25001/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:249: const char
kFileSystemFilename1[] = "FileSystemFile1.dat";
On 2012/10/19 04:07:19, kinuko wrote:
> I'd use shorter names if we only use this variable in a few lines below.

Done.

http://codereview.chromium.org/11103027/diff/25001/webkit/fileapi/file_system...
File webkit/fileapi/file_system_util.h (right):

http://codereview.chromium.org/11103027/diff/25001/webkit/fileapi/file_system...
webkit/fileapi/file_system_util.h:26: WEBKIT_STORAGE_EXPORT extern const char
kTestDir[];
On 2012/10/19 04:07:19, kinuko wrote:
> This change is not necessary now?

You're right, thanks.
Done.

[kinuko, 2012-10-19 11:18:36]

lgtm!

Thanks for cleaning up the task running part, which was making the code hard to
follow.

By the way if we fix the part I guess we can also completely get rid of the
outer RunTestOnIOThread part if we construct the current message loop type as IO
type (e.g. having MessageLoop in the test class and create it with
MessageLoop::TYPE_IO).  Then we can possibly get rid of the
ScopedNestableTaskAllower too.

http://codereview.chromium.org/11103027/diff/23019/webkit/blob/blob_url_reque...
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/11103027/diff/23019/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job.cc:320: } else {
nit: since we're returning in each if we may not need else's.

http://codereview.chromium.org/11103027/diff/23019/webkit/blob/blob_url_reque...
File webkit/blob/blob_url_request_job_factory.h (right):

http://codereview.chromium.org/11103027/diff/23019/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_factory.h:34: explicit BlobProtocolHandler(
looks like we can drop this explicit now (not directly related to this cl
though)

[hashimoto, 2012-10-19 14:59:07]

http://codereview.chromium.org/11103027/diff/23019/webkit/blob/blob_url_reque...
File webkit/blob/blob_url_request_job.cc (right):

http://codereview.chromium.org/11103027/diff/23019/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job.cc:320: } else {
On 2012/10/19 11:18:36, kinuko wrote:
> nit: since we're returning in each if we may not need else's.

Done.

http://codereview.chromium.org/11103027/diff/23019/webkit/blob/blob_url_reque...
File webkit/blob/blob_url_request_job_factory.h (right):

http://codereview.chromium.org/11103027/diff/23019/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_factory.h:34: explicit BlobProtocolHandler(
On 2012/10/19 11:18:36, kinuko wrote:
> looks like we can drop this explicit now (not directly related to this cl
> though)
Good catch,
Done.

[hashimoto, 2012-10-19 15:06:17]

On 2012/10/19 11:18:36, kinuko wrote:
> lgtm!
> 
> Thanks for cleaning up the task running part, which was making the code hard
to
> follow.
> 
> By the way if we fix the part I guess we can also completely get rid of the
> outer RunTestOnIOThread part if we construct the current message loop type as
IO
> type (e.g. having MessageLoop in the test class and create it with
> MessageLoop::TYPE_IO).  Then we can possibly get rid of the
> ScopedNestableTaskAllower too.
Sounds good.
Will do it in the next patch.
> 
>
http://codereview.chromium.org/11103027/diff/23019/webkit/blob/blob_url_reque...
> File webkit/blob/blob_url_request_job.cc (right):
> 
>
http://codereview.chromium.org/11103027/diff/23019/webkit/blob/blob_url_reque...
> webkit/blob/blob_url_request_job.cc:320: } else {
> nit: since we're returning in each if we may not need else's.
> 
>
http://codereview.chromium.org/11103027/diff/23019/webkit/blob/blob_url_reque...
> File webkit/blob/blob_url_request_job_factory.h (right):
> 
>
http://codereview.chromium.org/11103027/diff/23019/webkit/blob/blob_url_reque...
> webkit/blob/blob_url_request_job_factory.h:34: explicit BlobProtocolHandler(
> looks like we can drop this explicit now (not directly related to this cl
> though)

[hashimoto, 2012-10-19 15:08:22]

TBR-ing jam@ for content change

[commit-bot: I haz the power, 2012-10-19 15:08:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/11103027/35001

[jam, 2012-10-19 16:49:01]

lgtm

[commit-bot: I haz the power, 2012-10-19 18:12:05]

Retried try job too often for step(s) content_unittests

[hashimoto, 2012-10-22 03:52:10]

New patch set which stops creating IO thread for BlobURLRequestJobTest because
win bots do not allow nesting IO message loop.
PTAL.

[kinuko, 2012-10-22 05:29:59]

It's getting much simpler now.. :)

http://codereview.chromium.org/11103027/diff/16050/webkit/blob/blob_url_reque...
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/11103027/diff/16050/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:154: BlobURLRequestJobTest()
nit: To follow the coding convention can we put this at the top of the class
(i.e. after MockURLRequestDelegate)

http://codereview.chromium.org/11103027/diff/16050/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:426: SetUpFileSystem();
No need to call this? (Here and below)

[hashimoto, 2012-10-22 06:29:03]

http://codereview.chromium.org/11103027/diff/16050/webkit/blob/blob_url_reque...
File webkit/blob/blob_url_request_job_unittest.cc (right):

http://codereview.chromium.org/11103027/diff/16050/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:154: BlobURLRequestJobTest()
On 2012/10/22 05:29:59, kinuko wrote:
> nit: To follow the coding convention can we put this at the top of the class
> (i.e. after MockURLRequestDelegate)

Done.

http://codereview.chromium.org/11103027/diff/16050/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job_unittest.cc:426: SetUpFileSystem();
On 2012/10/22 05:29:59, kinuko wrote:
> No need to call this? (Here and below)

BuildComplicatedData() appends files with FileSystem URL.
In previous patch sets, SetUpFileSystem() is called in BuildComplicatedData(),
but I thought it should be called in each test and moved it.
Sorry for confusing.

[kinuko, 2012-10-22 06:38:22]

lgtm

[commit-bot: I haz the power, 2012-10-22 06:41:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/11103027/39005

[commit-bot: I haz the power, 2012-10-22 09:24:34]

Change committed as 163245