Make ResourceHandler::OnWillRead able to complete asynchronously.

Make ResourceHandler::OnWillRead able to complete asynchronously.

This will let us simplify the Mojo code a bit in the future, and it
makes it consistent with all other ResourceHandler methods, which
always use ResourceController for resuming and cancellation, rather
than returning a bool.

No ResourceHandler currently takes advantage of this behavior, though
this CL updates all intermediary ResourceHandlers to support it.

BUG=687240
Review-Url: https://codereview.chromium.org/2668603003
Cr-Commit-Position: refs/heads/master@{#455759}
Committed: https://chromium.googlesource.com/chromium/src/+/3c1d10caa48428f7a185458ae7e9f70211d7c9fd

[mmenke, 2017-01-31 22:42:48]

Description was changed from

==========
.


.


merge


backup


.

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

to

==========
This simplifies the Mojo code a bit, and it makes it consistent
with all other ResourceHandler methods, which always use
ResourceController for resuming and cancellation, rather than
returning a bool.

BUG=687240
==========

[mmenke, 2017-01-31 22:44:51]

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

[commit-bot: I haz the power, 2017-01-31 22:45:46]

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-31 23:00:53]

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

[commit-bot: I haz the power, 2017-01-31 23:00:54]

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

[mmenke, 2017-01-31 23:04:08]

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

[commit-bot: I haz the power, 2017-01-31 23:05:09]

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-31 23:18:55]

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

[commit-bot: I haz the power, 2017-01-31 23:18:56]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mmenke, 2017-02-01 16:06:42]

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

[commit-bot: I haz the power, 2017-02-01 16:06:55]

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-02-01 16:32:26]

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

[commit-bot: I haz the power, 2017-02-01 16:32:27]

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

[mmenke, 2017-02-01 20:22:01]

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

[commit-bot: I haz the power, 2017-02-01 20:22:22]

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

[mmenke, 2017-02-01 20:56:02]

Patchset #6 (id:100001) has been deleted

[mmenke, 2017-02-01 21:02:40]

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

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

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

[mmenke, 2017-02-01 21:10:02]

Patchset #8 (id:160001) has been deleted

[mmenke, 2017-02-01 21:10:51]

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

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

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

[mmenke, 2017-02-01 21:13:16]

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

[mmenke, 2017-02-01 21:13:17]

csharrison:  As I said on the other CL, if you don't get to reviewing this
before I leave, not a problem.  This CL is much bigger than I had anticipated -
almost as big as the major refactor, though not nearly as complicated (Not that
it's all simple).

This does not make MojoAsyncResourceHandler handle OnWillRead asynchronously
when mojo can't give us a buffer synchronously.  Instead, it just adds the
ability for the ResourceHandler to support that, updating all intermediary
ResourceHandlers correspondingly, with tests.

[mmenke, 2017-02-01 21:39:21]

Description was changed from

==========
This simplifies the Mojo code a bit, and it makes it consistent
with all other ResourceHandler methods, which always use
ResourceController for resuming and cancellation, rather than
returning a bool.

BUG=687240
==========

to

==========
Make ResourceHandler::OnWillRead able to complete asynchronously.

This will let ussimplify the Mojo code a bit in the future, and it
makes it consistent with all other ResourceHandler methods, which
always use ResourceController for resuming and cancellation, rather
than returning a bool.

No ResourceHandler currently takes advantage of this behavior, though
this CL updates all intermediary ResourceHandlers to support it.

BUG=687240
==========

[commit-bot: I haz the power, 2017-02-01 22:40:49]

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

[commit-bot: I haz the power, 2017-02-01 22:40:50]

Dry run: 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-02-01 23:01:42]

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

[commit-bot: I haz the power, 2017-02-01 23:02:03]

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

[Charlie Harrison, 2017-02-01 23:32:12]

It will likely take me a while to get this done with conferences, etc. this
week.

[mmenke, 2017-02-01 23:34:44]

On 2017/02/01 23:32:12, Charlie Harrison-slow til Feb3 wrote:
> It will likely take me a while to get this done with conferences, etc. this
> week.

If you can't get to it before I leave, that's totally fine.  I don't expect many
merge conflicts if this sits for a month, and it doesn't fix any bugs.  And you
should prioritize stuff you can only do while you're there over code reviews -
including just socializing with people you never see in person, otherwise.

[commit-bot: I haz the power, 2017-02-02 00:23:32]

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

[commit-bot: I haz the power, 2017-02-02 00:23:33]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-02-16 21:25:05]

Generally looks good, first round of comments.

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/async_resource_handler.cc (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/async_resource_handler.cc:377:
controller->CancelWithError(net::ERR_INSUFFICIENT_RESOURCES);
w00t!

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/detachable_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/detachable_resource_handler_unittest.cc:56: : public
testing::TestWithParam<DetachPhase> {
Very clever

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/detachable_resource_handler_unittest.cc:79: //
TODO(mmenke):  This file currently has no timeout tests.  Should it?
You have a bunch of double spaces here and in a few other places. git cl format
should (I think) fix this for you nowadays.

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/detachable_resource_handler_unittest.cc:101: if
(GetParam() == DetachPhase::ON_WILL_START) {
One way to condense these tests would be to do something like:

void MaybeDetachAt(param) {
  if (GetParam() == param) {
    detachable_handler_->Detach();
    ASSERT_FALSE(test_handler_);
  }
}

bool WasDetachedBy(param) {
  if (GetParam() <= param) {
    EXPECT_FALSE(test_handler_);
    return true;
  }
  EXPECT_TRUE(test_handler_);
  return false;
}

then each leg of the test will be condensed to:

MaybeDetachAt(DetachPhase::ON_WILL_START);
ASSERT_EQ(MockResourceLoader::Status::IDLE,
          mock_loader_->OnWillStart(request_->url()));
if (!WasDetachedBy(DetachPhase::ON_WILL_START)) {
  EXPECT_EQ(1, test_handler_->on_will_start_called())
  EXPECT_EQ(0, test_handler_->on_request_redirected_called());
}


WDYT? I will have to look at async test to see if they can be combined...

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/detachable_resource_handler_unittest.cc:234: // during
the phase indicated by the DetachPhase parameter. Async casees where
s/casees/cases

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/detachable_resource_handler_unittest.cc:270: if
(GetParam() < DetachPhase::REQUEST_REDIRECTED) {
I don't think we could use the helper methods I described by the sync case, but
these could probably be condensed as well in a similar way. Up to you.

I think I would be happier if you pulled out the calls to the loader outside the
conditions, e.g.
auto status = mock_loader_->OnRequestRedirected(net::RedirectInfo(),
make_scoped_refptr(new ResourceResponse())));
if (GetParam() < REQUEST_REDIRECTED) {
  ASSERT_EQ(IDLE, status);
}
...

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/detachable_resource_handler_unittest.cc:386: // Test
doesn't check detaching on thie third OnWillRead call.
s/thie/the

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler.cc (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.cc:386: state_ =
State::SENDING_BUFFER_TO_NEW_HANDLER_WAITING_FOR_BUFFER;
Shouldn't this go above the OnWillReadCall?

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler.h (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.h:85: // States where the
InterceptingResourceHandler passes the initial
:/ this is a lot of extra complexity to this class. 4 new states in the state
machine + 4 new member vars is not great.

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/intercepting_resource_handler_unittest.cc:400: //
OnWillRead completes asynchornously when passing the payload to the old
s/asynchornously/asynchronously

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/mime_sniffing_resource_handler.cc (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.cc:391:
*parent_read_buffer_ = read_buffer_;
I think you should std::move() here to condense this operation and the = nullptr
operation, without causing ref churn (here and in other files).

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/resource_handler.h (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/resource_handler.h:113: virtual void
OnWillRead(scoped_refptr<net::IOBuffer>* buf,
Please update the comment.

[mmenke, 2017-03-06 16:18:32]

Description was changed from

==========
Make ResourceHandler::OnWillRead able to complete asynchronously.

This will let ussimplify the Mojo code a bit in the future, and it
makes it consistent with all other ResourceHandler methods, which
always use ResourceController for resuming and cancellation, rather
than returning a bool.

No ResourceHandler currently takes advantage of this behavior, though
this CL updates all intermediary ResourceHandlers to support it.

BUG=687240
==========

to

==========
Make ResourceHandler::OnWillRead able to complete asynchronously.

This will let us simplify the Mojo code a bit in the future, and it
makes it consistent with all other ResourceHandler methods, which
always use ResourceController for resuming and cancellation, rather
than returning a bool.

No ResourceHandler currently takes advantage of this behavior, though
this CL updates all intermediary ResourceHandlers to support it.

BUG=687240
==========

[mmenke, 2017-03-08 16:19:46]

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

[commit-bot: I haz the power, 2017-03-08 16:20:09]

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

[mmenke, 2017-03-08 16:38:42]

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

[commit-bot: I haz the power, 2017-03-08 16:39:05]

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-03-08 17:08:57]

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

[commit-bot: I haz the power, 2017-03-08 17:08:58]

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, 2017-03-08 18:13:28]

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

[commit-bot: I haz the power, 2017-03-08 18:13:59]

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

[mmenke, 2017-03-08 18:24:47]

Patchset #13 (id:280001) has been deleted

[mmenke, 2017-03-08 19:16:05]

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

[mmenke, 2017-03-08 19:16:07]

[+rdsmith]:  Please review content/browser/download

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/detachable_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/detachable_resource_handler_unittest.cc:79: //
TODO(mmenke):  This file currently has no timeout tests.  Should it?
On 2017/02/16 21:25:04, Charlie Harrison wrote:
> You have a bunch of double spaces here and in a few other places. git cl
format
> should (I think) fix this for you nowadays.

It doesn't.  And you're supposed to use double-spaces with fixed-width fonts,
dangit!  My general feeling is this doesn't matter enough to be worth being
consistent - you'll see both single and double space comments all over the
network stack.  However, the header of this file uses single spaces, so I'll
switch.

The Google style guide used to explicitly allow both, now it's silent on the
subject, though some of its examples do use double spacing.

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/detachable_resource_handler_unittest.cc:101: if
(GetParam() == DetachPhase::ON_WILL_START) {
On 2017/02/16 21:25:04, Charlie Harrison wrote:
> One way to condense these tests would be to do something like:
> 
> void MaybeDetachAt(param) {
>   if (GetParam() == param) {
>     detachable_handler_->Detach();
>     ASSERT_FALSE(test_handler_);
>   }
> }
> 
> bool WasDetachedBy(param) {
>   if (GetParam() <= param) {
>     EXPECT_FALSE(test_handler_);
>     return true;
>   }
>   EXPECT_TRUE(test_handler_);
>   return false;
> }
> 
> then each leg of the test will be condensed to:
> 
> MaybeDetachAt(DetachPhase::ON_WILL_START);
> ASSERT_EQ(MockResourceLoader::Status::IDLE,
>           mock_loader_->OnWillStart(request_->url()));
> if (!WasDetachedBy(DetachPhase::ON_WILL_START)) {
>   EXPECT_EQ(1, test_handler_->on_will_start_called())
>   EXPECT_EQ(0, test_handler_->on_request_redirected_called());
> }
> 
> 
> WDYT? I will have to look at async test to see if they can be combined...

Done.  Not sure it makes the tests any easier to read, though.

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/detachable_resource_handler_unittest.cc:234: // during
the phase indicated by the DetachPhase parameter. Async casees where
On 2017/02/16 21:25:04, Charlie Harrison wrote:
> s/casees/cases

Done.

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/detachable_resource_handler_unittest.cc:270: if
(GetParam() < DetachPhase::REQUEST_REDIRECTED) {
On 2017/02/16 21:25:04, Charlie Harrison wrote:
> I don't think we could use the helper methods I described by the sync case,
but
> these could probably be condensed as well in a similar way. Up to you.

Done.

> I think I would be happier if you pulled out the calls to the loader outside
the
> conditions, e.g.
> auto status = mock_loader_->OnRequestRedirected(net::RedirectInfo(),
> make_scoped_refptr(new ResourceResponse())));
> if (GetParam() < REQUEST_REDIRECTED) {
>   ASSERT_EQ(IDLE, status);
> }
> ...

Done.

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/detachable_resource_handler_unittest.cc:386: // Test
doesn't check detaching on thie third OnWillRead call.
On 2017/02/16 21:25:04, Charlie Harrison wrote:
> s/thie/the

Done.

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler.cc (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.cc:386: state_ =
State::SENDING_BUFFER_TO_NEW_HANDLER_WAITING_FOR_BUFFER;
On 2017/02/16 21:25:04, Charlie Harrison wrote:
> Shouldn't this go above the OnWillReadCall?

Done.  Doesn't matter for correctness, but it's more consistent with everything
else, and seems less regression-prone.

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler.h (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.h:85: // States where the
InterceptingResourceHandler passes the initial
On 2017/02/16 21:25:04, Charlie Harrison wrote:
> :/ this is a lot of extra complexity to this class. 4 new states in the state
> machine + 4 new member vars is not great.

I don't think there a whole lot we can do about the new member variables -
they're things that used to be written to synchronously that may now be written
to asynchronously.

For the states:  We have three OnWillRead calls, so need three states for each
to return asynchronously.

In one case, we need another state because what was done synchronously now needs
us to enter the state machine, run one state, wait for resume, and enter the
other.  That's easier to read than doing the actions for the first state inline,
I think.

Also worth noting we'll hopefully be getting rid of this class eventually, with
the switch to mojo.

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/intercepting_resource_handler_unittest.cc:400: //
OnWillRead completes asynchornously when passing the payload to the old
On 2017/02/16 21:25:04, Charlie Harrison wrote:
> s/asynchornously/asynchronously

Done.

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/mime_sniffing_resource_handler.cc (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.cc:391:
*parent_read_buffer_ = read_buffer_;
On 2017/02/16 21:25:04, Charlie Harrison wrote:
> I think you should std::move() here to condense this operation and the =
nullptr
> operation, without causing ref churn (here and in other files).

That wouldn't be correct - we need the buffer pointer, so we can consume any
data we receive.  The nullptr lines are not nulling out the scoped_refptrs, but
the pointers to the parent's scoped_refptrs.

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/resource_handler.h (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/resource_handler.h:113: virtual void
OnWillRead(scoped_refptr<net::IOBuffer>* buf,
On 2017/02/16 21:25:04, Charlie Harrison wrote:
> Please update the comment.

Done.

[commit-bot: I haz the power, 2017-03-08 19:50:03]

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

[commit-bot: I haz the power, 2017-03-08 19:50:04]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-03-08 21:12:46]

Just a few minor comments. LGTM

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/detachable_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/detachable_resource_handler_unittest.cc:79: //
TODO(mmenke):  This file currently has no timeout tests.  Should it?
On 2017/03/08 19:16:07, mmenke (getting caught up) wrote:
> On 2017/02/16 21:25:04, Charlie Harrison wrote:
> > You have a bunch of double spaces here and in a few other places. git cl
> format
> > should (I think) fix this for you nowadays.
> 
> It doesn't.  And you're supposed to use double-spaces with fixed-width fonts,
> dangit!  My general feeling is this doesn't matter enough to be worth being
> consistent - you'll see both single and double space comments all over the
> network stack.  However, the header of this file uses single spaces, so I'll
> switch.
> 
> The Google style guide used to explicitly allow both, now it's silent on the
> subject, though some of its examples do use double spacing.

OK. It doesn't matter to me as long as it's consistent. If net stack code uses
double spaces I don't care one way or another :)

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/detachable_resource_handler_unittest.cc:101: if
(GetParam() == DetachPhase::ON_WILL_START) {
On 2017/03/08 19:16:06, mmenke (getting caught up) wrote:
> On 2017/02/16 21:25:04, Charlie Harrison wrote:
> > One way to condense these tests would be to do something like:
> > 
> > void MaybeDetachAt(param) {
> >   if (GetParam() == param) {
> >     detachable_handler_->Detach();
> >     ASSERT_FALSE(test_handler_);
> >   }
> > }
> > 
> > bool WasDetachedBy(param) {
> >   if (GetParam() <= param) {
> >     EXPECT_FALSE(test_handler_);
> >     return true;
> >   }
> >   EXPECT_TRUE(test_handler_);
> >   return false;
> > }
> > 
> > then each leg of the test will be condensed to:
> > 
> > MaybeDetachAt(DetachPhase::ON_WILL_START);
> > ASSERT_EQ(MockResourceLoader::Status::IDLE,
> >           mock_loader_->OnWillStart(request_->url()));
> > if (!WasDetachedBy(DetachPhase::ON_WILL_START)) {
> >   EXPECT_EQ(1, test_handler_->on_will_start_called())
> >   EXPECT_EQ(0, test_handler_->on_request_redirected_called());
> > }
> > 
> > 
> > WDYT? I will have to look at async test to see if they can be combined...
> 
> Done.  Not sure it makes the tests any easier to read, though.

Thanks. For me these tests are now much easier to read.

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler.h (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.h:85: // States where the
InterceptingResourceHandler passes the initial
On 2017/03/08 19:16:07, mmenke (getting caught up) wrote:
> On 2017/02/16 21:25:04, Charlie Harrison wrote:
> > :/ this is a lot of extra complexity to this class. 4 new states in the
state
> > machine + 4 new member vars is not great.
> 
> I don't think there a whole lot we can do about the new member variables -
> they're things that used to be written to synchronously that may now be
written
> to asynchronously.
> 
> For the states:  We have three OnWillRead calls, so need three states for each
> to return asynchronously.
> 
> In one case, we need another state because what was done synchronously now
needs
> us to enter the state machine, run one state, wait for resume, and enter the
> other.  That's easier to read than doing the actions for the first state
inline,
> I think.
> 
> Also worth noting we'll hopefully be getting rid of this class eventually,
with
> the switch to mojo.

Yeah it does make sense. I think in reality this is just highlighting the
complexity already here.

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
File content/browser/loader/mime_sniffing_resource_handler.cc (right):

https://codereview.chromium.org/2668603003/diff/200001/content/browser/loader...
content/browser/loader/mime_sniffing_resource_handler.cc:391:
*parent_read_buffer_ = read_buffer_;
On 2017/03/08 19:16:07, mmenke (getting caught up) wrote:
> On 2017/02/16 21:25:04, Charlie Harrison wrote:
> > I think you should std::move() here to condense this operation and the =
> nullptr
> > operation, without causing ref churn (here and in other files).
> 
> That wouldn't be correct - we need the buffer pointer, so we can consume any
> data we receive.  The nullptr lines are not nulling out the scoped_refptrs,
but
> the pointers to the parent's scoped_refptrs.

You're completely right, apologies.

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
File content/browser/loader/detachable_resource_handler.cc (right):

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
content/browser/loader/detachable_resource_handler.cc:82:
parent_read_buffer_size_(0),
nit: Shouldn't this be nullptr as well? Same elsewhere in the file.

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
content/browser/loader/detachable_resource_handler.cc:148:
parent_read_buffer_size_ = 0;
= nullptr

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler.cc (right):

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.cc:267:
DCHECK_EQ(State::SENDING_ON_WILL_READ_TO_OLD_HANDLER, state_);
DCHECK(has_controller());

I wonder if we should add this to the Controller classes in the constructor,
because we should probably never be making a Controller unless we have the
controller here.

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
File content/browser/loader/mock_resource_loader.h (right):

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
content/browser/loader/mock_resource_loader.h:107: bool waiting_on_buffer_ =
false;
Optional: Maybe add a note that this bool doesn't affect logic and is only for
EXPECTing.

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
content/browser/loader/resource_loader.cc:520: weak_ptr_factory_.GetWeakPtr(),
false));
nit: false  /* handle_result_asynchronously */

[Randy Smith (Not in Mondays), 2017-03-08 21:43:16]

content/browser/download LGTM.

[mmenke, 2017-03-08 21:49:35]

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

[commit-bot: I haz the power, 2017-03-08 21:50:04]

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-03-08 22:47:03]

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

[commit-bot: I haz the power, 2017-03-08 22:47:04]

Dry run: Try jobs failed on following builders:
  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, 2017-03-08 22:51:30]

Thanks for the reviews!

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
File content/browser/loader/detachable_resource_handler.cc (right):

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
content/browser/loader/detachable_resource_handler.cc:82:
parent_read_buffer_size_(0),
On 2017/03/08 21:12:46, Charlie Harrison wrote:
> nit: Shouldn't this be nullptr as well? Same elsewhere in the file.

Done.  You're right, I just see "size" in a name and I think "int", and
brainlessly change from nullptr to 0.

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
content/browser/loader/detachable_resource_handler.cc:148:
parent_read_buffer_size_ = 0;
On 2017/03/08 21:12:46, Charlie Harrison wrote:
> = nullptr

Done.

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
File content/browser/loader/intercepting_resource_handler.cc (right):

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
content/browser/loader/intercepting_resource_handler.cc:267:
DCHECK_EQ(State::SENDING_ON_WILL_READ_TO_OLD_HANDLER, state_);
On 2017/03/08 21:12:46, Charlie Harrison wrote:
> DCHECK(has_controller());

Done.

> I wonder if we should add this to the Controller classes in the constructor,
> because we should probably never be making a Controller unless we have the
> controller here.

Good idea, done (Could ignore your first comment in favor of this, but I think
it's nice to have it as documentation)

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
File content/browser/loader/mock_resource_loader.h (right):

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
content/browser/loader/mock_resource_loader.h:107: bool waiting_on_buffer_ =
false;
On 2017/03/08 21:12:46, Charlie Harrison wrote:
> Optional: Maybe add a note that this bool doesn't affect logic and is only for
> EXPECTing.

Done.

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2668603003/diff/300001/content/browser/loader...
content/browser/loader/resource_loader.cc:520: weak_ptr_factory_.GetWeakPtr(),
false));
On 2017/03/08 21:12:46, Charlie Harrison wrote:
> nit: false  /* handle_result_asynchronously */

Done.

[mmenke, 2017-03-09 15:54:07]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-03-09 15:54:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdsmith@chromium.org, csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2668603003/#ps320001
(title: "Response to comments")

[commit-bot: I haz the power, 2017-03-09 15:54:23]

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-03-09 16:02:45]

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

[commit-bot: I haz the power, 2017-03-09 16:02:47]

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-03-09 16:08:42]

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

[mmenke, 2017-03-09 16:08:44]

[+asanka]:  Please review content/browser/download (Oops, thought rdsmith was an
owner!)

[asanka, 2017-03-09 16:13:24]

lgtm

[mmenke, 2017-03-09 16:18:47]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-03-09 16:19:14]

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-03-09 16:25:49]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1489076327901570,
"parent_rev": "2da862c5f16c22f69358d30c4bd500b87dad91a7", "commit_rev":
"3c1d10caa48428f7a185458ae7e9f70211d7c9fd"}

[commit-bot: I haz the power, 2017-03-09 16:26:30]

Description was changed from

==========
Make ResourceHandler::OnWillRead able to complete asynchronously.

This will let us simplify the Mojo code a bit in the future, and it
makes it consistent with all other ResourceHandler methods, which
always use ResourceController for resuming and cancellation, rather
than returning a bool.

No ResourceHandler currently takes advantage of this behavior, though
this CL updates all intermediary ResourceHandlers to support it.

BUG=687240
==========

to

==========
Make ResourceHandler::OnWillRead able to complete asynchronously.

This will let us simplify the Mojo code a bit in the future, and it
makes it consistent with all other ResourceHandler methods, which
always use ResourceController for resuming and cancellation, rather
than returning a bool.

No ResourceHandler currently takes advantage of this behavior, though
this CL updates all intermediary ResourceHandlers to support it.

BUG=687240

Review-Url: https://codereview.chromium.org/2668603003
Cr-Commit-Position: refs/heads/master@{#455759}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c1d10caa48428f7a185458ae7e9...
==========

[commit-bot: I haz the power, 2017-03-09 16:26:31]

Committed patchset #14 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/3c1d10caa48428f7a185458ae7e9...