Support auto-reload on errors.

chrome/renderer/net/net_error_helper_core_unittest.cc

Index: chrome/renderer/net/net_error_helper_core_unittest.cc
diff --git a/chrome/renderer/net/net_error_helper_core_unittest.cc b/chrome/renderer/net/net_error_helper_core_unittest.cc
index bd451d6d876d34ac999d08f39c6e166ec80c5704..484517d2a0b4e1f05b338674f21814c375429c5f 100644
--- a/chrome/renderer/net/net_error_helper_core_unittest.cc
+++ b/chrome/renderer/net/net_error_helper_core_unittest.cc
@@ -64,12 +64,44 @@ std::string NetErrorString(net::Error net_error) {
   return ErrorToString(NetError(net_error), false);
 }
 
+class MockOneShotTimer : public MockableOneShotTimer {

[mmenke, 2014/02/05 23:31:43]

Wonder if we should put this in its own file, and put them in a more accessible
location.

[Randy Smith (Not in Mondays), 2014/02/06 16:47:16]

The natural location is base, which is a pain to get things in.  But agreed it
would be a nice thing.  Maybe a separate CL, just so debates over where to put
it don't hold up this one?

+ public:
+  MockOneShotTimer() {}
+  virtual ~MockOneShotTimer() {}
+
+  virtual void Start(const tracked_objects::Location& posted_from,
+                     base::TimeDelta delay,
+                     const base::Closure& user_task) {

[mmenke, 2014/02/05 23:31:43]

EXPECT_FALSE(running_)

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+    user_task_ = user_task;
+    running_ = true;
+    delay_ = delay;
+  }
+
+  virtual void Stop() {
+    running_ = false;
+  }
+
+  virtual bool IsRunning() const {
+    return running_;
+  }
+
+  virtual void Fire() {

[mmenke, 2014/02/05 23:31:43]

EXPECT_TRUE(running_)

[Randy Smith (Not in Mondays), 2014/02/06 16:47:16]

Doesn't need to be virtual.

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+    running_ = false;
+    user_task_.Run();
+  }
+
+  base::TimeDelta delay_;
+  base::Closure user_task_;
+  bool running_;

[mmenke, 2014/02/05 23:31:43]

Style guide rather discourages public variables in classes.  Just add accessors
as needed.

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+};
+
 class NetErrorHelperCoreTest : public testing::Test,
                                public NetErrorHelperCore::Delegate {
  public:
   NetErrorHelperCoreTest() : core_(this),
                              update_count_(0),
-                             error_html_update_count_(0) {
+                             error_html_update_count_(0),
+                             last_one_shot_timer_(NULL) {
   }
 
   virtual ~NetErrorHelperCoreTest() {
@@ -82,12 +114,18 @@ class NetErrorHelperCoreTest : public testing::Test,
   const GURL& url_being_fetched() const { return url_being_fetched_; }
   bool is_url_being_fetched() const { return !url_being_fetched_.is_empty(); }
 
+  bool auto_reloading() const {
+    return auto_reloading_;
+  }
+
   const std::string& last_update_string() const { return last_update_string_; }
   int update_count() const { return update_count_;  }
 
   const std::string& last_error_html() const { return last_error_html_; }
   int error_html_update_count() const { return error_html_update_count_; }
 
+  MockOneShotTimer* last_one_shot_timer() { return last_one_shot_timer_; }
+
   void LinkDoctorLoadSuccess() {
     LinkDoctorLoadFinished(kLinkDoctorBody);
   }
@@ -96,6 +134,29 @@ class NetErrorHelperCoreTest : public testing::Test,
     LinkDoctorLoadFinished("");
   }
 
+ protected:
+  void ErrorLoad(net::Error error) {
+    core().OnStartLoad(NetErrorHelperCore::MAIN_FRAME,
+                       NetErrorHelperCore::NON_ERROR_PAGE);
+    std::string html;
+    core().GetErrorHTML(NetErrorHelperCore::MAIN_FRAME,
+                        NetError(error), false, &html);
+    EXPECT_FALSE(html.empty());
+    EXPECT_EQ(NetErrorString(error), html);
+
+    core().OnStartLoad(NetErrorHelperCore::MAIN_FRAME,
+                       NetErrorHelperCore::ERROR_PAGE);
+    core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME);
+    core().OnFinishLoad(NetErrorHelperCore::MAIN_FRAME);
+  }
+
+  void SuccessLoad() {

[mmenke, 2014/02/05 23:31:43]

Maybe RunSuccessfulLoad()?  DoSuccessfulLoad()?  MockSuccessfulLoad()?  Method
names should generally be verbs.  Same goes for ErrorLoad.

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+    core().OnStartLoad(NetErrorHelperCore::MAIN_FRAME,
+                       NetErrorHelperCore::NON_ERROR_PAGE);
+    core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME);
+    core().OnFinishLoad(NetErrorHelperCore::MAIN_FRAME);
+  }
+
  private:
   void LinkDoctorLoadFinished(const std::string& result) {
     url_being_fetched_ = GURL();
@@ -133,6 +194,15 @@ class NetErrorHelperCoreTest : public testing::Test,
     url_being_fetched_ = GURL();
   }
 
+  virtual void ReloadPage() OVERRIDE {
+    auto_reloading_ = true;

[mmenke, 2014/02/05 23:31:43]

Having this and core()->IsAutoReloading is very confusing.  I suggest calling
this is_reloading_.  It's also never reset, which seems weird.  It's a pain, but
maybe make all calls to core() go through the test fixture, and reset it on
commit or something?  Open to other ideas.

[mmenke, 2014/02/05 23:31:43]

Suggest keeping track of how often this is called, and making sure there are no
extra reloads.

Could do the same for timers, but I think that's overkill.

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+  }
+
+  virtual MockableOneShotTimer* NewMockableOneShotTimer() {
+    last_one_shot_timer_ = new MockOneShotTimer();
+    return last_one_shot_timer_;
+  }
+
   NetErrorHelperCore core_;
 
   GURL url_being_fetched_;
@@ -147,6 +217,10 @@ class NetErrorHelperCoreTest : public testing::Test,
   std::string last_error_html_;
   // Number of times |last_error_html_| has been changed.
   int error_html_update_count_;
+
+  MockOneShotTimer* last_one_shot_timer_;
+
+  bool auto_reloading_;
 };
 
 //------------------------------------------------------------------------------
@@ -1302,3 +1376,116 @@ TEST_F(NetErrorHelperCoreTest, LinkDoctorStopped) {
             last_error_html());
   EXPECT_EQ(1, error_html_update_count());
 }
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadDisabled) {
+  core().set_auto_reload_enabled(false);
+  ErrorLoad(net::ERR_CONNECTION_RESET);
+
+  EXPECT_EQ(NULL, last_one_shot_timer());
+  EXPECT_FALSE(auto_reloading());

[mmenke, 2014/02/05 23:31:43]

EXPECT_FALSE(core().IsAutoReloading());?

[Elly Fong-Jones, 2014/02/10 21:42:06]

Removed that function altogether.

+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadSucceeds) {
+  core().set_auto_reload_enabled(true);
+  ErrorLoad(net::ERR_CONNECTION_RESET);
+
+  ASSERT_TRUE(last_one_shot_timer());
+  EXPECT_TRUE(last_one_shot_timer()->IsRunning());
+  EXPECT_FALSE(auto_reloading());
+
+  last_one_shot_timer()->Fire();
+  EXPECT_FALSE(last_one_shot_timer()->IsRunning());
+  EXPECT_TRUE(core().IsAutoReloading());
+  EXPECT_TRUE(auto_reloading());
+
+  SuccessLoad();
+
+  EXPECT_FALSE(last_one_shot_timer()->IsRunning());

[mmenke, 2014/02/05 23:31:43]

EXPECT_FALSE(core().IsAutoReloading());?  (At the end of all of these, or true
on the ones where it's running)

[Elly Fong-Jones, 2014/02/10 21:42:06]

Removed it.

+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadRetries) {
+  core().set_auto_reload_enabled(true);
+  ErrorLoad(net::ERR_CONNECTION_RESET);
+
+  base::TimeDelta delay;

[mmenke, 2014/02/05 23:31:43]

Can declare this on the line where you set it.

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+  EXPECT_TRUE(last_one_shot_timer());
+  EXPECT_TRUE(last_one_shot_timer()->IsRunning());
+  delay = last_one_shot_timer()->delay_;

[mmenke, 2014/02/05 23:31:43]

Maybe first_delay?

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+  EXPECT_FALSE(auto_reloading());
+
+  last_one_shot_timer()->Fire();
+  EXPECT_FALSE(last_one_shot_timer()->IsRunning());
+  EXPECT_TRUE(auto_reloading());
+
+  ErrorLoad(net::ERR_CONNECTION_RESET);
+
+  EXPECT_TRUE(last_one_shot_timer()->IsRunning());
+  EXPECT_NE(last_one_shot_timer()->delay_, delay);

[mmenke, 2014/02/05 23:31:43]

EXPECT_GT?

[Randy Smith (Not in Mondays), 2014/02/06 16:47:16]

Test to confirm we eventually level off (i.e. probing at the saturation code)?

+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadStopsOnSuccess) {

[mmenke, 2014/02/05 23:31:43]

This seems to be a subset of AutoReloadSucceeds

[Elly Fong-Jones, 2014/02/10 21:42:06]

Haha, so it is. Removed.

+  core().set_auto_reload_enabled(true);
+  ErrorLoad(net::ERR_CONNECTION_RESET);
+  last_one_shot_timer()->Fire();
+  EXPECT_TRUE(auto_reloading());
+  SuccessLoad();
+  EXPECT_FALSE(last_one_shot_timer()->IsRunning());
+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadStopsTimerOnStop) {
+  core().set_auto_reload_enabled(true);
+  ErrorLoad(net::ERR_CONNECTION_RESET);
+  EXPECT_TRUE(last_one_shot_timer()->IsRunning());
+  core().OnStop();
+  EXPECT_FALSE(last_one_shot_timer()->IsRunning());

[Randy Smith (Not in Mondays), 2014/02/06 16:47:16]

Should we confirm we do/don't (don't remember the behavior) reset the
auto_reload_count_ here?

+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadStopsLoadingOnStop) {
+  core().set_auto_reload_enabled(true);
+  ErrorLoad(net::ERR_CONNECTION_RESET);
+  last_one_shot_timer()->Fire();
+  EXPECT_TRUE(auto_reloading());
+  core().OnStop();

[mmenke, 2014/02/05 23:31:43]

This doesn't seem to test anything.  auto_reloading() will still be true at the
end of this test.

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadResetsCountOnSuccess) {
+  core().set_auto_reload_enabled(true);
+  ErrorLoad(net::ERR_CONNECTION_RESET);
+  base::TimeDelta delay = last_one_shot_timer()->delay_;
+  last_one_shot_timer()->Fire();
+  SuccessLoad();
+  ErrorLoad(net::ERR_CONNECTION_RESET);
+  EXPECT_EQ(last_one_shot_timer()->delay_, delay);

[mmenke, 2014/02/05 23:31:43]

Should make sure the original timer was stopped and a new one was started, or
just make sure the second reload can be triggered, by triggering the time
again..

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadStopsOnOtherLoadStart) {

[mmenke, 2014/02/05 23:31:43]

AutoReloadResetsCountOnSuccess assumes this test passes, so I suggest putting it
first (Admittedly, tests are now run out of order on the bots, but still think
it makes sense to roughly order tests in terms of which tests they are assuming
pass).

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+  core().set_auto_reload_enabled(true);
+  ErrorLoad(net::ERR_CONNECTION_RESET);
+  EXPECT_TRUE(last_one_shot_timer()->IsRunning());
+  core().OnStartLoad(NetErrorHelperCore::MAIN_FRAME,
+                     NetErrorHelperCore::NON_ERROR_PAGE);
+  EXPECT_FALSE(last_one_shot_timer()->IsRunning());

[Randy Smith (Not in Mondays), 2014/02/06 16:47:16]

Confirm the right thing has happened to the auto-reload count?

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.

+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadRestartsOnNetworkStateChanged) {
+  core().set_auto_reload_enabled(true);
+  ErrorLoad(net::ERR_CONNECTION_RESET);
+  base::TimeDelta delay = last_one_shot_timer()->delay_;
+  last_one_shot_timer()->Fire();
+  ErrorLoad(net::ERR_CONNECTION_RESET);
+  EXPECT_TRUE(last_one_shot_timer()->IsRunning());
+  EXPECT_NE(delay, last_one_shot_timer()->delay_);
+  core().NetworkStateChanged(true);
+  EXPECT_TRUE(last_one_shot_timer()->IsRunning());
+  EXPECT_EQ(delay, last_one_shot_timer()->delay_);
+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadDoesNotStartOnNetworkStateChanged) {
+  core().set_auto_reload_enabled(true);
+  ErrorLoad(net::ERR_CONNECTION_RESET);
+  last_one_shot_timer()->Fire();
+  SuccessLoad();
+  EXPECT_FALSE(last_one_shot_timer()->IsRunning());
+  core().NetworkStateChanged(true);
+  EXPECT_FALSE(last_one_shot_timer()->IsRunning());
+}

[mmenke, 2014/02/05 23:31:43]

Other test suggestions:

No auto reload on network state change after Stop().

Test in combination with a successful Link Doctor load (I...think we use it in
this case?)

Test in combination with Link Doctor failing to load (Again, I think we'll use
it here, with the current code.  If not, we probably should, but fine to worry
about later, I'm reworking the Link Doctor code anyways).

Test in combination with DNS probe results (Display DNS probe results, then
reload, then get new DNS probe results, which we should display, rather than the
original results.

[Elly Fong-Jones, 2014/02/10 21:42:06]

Done.