Add the UMA recording recall of URLRequests preconnected.

Add the UMA recording recall of URLRequests preconnected.
This is to be observed in the "preconnect on link UIEvents" Finch field trial to see its effect.

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

[kouhei (in TOK), 2013-05-22 06:37:43]

mmenke: Would you take a look?

I think this is similar to the metric you suggested me previously:
https://codereview.chromium.org/14749006/#msg7

[mmenke, 2013-05-22 17:58:43]

This is very much like what I was thinking.

https://codereview.chromium.org/15675002/diff/1008/chrome/browser/net/connect...
File chrome/browser/net/connect_interceptor.cc (right):

https://codereview.chromium.org/15675002/diff/1008/chrome/browser/net/connect...
chrome/browser/net/connect_interceptor.cc:98:
recent_preconnects_.SetRecentlySeen(request_scheme_host);
I think this could actually all be done in the predictor more simply, since
|this| owns it.

https://codereview.chromium.org/15675002/diff/1008/net/base/network_delegate.h
File net/base/network_delegate.h (right):

https://codereview.chromium.org/15675002/diff/1008/net/base/network_delegate....
net/base/network_delegate.h:68: void NotifyBeforePreconnect(const GURL& url);
This is an interface for the network stack, in net/, to communicate with the
consumer (The browser).  We shouldn't have functions here just for non-src/net
functions to call.

[kouhei (in TOK), 2013-05-23 07:31:13]

Thanks for your comments. Updated patch.

https://codereview.chromium.org/15675002/diff/1008/chrome/browser/net/connect...
File chrome/browser/net/connect_interceptor.cc (right):

https://codereview.chromium.org/15675002/diff/1008/chrome/browser/net/connect...
chrome/browser/net/connect_interceptor.cc:98:
recent_preconnects_.SetRecentlySeen(request_scheme_host);
I tried moving the logic to Predictor. TimedCache is also moved out from
ConnectInterceptor so that I can use it from Predictor too.

https://codereview.chromium.org/15675002/diff/1008/net/base/network_delegate.h
File net/base/network_delegate.h (right):

https://codereview.chromium.org/15675002/diff/1008/net/base/network_delegate....
net/base/network_delegate.h:68: void NotifyBeforePreconnect(const GURL& url);
ok.

[mmenke, 2013-05-23 15:28:36]

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/predict...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/predict...
chrome/browser/net/predictor.cc:842: motivation, count));
I hadn't realized the Predictor lived on multiple threads.  Have you
investigated whether this is safe?

If the predictor is destroyed before the IOThread, and we have a message pending
to the IOThread from this, we'll crash.

PreconnectOnUIThread in preconnect.cc doesn't have that problem, since it owns a
reference of the getter.

Sorry for sending you around in circles, but we may have to do this in the
ConnectInterceptor afterall, or at least in some object that lives on the IO
thread after all.

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/timed_c...
File chrome/browser/net/timed_cache.cc (right):

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/timed_c...
chrome/browser/net/timed_cache.cc:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
See other comment about copyright notice.

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/timed_c...
chrome/browser/net/timed_cache.cc:7: #include "net/url_request/url_request.h"
I don't think we need this header.  All we need is "googleurl/src/gurl.h".

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/timed_c...
File chrome/browser/net/timed_cache.h (right):

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/timed_c...
chrome/browser/net/timed_cache.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
nit:  2013.  Also, remove the (c) (The latest preferred header format is not to
include it, I believe).

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/timed_c...
chrome/browser/net/timed_cache.h:20: static const int
kMaxUnusedSocketLifetimeSecondsWithoutAGet = 10;
This comment and variable seem a little odd here.

I'd suggest moving them into predictor.h.

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/timed_c...
chrome/browser/net/timed_cache.h:52: DISALLOW_COPY_AND_ASSIGN(TimedCache);
Include base/basictypes.h for this.

[kouhei (in TOK), 2013-05-24 00:34:36]

Thank you for your comments. Updated patch.

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/predict...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/predict...
chrome/browser/net/predictor.cc:842: motivation, count));
The Predictor instance is created in UIthread, but destroyed on IOThread, so I
think this is OK.

see comments on:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...

Also, there are other Predictor methods using the same technique.

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/timed_c...
File chrome/browser/net/timed_cache.cc (right):

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/timed_c...
chrome/browser/net/timed_cache.cc:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2013/05/23 15:28:36, mmenke wrote:
> See other comment about copyright notice.

Done.

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/timed_c...
File chrome/browser/net/timed_cache.h (right):

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/timed_c...
chrome/browser/net/timed_cache.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2013/05/23 15:28:36, mmenke wrote:
> nit:  2013.  Also, remove the (c) (The latest preferred header format is not
to
> include it, I believe).

Done.

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/timed_c...
chrome/browser/net/timed_cache.h:20: static const int
kMaxUnusedSocketLifetimeSecondsWithoutAGet = 10;
On 2013/05/23 15:28:36, mmenke wrote:
> This comment and variable seem a little odd here.
> 
> I'd suggest moving them into predictor.h.

Done.

[mmenke, 2013-05-24 18:28:55]

Looks good.  LGTM!

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/predict...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/15675002/diff/6001/chrome/browser/net/predict...
chrome/browser/net/predictor.cc:842: motivation, count));
On 2013/05/24 00:34:37, kouhei wrote:
> The Predictor instance is created in UIthread, but destroyed on IOThread, so I
> think this is OK.
> 
> see comments on:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...
> 
> Also, there are other Predictor methods using the same technique.

Thanks for investigating this!  Looks like you're correct.

[commit-bot: I haz the power, 2013-05-27 00:05:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/15675002/13001

[commit-bot: I haz the power, 2013-05-27 00:05:16]

Failed to apply patch for chrome/browser/net/connect_interceptor.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/net/connect_interceptor.cc
  Hunk #4 FAILED at 82.
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/net/connect_interceptor.cc.rej

Patch:       chrome/browser/net/connect_interceptor.cc
Index: chrome/browser/net/connect_interceptor.cc
diff --git a/chrome/browser/net/connect_interceptor.cc
b/chrome/browser/net/connect_interceptor.cc
index
e2bf67b57e10e8499e4cb055accf32481e7e0a1f..492ad3d22ef54d2f92f234639a3500e8ab7ba4bf
100644
--- a/chrome/browser/net/connect_interceptor.cc
+++ b/chrome/browser/net/connect_interceptor.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -10,16 +10,9 @@
 
 namespace chrome_browser_net {
 
-// We don't bother learning to preconnect via a GET if the original URL
-// navigation was so long ago, that a preconnection would have been dropped
-// anyway.  We believe most servers will drop the connection in 10 seconds, so
-// we currently estimate this time-till-drop at 10 seconds.
-// TODO(jar): We should do a persistent field trial to validate/optimize this.
-static const int kMaxUnusedSocketLifetimeSecondsWithoutAGet = 10;
-
 ConnectInterceptor::ConnectInterceptor(Predictor* predictor)
     : timed_cache_(base::TimeDelta::FromSeconds(
-          kMaxUnusedSocketLifetimeSecondsWithoutAGet)),
+          Predictor::kMaxUnusedSocketLifetimeSecondsWithoutAGet)),
       predictor_(predictor) {
   DCHECK(predictor);
 }
@@ -78,6 +71,8 @@ void ConnectInterceptor::WitnessURLRequest(net::URLRequest*
request) const {
   }
   timed_cache_.SetRecentlySeen(request_scheme_host);
 
+  predictor_->RecordPreconnectNavigationStats(request_scheme_host);
+
   // Subresources for main frames usually get predicted when we detected the
   // main frame request - way back in RenderViewHost::Navigate.  So only handle
   // predictions now for subresources or for redirected hosts.
@@ -87,31 +82,4 @@ void ConnectInterceptor::WitnessURLRequest(net::URLRequest*
request) const {
   return;
 }
 
-ConnectInterceptor::TimedCache::TimedCache(const base::TimeDelta& max_duration)
-    : mru_cache_(UrlMruTimedCache::NO_AUTO_EVICT),
-      max_duration_(max_duration) {
-}
-
-// Make Clang compilation happy with explicit destructor.
-ConnectInterceptor::TimedCache::~TimedCache() {}
-
-bool ConnectInterceptor::TimedCache::WasRecentlySeen(const GURL& url) const {
-  DCHECK_EQ(url.GetWithEmptyPath(), url);
-  // Evict any overly old entries.
-  base::TimeTicks now = base::TimeTicks::Now();
-  UrlMruTimedCache::reverse_iterator eldest = mru_cache_.rbegin();
-  while (!mru_cache_.empty()) {
-    DCHECK(eldest == mru_cache_.rbegin());
-    if (now - eldest->second < max_duration_)
-      break;
-    eldest = mru_cache_.Erase(eldest);
-  }
-  return mru_cache_.end() != mru_cache_.Peek(url);
-}
-
-void ConnectInterceptor::TimedCache::SetRecentlySeen(const GURL& url) const {
-  DCHECK_EQ(url.GetWithEmptyPath(), url);
-  mru_cache_.Put(url, base::TimeTicks::Now());
-}
-
 }  // namespace chrome_browser_net

[commit-bot: I haz the power, 2013-05-27 00:37:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/15675002/23001

[commit-bot: I haz the power, 2013-05-27 05:48:21]

Failed to apply patch for chrome/browser/net/predictor.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/net/predictor.cc
  Hunk #4 FAILED at 286.
  Hunk #5 succeeded at 831 (offset 5 lines).
  Hunk #6 succeeded at 906 (offset 5 lines).
  Hunk #7 succeeded at 931 (offset 5 lines).
  1 out of 7 hunks FAILED -- saving rejects to file
chrome/browser/net/predictor.cc.rej

Patch:       chrome/browser/net/predictor.cc
Index: chrome/browser/net/predictor.cc
diff --git a/chrome/browser/net/predictor.cc b/chrome/browser/net/predictor.cc
index
3af1cf3a2b5388e70d6a69964fb21b77574991be..2a2989902e80eb3d31a2a8abf595496c18d07f0b
100644
--- a/chrome/browser/net/predictor.cc
+++ b/chrome/browser/net/predictor.cc
@@ -64,6 +64,7 @@ const int64 Predictor::kDurationBetweenTrimmingsHours = 1;
 const int64 Predictor::kDurationBetweenTrimmingIncrementsSeconds = 15;
 const size_t Predictor::kUrlsTrimmedPerIncrement = 5u;
 const size_t Predictor::kMaxSpeculativeParallelResolves = 3;
+const int Predictor::kMaxUnusedSocketLifetimeSecondsWithoutAGet = 10;
 // To control our congestion avoidance system, which discards a queue when
 // resolutions are "taking too long," we need an expected resolution time.
 // Common average is in the range of 300-500ms.
@@ -136,6 +137,8 @@ Predictor::Predictor(bool preconnect_enabled)
       host_resolver_(NULL),
       preconnect_enabled_(preconnect_enabled),
       consecutive_omnibox_preconnect_count_(0),
+      recent_preconnects_(
+          TimeDelta::FromSeconds(kMaxUnusedSocketLifetimeSecondsWithoutAGet)),
       next_trim_time_(base::TimeTicks::Now() +
                       TimeDelta::FromHours(kDurationBetweenTrimmingsHours)) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -243,9 +246,8 @@ void Predictor::AnticipateOmniboxUrl(const GURL& url, bool
preconnectable) {
           return;  // We've done a preconnect recently.
         last_omnibox_preconnect_ = now;
         const int kConnectionsNeeded = 1;
-        PreconnectOnUIThread(CanonicalizeUrl(url), GURL(), motivation,
-                             kConnectionsNeeded,
-                             url_request_context_getter_);
+        PreconnectUrl(CanonicalizeUrl(url), GURL(), motivation,
+                      kConnectionsNeeded);
         return;  // Skip pre-resolution, since we'll open a connection.
       }
     } else {
@@ -284,9 +286,8 @@ void Predictor::PreconnectUrlAndSubresources(const GURL&
url,
     std::string host = url.HostNoBrackets();
     UrlInfo::ResolutionMotivation motivation(UrlInfo::EARLY_LOAD_MOTIVATED);
     const int kConnectionsNeeded = 1;
-    PreconnectOnUIThread(CanonicalizeUrl(url), first_party_for_cookies,
-                         motivation, kConnectionsNeeded,
-                         url_request_context_getter_);
+    PreconnectUrl(CanonicalizeUrl(url), first_party_for_cookies,
+                  motivation, kConnectionsNeeded);
     PredictFrameSubresources(url.GetWithEmptyPath(), first_party_for_cookies);
   }
 }
@@ -824,6 +825,41 @@ void Predictor::EnablePredictorOnIOThread(bool enable) {
   predictor_enabled_ = enable;
 }
 
+void Predictor::PreconnectUrl(const GURL& url,
+                              const GURL& first_party_for_cookies,
+                              UrlInfo::ResolutionMotivation motivation,
+                              int count) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
+         BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  if (BrowserThread::CurrentlyOn(BrowserThread::IO)) {
+    PreconnectUrlOnIOThread(url, first_party_for_cookies, motivation, count);
+  } else {
+    BrowserThread::PostTask(
+        BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(&Predictor::PreconnectUrlOnIOThread,
+                   base::Unretained(this), url, first_party_for_cookies,
+                   motivation, count));
+  }
+}
+
+void Predictor::PreconnectUrlOnIOThread(
+    const GURL& url, const GURL& first_party_for_cookies,
+    UrlInfo::ResolutionMotivation motivation, int count) {
+  GURL canonical_url(CanonicalizeUrl(url));
+  recent_preconnects_.SetRecentlySeen(canonical_url);
+
+  PreconnectOnIOThread(url, first_party_for_cookies, motivation, count,
+                       url_request_context_getter_);
+}
+
+void Predictor::RecordPreconnectNavigationStats(const GURL& url) {
+  UMA_HISTOGRAM_BOOLEAN(
+      "Net.PreconnectedNavigation",
+      recent_preconnects_.WasRecentlySeen(url));
+}
+
 void Predictor::PredictFrameSubresources(const GURL& url,
                                          const GURL& first_party_for_cookies) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
@@ -864,9 +900,8 @@ void Predictor::PrepareFrameSubresources(const GURL& url,
     // load any subresources).  If we learn about this resource, we will
instead
     // provide a more carefully estimated preconnection count.
     if (preconnect_enabled_) {
-      PreconnectOnIOThread(url, first_party_for_cookies,
-                           UrlInfo::SELF_REFERAL_MOTIVATED, 2,
-                           url_request_context_getter_);
+      PreconnectUrlOnIOThread(url, first_party_for_cookies,
+                              UrlInfo::SELF_REFERAL_MOTIVATED, 2);
     }
     return;
   }
@@ -890,8 +925,8 @@ void Predictor::PrepareFrameSubresources(const GURL& url,
       int count = static_cast<int>(std::ceil(connection_expectation));
       if (url.host() == future_url->first.host())
         ++count;
-      PreconnectOnIOThread(future_url->first, first_party_for_cookies,
-                           motivation, count, url_request_context_getter_);
+      PreconnectUrlOnIOThread(future_url->first, first_party_for_cookies,
+                              motivation, count);
     } else if (connection_expectation > kDNSPreresolutionWorthyExpectedValue) {
       evalution = PRERESOLUTION;
       future_url->second.preresolution_increment();

[commit-bot: I haz the power, 2013-05-27 07:22:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/15675002/36002

[commit-bot: I haz the power, 2013-05-27 09:44:04]

Change committed as 202395