Disabled resource scheduling when using a SPDY proxy

Disabled resource scheduling when using a SPDY proxy

We already disable the artificial delaying of resources when connected
to a SPDY-capable host, this extends that to also include pages that are
served through a SPDY proxy.

As-implemented, as soon as any request for a given client completes and
was served using SPDY over a proxy connection, that client will be
treated as a SPDY-capable client.  Some resources will still be delayed
until the base page (or another request) completes but it is much better
than it was before.

BUG=324789
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243698

[Pat Meenan, 2013-12-02 17:42:29]

simonjam@, not sure if you're still reviewing patches as the only owner of
loader but if so, ptal.

[James Simonsen, 2013-12-02 18:59:31]

Yeah, I still do the reviews.

Can you also add a test case for this? We actually have really good coverage for
this file.

https://codereview.chromium.org/99533002/diff/1/content/browser/loader/resour...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/99533002/diff/1/content/browser/loader/resour...
content/browser/loader/resource_scheduler.cc:201:
response_info.was_fetched_via_proxy) {
Is this really the only way to determine it? It seems unfortunate we have to
wait so long. How does the network stack decide to use SPDY proxy? Can we
extract that code instead?

[Pat Meenan, 2013-12-02 19:35:51]

I'll get a test case worked up in a minute - in the meantime I wanted to provide
some context for the detection part.

https://codereview.chromium.org/99533002/diff/1/content/browser/loader/resour...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/99533002/diff/1/content/browser/loader/resour...
content/browser/loader/resource_scheduler.cc:201:
response_info.was_fetched_via_proxy) {
On 2013/12/02 18:59:31, James Simonsen wrote:
> Is this really the only way to determine it? It seems unfortunate we have to
> wait so long. How does the network stack decide to use SPDY proxy? Can we
> extract that code instead?

This is the lowest-impact way I could come up with (there's a thread on
chromium-dev with Will Chan and Ryan Sleevi:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ziP_vgEQ2AQ
).

The network stack needs to walk through the configured proxies, execute any PAC
scripts and then only after the TLS negotiation with the host is complete do we
know it's actually a SPDY proxy which is way more overhead than we want to do
here.

It isn't quite as bad as it seems.  We can only discover the resources after the
first request starts to come back so the lost opportunity is between the TTFB
and when the base page (or any script or other request) completes at which point
we'd open the flood-gates.  It's a bit conservative but should get most of the
benefit and we can improve it further for some cases if we think it's worthwhile
(like the data reduction proxy).

[Pat Meenan, 2014-01-08 14:05:47]

simonjam, ptal

Sorry for the delay, there were more changes than I expected.  The original
implementation was checking requests when they were removed from the active
request queue but this is done in the request destructor and querying request
info at that point was a bad idea.

The new code checks a request in the received response notifier to see if it was
served over spdy and a proxy and then passes that info to the scheduler.  As an
added benefit this is a lot sooner and triggers as soon as we get the headers
for the first request.

I also added a unit test that makes sure existing delayed requests get scheduled
as soon as we set the spdy proxy flag  and that any new requests after that are
not delayed.

[James Simonsen, 2014-01-08 18:50:11]

lgtm

[Pat Meenan, 2014-01-08 18:53:34]

willchan@chromium.org: Please review changes in urlrequest.h

Just added an accessor to get the spdy flag state on a request (identical to the
proxy flag state accessor).

[willchan no longer on Chromium, 2014-01-08 19:03:09]

https://codereview.chromium.org/99533002/diff/40001/net/url_request/url_reque...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/99533002/diff/40001/net/url_request/url_reque...
net/url_request/url_request.h:503: // Returns true if the URLRequest was
delivered over SPDY.
I'm mostly fine with this, but it might be the time for us to do a rename. Is
this set to true for SPDY proxies & origins? Does your code care to
differentiate?

[Pat Meenan, 2014-01-08 19:10:56]

https://codereview.chromium.org/99533002/diff/40001/net/url_request/url_reque...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/99533002/diff/40001/net/url_request/url_reque...
net/url_request/url_request.h:503: // Returns true if the URLRequest was
delivered over SPDY.
On 2014/01/08 19:03:09, willchan wrote:
> I'm mostly fine with this, but it might be the time for us to do a rename. Is
> this set to true for SPDY proxies & origins? Does your code care to
> differentiate?

It is true for both cases, the flag gets set as long as the request was routed
over a SPDY connection (which works fine for my case - tested both scenarios
just to be sure).

[willchan no longer on Chromium, 2014-01-08 19:37:37]

LGTM

Maybe I oughta file a bug to differentiate this at some point...but if it
doesn't matter yet, it doesn't matter.

[commit-bot: I haz the power, 2014-01-08 19:53:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pmeenan@chromium.org/99533002/40001

[commit-bot: I haz the power, 2014-01-08 23:47:58]

Change committed as 243698