net: Stop using base::WorkerPool from UploadFileElementReader

net: Stop using base::WorkerPool from UploadFileElementReader

Pass TaskRunner to ResourceRequestBody::ResolveElementsAndCreateUploadDataStream and use it for file operations.

BUG=167524
TEST=net_unittests

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

[hashimoto, 2013-01-07 07:47:32]

mmenke@: Please review src/net.
kinuko@: Please review other parts.

[kinuko, 2013-01-07 08:16:22]

https://codereview.chromium.org/11778016/diff/2001/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.h (right):

https://codereview.chromium.org/11778016/diff/2001/net/base/upload_file_eleme...
net/base/upload_file_element_reader.h:32: FileStreamDeleter(base::TaskRunner*
task_runner);
explicit

https://codereview.chromium.org/11778016/diff/2001/webkit/glue/resource_reque...
File webkit/glue/resource_request_body.cc (right):

https://codereview.chromium.org/11778016/diff/2001/webkit/glue/resource_reque...
webkit/glue/resource_request_body.cc:124:
file_system_context->task_runners()->file_task_runner(),
While I can see why you wanted to pass file_system_context to this method for
future use it feels a bit weird to me using the FileSystemContext's task runner
for TYPE_FILE here (as it's not necessarily for fileapi files).  At least in
this patch shouldn't we simply pass in a task runner?

[hashimoto, 2013-01-07 09:12:33]

https://codereview.chromium.org/11778016/diff/2001/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.h (right):

https://codereview.chromium.org/11778016/diff/2001/net/base/upload_file_eleme...
net/base/upload_file_element_reader.h:32: FileStreamDeleter(base::TaskRunner*
task_runner);
On 2013/01/07 08:16:22, kinuko wrote:
> explicit

Done.

https://codereview.chromium.org/11778016/diff/2001/webkit/glue/resource_reque...
File webkit/glue/resource_request_body.cc (right):

https://codereview.chromium.org/11778016/diff/2001/webkit/glue/resource_reque...
webkit/glue/resource_request_body.cc:124:
file_system_context->task_runners()->file_task_runner(),
On 2013/01/07 08:16:22, kinuko wrote:
> While I can see why you wanted to pass file_system_context to this method for
> future use it feels a bit weird to me using the FileSystemContext's task
runner
> for TYPE_FILE here (as it's not necessarily for fileapi files).  At least in
> this patch shouldn't we simply pass in a task runner?

Done.

When we support Filesystem files, are we going to replace the TaskRunner with
FileSystemContext, or pass both TaskRunner and FileSystemContext?

[mmenke, 2013-01-07 16:40:51]

net/ LGTM

https://codereview.chromium.org/11778016/diff/9003/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.cc (right):

https://codereview.chromium.org/11778016/diff/9003/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:260: // It's safe to pass NULL for
|deleter| here because it will not be used.
nit:  "NULL to deleter" - The deleter itself is not NULL, just its task_runner_
is.

[mmenke, 2013-01-07 16:42:24]

https://codereview.chromium.org/11778016/diff/9003/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.h (right):

https://codereview.chromium.org/11778016/diff/9003/net/base/upload_file_eleme...
net/base/upload_file_element_reader.h:42:
UploadFileElementReader(base::TaskRunner* task_runner,
Think it's worth mentioning that |task_runner| should never be NULL, except in
unit tests.  May even be better to DCHECK that it's not NULL, and just pass in a
junk message loop in the unit tests, actually.

[hashimoto, 2013-01-08 02:27:36]

https://codereview.chromium.org/11778016/diff/9003/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.cc (right):

https://codereview.chromium.org/11778016/diff/9003/net/base/upload_file_eleme...
net/base/upload_file_element_reader.cc:260: // It's safe to pass NULL for
|deleter| here because it will not be used.
On 2013/01/07 16:40:51, Matt Menke wrote:
> nit:  "NULL to deleter" - The deleter itself is not NULL, just its
task_runner_
> is.

Now passing MessageLoopProxy::current() to satisfy DCHECK

https://codereview.chromium.org/11778016/diff/9003/net/base/upload_file_eleme...
File net/base/upload_file_element_reader.h (right):

https://codereview.chromium.org/11778016/diff/9003/net/base/upload_file_eleme...
net/base/upload_file_element_reader.h:42:
UploadFileElementReader(base::TaskRunner* task_runner,
On 2013/01/07 16:42:24, Matt Menke wrote:
> Think it's worth mentioning that |task_runner| should never be NULL, except in
> unit tests.  May even be better to DCHECK that it's not NULL, and just pass in
a
> junk message loop in the unit tests, actually.

Stopped allowing NULL task_runner.

[kinuko, 2013-01-08 03:32:47]

lgtm

https://codereview.chromium.org/11778016/diff/2001/webkit/glue/resource_reque...
File webkit/glue/resource_request_body.cc (right):

https://codereview.chromium.org/11778016/diff/2001/webkit/glue/resource_reque...
webkit/glue/resource_request_body.cc:124:
file_system_context->task_runners()->file_task_runner(),
On 2013/01/07 09:12:33, hashimoto wrote:
> On 2013/01/07 08:16:22, kinuko wrote:
> > While I can see why you wanted to pass file_system_context to this method
for
> > future use it feels a bit weird to me using the FileSystemContext's task
> runner
> > for TYPE_FILE here (as it's not necessarily for fileapi files).  At least in
> > this patch shouldn't we simply pass in a task runner?
> 
> Done.
> 
> When we support Filesystem files, are we going to replace the TaskRunner with
> FileSystemContext, or pass both TaskRunner and FileSystemContext?

To me passing both sounds more reasonable, though it's a bit unfortunate.

https://codereview.chromium.org/11778016/diff/25/webkit/glue/resource_request...
File webkit/glue/resource_request_body.h (right):

https://codereview.chromium.org/11778016/diff/25/webkit/glue/resource_request...
webkit/glue/resource_request_body.h:49: // any blob references using given
|blob_controller|.
Can you add a comment about |task_runner|?

https://codereview.chromium.org/11778016/diff/25/webkit/glue/resource_request...
webkit/glue/resource_request_body.h:50: // TODO(kinuko): Clean up this hack.
(While you're there can you remove this line... without detailed comment this
TODO wouldn't add any value to the code. Thanks,)

[hashimoto, 2013-01-08 04:09:54]

https://codereview.chromium.org/11778016/diff/25/webkit/glue/resource_request...
File webkit/glue/resource_request_body.h (right):

https://codereview.chromium.org/11778016/diff/25/webkit/glue/resource_request...
webkit/glue/resource_request_body.h:49: // any blob references using given
|blob_controller|.
On 2013/01/08 03:32:48, kinuko wrote:
> Can you add a comment about |task_runner|?

Done.

https://codereview.chromium.org/11778016/diff/25/webkit/glue/resource_request...
webkit/glue/resource_request_body.h:50: // TODO(kinuko): Clean up this hack.
On 2013/01/08 03:32:48, kinuko wrote:
> (While you're there can you remove this line... without detailed comment this
> TODO wouldn't add any value to the code. Thanks,)

Done.

[hashimoto, 2013-01-08 04:14:51]

tony, sky, mpcomplete,
Could you review this change as an owner of each directory?

tony: webkit
sky: content/browser
mpcomplete: chrome/browser/extensions/api/web_request

[sky, 2013-01-08 18:50:55]

Sorry, I'm not familiar with that particular file
(resource_dispatcher_host_impl.cc). Maybe John is (I've added him).

[Matt Perry, 2013-01-08 18:53:44]

lgtm

[jam, 2013-01-08 19:04:09]

lgtm

[tony, 2013-01-08 19:05:42]

webkit lgtm

[commit-bot: I haz the power, 2013-01-09 02:29:12]

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

[commit-bot: I haz the power, 2013-01-09 06:22:38]

Retried try job too often on win_rel for step(s) chrome_frame_net_tests

[hashimoto, 2013-01-09 10:52:09]

Chrome Frame looks angry about using MessageLoopProxy::current.
Converted InitInternal() in upload_file_element_reader.cc to a template function
to satisfy CF.

[commit-bot: I haz the power, 2013-01-10 03:54:22]

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

[commit-bot: I haz the power, 2013-01-10 07:23:10]

Retried try job too often on win_aura for step(s) content_browsertests

[commit-bot: I haz the power, 2013-01-10 07:27:39]

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

[commit-bot: I haz the power, 2013-01-10 14:08:13]

Change committed as 176063