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..091cbf529c9984de668f6e2f92d39e6069e8d744 100644
--- a/chrome/browser/errorpage_browsertest.cc
+++ b/chrome/browser/errorpage_browsertest.cc
@@ -26,11 +26,55 @@
 #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::URLRequestTestJob;
+using net::TestJobInterceptor;

[mmenke, 2014/02/13 20:42:17]

nit:  Alphabetize these two.

[Elly Fong-Jones, 2014/02/25 19:19:52]

Done.

+
+class TestProtocolHandler : public net::URLRequestJobFactory::ProtocolHandler {

[mmenke, 2014/02/13 20:42:17]

Think this should have a comment, and maybe a more informative name
(FailFirst[N]RequestsProtocolHandler, maybe?)

[Elly Fong-Jones, 2014/02/25 19:19:52]

Done.

+ public:
+  TestProtocolHandler(const GURL& url, int failure_threshold)
+      : url_(url), requests_(0), failures_(0),
+        failure_threshold_(failure_threshold) {}

[mmenke, 2014/02/13 20:42:17]

I think "threshold" is ambiguous.  Maybe "requests_to_fail"?

[Elly Fong-Jones, 2014/02/25 19:19:52]

Done.

+  virtual ~TestProtocolHandler() {}
+
+  virtual net::URLRequestJob* MaybeCreateJob(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const {

[mmenke, 2014/02/13 20:42:17]

nit:  OVERRIDE

[Elly Fong-Jones, 2014/02/25 19:19:52]

Done.

+    if (request->url() != url_)
+      return NULL;
+    requests_++;
+    if (failures_ < failure_threshold_) {
+      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 threshold() const { return failure_threshold_; }

[mmenke, 2014/02/13 20:42:17]

nit:  Don't think this one is needed?

[Elly Fong-Jones, 2014/02/25 19:19:52]

It is used in the tests below.

+
+ private:
+  GURL url_;
+  // these are mutable because MaybeCreateJob is const but we want this state
+  // for testing.

[Randy Smith (Not in Mondays), 2014/02/13 18:47:40]

Should it be declared const in this class as well, then?  and OVERRIDE?

[Elly Fong-Jones, 2014/02/25 19:19:52]

It is declared const here.

[Randy Smith (Not in Mondays), 2014/02/26 00:07:37]

Huh.  Weird.  I have no idea what I meant by that comment.  

+  mutable int requests_;
+  mutable int failures_;
+  int failure_threshold_;
+};
 
 namespace {
 
@@ -356,6 +400,75 @@ IN_PROC_BROWSER_TEST_F(ErrorPageTest, Page404) {
       1);
 }
 
+class ErrorPageAutoReloadTest : public InProcessBrowserTest {

[mmenke, 2014/02/13 20:42:17]

Need to update the command line to enable auto-reload in SetUpCommandLine.

+ public:
+  virtual void SetUpOnMainThread() OVERRIDE {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&ErrorPageAutoReloadTest::AddFilters));
+  }
+
+  virtual void CleanUpOnMainThread() OVERRIDE {
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        base::Bind(&ErrorPageAutoReloadTest::RemoveFilters));

[Randy Smith (Not in Mondays), 2014/02/13 18:47:40]

No objection, but you probably don't need this, since the process is being torn
down.

[Elly Fong-Jones, 2014/02/25 19:19:52]

Done.

+  }
+
+ protected:
+  void NavigateToURLAndWaitForTitle(const GURL& url,

[Randy Smith (Not in Mondays), 2014/02/13 18:47:40]

I wonder if this is being implemented in enough different places that we should
consolidate in a test utils class or something.  (No action on your part
necessary unless you feel really inspired.)

[Elly Fong-Jones, 2014/02/25 19:19:52]

Done.

+                                    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());
+  }
+
+  static std::string test_schema() { return "http"; }

[Randy Smith (Not in Mondays), 2014/02/13 18:47:40]

Why not use const char * constants?

[Elly Fong-Jones, 2014/02/25 19:19:52]

Done.

+  static std::string test_domain() { return "error.page.auto.reload"; }
+  static GURL test_url() {
+    return GURL(test_schema() + "://" + test_domain());
+  }
+  static TestProtocolHandler* protocol_handler() { return protocol_handler_; }
+
+ private:
+  static void AddFilters() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+    content::URLRequestFailedJob::AddUrlHandler();
+    protocol_handler_ = new TestProtocolHandler(test_url(), 2);

[Randy Smith (Not in Mondays), 2014/02/13 18:47:40]

I'd make the failure threshold configurable, either by the test (best) or by a
constant somewhere.

[Elly Fong-Jones, 2014/02/25 19:19:52]

Done.

+    // pass ownership of the pointer to the URLRequestFilter, which outlives
+    // this object, but keep a copy of it so tests can query its state.
+    scoped_ptr<net::URLRequestJobFactory::ProtocolHandler>
+        scoped_thing(protocol_handler_);

[Randy Smith (Not in Mondays), 2014/02/13 18:47:40]

I'd be inclined to just stuff the construction of the scoped pointer into the
argument to AddHostnameProtocolHandler, just so you can avoid the temporary
variable (and having to name it :-}). You can use
PassAs<net::URLRequestJobFactory::ProtocolHandler> to do the cast.

Also note that while the comment is true in spirit, because protocol_handler_ is
a static variable, it's not really relevant that URLRequestFilter will outlive
the class instance, and classes themselves live forever :-} (well, until the
process is killed).

[Elly Fong-Jones, 2014/02/25 19:19:52]

Done.

+
+    net::URLRequestFilter::GetInstance()->AddHostnameProtocolHandler(
+      test_schema(), test_domain(), scoped_thing.Pass());

[mmenke, 2014/02/13 20:42:17]

Think AddUrlProtocolHandler is simpler, and avoids any weirdness due to favicon
requests.  Those requests may make this test flaky, as-is.

[Elly Fong-Jones, 2014/02/25 19:19:52]

I rejiggered this entire class so the handler is installed in the protocol
handler itself, mirroring how the other test handlers work.

+  }
+
+  static void RemoveFilters() {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    net::URLRequestFilter::GetInstance()->ClearHandlers();
+  }
+
+  static TestProtocolHandler* protocol_handler_;

[Randy Smith (Not in Mondays), 2014/02/13 18:47:40]

Why static?  It seems like this could simply be a member variable on the class?

[Elly Fong-Jones, 2014/02/25 19:19:52]

These are all de-staticed now.

+

[Randy Smith (Not in Mondays), 2014/02/13 18:47:40]

nit: blank line.

[Elly Fong-Jones, 2014/02/25 19:19:52]

Done.

+};
+
+TestProtocolHandler* ErrorPageAutoReloadTest::protocol_handler_ = NULL;
+
+IN_PROC_BROWSER_TEST_F(ErrorPageAutoReloadTest, AutoReload) {
+  NavigateToURLAndWaitForTitle(test_url(), "Test One", 3);

[Randy Smith (Not in Mondays), 2014/02/13 18:47:40]

Are you going to (or have you and I haven't noticed) do something to
short-circuit the timeouts for the browser test?

[Elly Fong-Jones, 2014/02/25 19:19:52]

Hm. Currently they aren't short-circuited because the first couple are short
enough to not be bothersome, but maybe I should do that...

+  EXPECT_EQ(protocol_handler()->failures(), protocol_handler()->threshold());
+  EXPECT_EQ(protocol_handler()->requests(),
+            protocol_handler()->threshold() + 1);

[mmenke, 2014/02/13 20:42:17]

Should comment on thread safety here.

[Elly Fong-Jones, 2014/02/25 19:19:52]

Done.

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