DownloadBuffer is used in two different ways, only one of which is used across threads.

This CL splits a ContentVector typedef from the DownloadBuffer class.

DownloadBuffer is also made RefCountedThreadSafe.

BUG=None
TEST=None


Review URL: http://codereview.chromium.org/8036038

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@106640 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 292 insertions, 58 deletions

diff --git a/content/browser/download/download_buffer.cc b/content/browser/download/download_buffer.cc
new file mode 100644
index 0000000..eadb60b
--- /dev/null
+++ b/content/browser/download/download_buffer.cc
@@ -0,0 +1,71 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/download/download_buffer.h"
+
+#include "net/base/io_buffer.h"
+
+namespace content {
+
+net::IOBuffer* AssembleData(const ContentVector& contents, size_t* num_bytes) {
+  if (*num_bytes)
+    *num_bytes = 0;
+
+  size_t data_len;
+  // Determine how large a buffer we need.
+  size_t assembly_buffer_length = 0;
+  for (size_t i = 0; i < contents.size(); ++i) {
+    data_len = contents[i].second;
+    assembly_buffer_length += data_len;
+  }
+
+  // 0-length IOBuffers are not allowed.
+  if (assembly_buffer_length == 0)
+    return NULL;
+
+  net::IOBuffer* assembly_buffer = new net::IOBuffer(assembly_buffer_length);
+
+  // Copy the data into |assembly_buffer|.
+  size_t bytes_copied = 0;
+  for (size_t i = 0; i < contents.size(); ++i) {
+    net::IOBuffer* data = contents[i].first;
+    data_len = contents[i].second;
+    DCHECK(data != NULL);
+    DCHECK_LE(bytes_copied + data_len, assembly_buffer_length);
+    memcpy(assembly_buffer->data() + bytes_copied, data->data(), data_len);
+    bytes_copied += data_len;
+  }
+  DCHECK_EQ(assembly_buffer_length, bytes_copied);
+
+  if (num_bytes)
+    *num_bytes = assembly_buffer_length;
+
+  return assembly_buffer;
+}
+
+DownloadBuffer::DownloadBuffer() {
+}
+
+DownloadBuffer::~DownloadBuffer() {
+}
+
+size_t DownloadBuffer::AddData(net::IOBuffer* io_buffer, size_t byte_count) {
+  base::AutoLock auto_lock(lock_);
+  contents_.push_back(std::make_pair(io_buffer, byte_count));
+  return contents_.size();
+}
+
+ContentVector* DownloadBuffer::ReleaseContents() {
+  base::AutoLock auto_lock(lock_);
+  ContentVector* other_contents = new ContentVector;
+  other_contents->swap(contents_);
+  return other_contents;
+}
+
+size_t DownloadBuffer::size() const {
+  base::AutoLock auto_lock(lock_);
+  return contents_.size();
+}
+
+}  // namespace content
diff --git a/content/browser/download/download_buffer.h b/content/browser/download/download_buffer.h
new file mode 100644
index 0000000..acbd28a
--- /dev/null
+++ b/content/browser/download/download_buffer.h
@@ -0,0 +1,70 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_BUFFER_H_
+#define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_BUFFER_H_
+#pragma once
+
+#include <vector>
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/synchronization/lock.h"
+#include "content/common/content_export.h"
+
+namespace net {
+class IOBuffer;
+}
+
+namespace content {
+
+typedef std::pair<scoped_refptr<net::IOBuffer>, size_t> ContentElement;
+typedef std::vector<ContentElement> ContentVector;
+
+// Helper function for ContentVector.
+// Assembles the data from |contents| and returns it in an |IOBuffer|.
+// The number of bytes in the |IOBuffer| is returned in |*num_bytes|
+// (if |num_bytes| is not NULL).
+// If |contents| is empty, returns NULL as an |IOBuffer| is not allowed
+// to have 0 bytes.
+net::IOBuffer* AssembleData(const ContentVector& contents, size_t* num_bytes);
+
+// |DownloadBuffer| is a thread-safe wrapper around |ContentVector|.
+//
+// It is created and populated on the IO thread, and passed to the
+// FILE thread for writing. In order to avoid flooding the FILE thread with
+// too many small write messages, each write is appended to the
+// |DownloadBuffer| while waiting for the task to run on the FILE
+// thread. Access to the write buffers is synchronized via the lock.
+class CONTENT_EXPORT DownloadBuffer :
+    public base::RefCountedThreadSafe<DownloadBuffer> {
+ public:
+  DownloadBuffer();
+
+  // Adds data to the buffers.
+  // Returns the number of |IOBuffer|s in the ContentVector.
+  size_t AddData(net::IOBuffer* io_buffer, size_t byte_count);
+
+  // Retrieves the ContentVector of buffers, clearing the contents.
+  // The caller takes ownership.
+  ContentVector* ReleaseContents();
+
+  // Gets the number of |IOBuffers| we have.
+  size_t size() const;
+
+ private:
+  friend class base::RefCountedThreadSafe<DownloadBuffer>;
+
+  // Do not allow explicit destruction by anyone else.
+  ~DownloadBuffer();
+
+  mutable base::Lock lock_;
+  ContentVector contents_;
+
+  DISALLOW_COPY_AND_ASSIGN(DownloadBuffer);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_BUFFER_H_
diff --git a/content/browser/download/download_buffer_unittest.cc b/content/browser/download/download_buffer_unittest.cc
new file mode 100644
index 0000000..137e612
--- /dev/null
+++ b/content/browser/download/download_buffer_unittest.cc
@@ -0,0 +1,123 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/download/download_buffer.h"
+
+#include <string.h>
+
+#include "net/base/io_buffer.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace content {
+
+namespace {
+
+const char* kTestData[] = {
+  "Four score and twenty years ago",
+  "Our four (or five) fathers",
+  "Danced and sang in the woods"
+};
+const size_t kTestDataQty = arraysize(kTestData);
+
+class DownloadBufferTest : public testing::Test {
+ public:
+  DownloadBufferTest() {
+  }
+  ~DownloadBufferTest() {
+  }
+
+  void CreateBuffer() {
+    EXPECT_EQ(0u, content_buffer_.size());
+
+    for (size_t i = 0; i < kTestDataQty; ++i) {
+      net::StringIOBuffer* io_buffer = new net::StringIOBuffer(kTestData[i]);
+      content_buffer_.push_back(std::make_pair(io_buffer, io_buffer->size()));
+      EXPECT_EQ(i + 1, content_buffer_.size());
+    }
+  }
+
+  ContentVector* content_buffer() {
+    return &content_buffer_;
+  }
+
+ private:
+  ContentVector content_buffer_;
+};
+
+TEST_F(DownloadBufferTest, CreateContentVector) {
+  CreateBuffer();
+
+  ContentVector* contents = content_buffer();
+
+  EXPECT_EQ(kTestDataQty, content_buffer()->size());
+  EXPECT_EQ(kTestDataQty, contents->size());
+
+  for (size_t i = 0; i < kTestDataQty; ++i) {
+    size_t io_buffer_size = strlen(kTestData[i]);
+    EXPECT_STREQ(kTestData[i], (*contents)[i].first->data());
+    EXPECT_EQ(io_buffer_size, (*contents)[i].second);
+  }
+}
+
+TEST_F(DownloadBufferTest, CreateDownloadBuffer) {
+  scoped_refptr<DownloadBuffer> content_buffer(new DownloadBuffer);
+  EXPECT_EQ(0u, content_buffer->size());
+
+  for (size_t i = 0; i < kTestDataQty; ++i) {
+    net::StringIOBuffer* io_buffer = new net::StringIOBuffer(kTestData[i]);
+    EXPECT_EQ(i + 1, content_buffer->AddData(io_buffer, io_buffer->size()));
+  }
+  scoped_ptr<ContentVector> contents(content_buffer->ReleaseContents());
+
+  EXPECT_EQ(0u, content_buffer->size());
+  EXPECT_EQ(kTestDataQty, contents->size());
+
+  for (size_t i = 0; i < kTestDataQty; ++i) {
+    size_t io_buffer_size = strlen(kTestData[i]);
+    EXPECT_STREQ(kTestData[i], (*contents)[i].first->data());
+    EXPECT_EQ(io_buffer_size, (*contents)[i].second);
+  }
+}
+
+TEST_F(DownloadBufferTest, AssembleData) {
+  CreateBuffer();
+
+  size_t assembled_bytes = 0;
+  scoped_refptr<net::IOBuffer> assembled_buffer =
+      AssembleData(*content_buffer(), &assembled_bytes);
+
+  // Did not change the content buffer.
+  EXPECT_EQ(kTestDataQty, content_buffer()->size());
+
+  // Verify the data.
+  size_t total_size = 0;
+  for (size_t i = 0; i < kTestDataQty; ++i) {
+    size_t len = strlen(kTestData[i]);
+    // assembled_buffer->data() may not be NULL-terminated, so we can't use
+    // EXPECT_STREQ().
+    EXPECT_EQ(
+        0, memcmp(kTestData[i], assembled_buffer->data() + total_size, len));
+    total_size += len;
+  }
+  // Verify the data length.
+  EXPECT_EQ(total_size, assembled_bytes);
+}
+
+TEST_F(DownloadBufferTest, AssembleDataWithEmptyBuffer) {
+  ContentVector buffer;
+  EXPECT_EQ(0u, buffer.size());
+
+  size_t assembled_bytes = 0;
+  scoped_refptr<net::IOBuffer> assembled_buffer =
+      AssembleData(buffer, &assembled_bytes);
+
+  // Did not change the content buffer.
+  EXPECT_EQ(0u, buffer.size());
+  EXPECT_EQ(0u, assembled_bytes);
+  EXPECT_TRUE(NULL == assembled_buffer.get());
+}
+
+}  // namespace
+
+}  // namespace content
diff --git a/content/browser/download/download_file_manager.cc b/content/browser/download/download_file_manager.cc
index 1444db3..e5a7285 100644
--- a/content/browser/download/download_file_manager.cc
+++ b/content/browser/download/download_file_manager.cc
@@ -10,6 +10,7 @@
 #include "base/task.h"
 #include "base/utf_string_conversions.h"
 #include "content/browser/browser_thread.h"
+#include "content/browser/download/download_buffer.h"
 #include "content/browser/download/download_file.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_manager.h"
@@ -143,19 +144,15 @@ void DownloadFileManager::StartDownload(DownloadCreateInfo* info) {
 // thread gets the cancel message: we just delete the data since the
 // DownloadFile has been deleted.
 void DownloadFileManager::UpdateDownload(
-    DownloadId global_id, DownloadBuffer* buffer) {
+    DownloadId global_id, content::DownloadBuffer* buffer) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  std::vector<DownloadBuffer::Contents> contents;
-  {
-    base::AutoLock auto_lock(buffer->lock);
-    contents.swap(buffer->contents);
-  }
+  scoped_ptr<content::ContentVector> contents(buffer->ReleaseContents());
 
   DownloadFile* download_file = GetDownloadFile(global_id);
   bool had_error = false;
-  for (size_t i = 0; i < contents.size(); ++i) {
-    net::IOBuffer* data = contents[i].first;
-    const int data_len = contents[i].second;
+  for (size_t i = 0; i < contents->size(); ++i) {
+    net::IOBuffer* data = (*contents)[i].first;
+    const int data_len = (*contents)[i].second;
     if (!had_error && download_file) {
       net::Error write_result =
           download_file->AppendDataToFile(data->data(), data_len);
@@ -192,14 +189,12 @@ void DownloadFileManager::UpdateDownload(
 
 void DownloadFileManager::OnResponseCompleted(
     DownloadId global_id,
-    DownloadBuffer* buffer,
     net::Error net_error,
     const std::string& security_info) {
   VLOG(20) << __FUNCTION__ << "()" << " id = " << global_id
            << " net_error = " << net_error
            << " security_info = \"" << security_info << "\"";
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  delete buffer;
   DownloadFile* download_file = GetDownloadFile(global_id);
   if (!download_file)
     return;
diff --git a/content/browser/download/download_file_manager.h b/content/browser/download/download_file_manager.h
index 20a2b6a..715d791 100644
--- a/content/browser/download/download_file_manager.h
+++ b/content/browser/download/download_file_manager.h
@@ -54,7 +54,6 @@
 #include "net/base/net_errors.h"
 #include "ui/gfx/native_widget_types.h"
 
-struct DownloadBuffer;
 struct DownloadCreateInfo;
 struct DownloadSaveInfo;
 class DownloadFile;
@@ -63,6 +62,10 @@ class FilePath;
 class GURL;
 class ResourceDispatcherHost;
 
+namespace content {
+class DownloadBuffer;
+}
+
 namespace net {
 class URLRequestContextGetter;
 }
@@ -81,14 +84,14 @@ class CONTENT_EXPORT DownloadFileManager
 
   // Handlers for notifications sent from the IO thread and run on the
   // FILE thread.
-  void UpdateDownload(DownloadId global_id, DownloadBuffer* buffer);
+  void UpdateDownload(DownloadId global_id, content::DownloadBuffer* buffer);
+
   // |net_error| is 0 for normal completions, and non-0 for errors.
   // |security_info| contains SSL information (cert_id, cert_status,
   // security_bits, ssl_connection_status), which can be used to
   // fine-tune the error message.  It is empty if the transaction
   // was not performed securely.
   void OnResponseCompleted(DownloadId global_id,
-                           DownloadBuffer* buffer,
                            net::Error net_error,
                            const std::string& security_info);
 
diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc
index e726bfb..8e98cb7 100644
--- a/content/browser/download/download_resource_handler.cc
+++ b/content/browser/download/download_resource_handler.cc
@@ -11,6 +11,7 @@
 #include "base/metrics/stats_counters.h"
 #include "base/stringprintf.h"
 #include "content/browser/browser_thread.h"
+#include "content/browser/download/download_buffer.h"
 #include "content/browser/download/download_create_info.h"
 #include "content/browser/download/download_file_manager.h"
 #include "content/browser/download/download_item.h"
@@ -46,7 +47,7 @@ DownloadResourceHandler::DownloadResourceHandler(
       save_as_(save_as),
       started_cb_(started_cb),
       save_info_(save_info),
-      buffer_(new DownloadBuffer),
+      buffer_(new content::DownloadBuffer),
       rdh_(rdh),
       is_paused_(false) {
   DCHECK(dl_id.IsValid());
@@ -162,25 +163,25 @@ bool DownloadResourceHandler::OnReadCompleted(int request_id, int* bytes_read) {
   if (!*bytes_read)
     return true;
   DCHECK(read_buffer_);
-  base::AutoLock auto_lock(buffer_->lock);
-  bool need_update = buffer_->contents.empty();
+  // Swap the data.
+  net::IOBuffer* io_buffer = NULL;
+  read_buffer_.swap(&io_buffer);
+  size_t vector_size = buffer_->AddData(io_buffer, *bytes_read);
+  bool need_update = (vector_size == 1);  // Buffer was empty.
 
   // We are passing ownership of this buffer to the download file manager.
-  net::IOBuffer* buffer = NULL;
-  read_buffer_.swap(&buffer);
-  buffer_->contents.push_back(std::make_pair(buffer, *bytes_read));
   if (need_update) {
     BrowserThread::PostTask(
         BrowserThread::FILE, FROM_HERE,
         NewRunnableMethod(download_file_manager_,
                           &DownloadFileManager::UpdateDownload,
                           download_id_,
-                          buffer_.get()));
+                          buffer_));
   }
 
   // We schedule a pause outside of the read loop if there is too much file
   // writing work to do.
-  if (buffer_->contents.size() > kLoadsToWrite)
+  if (vector_size > kLoadsToWrite)
     StartPauseTimer();
 
   return true;
@@ -206,9 +207,9 @@ bool DownloadResourceHandler::OnResponseCompleted(
       NewRunnableMethod(download_file_manager_,
                         &DownloadFileManager::OnResponseCompleted,
                         download_id_,
-                        buffer_.release(),
                         error_code,
                         security_info));
+  buffer_ = NULL;  // The buffer is longer needed by |DownloadResourceHandler|.
   read_buffer_ = NULL;
   return true;
 }
@@ -236,11 +237,7 @@ void DownloadResourceHandler::CheckWriteProgress() {
   if (!buffer_.get())
     return;  // The download completed while we were waiting to run.
 
-  size_t contents_size;
-  {
-    base::AutoLock lock(buffer_->lock);
-    contents_size = buffer_->contents.size();
-  }
+  size_t contents_size = buffer_->size();
 
   bool should_pause = contents_size > kLoadsToWrite;
 
diff --git a/content/browser/download/download_resource_handler.h b/content/browser/download/download_resource_handler.h
index ef3ec8f..a7dc77c 100644
--- a/content/browser/download/download_resource_handler.h
+++ b/content/browser/download/download_resource_handler.h
@@ -18,7 +18,10 @@
 
 class DownloadFileManager;
 class ResourceDispatcherHost;
-struct DownloadBuffer;
+
+namespace content {
+class DownloadBuffer;
+}
 
 namespace net {
 class URLRequest;
@@ -95,7 +98,7 @@ class DownloadResourceHandler : public ResourceHandler {
   bool save_as_;  // Request was initiated via "Save As" by the user.
   OnStartedCallback started_cb_;
   DownloadSaveInfo save_info_;
-  scoped_ptr<DownloadBuffer> buffer_;
+  scoped_refptr<content::DownloadBuffer> buffer_;
   ResourceDispatcherHost* rdh_;
   bool is_paused_;
   base::OneShotTimer<DownloadResourceHandler> pause_timer_;
diff --git a/content/browser/download/download_types.cc b/content/browser/download/download_types.cc
index b54d7f2..007ca56 100644
--- a/content/browser/download/download_types.cc
+++ b/content/browser/download/download_types.cc
@@ -4,12 +4,6 @@
 
 #include "content/browser/download/download_types.h"
 
-DownloadBuffer::DownloadBuffer() {
-}
-
-DownloadBuffer::~DownloadBuffer() {
-}
-
 DownloadSaveInfo::DownloadSaveInfo() {
 }
 
diff --git a/content/browser/download/download_types.h b/content/browser/download/download_types.h
index 7ee8043..f0153a5 100644
--- a/content/browser/download/download_types.h
+++ b/content/browser/download/download_types.h
@@ -6,33 +6,11 @@
 #define CONTENT_BROWSER_DOWNLOAD_DOWNLOAD_TYPES_H_
 #pragma once
 
-#include <vector>
-
 #include "base/file_path.h"
 #include "base/memory/linked_ptr.h"
-#include "base/synchronization/lock.h"
 #include "content/common/content_export.h"
 #include "net/base/file_stream.h"
 
-namespace net {
-class IOBuffer;
-}
-
-// DownloadBuffer is created and populated on the IO thread, and passed to the
-// file thread for writing. In order to avoid flooding the file thread with too
-// many small write messages, each write is appended to the DownloadBuffer while
-// waiting for the task to run on the file thread. Access to the write buffers
-// is synchronized via the lock. Each entry in 'contents' represents one data
-// buffer and its size in bytes.
-struct CONTENT_EXPORT DownloadBuffer {
-  DownloadBuffer();
-  ~DownloadBuffer();
-
-  base::Lock lock;
-  typedef std::pair<net::IOBuffer*, int> Contents;
-  std::vector<Contents> contents;
-};
-
 // Holds the information about how to save a download file.
 struct CONTENT_EXPORT DownloadSaveInfo {
   DownloadSaveInfo();