Move URLLoaderClientImpl to a separate file

Move URLLoaderClientImpl to a separate file

This CL moves URLLoaderClientImpl defined in resource_dispatcher.cc to a separate file
as content::URLLoaderClientImpl and adds unittests.

BUG=603396
Committed: https://crrev.com/f6c76cf17426aa54344a27ee8ece4bfae901719c
Cr-Commit-Position: refs/heads/master@{#440386}

[yhirano, 2016-12-22 02:09:10]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 02:09:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 02:44:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 02:44:32]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[yhirano, 2016-12-22 03:28:56]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 03:29:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 03:55:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 03:55:21]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[yhirano, 2016-12-22 03:58:18]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 03:58:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 05:11:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 05:11:48]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-12-22 05:20:21]

yhirano@chromium.org changed reviewers:
+ jam@chromium.org

[yhirano, 2016-12-22 05:20:22]

PTAL

[kinuko, 2016-12-22 06:34:18]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2016-12-22 06:34:18]

Wouldn't it be better to split URLLoaderClientImpl first, and then add
SetDefersLoading support in a different CL?  I can quickly stamp if you can
upload it today.

(Tangentially it might be also more convenient to start considering having a
separate sub-directory e.g. content/child/url_loader)

https://codereview.chromium.org/2597873002/diff/40001/content/child/url_loade...
File content/child/url_loader_client_impl.cc (right):

https://codereview.chromium.org/2597873002/diff/40001/content/child/url_loade...
content/child/url_loader_client_impl.cc:151: void
URLLoaderClientImpl::SetDefersLoading() {
Any reason we don't make this take a boolean flag as
ResourceDispatcher::SetDefersLoading does?

[yhirano, 2016-12-22 06:54:25]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 06:54:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-12-22 07:03:16]

Description was changed from

==========
Implement SetDefersLoading on content::URLLoaderClientImpl

This CL adds the deferred loading support to URLLoaderClientImpl. This CL
also moves URLLoaderClientImpl defined in resource_dispatcher.cc to a
separate file as content::URLLoaderClientImpl and adds unittests.

BUG=603396
==========

to

==========
Move URLLoaderClientImpl to a separate file

This CL moves URLLoaderClientImpl defined in resource_dispatcher.cc to a
separate file
as content::URLLoaderClientImpl and adds unittests.

BUG=603396
==========

[yhirano, 2016-12-22 07:05:15]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 07:05:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-12-22 07:08:59]

> Wouldn't it be better to split URLLoaderClientImpl first, and then add
> SetDefersLoading support in a different CL?  I can quickly stamp if you can
> upload it today.

Done (w/https://codereview.chromium.org/2591383003/).

[kinuko, 2016-12-22 07:54:49]

looking good, some nits

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
File content/child/url_loader_client_impl.cc (right):

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
content/child/url_loader_client_impl.cc:30: is_destroyed_->set(true);
Looks like this shared var is not yet necessary

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
File content/child/url_loader_client_impl.h (right):

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
content/child/url_loader_client_impl.h:53: mojo::AssociatedGroup*
associated_group);
nit: maybe have this above the overriding methods

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
content/child/url_loader_client_impl.h:56: class SharedBoolean : public
base::RefCounted<SharedBoolean> {
Hmm... could we instead use base::RefCountedData?  (It's thread-safe-refcounted,
but not a big fan of bloating code with its own utilities)

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
File content/child/url_loader_client_impl_unittest.cc (right):

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
content/child/url_loader_client_impl_unittest.cc:49: ADD_FAILURE() <<
"IPC::Sneder::Send should not be called.";
Sneder -> Sender

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
content/child/url_loader_client_impl_unittest.cc:69: static
MojoCreateDataPipeOptions dataPipeOptions() {
DataPipeOptions()

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
content/child/url_loader_client_impl_unittest.cc:294:
TEST_F(URLLoaderClientImplTest, Cancel2) {
Could Cancel1, Cancel2, Cancel3 be given slightly more descriptive names?

[yhirano, 2016-12-22 08:02:35]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 08:02:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-12-22 08:03:32]

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
File content/child/url_loader_client_impl.cc (right):

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
content/child/url_loader_client_impl.cc:30: is_destroyed_->set(true);
On 2016/12/22 07:54:48, kinuko wrote:
> Looks like this shared var is not yet necessary

Done.

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
File content/child/url_loader_client_impl.h (right):

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
content/child/url_loader_client_impl.h:53: mojo::AssociatedGroup*
associated_group);
On 2016/12/22 07:54:48, kinuko wrote:
> nit: maybe have this above the overriding methods

Done.

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
content/child/url_loader_client_impl.h:56: class SharedBoolean : public
base::RefCounted<SharedBoolean> {
On 2016/12/22 07:54:48, kinuko wrote:
> Hmm... could we instead use base::RefCountedData?  (It's
thread-safe-refcounted,
> but not a big fan of bloating code with its own utilities)

OK, I will address it in the next CL.

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
File content/child/url_loader_client_impl_unittest.cc (right):

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
content/child/url_loader_client_impl_unittest.cc:49: ADD_FAILURE() <<
"IPC::Sneder::Send should not be called.";
On 2016/12/22 07:54:48, kinuko wrote:
> Sneder -> Sender

Done.

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
content/child/url_loader_client_impl_unittest.cc:69: static
MojoCreateDataPipeOptions dataPipeOptions() {
On 2016/12/22 07:54:48, kinuko wrote:
> DataPipeOptions()

Done.

https://codereview.chromium.org/2597873002/diff/80001/content/child/url_loade...
content/child/url_loader_client_impl_unittest.cc:294:
TEST_F(URLLoaderClientImplTest, Cancel2) {
On 2016/12/22 07:54:48, kinuko wrote:
> Could Cancel1, Cancel2, Cancel3 be given slightly more descriptive names?

Done.

[kinuko, 2016-12-22 08:12:37]

lgtm

https://codereview.chromium.org/2597873002/diff/100001/content/child/url_load...
File content/child/url_loader_client_impl.cc (right):

https://codereview.chromium.org/2597873002/diff/100001/content/child/url_load...
content/child/url_loader_client_impl.cc:82: }
nit: match the method order as in .h

[yhirano, 2016-12-22 08:15:34]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 08:15:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yhirano, 2016-12-22 08:18:14]

https://codereview.chromium.org/2597873002/diff/100001/content/child/url_load...
File content/child/url_loader_client_impl.cc (right):

https://codereview.chromium.org/2597873002/diff/100001/content/child/url_load...
content/child/url_loader_client_impl.cc:82: }
On 2016/12/22 08:12:37, kinuko wrote:
> nit: match the method order as in .h

Done.

[yhirano, 2016-12-22 08:18:15]

https://codereview.chromium.org/2597873002/diff/100001/content/child/url_load...
File content/child/url_loader_client_impl.cc (right):

https://codereview.chromium.org/2597873002/diff/100001/content/child/url_load...
content/child/url_loader_client_impl.cc:82: }
On 2016/12/22 08:12:37, kinuko wrote:
> nit: match the method order as in .h

Done.

[commit-bot: I haz the power, 2016-12-22 09:11:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 09:11:48]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-12-22 11:38:40]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-12-22 11:38:40]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2597873002/#ps120001
(title: "fix")

[commit-bot: I haz the power, 2016-12-22 11:38:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 11:42:21]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482406720299180,
"parent_rev": "3ee97a35536ccb2fa1c277d04de21aef54c9baf0", "commit_rev":
"0e9901eb51bc1e44d5e906f1d0189b4c2b93fbcd"}

[commit-bot: I haz the power, 2016-12-22 11:42:48]

Description was changed from

==========
Move URLLoaderClientImpl to a separate file

This CL moves URLLoaderClientImpl defined in resource_dispatcher.cc to a
separate file
as content::URLLoaderClientImpl and adds unittests.

BUG=603396
==========

to

==========
Move URLLoaderClientImpl to a separate file

This CL moves URLLoaderClientImpl defined in resource_dispatcher.cc to a
separate file
as content::URLLoaderClientImpl and adds unittests.

BUG=603396

Review-Url: https://codereview.chromium.org/2597873002
==========

[commit-bot: I haz the power, 2016-12-22 11:42:49]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-22 11:46:34]

Description was changed from

==========
Move URLLoaderClientImpl to a separate file

This CL moves URLLoaderClientImpl defined in resource_dispatcher.cc to a
separate file
as content::URLLoaderClientImpl and adds unittests.

BUG=603396

Review-Url: https://codereview.chromium.org/2597873002
==========

to

==========
Move URLLoaderClientImpl to a separate file

This CL moves URLLoaderClientImpl defined in resource_dispatcher.cc to a
separate file
as content::URLLoaderClientImpl and adds unittests.

BUG=603396

Committed: https://crrev.com/f6c76cf17426aa54344a27ee8ece4bfae901719c
Cr-Commit-Position: refs/heads/master@{#440386}
==========

[commit-bot: I haz the power, 2016-12-22 11:46:35]

Patchset 7 (id:??) landed as
https://crrev.com/f6c76cf17426aa54344a27ee8ece4bfae901719c
Cr-Commit-Position: refs/heads/master@{#440386}