Refuse to pipeline frames, prefetches, and downloads.

Refuse to pipeline frames, prefetches, and downloads.


BUG=119287
TEST=Follow steps in bug


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134621

[James Simonsen, 2012-04-19 01:28:37]

Originally, I was planning to plumb resource_type all the way down from
ResourceDispatcherHostImpl, but it already sort of does that with load_flags.
It's possible we'll still to know the specific type of subresource, but we can
do that when it comes up.

[mmenke, 2012-04-19 14:40:20]

https://chromiumcodereview.appspot.com/10095024/diff/1/net/http/http_stream_f...
File net/http/http_stream_factory_impl_job.cc (right):

https://chromiumcodereview.appspot.com/10095024/diff/1/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:911: // TODO(simonjam): Support
proxies.
nit:  This should be indented two spaces.  Suggest you move it one line down.

https://chromiumcodereview.appspot.com/10095024/diff/1/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:1251: net::LOAD_IS_DOWNLOAD)) {
nit:  Think a comment might be nice.

While most downloads should be main frames, subframes, or downloads, I wonder if
it includes all downloads.  Wonder how only pipelining resources of the same
type compares in terms of success rate to this.

https://chromiumcodereview.appspot.com/10095024/diff/1/net/http/http_stream_f...
net/http/http_stream_factory_impl_job.cc:1251: net::LOAD_IS_DOWNLOAD)) {
Might be nice to have a regression test for this as well.

[mmenke, 2012-04-19 15:08:19]

Another concern...  We don't really understand all the underlying issues, except
the big download before a small file issue, which this also fixes.  We really
should add some instrumentation to test bad servers, and then find some, to get
a better understanding of typical pipelining problems in the wild, and see if
this fixes them.

[James Simonsen, 2012-04-26 19:17:02]

Sorry, I was at the WebKit meeting last week, then got side-tracked when I
returned.

On 2012/04/19 15:08:19, Matt Menke wrote:
> Another concern...  We don't really understand all the underlying issues,
except
> the big download before a small file issue, which this also fixes.  We really
> should add some instrumentation to test bad servers, and then find some, to
get
> a better understanding of typical pipelining problems in the wild, and see if
> this fixes them.

I view this patch slightly differently. I view this as avoiding resources that
are likely to stream. That's the build.chromium.org bug. We'll still need other
solutions to address front-of-queue blocking.

Will and I were talking about rolling out pipelining to canary channel soon.
That should get us a lot more failure cases to work on.

I'm going on vacation next week, we can roll it out when I get back.

http://codereview.chromium.org/10095024/diff/1/net/http/http_stream_factory_i...
File net/http/http_stream_factory_impl_job.cc (right):

http://codereview.chromium.org/10095024/diff/1/net/http/http_stream_factory_i...
net/http/http_stream_factory_impl_job.cc:911: // TODO(simonjam): Support
proxies.
On 2012/04/19 14:40:20, Matt Menke wrote:
> nit:  This should be indented two spaces.  Suggest you move it one line down.

Done.

http://codereview.chromium.org/10095024/diff/1/net/http/http_stream_factory_i...
net/http/http_stream_factory_impl_job.cc:1251: net::LOAD_IS_DOWNLOAD)) {
On 2012/04/19 14:40:20, Matt Menke wrote:
> Might be nice to have a regression test for this as well.

Done.

http://codereview.chromium.org/10095024/diff/1/net/http/http_stream_factory_i...
net/http/http_stream_factory_impl_job.cc:1251: net::LOAD_IS_DOWNLOAD)) {
On 2012/04/19 14:40:20, Matt Menke wrote:
> nit:  Think a comment might be nice.

Done.

> While most downloads should be main frames, subframes, or downloads, I wonder
if
> it includes all downloads.  Wonder how only pipelining resources of the same
> type compares in terms of success rate to this.

Yeah, that was my original plan: to make it opt-in instead of opt-out. WebKit
tells the browser process what type of resource it's downloading (image, css,
js, etc.). That info doesn't make it to net/ though. Instead, that type is
translated into load flags.

This patch is simple and fixes the build.chromium.org/.../stdio case. If we find
other bugs, I can switch it around.

[mmenke, 2012-04-30 18:48:21]

LGTM.  Sorry I didn't get to this last week - was taking a couple days off.

[commit-bot: I haz the power, 2012-04-30 20:34:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/10095024/5001

[commit-bot: I haz the power, 2012-04-30 22:37:48]

Change committed as 134621