Remove ResourceLoader PostTask after reading headers.

Remove ResourceLoader PostTask after reading headers.

https://codereview.chromium.org/2526983002 incidentally introduced a
PostTask when ResourceHandler::OnResponseStarted completed
synchronously, and the following body read did as well. This may have
resulted in a regression in some ServiceWorker perf tests.

In general, it's unclear how aggressively we should be posting tasks
while reading the body, this CL is just to restore the earlier behavior,
and (hopefully) make the tests happy again.

BUG=673625
Review-Url: https://codereview.chromium.org/2669243006
Cr-Commit-Position: refs/heads/master@{#447863}
Committed: https://chromium.googlesource.com/chromium/src/+/9f855ec8d57996af189f479bbb5f37a378921250

[mmenke, 2017-02-02 21:33:15]

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

[mmenke, 2017-02-02 21:33:16]

Perf testing is sufficiently finicky that I have no idea if this fixes the
regression, or even if it partially fixes the regression.

Figure it's simplest just to land the patch and keep an eye on the bots.  It's
entirely possible that there's more than one PostTask to track down.  If so, the
others are most likely in ResourceHandlers further down the stack - this removes
the only ResourceLoader PostTask that triggers a lot during the regressed perf
tests.

[mmenke, 2017-02-02 21:34:06]

Oh, and this is another trivial 6 line CL, so be great if you could get to this
before I leave.  Don't both with the big one - I'm going to be continuing to
look at the other perf regression tomorrow.

[Charlie Harrison, 2017-02-02 21:37:38]

https://codereview.chromium.org/2669243006/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2669243006/diff/1/content/browser/loader/reso...
content/browser/loader/resource_loader.cc:645: deferred_stage_ = DEFERRED_SYNC;
Please comment when we can't use scoped deferral.

[mmenke, 2017-02-02 21:41:07]

Thanks!

https://codereview.chromium.org/2669243006/diff/1/content/browser/loader/reso...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/2669243006/diff/1/content/browser/loader/reso...
content/browser/loader/resource_loader.cc:645: deferred_stage_ = DEFERRED_SYNC;
On 2017/02/02 21:37:38, Charlie Harrison-slow til Feb3 wrote:
> Please comment when we can't use scoped deferral.

You're completely right.  Done.

[mmenke, 2017-02-02 21:42:13]

On 2017/02/02 21:41:07, mmenke (Out Feb 4 to March 5) wrote:
> Thanks!
> 
>
https://codereview.chromium.org/2669243006/diff/1/content/browser/loader/reso...
> File content/browser/loader/resource_loader.cc (right):
> 
>
https://codereview.chromium.org/2669243006/diff/1/content/browser/loader/reso...
> content/browser/loader/resource_loader.cc:645: deferred_stage_ =
DEFERRED_SYNC;
> On 2017/02/02 21:37:38, Charlie Harrison-slow til Feb3 wrote:
> > Please comment when we can't use scoped deferral.
> 
> You're completely right.  Done.

(Another option would be to add a DEFERRED_INITIAL_READ state or somesuch, but
that seems overkill, unless we decide we want to allow two reads to complete
synchronously in a row or something)

[mmenke, 2017-02-02 21:44:11]

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

[commit-bot: I haz the power, 2017-02-02 21:44:40]

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-02 21:45:21]

lgtm

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

https://codereview.chromium.org/2669243006/diff/20001/content/browser/loader/...
content/browser/loader/resource_loader.cc:647: // handled completely
synchronously, if not ResourceHandler defers handling of
This sentence is a bit confusing. I think you either want a period after
"synchronously" or "if ResourceHandler doesn't defer handling of the response".

[mmenke, 2017-02-02 21:46:49]

Thanks!

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

https://codereview.chromium.org/2669243006/diff/20001/content/browser/loader/...
content/browser/loader/resource_loader.cc:647: // handled completely
synchronously, if not ResourceHandler defers handling of
On 2017/02/02 21:45:21, Charlie Harrison-slow til Feb3 wrote:
> This sentence is a bit confusing. I think you either want a period after
> "synchronously" or "if ResourceHandler doesn't defer handling of the
response".

Actually, it should just be "no" instead of "not".  Fixed.

[mmenke, 2017-02-02 21:47:00]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-02-02 21:47:36]

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-02 22:42:27]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486072020691090,
"parent_rev": "6e36a3d8fa15b1d2977193421ab7fbb18e050ac1", "commit_rev":
"9f855ec8d57996af189f479bbb5f37a378921250"}

[commit-bot: I haz the power, 2017-02-02 22:43:06]

Description was changed from

==========
Remove ResourceLoader PostTask after reading headers.

https://codereview.chromium.org/2526983002 incidentally introduced a
PostTask when ResourceHandler::OnResponseStarted completed
synchronously, and the following body read did as well. This may have
resulted in a regression in some ServiceWorker perf tests.

In general, it's unclear how aggressively we should be posting tasks
while reading the body, this CL is just to restore the earlier behavior,
and (hopefully) make the tests happy again.

BUG=673625
==========

to

==========
Remove ResourceLoader PostTask after reading headers.

https://codereview.chromium.org/2526983002 incidentally introduced a
PostTask when ResourceHandler::OnResponseStarted completed
synchronously, and the following body read did as well. This may have
resulted in a regression in some ServiceWorker perf tests.

In general, it's unclear how aggressively we should be posting tasks
while reading the body, this CL is just to restore the earlier behavior,
and (hopefully) make the tests happy again.

BUG=673625

Review-Url: https://codereview.chromium.org/2669243006
Cr-Commit-Position: refs/heads/master@{#447863}
Committed:
https://chromium.googlesource.com/chromium/src/+/9f855ec8d57996af189f479bbb5f...
==========

[commit-bot: I haz the power, 2017-02-02 22:43:08]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/9f855ec8d57996af189f479bbb5f...