Don't share ResourceDispatcherHostImpl's timer for reporting upload progress.

content/browser/loader/resource_loader_unittest.cc

Index: content/browser/loader/resource_loader_unittest.cc
diff --git a/content/browser/loader/resource_loader_unittest.cc b/content/browser/loader/resource_loader_unittest.cc
index 8be5ec1d0ebdc82e3b30c215e976ef44f6ca83e0..8110f0f1aa95e55f8bc099542f9badb8170cf5d1 100644
--- a/content/browser/loader/resource_loader_unittest.cc
+++ b/content/browser/loader/resource_loader_unittest.cc
@@ -14,6 +14,7 @@
 #include "content/browser/loader/resource_loader_delegate.h"
 #include "content/public/browser/client_certificate_delegate.h"
 #include "content/public/browser/resource_request_info.h"
+#include "content/public/common/content_paths.h"
 #include "content/public/common/resource_response.h"
 #include "content/public/test/mock_resource_context.h"
 #include "content/public/test/test_browser_context.h"
@@ -22,13 +23,16 @@
 #include "content/test/test_content_browser_client.h"
 #include "content/test/test_web_contents.h"
 #include "ipc/ipc_message.h"
+#include "net/base/elements_upload_data_stream.h"
 #include "net/base/io_buffer.h"
 #include "net/base/mock_file_stream.h"
 #include "net/base/net_errors.h"
 #include "net/base/request_priority.h"
+#include "net/base/upload_bytes_element_reader.h"
 #include "net/cert/x509_certificate.h"
 #include "net/ssl/client_cert_store.h"
 #include "net/ssl/ssl_cert_request_info.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_job_factory.h"
 #include "net/url_request/url_request_job_factory_impl.h"
@@ -175,7 +179,8 @@ class ResourceHandlerStub : public ResourceHandler {
         received_on_will_read_(false),
         received_eof_(false),
         received_response_completed_(false),
-        total_bytes_downloaded_(0) {
+        total_bytes_downloaded_(0),
+        upload_position_(0) {
   }
 
   // If true, defers the resource load in OnWillStart.
@@ -207,9 +212,19 @@ class ResourceHandlerStub : public ResourceHandler {
     controller()->Resume();
   }
 
+  // Waits until OnUploadProgress is called and returns the upload position.
+  uint64 WaitForUploadProgress() {
+    wait_for_progress_loop_.reset(new base::RunLoop());
+    wait_for_progress_loop_->Run();
+    wait_for_progress_loop_.reset();
+    return upload_position_;
+  }
+
   // ResourceHandler implementation:
   bool OnUploadProgress(uint64 position, uint64 size) override {
-    NOTREACHED();
+    upload_position_ = position;
+    if (wait_for_progress_loop_)
+      wait_for_progress_loop_->Quit();
     return true;
   }
 
@@ -301,6 +316,8 @@ class ResourceHandlerStub : public ResourceHandler {
   bool received_response_completed_;
   net::URLRequestStatus status_;
   int total_bytes_downloaded_;
+  scoped_ptr<base::RunLoop> wait_for_progress_loop_;
+  uint64 upload_position_;
 };
 
 // Test browser client that captures calls to SelectClientCertificates and
@@ -390,22 +407,10 @@ class ResourceLoaderTest : public testing::Test,
     return leaf_handler.Pass();
   }
 
-  void SetUp() override {
-    job_factory_.SetProtocolHandler("test", CreateProtocolHandler());
+  void SetUpResourceLoader(scoped_ptr<net::URLRequest> request) {

[mmenke, 2015/05/21 15:41:57]

Think this is worth a brief comment.  Just something along the lines of
"Replaces loader_ with a new one for the passed in request"

[Andre, 2015/05/21 20:59:31]

Done.

+    raw_ptr_to_request_ = request.get();
 
-    browser_context_.reset(new TestBrowserContext());
-    scoped_refptr<SiteInstance> site_instance =
-        SiteInstance::Create(browser_context_.get());
-    web_contents_.reset(
-        TestWebContents::Create(browser_context_.get(), site_instance.get()));
     RenderFrameHost* rfh = web_contents_->GetMainFrame();
-
-    scoped_ptr<net::URLRequest> request(
-        resource_context_.GetRequestContext()->CreateRequest(
-            test_url(),
-            net::DEFAULT_PRIORITY,
-            NULL /* delegate */));
-    raw_ptr_to_request_ = request.get();
     ResourceRequestInfo::AllocateForTesting(
         request.get(), RESOURCE_TYPE_MAIN_FRAME, &resource_context_,
         rfh->GetProcess()->GetID(), rfh->GetRenderViewHost()->GetRoutingID(),
@@ -421,6 +426,23 @@ class ResourceLoaderTest : public testing::Test,
         this));
   }
 
+  void SetUp() override {
+    job_factory_.SetProtocolHandler("test", CreateProtocolHandler());
+
+    browser_context_.reset(new TestBrowserContext());
+    scoped_refptr<SiteInstance> site_instance =
+        SiteInstance::Create(browser_context_.get());
+    web_contents_.reset(
+        TestWebContents::Create(browser_context_.get(), site_instance.get()));
+
+    scoped_ptr<net::URLRequest> request(
+        resource_context_.GetRequestContext()->CreateRequest(
+            test_url(),
+            net::DEFAULT_PRIORITY,
+            nullptr /* delegate */));
+    SetUpResourceLoader(request.Pass());
+  }
+
   void TearDown() override {
     // Destroy the WebContents and pump the event loop before destroying
     // |rvh_test_enabler_| and |thread_bundle_|. This lets asynchronous cleanup
@@ -640,6 +662,40 @@ TEST_F(ResourceLoaderTest, DeferEOF) {
             raw_ptr_resource_handler_->status().status());
 }
 
+TEST_F(ResourceLoaderTest, UploadProgress) {

[mmenke, 2015/05/21 15:41:57]

Could you add a TODO about adding a test for the redirect case?

[mmenke, 2015/05/21 15:41:57]

This test doesn't actually make sure the timer works (Which is my main concern)
- all it tests is that we get the final message when the upload is done.

That's why I was thinking we'd need to make a new class to slowly feed the data
along, and then make sure we get a couple intermediary upload progress messages.

[Andre, 2015/05/21 20:59:31]

Done. I wrapped ChunkedUploadDataStream to make it appear non-chunked.

+  // Set up a test server.
+  net::test_server::EmbeddedTestServer server;
+  ASSERT_TRUE(server.InitializeAndWaitUntilReady());
+  base::FilePath path;
+  PathService::Get(content::DIR_TEST_DATA, &path);
+  server.ServeFilesFromDirectory(path);
+
+  scoped_ptr<net::URLRequest> request(
+      resource_context_.GetRequestContext()->CreateRequest(
+          server.GetURL("/title1.html"),
+          net::DEFAULT_PRIORITY,
+          nullptr /* delegate */));
+
+  // Start an upload.
+  std::string upload_content("test");
+  scoped_ptr<net::UploadElementReader> reader(new net::UploadBytesElementReader(
+      upload_content.data(), upload_content.size()));
+  request->set_upload(
+      net::ElementsUploadDataStream::CreateWithReader(reader.Pass(), 0));
+
+  SetUpResourceLoader(request.Pass());
+  loader_->StartRequest();
+
+  // Expect at least one upload progress, and the last one must match the upload
+  // size.
+  while (true) {
+    uint64 position = raw_ptr_resource_handler_->WaitForUploadProgress();
+    EXPECT_LE(position, upload_content.size());

[mmenke, 2015/05/21 15:41:57]

Should also check that position is monotonically increasing.

[Andre, 2015/05/21 20:59:31]

Done. Added the check inside OnUploadProgress().

+    if (position == upload_content.size())
+      break;
+  }
+}
+
 class ResourceLoaderRedirectToFileTest : public ResourceLoaderTest {
  public:
   ResourceLoaderRedirectToFileTest()