Support both preconnection, and pre-resolution for subresources...

Support both preconnection, and pre-resolution for subresources

With this change, both preconnection and preresolution are
enabled when the --enable-preconnection flag is turned on.

I'm expecting to enable this feature by default soon, so
as to better tune the parameters.


BUG=42694
r=mbelshe

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

[jar (doing other things), 2010-07-27 19:32:29]

This CL provides basic integration, so that we can turn on preconnection by
default (and continue to support pre-resolution).

Items that are still TBD:
a) The startup list of resolution can also be gotten from the speculative
connection interceptor, rather than the DNS observer.

b) The Renderer scan can send actual URL specs (rather than just host names) so
that we don't need to guess on scheme and port.

c) Stats need to be gathered to see how many preconnections are wasted
(abandoned prior to use).

[Mike Belshe, 2010-07-27 20:49:25]

lgtm

http://codereview.chromium.org/3032014/diff/53001/50003
File chrome/browser/net/connect_interceptor.cc (right):

http://codereview.chromium.org/3032014/diff/53001/50003#newcode29
chrome/browser/net/connect_interceptor.cc:29: if
(!request->was_fetched_via_proxy())
I thought this would be:

if (!is_subresource && !request->was_fetched_via_proxy())

http://codereview.chromium.org/3032014/diff/53001/50004
File chrome/browser/net/preconnect.cc (right):

http://codereview.chromium.org/3032014/diff/53001/50004#newcode30
chrome/browser/net/preconnect.cc:30: UrlInfo::ResolutionMotivation motivation) {
Looks like you plumbed in the motivation but never use it?

http://codereview.chromium.org/3032014/diff/53001/50011
File chrome/browser/net/referrer.cc (right):

http://codereview.chromium.org/3032014/diff/53001/50011#newcode37
chrome/browser/net/referrer.cc:37: // when a referrer is navigated to, we start
with a the following rather low
nit: "with a the"

http://codereview.chromium.org/3032014/diff/53001/50012
File chrome/browser/net/referrer.h (right):

http://codereview.chromium.org/3032014/diff/53001/50012#newcode62
chrome/browser/net/referrer.h:62: // remains.
returns true if it still has subresources that could potentially be used?

[jar (doing other things), 2010-07-28 00:42:38]

Comment (in one case), plus changes made per mbelshe.

http://codereview.chromium.org/3032014/diff/53001/50003
File chrome/browser/net/connect_interceptor.cc (right):

http://codereview.chromium.org/3032014/diff/53001/50003#newcode29
chrome/browser/net/connect_interceptor.cc:29: if
(!request->was_fetched_via_proxy())
Subresources can themselves have subresources, so we don't really care if this
is a MAIN_FRAME or not.

The only thing we want to avoid is learning about a referer, when the fetch is
for a MAIN_FRAME (as was done above).

On 2010/07/27 20:49:25, Mike Belshe wrote:
> I thought this would be:
> 
> if (!is_subresource && !request->was_fetched_via_proxy())

http://codereview.chromium.org/3032014/diff/53001/50004
File chrome/browser/net/preconnect.cc (right):

http://codereview.chromium.org/3032014/diff/53001/50004#newcode30
chrome/browser/net/preconnect.cc:30: UrlInfo::ResolutionMotivation motivation) {
On 2010/07/27 20:49:25, Mike Belshe wrote:
> Looks like you plumbed in the motivation but never use it?

<oops>. It was meant to be histogramsed.  Code added below.
Thanks!!

http://codereview.chromium.org/3032014/diff/53001/50011
File chrome/browser/net/referrer.cc (right):

http://codereview.chromium.org/3032014/diff/53001/50011#newcode37
chrome/browser/net/referrer.cc:37: // when a referrer is navigated to, we start
with a the following rather low
On 2010/07/27 20:49:25, Mike Belshe wrote:
> nit: "with a the"

Done.

http://codereview.chromium.org/3032014/diff/53001/50012
File chrome/browser/net/referrer.h (right):

http://codereview.chromium.org/3032014/diff/53001/50012#newcode62
chrome/browser/net/referrer.h:62: // remains.
On 2010/07/27 20:49:25, Mike Belshe wrote:
> returns true if it still has subresources that could potentially be used?

Done.

[mbelshe, 2010-07-28 14:35:57]

On Tue, Jul 27, 2010 at 5:42 PM, <jar@chromium.org> wrote:

> Comment (in one case), plus changes made per mbelshe.
>
>
>
> http://codereview.chromium.org/3032014/diff/53001/50003
> File chrome/browser/net/connect_interceptor.cc (right):
>
> http://codereview.chromium.org/3032014/diff/53001/50003#newcode29
> chrome/browser/net/connect_interceptor.cc:29: if
> (!request->was_fetched_via_proxy())
> Subresources can themselves have subresources, so we don't really care
> if this is a MAIN_FRAME or not.
>
> The only thing we want to avoid is learning about a referer, when the
> fetch is for a MAIN_FRAME (as was done above).



This is not true.  Just because I am loading one subresource from
foo.comdoes not mean that I need to preconnect all subresources for
foo.com.

If the learning is based on main pages, then the actions from that learning
should only be applied to the main pages.

Mike


>
>
> On 2010/07/27 20:49:25, Mike Belshe wrote:
>
>> I thought this would be:
>>
>
>  if (!is_subresource && !request->was_fetched_via_proxy())
>>
>
> http://codereview.chromium.org/3032014/diff/53001/50004
> File chrome/browser/net/preconnect.cc (right):
>
> http://codereview.chromium.org/3032014/diff/53001/50004#newcode30
> chrome/browser/net/preconnect.cc:30: UrlInfo::ResolutionMotivation
> motivation) {
> On 2010/07/27 20:49:25, Mike Belshe wrote:
>
>> Looks like you plumbed in the motivation but never use it?
>>
>
> <oops>. It was meant to be histogramsed.  Code added below.
> Thanks!!
>
>
> http://codereview.chromium.org/3032014/diff/53001/50011
> File chrome/browser/net/referrer.cc (right):
>
> http://codereview.chromium.org/3032014/diff/53001/50011#newcode37
> chrome/browser/net/referrer.cc:37: // when a referrer is navigated to,
> we start with a the following rather low
> On 2010/07/27 20:49:25, Mike Belshe wrote:
>
>> nit: "with a the"
>>
>
> Done.
>
>
> http://codereview.chromium.org/3032014/diff/53001/50012
> File chrome/browser/net/referrer.h (right):
>
> http://codereview.chromium.org/3032014/diff/53001/50012#newcode62
> chrome/browser/net/referrer.h:62: // remains.
> On 2010/07/27 20:49:25, Mike Belshe wrote:
>
>> returns true if it still has subresources that could potentially be
>>
> used?
>
> Done.
>
>
> http://codereview.chromium.org/3032014/show
>

[Mike Belshe, 2010-07-28 15:42:03]

http://codereview.chromium.org/3032014/diff/53001/50011
File chrome/browser/net/referrer.cc (right):

http://codereview.chromium.org/3032014/diff/53001/50011#newcode17
chrome/browser/net/referrer.cc:17: // We always combine our old expected value,
weighted by some factor, with the
...weighted by some factor (we use kWeightingForOldExpectedValue), ...

http://codereview.chromium.org/3032014/diff/53001/50011#newcode19
chrome/browser/net/referrer.cc:19: // connections made due to the curernt
navigations.
nit: /curernt/current

http://codereview.chromium.org/3032014/diff/53001/50011#newcode21
chrome/browser/net/referrer.cc:21: // Eupdated = Eold * W  + Enew * (1 - W)
nix lines 20/21, they are duplicative with the below.

http://codereview.chromium.org/3032014/diff/53001/50011#newcode23
chrome/browser/net/referrer.cc:23: // Pupdated = Pold * W  +  Enew * (1 - W)
/P/E/

[jar (doing other things), 2010-07-28 18:54:05]

Change made per discussion to better distinguish frames (which may have
subresources) from non-frames (like js files), which can't have subresources. 

p.s., Please look at most recent version, even though my comments are on
outdated version as responses to your comments.

http://codereview.chromium.org/3032014/diff/53001/50003
File chrome/browser/net/connect_interceptor.cc (right):

http://codereview.chromium.org/3032014/diff/53001/50003#newcode29
chrome/browser/net/connect_interceptor.cc:29: if
(!request->was_fetched_via_proxy())
Per discussion I plumbed through another flag, which allows us to detect if this
is a frame, and only then will we predicts said frame's subresources.

On 2010/07/28 00:42:38, jar wrote:
> Subresources can themselves have subresources, so we don't really care if this
> is a MAIN_FRAME or not.
> 
> The only thing we want to avoid is learning about a referer, when the fetch is
> for a MAIN_FRAME (as was done above).
> 
> On 2010/07/27 20:49:25, Mike Belshe wrote:
> > I thought this would be:
> > 
> > if (!is_subresource && !request->was_fetched_via_proxy())
> 
>

http://codereview.chromium.org/3032014/diff/53001/50011
File chrome/browser/net/referrer.cc (right):

http://codereview.chromium.org/3032014/diff/53001/50011#newcode17
chrome/browser/net/referrer.cc:17: // We always combine our old expected value,
weighted by some factor, with the
On 2010/07/28 15:42:03, Mike Belshe wrote:
> ...weighted by some factor (we use kWeightingForOldExpectedValue), ...

Done.

http://codereview.chromium.org/3032014/diff/53001/50011#newcode19
chrome/browser/net/referrer.cc:19: // connections made due to the curernt
navigations.
On 2010/07/28 15:42:03, Mike Belshe wrote:
> nit: /curernt/current

Done.

http://codereview.chromium.org/3032014/diff/53001/50011#newcode21
chrome/browser/net/referrer.cc:21: // Eupdated = Eold * W  + Enew * (1 - W)
On 2010/07/28 15:42:03, Mike Belshe wrote:
> nix lines 20/21, they are duplicative with the below.

Done.

http://codereview.chromium.org/3032014/diff/53001/50011#newcode23
chrome/browser/net/referrer.cc:23: // Pupdated = Pold * W  +  Enew * (1 - W)
On 2010/07/28 15:42:03, Mike Belshe wrote:
> /P/E/

Done.

[Mike Belshe, 2010-07-28 19:30:14]

lgtm