Add more ResourceLoaderTests

Add more ResourceLoaderTests.

In particular, check the cases of async URLRequest::Read successes.

Also add ResourceHandler re-entrancy checks to TestResourceHandler,
which the current API is designed to avoid.

Committed: https://crrev.com/c8b5fe12fde4ec5eff2082b32711d39db8d90635
Cr-Commit-Position: refs/heads/master@{#437586}

[mmenke, 2016-12-05 18:05:00]

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

[commit-bot: I haz the power, 2016-12-05 18:05:14]

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

[mmenke, 2016-12-05 18:20:04]

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

[commit-bot: I haz the power, 2016-12-05 18:20:32]

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-05 19:56:03]

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

[commit-bot: I haz the power, 2016-12-05 19:56:04]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-12-05 20:00:18]

Description was changed from

==========
NOT READY FOR REVIEW:  More ResourceHandlerTests
==========

to

==========
Add more ResourceLoaderTests.

In particular, check the cases of async URLRequest::Read successes.

Also add ResourceHandler re-entrancy checks to TestResourceHandler,
which the current API is designed to avoid.
==========

[mmenke, 2016-12-05 20:03:26]

mmenke@chromium.org changed reviewers:
+ rdsmith@chromium.org

[mmenke, 2016-12-05 20:03:27]

I could combine this with the others, but hope that keeping it isolated will
make review easier.  I'm not planning on sending you any other CLs this year
(Other than possibly an updated version of the ResourceLoader refactor), though
there's probably a fair chance I'll find yet more areas that need more tests,
either for the ResourceLoader itself or for particular
ResourceHandlers/ResourceThrottles.

[mmenke, 2016-12-07 17:35:31]

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

[commit-bot: I haz the power, 2016-12-07 17:36:01]

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-07 17:51:19]

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

[commit-bot: I haz the power, 2016-12-07 17:51:22]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[mmenke, 2016-12-08 17:36:12]

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

[commit-bot: I haz the power, 2016-12-08 17:36:54]

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-08 19:22:18]

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

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

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Randy Smith (Not in Mondays), 2016-12-08 22:15:09]

LGTM; happy to leave all the below to your discretion.  

I'm beginning to feel like we have comprehensive tests for this stack.  Which of
course invites some failure to waltz through a hole I haven't seen :-}.

https://codereview.chromium.org/2552463002/diff/140001/content/browser/loader...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/2552463002/diff/140001/content/browser/loader...
content/browser/loader/resource_loader_unittest.cc:532: // Shouldn't be in a
recursice ResourceHandler call - this is normally where
nit: "recursice"

https://codereview.chromium.org/2552463002/diff/140001/content/browser/loader...
content/browser/loader/resource_loader_unittest.cc:1069:
raw_ptr_resource_handler_->set_on_on_read_eof_result(false);
I'm clearly confused because try jobs are passing, but "...on_on_read..."?

https://codereview.chromium.org/2552463002/diff/140001/content/browser/loader...
content/browser/loader/resource_loader_unittest.cc:1324:
static_cast<ResourceController*>(loader_.get())->Resume();
Side question, feel free to ignore: What is this testing?  What result is
expected from Resuming a cancelled request?

https://codereview.chromium.org/2552463002/diff/140001/content/browser/loader...
File content/browser/loader/test_resource_handler.cc (right):

https://codereview.chromium.org/2552463002/diff/140001/content/browser/loader...
content/browser/loader/test_resource_handler.cc:20: call_depth_[0]++;
nit, suggestion: I find this a weird syntax for something that isn't generally
an array (and certainly isn't an array from the perspective of these routines).
| (*call_depth_)++|?

https://codereview.chromium.org/2552463002/diff/140001/net/url_request/url_re...
File net/url_request/url_request_test_job.cc (right):

https://codereview.chromium.org/2552463002/diff/140001/net/url_request/url_re...
net/url_request/url_request_test_job.cc:274: weak_factory_.GetWeakPtr(),
bytes_read));
I assume you thought about whether there were possible test failures that might
occur from writing the data synchronously and signalling completion
asynchronously as opposed to doing both asynchronously and decided not?  (I do
see the code as simpler on this path--I just want to make sure the question got
asked.)

https://codereview.chromium.org/2552463002/diff/140001/net/url_request/url_re...
File net/url_request/url_request_test_job.h (right):

https://codereview.chromium.org/2552463002/diff/140001/net/url_request/url_re...
net/url_request/url_request_test_job.h:68: // complete synchronously.
nit, suggestion: Name the methods s.t. there's a gesture at their functionality?

ETA: If the answer is "Don't want to change all the call sites", I'm fine with
that.

[mmenke, 2016-12-08 22:46:37]

Thanks for the feedback!

https://codereview.chromium.org/2552463002/diff/140001/content/browser/loader...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/2552463002/diff/140001/content/browser/loader...
content/browser/loader/resource_loader_unittest.cc:532: // Shouldn't be in a
recursice ResourceHandler call - this is normally where
On 2016/12/08 22:15:09, Randy Smith - Not in Mondays wrote:
> nit: "recursice"

Done.

https://codereview.chromium.org/2552463002/diff/140001/content/browser/loader...
content/browser/loader/resource_loader_unittest.cc:1069:
raw_ptr_resource_handler_->set_on_on_read_eof_result(false);
On 2016/12/08 22:15:09, Randy Smith - Not in Mondays wrote:
> I'm clearly confused because try jobs are passing, but "...on_on_read..."?

Too much copy/paste + autocomplete.  Fixed.  :)

https://codereview.chromium.org/2552463002/diff/140001/content/browser/loader...
content/browser/loader/resource_loader_unittest.cc:1324:
static_cast<ResourceController*>(loader_.get())->Resume();
On 2016/12/08 22:15:09, Randy Smith - Not in Mondays wrote:
> Side question, feel free to ignore: What is this testing?  What result is
> expected from Resuming a cancelled request?

I looked at this before, I think there was a crash or DCHECK or something that
has long since been fixed.  Note that a resume can legitimately occur after an
out-of-band cancel, which is one of the things we're trying to get rid of.

https://codereview.chromium.org/2552463002/diff/140001/content/browser/loader...
File content/browser/loader/test_resource_handler.cc (right):

https://codereview.chromium.org/2552463002/diff/140001/content/browser/loader...
content/browser/loader/test_resource_handler.cc:20: call_depth_[0]++;
On 2016/12/08 22:15:09, Randy Smith - Not in Mondays wrote:
> nit, suggestion: I find this a weird syntax for something that isn't generally
> an array (and certainly isn't an array from the perspective of these
routines).
> | (*call_depth_)++|?

Works for me (I mostly like the [0] because, like ->, you see the name first,
and then what you're doing with it, but your view probably makes more sense)

https://codereview.chromium.org/2552463002/diff/140001/net/url_request/url_re...
File net/url_request/url_request_test_job.cc (right):

https://codereview.chromium.org/2552463002/diff/140001/net/url_request/url_re...
net/url_request/url_request_test_job.cc:274: weak_factory_.GetWeakPtr(),
bytes_read));
On 2016/12/08 22:15:09, Randy Smith - Not in Mondays wrote:
> I assume you thought about whether there were possible test failures that
might
> occur from writing the data synchronously and signalling completion
> asynchronously as opposed to doing both asynchronously and decided not?  (I do
> see the code as simpler on this path--I just want to make sure the question
got
> asked.)

This is a valid concern - I've suggested it to others who used this pattern
(Just a week or two back, actually).  Changed it.

https://codereview.chromium.org/2552463002/diff/140001/net/url_request/url_re...
File net/url_request/url_request_test_job.h (right):

https://codereview.chromium.org/2552463002/diff/140001/net/url_request/url_re...
net/url_request/url_request_test_job.h:68: // complete synchronously.
On 2016/12/08 22:15:09, Randy Smith - Not in Mondays wrote:
> nit, suggestion: Name the methods s.t. there's a gesture at their
functionality?
> 
> ETA: If the answer is "Don't want to change all the call sites", I'm fine with
> that.

Yea, I'm going with "You have a point, but I don't want to change all callsites"
(Though, admittedly, most are in browser/loader).  I do think this class is
worth cleaning up, and maybe even moving into browser/loader to discourage more
general use, since it's weird, but think that's for another day.

[mmenke, 2016-12-08 22:46:45]

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

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

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-08 23:07:51]

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

[commit-bot: I haz the power, 2016-12-08 23:07:52]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2016-12-09 16:17:06]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-12-09 16:17:06]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/2552463002/#ps180001
(title: "Fix")

[commit-bot: I haz the power, 2016-12-09 16:17:26]

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-09 18:12:09]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1481300226584650,
"parent_rev": "99fd50b1041a1360ed9c599f89c321046a19ad8d", "commit_rev":
"e88f50a350ab714da0f0a2cb6443dcf908c7c173"}

[commit-bot: I haz the power, 2016-12-09 18:12:38]

Description was changed from

==========
Add more ResourceLoaderTests.

In particular, check the cases of async URLRequest::Read successes.

Also add ResourceHandler re-entrancy checks to TestResourceHandler,
which the current API is designed to avoid.
==========

to

==========
Add more ResourceLoaderTests.

In particular, check the cases of async URLRequest::Read successes.

Also add ResourceHandler re-entrancy checks to TestResourceHandler,
which the current API is designed to avoid.

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

[commit-bot: I haz the power, 2016-12-09 18:12:39]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-12-12 14:36:12]

Description was changed from

==========
Add more ResourceLoaderTests.

In particular, check the cases of async URLRequest::Read successes.

Also add ResourceHandler re-entrancy checks to TestResourceHandler,
which the current API is designed to avoid.

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

to

==========
Add more ResourceLoaderTests.

In particular, check the cases of async URLRequest::Read successes.

Also add ResourceHandler re-entrancy checks to TestResourceHandler,
which the current API is designed to avoid.

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

[commit-bot: I haz the power, 2016-12-12 14:36:13]

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