Eliminated the logic that accumulated multiple preconnect requests

chrome/browser/net/predictor_browsertest.cc

Index: chrome/browser/net/predictor_browsertest.cc
diff --git a/chrome/browser/net/predictor_browsertest.cc b/chrome/browser/net/predictor_browsertest.cc
index 307c61c25604fb0f7ceb9c9982de881fc264bf9c..b8eab4eef9b8f1232c5cc2e9ce497ac797b22afc 100644
--- a/chrome/browser/net/predictor_browsertest.cc
+++ b/chrome/browser/net/predictor_browsertest.cc
@@ -2,8 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/base64.h"
+#include "base/command_line.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/prefs/pref_service.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/net/chrome_net_log.h"
 #include "chrome/browser/net/predictor.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
@@ -11,7 +15,9 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/test/test_utils.h"
+#include "net/base/capturing_net_log_observer.h"
 #include "net/base/net_errors.h"
+#include "net/base/net_log.h"
 #include "net/dns/host_resolver_proc.h"
 #include "net/dns/mock_host_resolver.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -21,6 +27,9 @@ using testing::HasSubstr;
 
 namespace {
 
+const char kEnablePreconnect[] = "--enable-blink-features=LinkPreconnect";
+const char kEnableExperimentalWebPlatformFeatures[] =
+    "--enable-experimental-web-platform-features";
 const char kChromiumHostname[] = "chromium.org";
 
 // Records a history of all hostnames for which resolving has been requested,
@@ -111,6 +120,11 @@ class PredictorBrowserTest : public InProcessBrowserTest {
     InProcessBrowserTest::SetUpInProcessBrowserTestFixture();
   }
 
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(kEnablePreconnect);
+    command_line->AppendSwitch(kEnableExperimentalWebPlatformFeatures);
+  }
+
   void TearDownInProcessBrowserTestFixture() override {
     InProcessBrowserTest::TearDownInProcessBrowserTestFixture();
     scoped_host_resolver_proc_.reset();
@@ -200,5 +214,42 @@ IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, DnsPrefetch) {
   WaitUntilHostHasBeenRequested(kChromiumHostname);
 }
 
+IN_PROC_BROWSER_TEST_F(PredictorBrowserTest, Preconnect) {
+  ASSERT_TRUE(test_server()->Start());
+
+  net::CapturingNetLogObserver net_log;

[mmenke, 2015/02/13 16:42:17]

Calling a NetLogObserver net_log is a little confusing.  I'd suggest
net_log_observer - don't think the extra characters hurt us.  I can also live
with just observer.

[Pat Meenan, 2015/02/13 20:45:26]

Done.

+  g_browser_process->net_log()->AddThreadSafeObserver(&net_log,
+    net::NetLog::LOG_ALL_BUT_BYTES);

[mmenke, 2015/02/13 16:42:16]

nit:  Should indent 4, and move &net_log on to the next line.

[Pat Meenan, 2015/02/13 20:45:26]

Done.

+
+  // Create a HTML preconnect reference to the local server in the form
+  // <link rel="preconnect" href="http://test-server/">
+  // and navigate to it as a data URI. The only netlog activity to the test
+  // server will be as a result of parsing and making the preconnect request.

[mmenke, 2015/02/13 16:42:17]

Claiming "The only netlog activity" is anything is a pretty strong claim.  I
suggest removing it.

[Pat Meenan, 2015/02/13 20:45:26]

Done.

+  std::string preconnect_url = test_server()->GetURL("").spec();
+  std::string preconnect_content = "<link rel=\"preconnect\" href=\"";
+  preconnect_content += preconnect_url + "\">";
+  std::string data_uri = "data:text/html;base64,";
+  std::string encoded;
+  base::Base64Encode(preconnect_content, &encoded);
+  data_uri += encoded;

[mmenke, 2015/02/13 16:42:17]

nit:  Suggest merging this line with the one where you declare data_url.  

std::string data_uri = "data:text/html;base64," + encoded;

Think it's more readable that way

[mmenke, 2015/02/13 16:42:17]

optional:  Do we have to base64 encode it?  "data:text/html;<link rel...." seems
to work fine, though I suppose it's not exactly pretty.

[Pat Meenan, 2015/02/13 20:45:26]

We either need to base64 encode or URL encode it.  The fact that it works as
plain text seems to be luck (it shouldn't).  Base64 encoding it was easy so I
just went that way.

Thanks for the pointer on being able to append the strings with a static string
as a l-value.  I didn't realize that was possible and cleans things up a lot.

+
+  ui_test_utils::NavigateToURL(browser(), GURL(data_uri));

[mmenke, 2015/02/13 16:42:16]

For this to be non-racy, we're depending on the preconnect to make it to the
network stack's HostResolver before we get news of the navigation completion on
the UI thread.

Two problems with this:  I'd like to check for it making it to an actually
connection attempt, and relying on everything up to that point that we do on the
IO thread happening synchronously seems like a bad idea.

To check for a connection attempt, we can just check that |entry. source.type|
(Or something like that) is of type net::NetLog::SOURCE_SOCKET.

To make sure there's no race, I suggest switching from a CapturingNetLogObserver
to a custom one with a RunLoop that exits the loop when it sees the event we're
waiting for.

[Pat Meenan, 2015/02/13 20:45:26]

My original implementation had a custom observer with a run loop (like the DNS
one) and a wait check.  When I forced it to not fire until after entering the
runloop I couldn't get the code to actually quit the runloop successfully (tried
both base::MessageLoop::current()->Quit(); and 
base::MessageLoopForUI::current()->Quit();).  I'll go back and give it another
shot but may upload an intermediate implementation with the same issue if I
can't figure it out.

[mmenke, 2015/02/13 20:55:05]

I must be missing something...Why doesn't this work:

class ObserverThing : public net::NetLog::ThreadSafeObserver {
 public:
  explicit ObserverThing(std::string host_port_pair)
      : host_port_pair_(host_port_pair) {}
  ~ObserverThing() {}

  void RunUntilThingWeCareAbout() {
    run_loop_.Run();
  }

 private:
  OnNetLogEvent(...) {
    if (<event is the one we care about>) {
      run_loop_.Quit();
    }
  }

  base::RunLoop run_loop_;
  const std::string host_port_pair_;
}

And then:

ObserverThing thing(host_port_pair_we_care_about);
g_browser_process->net_log()->AddThreadSafeObserver(thing);
ui_test_utils::NavigateToURL(browser(), GURL(data_uri));
thing.RunUntilThingWeCareAbout();
g_browser_process->net_log()->RemoveThreadSafeObserver(thing);

+
+  // Look through the recorded netlog events for the URL that we specified

[mmenke, 2015/02/13 16:42:17]

nit:  netlog -> NetLog

[Pat Meenan, 2015/02/13 20:45:26]

Done.

+  // in the preconnect request.
+  bool saw_preconnect_request = false;
+  g_browser_process->net_log()->RemoveThreadSafeObserver(&net_log);

[mmenke, 2015/02/13 16:42:17]

nit:  This should probably go above this code block, just after the navigation.

[Pat Meenan, 2015/02/13 20:45:26]

Done.

+  net::CapturedNetLogEntry::List list;
+  net_log.GetEntries(&list);
+  for (const auto& entry : list) {
+    if (entry.GetParamsJson().find(preconnect_url)) {
+      saw_preconnect_request = true;
+      break;
+    }
+  }
+
+  ASSERT_TRUE(saw_preconnect_request);

[mmenke, 2015/02/13 16:42:16]

nit:  EXPECT_TRUE?

[Pat Meenan, 2015/02/13 20:45:26]

Done.

+}
+
 }  // namespace chrome_browser_net