Update MojoAsyncResourceHandler tests to use MockResourceLoader.

Update MojoAsyncResourceHandler tests to use MockResourceLoader.

TBR=clamy@chromium.org
BUG=659317
Review-Url: https://codereview.chromium.org/2581393002
Cr-Commit-Position: refs/heads/master@{#443263}
Committed: https://chromium.googlesource.com/chromium/src/+/5159f105dbf404212d7d8e1b24539be049f67c5d

[mmenke, 2016-12-16 22:58:54]

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

[commit-bot: I haz the power, 2016-12-16 22:59:39]

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-16 23:04:07]

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

[mmenke, 2016-12-16 23:04:08]

Review this CL, or wait until I get back.  I haven't done a cleanup pass on it
yet.

This basically would replace ResourceHandlerWrapper, in the other CL.

[commit-bot: I haz the power, 2016-12-16 23:18:22]

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

[commit-bot: I haz the power, 2016-12-16 23:18:23]

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...)

[Randy Smith (Not in Mondays), 2016-12-21 16:47:38]

I'll want to do another pass after your cleanup pass, but this looks basically
good to me.

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mock...
File content/browser/loader/mock_resource_loader.cc (right):

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mock...
content/browser/loader/mock_resource_loader.cc:165: const net::URLRequestStatus&
status) {
nit, suggestion: This routine (and possibly other routines?) is a bit confusing
because of the use of similar variable names for different types
(MockResourceLoader::Status and URLRequestStatus).   Maybe an extra annotation
on one of the variable names?

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mock...
content/browser/loader/mock_resource_loader.cc:192: void
MockResourceLoader::ReleaseIOBuffer() {
Is there a reason this can't be done automatically when OnReadCompleted()
succeeds?

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mock...
File content/browser/loader/mock_resource_loader.h (right):

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mock...
content/browser/loader/mock_resource_loader.h:65: // ResourceHandler |this| was
created with (like a putative higher layer
Higher layer?  Maybe this is just a vocabulary issue, but I think of the RH that
the RL points to as being the highest layer RH.

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mojo...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mojo...
content/browser/loader/mojo_async_resource_handler_unittest.cc:642: const
std::string kData(mock_loader_->io_buffer_size(), 'a');
If kData is used below, shouldn't it be used here?

[mmenke, 2017-01-04 22:58:19]

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

[commit-bot: I haz the power, 2017-01-04 22:58:50]

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, 2017-01-04 23:28:09]

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

[commit-bot: I haz the power, 2017-01-04 23:28:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mmenke, 2017-01-05 17:45:50]

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

[commit-bot: I haz the power, 2017-01-05 17:46:03]

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

[mmenke, 2017-01-05 18:46:04]

Description was changed from

==========
Update MojoAsyncResourceHandler tests to use MockResourceLoader.

BUG=
==========

to

==========
Update MojoAsyncResourceHandler tests to use MockResourceLoader.

BUG=659317
==========

[commit-bot: I haz the power, 2017-01-05 18:47:28]

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

[commit-bot: I haz the power, 2017-01-05 18:47:29]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-05 18:50:26]

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mock...
File content/browser/loader/mock_resource_loader.cc (right):

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mock...
content/browser/loader/mock_resource_loader.cc:165: const net::URLRequestStatus&
status) {
On 2016/12/21 16:47:37, Randy Smith - Not in Mondays wrote:
> nit, suggestion: This routine (and possibly other routines?) is a bit
confusing
> because of the use of similar variable names for different types
> (MockResourceLoader::Status and URLRequestStatus).   Maybe an extra annotation
> on one of the variable names?

Done.

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mock...
content/browser/loader/mock_resource_loader.cc:192: void
MockResourceLoader::ReleaseIOBuffer() {
On 2016/12/21 16:47:37, Randy Smith - Not in Mondays wrote:
> Is there a reason this can't be done automatically when OnReadCompleted()
> succeeds?  

MojoAsyncResourceHandlerTest.IOBufferFromOnWillReadShouldRemainValidEvenIfHandlerIsGone
wants direct access to it, to make sure that the custom IOBuffer class remains
usable after the MojoAsyncResourceHandler is destroyed, as the API contract
requires.

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mock...
File content/browser/loader/mock_resource_loader.h (right):

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mock...
content/browser/loader/mock_resource_loader.h:65: // ResourceHandler |this| was
created with (like a putative higher layer
On 2016/12/21 16:47:37, Randy Smith - Not in Mondays wrote:
> Higher layer?  Maybe this is just a vocabulary issue, but I think of the RH
that
> the RL points to as being the highest layer RH.

Actually, I was just thinking in terms of the MojoAsyncResourceHandler, where it
would in fact be a ResourceHandler above it (Since none are below it), but the
MockResourceLoader does rather treat the ResourceHandler it's created with as
the top layer ResourceHandler.  Regardless, I've reworded.

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mojo...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mojo...
content/browser/loader/mojo_async_resource_handler_unittest.cc:642: const
std::string kData(mock_loader_->io_buffer_size(), 'a');
On 2016/12/21 16:47:37, Randy Smith - Not in Mondays wrote:
> If kData is used below, shouldn't it be used here?

This line is the one that declares and initializes kData, so I don't think so.

That having been said, the isn't really a compile-time constant, nor does it
resemble one (Like const GURL kFoo("http://baz/")), so kData is a misnomer,
which may be what was confusing you.  I've renamed it to just data.

[mmenke, 2017-01-10 17:27:22]

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

[commit-bot: I haz the power, 2017-01-10 17:27:36]

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, 2017-01-10 18:41:34]

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

[commit-bot: I haz the power, 2017-01-10 18:41:35]

Dry run: This issue passed the CQ dry run.

[Randy Smith (Not in Mondays), 2017-01-10 23:07:47]

LGTM; you may use your judgement around the comments below.

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mock...
File content/browser/loader/mock_resource_loader.cc (right):

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mock...
content/browser/loader/mock_resource_loader.cc:192: void
MockResourceLoader::ReleaseIOBuffer() {
On 2017/01/05 18:50:25, mmenke wrote:
> On 2016/12/21 16:47:37, Randy Smith - Not in Mondays wrote:
> > Is there a reason this can't be done automatically when OnReadCompleted()
> > succeeds?  
> 
>
MojoAsyncResourceHandlerTest.IOBufferFromOnWillReadShouldRemainValidEvenIfHandlerIsGone
> wants direct access to it, to make sure that the custom IOBuffer class remains
> usable after the MojoAsyncResourceHandler is destroyed, as the API contract
> requires.

If I understand this use case correctly, it could be addressed by just taking
another reference to the IOBuffer in that specific test, rather than having
another interface on MockResourceLoader.  But I'm happy to defer to your
preference.

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mojo...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2581393002/diff/1/content/browser/loader/mojo...
content/browser/loader/mojo_async_resource_handler_unittest.cc:642: const
std::string kData(mock_loader_->io_buffer_size(), 'a');
On 2017/01/05 18:50:25, mmenke wrote:
> On 2016/12/21 16:47:37, Randy Smith - Not in Mondays wrote:
> > If kData is used below, shouldn't it be used here?
> 
> This line is the one that declares and initializes kData, so I don't think so.
> 
> That having been said, the isn't really a compile-time constant, nor does it
> resemble one (Like const GURL kFoo("http://baz/")), so kData is a misnomer,
> which may be what was confusing you.  I've renamed it to just data.

Sorry, that looks like a braino on my part.  Regardless, thanks.

https://codereview.chromium.org/2581393002/diff/80001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2581393002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:433: //
ERR_INSUFFICIENT_RESOURCES here (Which it isn't currently doing).
I'm confused by this comment; it *looks* like
MojoAsyncResourceHandler::CheckForSufficientResources() is calling
ResourceController::CancelWithError(ERR_INSUFFICIENT_RESOURCES).  Am I not
tracing the dependent patchsets properly?

https://codereview.chromium.org/2581393002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:619: // This is
needed because |*io_buffer| may keep the data producer alive.
Note that this comment (here and elsewhere) is no longer technically correct,
and should be referring to something like "the IOBuffer held by the
mock_resource_loader_".

https://codereview.chromium.org/2581393002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:620:
mock_loader_->ReleaseIOBuffer();
Sorry, there's something about this interface that's still confusing me. 
mock_loader_->OnReadCompleted() sets the MockResourceLoader::io_buffer_ to
nullptr, as does ReleaseIOBuffer().  The only thing that allocates it is
OnWillRead().  Why are both calls needed?

[mmenke, 2017-01-11 20:39:20]

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

[commit-bot: I haz the power, 2017-01-11 20:40:03]

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

[mmenke, 2017-01-11 20:50:36]

Patchset #6 (id:100001) has been deleted

[mmenke, 2017-01-11 21:21:26]

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

[commit-bot: I haz the power, 2017-01-11 21:21:56]

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

[mmenke, 2017-01-11 22:12:23]

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

[mmenke, 2017-01-11 22:12:55]

Thanks for the review!

https://codereview.chromium.org/2581393002/diff/80001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2581393002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:433: //
ERR_INSUFFICIENT_RESOURCES here (Which it isn't currently doing).
On 2017/01/10 23:07:47, Randy Smith - Not in Mondays wrote:
> I'm confused by this comment; it *looks* like
> MojoAsyncResourceHandler::CheckForSufficientResources() is calling
> ResourceController::CancelWithError(ERR_INSUFFICIENT_RESOURCES).  Am I not
> tracing the dependent patchsets properly?

Oops.  This CL was copied from the final CL in the chain, and switched to use
MockResourceLoader (The final CL had added something similar, before I decided
to separate out the test changes).

So...The check will need to be removed in the final CL in the chain, but for
now, yes, we can check the error.

https://codereview.chromium.org/2581393002/diff/80001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:620:
mock_loader_->ReleaseIOBuffer();
On 2017/01/10 23:07:47, Randy Smith - Not in Mondays wrote:
> Sorry, there's something about this interface that's still confusing me. 
> mock_loader_->OnReadCompleted() sets the MockResourceLoader::io_buffer_ to
> nullptr, as does ReleaseIOBuffer().  The only thing that allocates it is
> OnWillRead().  Why are both calls needed?

Ah, you're absolutely correct.  I've removed the method and the calls.  (I also
need to clear it on cancel/complete for ResponseErrorDuringBodyTransmission
tests)

[commit-bot: I haz the power, 2017-01-11 22:14:03]

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, 2017-01-12 00:21:28]

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

[commit-bot: I haz the power, 2017-01-12 00:21:29]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-01-12 00:32:16]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-01-12 00:32:16]

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/2581393002/#ps140001
(title: "Switch to early return")

[commit-bot: I haz the power, 2017-01-12 00:34:35]

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, 2017-01-12 00:46:49]

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

[commit-bot: I haz the power, 2017-01-12 00:46:51]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mmenke, 2017-01-12 02:25:26]

Description was changed from

==========
Update MojoAsyncResourceHandler tests to use MockResourceLoader.

BUG=659317
==========

to

==========
Update MojoAsyncResourceHandler tests to use MockResourceLoader.

TBR=clamy@chromium.org
BUG=659317
==========

[mmenke, 2017-01-12 15:43:57]

Description was changed from

==========
Update MojoAsyncResourceHandler tests to use MockResourceLoader.

TBR=clamy@chromium.org
BUG=659317
==========

to

==========
Update MojoAsyncResourceHandler tests to use MockResourceLoader.

BUG=659317
==========

[mmenke, 2017-01-12 15:44:03]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-01-12 15:44:03]

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/2581393002/#ps160001
(title: "Looks like DEPS change no longer needed")

[commit-bot: I haz the power, 2017-01-12 15:44:30]

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, 2017-01-12 16:08:12]

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

[commit-bot: I haz the power, 2017-01-12 16:08:13]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2017-01-12 16:09:38]

Patchset #8 (id:160001) has been deleted

[mmenke, 2017-01-12 16:10:08]

Description was changed from

==========
Update MojoAsyncResourceHandler tests to use MockResourceLoader.

BUG=659317
==========

to

==========
Update MojoAsyncResourceHandler tests to use MockResourceLoader.

TBR=clamy@chromium.org
BUG=659317
==========

[mmenke, 2017-01-12 16:12:57]

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

[mmenke, 2017-01-12 16:12:57]

[+clamy]:  TBRing you for DEPS change (Not an actual DEPS change, just fixing
alphebetization of DEPS file)

[mmenke, 2017-01-12 16:13:08]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-01-12 16:13:36]

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

[clamy, 2017-01-12 16:16:33]

lgtm with one nit

https://codereview.chromium.org/2581393002/diff/140001/content/browser/loader...
File content/browser/loader/DEPS (right):

https://codereview.chromium.org/2581393002/diff/140001/content/browser/loader...
content/browser/loader/DEPS:109:
"+content/browser/loader/mock_resource_loader.h",
Shouldn't this go on the line above?

[mmenke, 2017-01-12 16:17:27]

The CQ bit was unchecked by mmenke@chromium.org

[mmenke, 2017-01-12 16:19:08]

https://codereview.chromium.org/2581393002/diff/140001/content/browser/loader...
File content/browser/loader/DEPS (right):

https://codereview.chromium.org/2581393002/diff/140001/content/browser/loader...
content/browser/loader/DEPS:109:
"+content/browser/loader/mock_resource_loader.h",
On 2017/01/12 16:16:33, clamy wrote:
> Shouldn't this go on the line above?

Erm...Oops.  Two lines, actually.  Thanks for catching that!  I completely
forget about the law of conservation of alphabetization errors.  :)

[mmenke, 2017-01-12 16:20:51]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-01-12 16:20:51]

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

[commit-bot: I haz the power, 2017-01-12 16:21:21]

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

[mmenke, 2017-01-12 16:21:23]

The CQ bit was unchecked by mmenke@chromium.org

[mmenke, 2017-01-12 16:22:40]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-01-12 16:22:40]

The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org,
rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/2581393002/#ps200001
(title: "Oops, had another local change I forgot about")

[commit-bot: I haz the power, 2017-01-12 16:22:58]

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, 2017-01-12 16:53:26]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1484238160087290,
"parent_rev": "063f0c9d4f0bad158ba2d556b0402dea8ff2c677", "commit_rev":
"5159f105dbf404212d7d8e1b24539be049f67c5d"}

[commit-bot: I haz the power, 2017-01-12 16:53:55]

Description was changed from

==========
Update MojoAsyncResourceHandler tests to use MockResourceLoader.

TBR=clamy@chromium.org
BUG=659317
==========

to

==========
Update MojoAsyncResourceHandler tests to use MockResourceLoader.

TBR=clamy@chromium.org
BUG=659317

Review-Url: https://codereview.chromium.org/2581393002
Cr-Commit-Position: refs/heads/master@{#443263}
Committed:
https://chromium.googlesource.com/chromium/src/+/5159f105dbf404212d7d8e1b2453...
==========

[commit-bot: I haz the power, 2017-01-12 16:53:56]

Committed patchset #9 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/5159f105dbf404212d7d8e1b2453...