Fixed a bug with URLFetcher where retrying the request would append the new r...

chrome/common/net/url_fetcher_unittest.cc

Index: chrome/common/net/url_fetcher_unittest.cc
===================================================================
--- chrome/common/net/url_fetcher_unittest.cc	(revision 67561)
+++ chrome/common/net/url_fetcher_unittest.cc	(working copy)
@@ -228,6 +228,20 @@
   scoped_refptr<URLRequestContext> context_;
 };
 
+// Version of URLFetcherTest that tests retying the same request twice.
+class URLFetcherMultipleAttemptTest : public URLFetcherTest {
+ public:
+  // URLFetcher::Delegate
+  virtual void OnURLFetchComplete(const URLFetcher* source,
+                                  const GURL& url,
+                                  const URLRequestStatus& status,
+                                  int response_code,
+                                  const ResponseCookies& cookies,
+                                  const std::string& data);
+ private:
+   std::string data_;

[eroman, 2010/12/01 01:34:40]

nit: looks like there is one space too many here.

[sanjeevr, 2010/12/01 02:10:01]

Done.

+};
+
 // Wrapper that lets us call CreateFetcher() on a thread of our choice.  We
 // could make URLFetcherTest refcounted and use PostTask(FROM_HERE.. ) to call
 // CreateFetcher() directly, but the ownership of the URLFetcherTest is a bit
@@ -448,6 +462,31 @@
   // did not work.
 }
 
+void URLFetcherMultipleAttemptTest::OnURLFetchComplete(
+    const URLFetcher* source,
+    const GURL& url,
+    const URLRequestStatus& status,
+    int response_code,
+    const ResponseCookies& cookies,
+    const std::string& data) {
+  EXPECT_TRUE(status.is_success());
+  EXPECT_EQ(200, response_code);  // HTTP OK
+  EXPECT_FALSE(data.empty());
+  if (!data.empty() && data_.empty()) {
+    data_ = data;
+    fetcher_->Start();

[eroman, 2010/12/01 01:34:40]

It is not clear from the docs whether calling Start() multiple times is valid. I
understand why you do this for the unit-test, but would be better if you could
cause it to internally retry instead.

I am OK with leaving as is if you can't find a clean way to do that though.

[sanjeevr, 2010/12/01 02:10:01]

Other tests also use fetcher_->Start to retry. Besides, I use this in production
code too. Perhaps update the docs? Do you think I should change my production
code to not use Start() to retry?

[eroman, 2010/12/01 02:13:38]

ah ok, didn't realize it was a common pattern.

+  } else {
+    EXPECT_EQ(data, data_);
+    delete fetcher_;  // Have to delete this here and not in the destructor,
+                      // because the destructor won't necessarily run on the
+                      // same thread that CreateFetcher() did.
+
+    io_message_loop_proxy()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+    // If the current message loop is not the IO loop, it will be shut down when
+    // the main loop returns and this thread subsequently goes out of scope.
+  }
+}
+
 TEST_F(URLFetcherTest, SameThreadsTest) {
   net::TestServer test_server(net::TestServer::TYPE_HTTP, FilePath(kDocRoot));
   ASSERT_TRUE(test_server.Start());
@@ -629,4 +668,16 @@
   net::URLRequestThrottlerManager::GetInstance()->EraseEntryForTests(url);
 }
 
+TEST_F(URLFetcherMultipleAttemptTest, SameData) {
+  net::TestServer test_server(net::TestServer::TYPE_HTTP, FilePath(kDocRoot));
+  ASSERT_TRUE(test_server.Start());
+
+  // Create the fetcher on the main thread.  Since IO will happen on the main
+  // thread, this will test URLFetcher's ability to do everything on one
+  // thread.
+  CreateFetcher(test_server.GetURL("defaultresponse"));
+
+  MessageLoop::current()->Run();
+}
+
 }  // namespace.