Support auto-reload on errors.

chrome/browser/errorpage_browsertest.cc

Index: chrome/browser/errorpage_browsertest.cc
diff --git a/chrome/browser/errorpage_browsertest.cc b/chrome/browser/errorpage_browsertest.cc
index e2afcbf59b7c601ef1b81187426d68a6d2e86b55..56343875d5168526edef02a1991fe1cdc5a7ff6c 100644
--- a/chrome/browser/errorpage_browsertest.cc
+++ b/chrome/browser/errorpage_browsertest.cc
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -12,6 +13,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
@@ -26,11 +28,66 @@
 #include "net/base/net_errors.h"
 #include "net/base/net_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"
+#include "net/url_request/url_request_test_job.h"
+#include "net/url_request/url_request_test_util.h"
 
 using content::BrowserThread;
 using content::NavigationController;
 using content::URLRequestFailedJob;
+using net::TestJobInterceptor;
+using net::URLRequestJobFactory;
+using net::URLRequestTestJob;
+
+// A protocol handler that fails a configurable number of requests, then
+// succeeds all requests after that, keeping count of failures and successes.
+class FailFirstNRequestsProtocolHandler

[mmenke, 2014/02/25 22:09:52]

This should be in an anonymous namespace below.

[Elly Fong-Jones, 2014/03/03 19:31:07]

Done.

+    : public URLRequestJobFactory::ProtocolHandler {
+ public:
+  FailFirstNRequestsProtocolHandler(const GURL& url, int requests_to_fail)
+      : url_(url), requests_(0), failures_(0),
+        requests_to_fail_(requests_to_fail) {}
+  virtual ~FailFirstNRequestsProtocolHandler() {}
+
+  void AddUrlHandler() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

[mmenke, 2014/02/25 22:09:52]

Should include base/logging.h for DCHECK

[Elly Fong-Jones, 2014/03/03 19:31:07]

Done.

+    scoped_ptr<URLRequestJobFactory::ProtocolHandler> scoped_handler(this);

[mmenke, 2014/02/25 22:09:52]

Should also probably include the header for scoped_ptr, though we do get it from
the methods that take them in as arguments.

[Elly Fong-Jones, 2014/03/03 19:31:07]

Done.

+    net::URLRequestFilter::GetInstance()->AddUrlProtocolHandler(url_,
+        scoped_handler.Pass());
+  }
+
+  virtual net::URLRequestJob* MaybeCreateJob(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) OVERRIDE const {
+    if (request->url() != url_)
+      return NULL;

[mmenke, 2014/02/25 22:09:52]

This shouldn't be needed.  Maybe a DCHECK_EQ instead?

[Elly Fong-Jones, 2014/03/03 19:31:07]

Done.

+    requests_++;
+    if (failures_ < requests_to_fail_) {
+      failures_++;
+      return new URLRequestFailedJob(request,
+                                     network_delegate,
+                                     net::ERR_CONNECTION_RESET);
+    } else {
+      return new URLRequestTestJob(request, network_delegate,
+                                   URLRequestTestJob::test_headers(),
+                                   URLRequestTestJob::test_data_1(),
+                                   true);
+    }
+  }
+
+  int requests() const { return requests_; }
+  int failures() const { return failures_; }
+  int requests_to_fail() const { return requests_to_fail_; }
+
+ private:
+  GURL url_;

[mmenke, 2014/02/25 22:09:52]

maybe const?

[Elly Fong-Jones, 2014/03/03 19:31:07]

Done.

+  // these are mutable because MaybeCreateJob is const but we want this state
+  // for testing.
+  mutable int requests_;
+  mutable int failures_;
+  int requests_to_fail_;
+};
 
 namespace {
 
@@ -356,6 +413,68 @@ IN_PROC_BROWSER_TEST_F(ErrorPageTest, Page404) {
       1);
 }
 
+class ErrorPageAutoReloadTest : public InProcessBrowserTest {
+ public:
+  virtual void SetUpCommandLine(CommandLine* command_line) {
+    LOG(ERROR) << "SetUpCommandLine";
+    command_line->AppendSwitch(switches::kEnableOfflineAutoReload);
+  }
+
+  void InstallProtocolHandler(const GURL& url, int requests_to_fail) {
+    protocol_handler_ = new FailFirstNRequestsProtocolHandler(url,
+        requests_to_fail);
+    // Tests don't need to wait for this task to complete before using the
+    // filter; any requests that might be affected by it will end up in the IO
+    // thread's message loop after this posted task anyway.
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&ErrorPageAutoReloadTest::AddFilters,
+            base::Unretained(this)));
+  }
+
+ protected:

[mmenke, 2014/02/25 22:09:52]

nit:  I don't think protected really matters for test classes.  Also seems
arbitrary to make InstallProtocolHandler public and everything else protected.

[Elly Fong-Jones, 2014/03/03 19:31:07]

Done.

+  void NavigateToURLAndWaitForTitle(const GURL& url,
+                                    const std::string& expected_title,
+                                    int num_navigations) {
+    content::TitleWatcher title_watcher(
+        browser()->tab_strip_model()->GetActiveWebContents(),
+        base::ASCIIToUTF16(expected_title));
+
+    ui_test_utils::NavigateToURLBlockUntilNavigationsComplete(
+        browser(), url, num_navigations);
+
+    EXPECT_EQ(base::ASCIIToUTF16(expected_title),
+              title_watcher.WaitAndGetTitle());
+  }
+
+  FailFirstNRequestsProtocolHandler* protocol_handler() {
+    return protocol_handler_;
+  }
+
+ private:
+  void AddFilters() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    protocol_handler_->AddUrlHandler();
+  }
+
+  void RemoveFilters() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    net::URLRequestFilter::GetInstance()->ClearHandlers();
+  }
+
+  FailFirstNRequestsProtocolHandler* protocol_handler_;
+};
+
+IN_PROC_BROWSER_TEST_F(ErrorPageAutoReloadTest, AutoReload) {

[mmenke, 2014/02/25 22:09:52]

If we could do it non-racily, I'd love to have a test that checks auto-reload in
conjunction with the Link Doctor.  However, since we reload on a timer, and Link
Doctor involves a double-commit, I don't think that can currently be done.  We
could allow a variable number of commits, but that has flake written all over
it.

[Elly Fong-Jones, 2014/03/03 19:31:07]

Yeah. I thought about this, but my mind recoiled, so I decided not to do it.

+  GURL test_url("http://error.page.auto.reload");
+  InstallProtocolHandler(test_url, 2);
+  NavigateToURLAndWaitForTitle(test_url, "Test One", 3);
+  EXPECT_EQ(protocol_handler()->failures(),
+            protocol_handler()->requests_to_fail());
+  EXPECT_EQ(protocol_handler()->requests(),
+            protocol_handler()->requests_to_fail() + 1);

[mmenke, 2014/02/25 22:09:52]

nit: Both of these pairs are in the wrong order.  Expected should be before
actual.

[mmenke, 2014/02/25 22:09:52]

nit: I think this test is clearer if you have:

"const int kRequestsToFail = 2;"

And then use that instead of 2, 3, and requests_to_fail() (Which you can then
get rid of).

[mmenke, 2014/02/25 22:09:52]

I don't see a comment about it.  I just think it's non-obvious that these are
updated on one thread and read on another, and depend on requests being created
before navigation completes for correctness.

[Elly Fong-Jones, 2014/03/03 19:31:07]

Done.

[Elly Fong-Jones, 2014/03/03 19:31:07]

Done.

[Elly Fong-Jones, 2014/03/03 19:31:07]

Done.

+}
+
 // Returns Javascript code that executes plain text search for the page.
 // Pass into content::ExecuteScriptAndExtractBool as |script| parameter.
 std::string GetTextContentContainsStringScript(