Display DNS probe results.

chrome/browser/net/dns_probe_browsertest.cc

+// 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.
+
+#include "base/bind.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"
+#include "base/path_service.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/google/google_util.h"
+#include "chrome/browser/io_thread.h"
+#include "chrome/browser/net/dns_probe_test_util.h"
+#include "chrome/browser/net/net_error_tab_helper.h"
+#include "chrome/browser/net/url_request_mock_util.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/net/net_error_info.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/test/net/url_request_failed_job.h"
+#include "content/test/net/url_request_mock_http_job.h"
+#include "net/base/net_errors.h"
+#include "net/dns/dns_test_util.h"
+#include "net/url_request/url_request_filter.h"
+#include "net/url_request/url_request_job.h"
+#include "net/url_request/url_request_job_factory.h"
+
+using base::Bind;
+using base::Callback;
+using base::Closure;
+using base::ConstRef;
+using base::FilePath;
+using base::Unretained;
+using chrome_common_net::DnsProbeStatus;
+using content::BrowserThread;
+using content::URLRequestFailedJob;
+using content::URLRequestMockHTTPJob;
+using content::WebContents;
+using google_util::LinkDoctorBaseURL;
+using net::MockDnsClientRule;
+using net::NetworkDelegate;
+using net::URLRequest;
+using net::URLRequestFilter;
+using net::URLRequestJob;
+using net::URLRequestJobFactory;
+using ui_test_utils::NavigateToURL;
+using ui_test_utils::NavigateToURLBlockUntilNavigationsComplete;
+
+namespace chrome_browser_net {
+
+namespace {
+
+class BrokenLinkDoctorProtocolHandlerDelegate {

[mmenke, 2013/06/20 15:47:36]

Do we really need a delegate class?  Seems like we currently don't get anything
from this class.

[Deprecated (see juliatuttle), 2013/06/21 19:55:52]

It's what keeps BrokenLinkDoctorProtocolHandler and
DnsProbeBrowserTestIOThreadHelper from having circular dependencies.  It is
actually less code than moving the declaration of
DnsProbeBrowserTestIOThreadHelper here and then splitting out the method
definitions below.

+ public:
+  virtual int link_doctor_net_error() const = 0;
+};
+
+class BrokenLinkDoctorProtocolHandler
+    : public URLRequestJobFactory::ProtocolHandler {
+ public:
+  explicit BrokenLinkDoctorProtocolHandler(
+      BrokenLinkDoctorProtocolHandlerDelegate* delegate)
+      : delegate_(delegate) {}
+
+  virtual URLRequestJob* MaybeCreateJob(
+      URLRequest* request, NetworkDelegate* network_delegate) const OVERRIDE {
+    int net_error = delegate_->link_doctor_net_error();
+    if (net_error != net::OK) {
+      return new URLRequestFailedJob(request, network_delegate, net_error);
+    } else {
+      FilePath file_path = GetMockLinkDoctorFilePath();
+      return new URLRequestMockHTTPJob(request, network_delegate, file_path);
+    }
+  }
+
+ private:
+  FilePath GetMockLinkDoctorFilePath() const {

[mmenke, 2013/06/20 15:47:36]

Suggest making this static, just to indicate it doesn't depend on anything. 
Alternatively, could just make it a global function.

[Deprecated (see juliatuttle), 2013/06/21 19:55:52]

Done.

+    FilePath root_http;
+    PathService::Get(chrome::DIR_TEST_DATA, &root_http);
+    return root_http.AppendASCII("mock-link-doctor.html");
+  }
+
+  const BrokenLinkDoctorProtocolHandlerDelegate* delegate_;
+};
+
+class DnsProbeBrowserTestIOThreadHelper
+    : public BrokenLinkDoctorProtocolHandlerDelegate {
+ public:
+  DnsProbeBrowserTestIOThreadHelper()
+      : io_thread_(NULL),

[mmenke, 2013/06/20 15:47:36]

I'm assuming io_thread_ hasn't been created yet by the time this class is
created?  Could create it in SetUpOnMainThread(). Could then grab the IOThread
on construction.  Not a huge simplification, just one less thing to think about.
 Could also then make it automatically call SetUpOnIOThread on construction.

[Deprecated (see juliatuttle), 2013/06/21 19:55:52]

Done.

+        link_doctor_net_error_(net::OK) {
+  }
+
+  virtual ~DnsProbeBrowserTestIOThreadHelper() {}

[mmenke, 2013/06/20 15:47:36]

Suggest CHECKs on all these functions (Including set_link_doctor_net_error) to
make sure they're called on the IO thread.

[Deprecated (see juliatuttle), 2013/06/21 19:55:52]

Done.

+
+  void SetUpOnIOThread(IOThread* io_thread);
+  void CleanUpOnIOThreadAndDeleteHelper();
+
+  void SetRules(MockDnsClientRule::Result system_good_result,

[mmenke, 2013/06/20 15:47:36]

Think this function name could use improvement.

[Deprecated (see juliatuttle), 2013/06/21 19:55:52]

Done.

+                MockDnsClientRule::Result public_good_result);
+
+  virtual int link_doctor_net_error() const OVERRIDE {
+    return link_doctor_net_error_;
+  }
+  void set_link_doctor_net_error(int link_doctor_net_error) {
+    link_doctor_net_error_ = link_doctor_net_error;
+  }
+
+ private:
+  IOThread* io_thread_;
+  int link_doctor_net_error_;
+};
+
+void DnsProbeBrowserTestIOThreadHelper::SetUpOnIOThread(
+    IOThread* io_thread) {
+  DCHECK(io_thread);
+  DCHECK(!io_thread_);
+
+  io_thread_ = io_thread;
+
+  // SetUrlRequestMocksEnabled clears the filter list and then adds filters
+  // for several things, including the mock link doctor.  So, we call it
+  // first, then remove the handler it's added for the mock link doctor and
+  // add our own.
+  SetUrlRequestMocksEnabled(true);

[mmenke, 2013/06/20 15:47:36]

Think it may just be simpler to install only the URLRequest mock classes we use,
rather than set up them all, and then remove one (One could imaging the way the
link doctor is mocked out by SetUrlRequestMocksEnabled changing, so I'd rather
not rely on it).

[Deprecated (see juliatuttle), 2013/06/21 19:55:52]

Done.

+
+  URLRequestFilter* filter = URLRequestFilter::GetInstance();
+  const GURL link_doctor_base_url = LinkDoctorBaseURL();
+  const std::string host = link_doctor_base_url.host();
+  scoped_ptr<URLRequestJobFactory::ProtocolHandler> handler(
+      new BrokenLinkDoctorProtocolHandler(this));
+  filter->RemoveHostnameHandler("http", host);
+  filter->AddHostnameProtocolHandler("http", host, handler.Pass());
+}
+
+void DnsProbeBrowserTestIOThreadHelper::CleanUpOnIOThreadAndDeleteHelper() {
+  DCHECK(io_thread_);
+
+  SetUrlRequestMocksEnabled(false);
+  io_thread_ = NULL;

[mmenke, 2013/06/20 15:47:36]

Should also call filter->ClearHandlers();

As a singleton, I believe the filter is leaked, so probably a good idea to make
sure what we added to it is cleaned up.

[Deprecated (see juliatuttle), 2013/06/21 19:55:52]

Done.

+
+  delete this;
+}
+
+void DnsProbeBrowserTestIOThreadHelper::SetRules(
+    MockDnsClientRule::Result system_good_result,
+    MockDnsClientRule::Result public_good_result) {
+  DnsProbeService* service = io_thread_->globals()->dns_probe_service.get();
+  service->SetSystemClientForTesting(
+      CreateMockDnsClientForProbes(system_good_result));
+  service->SetPublicClientForTesting(
+      CreateMockDnsClientForProbes(public_good_result));
+}
+
+class DnsProbeBrowserTest : public InProcessBrowserTest {
+ public:
+  DnsProbeBrowserTest();
+
+  virtual void SetUpOnMainThread() OVERRIDE;
+  virtual void CleanUpOnMainThread() OVERRIDE;
+
+ protected:
+  void SetLinkDoctorBroken(bool broken);
+  void SetRules(MockDnsClientRule::Result system_good_result,
+                MockDnsClientRule::Result public_good_result);
+  void NavigateToDnsError();
+  void NavigateToOtherError();
+
+  bool TitleContains(const std::string& expected);
+  bool PageContains(const std::string& expected);
+
+ private:
+  void SetLinkDoctorNetError(int net_error);
+  void OnProbeStarted();
+
+  DnsProbeBrowserTestIOThreadHelper* helper_;
+};
+
+DnsProbeBrowserTest::DnsProbeBrowserTest()
+    : helper_(new DnsProbeBrowserTestIOThreadHelper()) {}
+
+void DnsProbeBrowserTest::SetUpOnMainThread() {
+  NetErrorTabHelper::set_state_for_testing(
+      NetErrorTabHelper::TESTING_FORCE_ENABLED);
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      Bind(&DnsProbeBrowserTestIOThreadHelper::SetUpOnIOThread,
+           Unretained(helper_),
+           g_browser_process->io_thread()));
+}
+
+void DnsProbeBrowserTest::CleanUpOnMainThread() {
+  NetErrorTabHelper::set_state_for_testing(
+      NetErrorTabHelper::TESTING_DEFAULT);
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      Bind(&DnsProbeBrowserTestIOThreadHelper::CleanUpOnIOThreadAndDeleteHelper,
+           Unretained(helper_)));

[mmenke, 2013/06/20 15:47:36]

nit:  Suggest reversing the order of setup.  Doesn't matter here, but think it's
a good habit to get into.  Otherwise, can miss cases where it does matter.

[Deprecated (see juliatuttle), 2013/06/21 19:55:52]

Done.

+}
+
+void DnsProbeBrowserTest::SetLinkDoctorBroken(bool broken) {

[mmenke, 2013/06/20 15:47:36]

Do we really need both this and SetLinkDoctorNetError?

[Deprecated (see juliatuttle), 2013/06/21 19:55:52]

No.

+  SetLinkDoctorNetError(broken ? net::ERR_NAME_NOT_RESOLVED : net::OK);
+}
+
+// These two functions wait for two navigations because Link Doctor loads two
+// pages: a blank page, so the user stops seeing the previous page, and then
+// either the Link Doctor page or a regular error page.  We want to wait for
+// the error page, so we wait for both loads to finish.
+
+void DnsProbeBrowserTest::NavigateToDnsError() {
+  NavigateToURLBlockUntilNavigationsComplete(
+      browser(),
+      URLRequestFailedJob::GetMockHttpUrl(net::ERR_NAME_NOT_RESOLVED),
+      2);
+}
+
+void DnsProbeBrowserTest::NavigateToOtherError() {
+  NavigateToURLBlockUntilNavigationsComplete(
+      browser(),
+      URLRequestFailedJob::GetMockHttpUrl(net::ERR_CONNECTION_REFUSED),
+      2);
+}
+
+void DnsProbeBrowserTest::SetRules(
+    MockDnsClientRule::Result system_good_result,
+    MockDnsClientRule::Result public_good_result) {
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      Bind(&DnsProbeBrowserTestIOThreadHelper::SetRules,
+           Unretained(helper_),
+           system_good_result,
+           public_good_result));
+}
+
+bool DnsProbeBrowserTest::TitleContains(const std::string& expected) {

[mmenke, 2013/06/20 15:47:36]

Think it's worth mentioning why this is used instead of WebContents::GetTitle or
a TitleWatcher.

[Deprecated (see juliatuttle), 2013/06/21 19:55:52]

Done.

+  std::string title;
+
+  WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  bool rv = content::ExecuteScriptAndExtractString(
+      contents,
+      "domAutomationController.send(document.title);",
+      &title);
+  if (!rv)
+    return false;
+
+  return title.find(expected) != std::string::npos;

[mmenke, 2013/06/20 15:47:36]

Is there a reason for not testing for an exact match?

[Deprecated (see juliatuttle), 2013/06/21 19:55:52]

No!  Done.

+}
+
+bool DnsProbeBrowserTest::PageContains(const std::string& expected) {
+  std::string text_content;
+
+  bool rv = content::ExecuteScriptAndExtractString(
+      browser()->tab_strip_model()->GetActiveWebContents(),
+      "domAutomationController.send(document.body.textContent);",
+      &text_content);
+  if (!rv)
+    return false;
+
+  return text_content.find(expected) != std::string::npos;
+}
+
+void DnsProbeBrowserTest::SetLinkDoctorNetError(int net_error) {
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      Bind(&DnsProbeBrowserTestIOThreadHelper::set_link_doctor_net_error,
+           Unretained(helper_),
+           net_error));
+}
+
+IN_PROC_BROWSER_TEST_F(DnsProbeBrowserTest, OtherErrorWithLinkDoctor) {
+  SetLinkDoctorBroken(false);
+
+  NavigateToOtherError();
+  EXPECT_TRUE(TitleContains("Mock Link Doctor"));

[mmenke, 2013/06/20 15:47:36]

Suggest adding a way to check if probes are queued and / or if a result is
cached, and checking them at the end of each test.

[Deprecated (see juliatuttle), 2013/06/21 19:55:52]

Done.

+}
+
+IN_PROC_BROWSER_TEST_F(DnsProbeBrowserTest, OtherErrorWithoutLinkDoctor) {
+  SetLinkDoctorBroken(true);
+
+  NavigateToOtherError();
+  EXPECT_TRUE(PageContains("CONNECTION_REFUSED"));
+}
+
+IN_PROC_BROWSER_TEST_F(DnsProbeBrowserTest, NxdomainWithLinkDoctor) {
+  SetLinkDoctorBroken(false);
+  SetRules(MockDnsClientRule::OK, MockDnsClientRule::OK);
+
+  NavigateToDnsError();
+  EXPECT_TRUE(TitleContains("Mock Link Doctor"));
+}
+
+IN_PROC_BROWSER_TEST_F(DnsProbeBrowserTest, NoInternetWithoutLinkDoctor) {
+  SetLinkDoctorBroken(true);
+  SetRules(MockDnsClientRule::TIMEOUT, MockDnsClientRule::TIMEOUT);
+
+  NavigateToDnsError();
+  // EXPECT_TRUE(PageContains("DNS_PROBE_STARTED"));
+  EXPECT_TRUE(PageContains("DNS_PROBE_FINISHED_NO_INTERNET"));

[mmenke, 2013/06/20 15:47:36]

Why are we guaranteed the probes have completed by this point?

[Deprecated (see juliatuttle), 2013/06/21 19:55:52]

We're not.  Fixed.

+}
+
+/* static const FilePath::CharType kIframeDnsErrorHtmlName[] =
+    FILE_PATH_LITERAL("iframe_dns_error.html");
+
+IN_PROC_BROWSER_TEST_F(DnsProbeBrowserTest, NoProbeInSubframe) {
+  SetLinkDoctorBroken(false);
+
+  NavigateToURL(
+      browser(),
+      URLRequestMockHTTPJob::GetMockUrl(FilePath(kIframeDnsErrorHtmlName)));
+  EXPECT_EQ(0, probe_count());
+} */

[mmenke, 2013/06/20 15:47:36]

You plan to enable this?

[Deprecated (see juliatuttle), 2013/06/21 19:55:52]

Done.

+

[mmenke, 2013/06/20 15:47:36]

Other test suggestion:  Check the case that DNS probes are disabled.

[Deprecated (see juliatuttle), 2013/06/21 19:55:52]

Done.

+}  // namespace
+
+}  // namespace chrome_browser_net