Remove WorkerPool dependency from URLRequestFileJob.

Remove WorkerPool dependency from URLRequestFileJob.

Make net::URLRequestFileJob pass a TaskRunner to FileStream, instead of using a deprecated FileStream constructor that depends on WorkerPool. This TaskRunner is obtained from the blocking pool where possible. Also, remove two explicit uses of WorkerPool in net/url_request/ and chrome/browser/.

Removes dependency on deprecated code. Also fixes memory leaks in unit tests caused by FileStream destruction tasks not running to completion in WorkerPool.

BUG=248513, 251774
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218273

[earthdok, 2013-08-15 14:58:50]

Please take a look.

akalin:
net/
sync/

mmenke:
content/test/net/

jochen:
chrome/
content/shell/

benm:
android_webview/

scottbyer:
cloud_print/


No behavior change (only refactoring) in the following:

sync/
cloud_print/
chrome/test/

[mmenke, 2013-08-15 15:03:46]

content/test/net/ LGTM.

On 2013/08/15 14:58:50, earthdok wrote:
> Please take a look.
> 
> akalin:
> net/
> sync/
> 
> mmenke:
> content/test/net/
> 
> jochen:
> chrome/
> content/shell/
> 
> benm:
> android_webview/
> 
> scottbyer:
> cloud_print/
> 
> 
> No behavior change (only refactoring) in the following:
> 
> sync/
> cloud_print/
> chrome/test/

[benm (inactive), 2013-08-15 16:27:54]

aw lgtm

[akalin, 2013-08-15 17:43:05]

a few comments

https://codereview.chromium.org/22795006/diff/47001/net/url_request/file_prot...
File net/url_request/file_protocol_handler.h (right):

https://codereview.chromium.org/22795006/diff/47001/net/url_request/file_prot...
net/url_request/file_protocol_handler.h:26: FileProtocolHandler(const
scoped_refptr<base::TaskRunner>& task_runner);
explicit (also, all other similar places)

https://codereview.chromium.org/22795006/diff/47001/net/url_request/file_prot...
net/url_request/file_protocol_handler.h:26: FileProtocolHandler(const
scoped_refptr<base::TaskRunner>& task_runner);
can you name this something other than just task_runner? If this is for File IO,
maybe something like file_task_runner or file_io_task_runner

https://codereview.chromium.org/22795006/diff/47001/net/url_request/file_prot...
net/url_request/file_protocol_handler.h:33: scoped_refptr<base::TaskRunner>
task_runner_;
const (also, all other similar places)

https://codereview.chromium.org/22795006/diff/47001/net/url_request/url_reque...
File net/url_request/url_request_context_builder.cc (right):

https://codereview.chromium.org/22795006/diff/47001/net/url_request/url_reque...
net/url_request/url_request_context_builder.cc:188: const
scoped_refptr<base::TaskRunner>& task_runner)
don't think you need this parameter -- see context->file_message_loop() used
below

https://codereview.chromium.org/22795006/diff/47001/net/url_request/url_reque...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/22795006/diff/47001/net/url_request/url_reque...
net/url_request/url_request_context_builder.h:70: URLRequestContextBuilder(const
scoped_refptr<base::TaskRunner> &task_runner);
' &' -> '& '

[earthdok, 2013-08-15 17:57:01]

FTR, my only doubt about this CL is that we're no longer differentiating between
slow and fast tasks (as we did in WorkerPool). I'm concerned that posting a lot
of slow tasks to the blocking pool might impair the performance. Cf. jam's
comment here https://code.google.com/p/chromium/issues/detail?id=251774

> We'll probably want to have multiple instances of SWP (say for slow crypto
calls), or maybe multiple modes of using SWP so that one can specify if it's a
slow call.

There were a couple places where WorkerPool was used for ServerBoundCertService.
At first I wanted to switch those to the blocking pool as well, but because of
this comment I decided not to. I'm still wondering if the way I'm using it for
file I/O is ok though.

[akalin, 2013-08-15 21:23:19]

On 2013/08/15 17:57:01, earthdok wrote:
> FTR, my only doubt about this CL is that we're no longer differentiating
between
> slow and fast tasks (as we did in WorkerPool). I'm concerned that posting a
lot
> of slow tasks to the blocking pool might impair the performance. Cf. jam's
> comment here https://code.google.com/p/chromium/issues/detail?id=251774
> 
> > We'll probably want to have multiple instances of SWP (say for slow crypto
> calls), or maybe multiple modes of using SWP so that one can specify if it's a
> slow call.
> 
> There were a couple places where WorkerPool was used for
ServerBoundCertService.
> At first I wanted to switch those to the blocking pool as well, but because of
> this comment I decided not to. I'm still wondering if the way I'm using it for
> file I/O is ok though.

I'm okay with having a separate SWP, but adding that in might be better done in
another CL. If you want to keep them distinct in this CL, perhaps add a method
to URLRequestContext that returns the separate one (like
slow_tasks_message_loop() or something).

[jochen (gone - plz use gerrit), 2013-08-16 09:05:41]

lgtm

[earthdok, 2013-08-16 13:55:57]

Fred: all comments addressed, ptal

[akalin, 2013-08-16 21:22:20]

lgtm after nits

https://codereview.chromium.org/22795006/diff/65001/net/url_request/file_prot...
File net/url_request/file_protocol_handler.cc (right):

https://codereview.chromium.org/22795006/diff/65001/net/url_request/file_prot...
net/url_request/file_protocol_handler.cc:8: #include "base/memory/ref_counted.h"
no need for this include

https://codereview.chromium.org/22795006/diff/65001/net/url_request/file_prot...
File net/url_request/file_protocol_handler.h (right):

https://codereview.chromium.org/22795006/diff/65001/net/url_request/file_prot...
net/url_request/file_protocol_handler.h:11: #include "base/task_runner.h"
forward-declare task runner

https://codereview.chromium.org/22795006/diff/65001/net/url_request/url_reque...
File net/url_request/url_request_file_job.cc (right):

https://codereview.chromium.org/22795006/diff/65001/net/url_request/url_reque...
net/url_request/url_request_file_job.cc:57: URLRequest* request,
NetworkDelegate* network_delegate,
one param per line

https://codereview.chromium.org/22795006/diff/65001/net/url_request/url_reque...
File net/url_request/url_request_file_job.h (right):

https://codereview.chromium.org/22795006/diff/65001/net/url_request/url_reque...
net/url_request/url_request_file_job.h:35: const
scoped_refptr<base::TaskRunner>& file_task_runner);
#include ref_counted.h

forward-declare TaskRunner

https://codereview.chromium.org/22795006/diff/65001/net/url_request/url_reque...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/22795006/diff/65001/net/url_request/url_reque...
net/url_request/url_request_unittest.cc:596: "file", new
FileProtocolHandler(base::MessageLoopProxy::current()));
#include for message_loop_proxy

[commit-bot: I haz the power, 2013-08-19 08:18:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/earthdok@chromium.org/22795006/74001

[commit-bot: I haz the power, 2013-08-19 08:42:03]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-19 08:54:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/earthdok@chromium.org/22795006/98001

[commit-bot: I haz the power, 2013-08-19 13:14:36]

Change committed as 218273