Migrate HttpCache adn HttpCacheTransaction to base::Bind().

BUG=none
TEST=existing

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

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

6 files changed, 80 insertions, 138 deletions

diff --git a/net/http/http_cache.cc b/net/http/http_cache.cc
index a638326..9071b1b 100644
--- a/net/http/http_cache.cc
+++ b/net/http/http_cache.cc
@@ -119,13 +119,13 @@ HttpCache::ActiveEntry::~ActiveEntry() {
 // This structure keeps track of work items that are attempting to create or
 // open cache entries or the backend itself.
 struct HttpCache::PendingOp {
-  PendingOp() : disk_entry(NULL), backend(NULL), writer(NULL), callback(NULL) {}
+  PendingOp() : disk_entry(NULL), backend(NULL), writer(NULL) {}
   ~PendingOp() {}
 
   disk_cache::Entry* disk_entry;
   disk_cache::Backend* backend;
   WorkItem* writer;
-  OldCompletionCallback* callback;  // BackendCallback.
+  CompletionCallback callback;  // BackendCallback.
   WorkItemList pending_queue;
 };
 
@@ -195,37 +195,6 @@ class HttpCache::WorkItem {
 
 //-----------------------------------------------------------------------------
 
-// This class is a specialized type of OldCompletionCallback that allows us to
-// pass multiple arguments to the completion routine.
-class HttpCache::BackendCallback : public CallbackRunner<Tuple1<int> > {
- public:
-  BackendCallback(HttpCache* cache, PendingOp* pending_op)
-      : cache_(cache), pending_op_(pending_op) {}
-  ~BackendCallback() {}
-
-  virtual void RunWithParams(const Tuple1<int>& params) {
-    if (cache_) {
-      cache_->OnIOComplete(params.a, pending_op_);
-    } else {
-      // The callback was cancelled so we should delete the pending_op that
-      // was used with this callback.
-      delete pending_op_;
-    }
-    delete this;
-  }
-
-  void Cancel() {
-    cache_ = NULL;
-  }
-
- private:
-  HttpCache* cache_;
-  PendingOp* pending_op_;
-  DISALLOW_COPY_AND_ASSIGN(BackendCallback);
-};
-
-//-----------------------------------------------------------------------------
-
 // This class encapsulates a transaction whose only purpose is to write metadata
 // to a given entry.
 class HttpCache::MetadataWriter {
@@ -414,15 +383,12 @@ HttpCache::~HttpCache() {
     if (building_backend_) {
       // If we don't have a backend, when its construction finishes it will
       // deliver the callbacks.
-      BackendCallback* callback =
-          static_cast<BackendCallback*>(pending_op->callback);
-      if (callback) {
-        // The callback will delete the pending operation.
-        callback->Cancel();
+      if (!pending_op->callback.is_null()) {
+        // If not null, the callback will delete the pending operation later.
         delete_pending_op = false;
       }
     } else {
-      delete pending_op->callback;
+      pending_op->callback.Reset();
     }
 
     STLDeleteElements(&pending_op->pending_queue);
@@ -432,7 +398,7 @@ HttpCache::~HttpCache() {
 }
 
 int HttpCache::GetBackend(disk_cache::Backend** backend,
-                          const net::CompletionCallback& callback) {
+                          const CompletionCallback& callback) {
   DCHECK(!callback.is_null());
 
   if (disk_cache_.get()) {
@@ -547,15 +513,14 @@ int HttpCache::CreateBackend(disk_cache::Backend** backend,
   DCHECK(pending_op->pending_queue.empty());
 
   pending_op->writer = item.release();
-  BackendCallback* my_callback = new BackendCallback(this, pending_op);
-  pending_op->callback = my_callback;
+  pending_op->callback = base::Bind(&HttpCache::OnPendingOpComplete,
+                                    AsWeakPtr(), pending_op);
 
-  int rv = backend_factory_->CreateBackend(
-      net_log_, &pending_op->backend,
-      base::Bind(&net::OldCompletionCallbackAdapter, my_callback));
+  int rv = backend_factory_->CreateBackend(net_log_, &pending_op->backend,
+                                           pending_op->callback);
   if (rv != ERR_IO_PENDING) {
     pending_op->writer->ClearCallback();
-    my_callback->Run(rv);
+    pending_op->callback.Run(rv);
   }
 
   return rv;
@@ -651,14 +616,13 @@ int HttpCache::AsyncDoomEntry(const std::string& key, Transaction* trans) {
   DCHECK(pending_op->pending_queue.empty());
 
   pending_op->writer = item;
-  BackendCallback* my_callback = new BackendCallback(this, pending_op);
-  pending_op->callback = my_callback;
+  pending_op->callback = base::Bind(&HttpCache::OnPendingOpComplete,
+                                    AsWeakPtr(), pending_op);
 
-  int rv = disk_cache_->DoomEntry(
-      key, base::Bind(&net::OldCompletionCallbackAdapter, my_callback));
+  int rv = disk_cache_->DoomEntry(key, pending_op->callback);
   if (rv != ERR_IO_PENDING) {
     item->ClearTransaction();
-    my_callback->Run(rv);
+    pending_op->callback.Run(rv);
   }
 
   return rv;
@@ -775,15 +739,14 @@ int HttpCache::OpenEntry(const std::string& key, ActiveEntry** entry,
   DCHECK(pending_op->pending_queue.empty());
 
   pending_op->writer = item;
-  BackendCallback* my_callback = new BackendCallback(this, pending_op);
-  pending_op->callback = my_callback;
+  pending_op->callback = base::Bind(&HttpCache::OnPendingOpComplete,
+                                    AsWeakPtr(), pending_op);
 
-  int rv = disk_cache_->OpenEntry(
-      key, &(pending_op->disk_entry),
-      base::Bind(&net::OldCompletionCallbackAdapter, my_callback));
+  int rv = disk_cache_->OpenEntry(key, &(pending_op->disk_entry),
+                                  pending_op->callback);
   if (rv != ERR_IO_PENDING) {
     item->ClearTransaction();
-    my_callback->Run(rv);
+    pending_op->callback.Run(rv);
   }
 
   return rv;
@@ -803,15 +766,14 @@ int HttpCache::CreateEntry(const std::string& key, ActiveEntry** entry,
   DCHECK(pending_op->pending_queue.empty());
 
   pending_op->writer = item;
-  BackendCallback* my_callback = new BackendCallback(this, pending_op);
-  pending_op->callback = my_callback;
+  pending_op->callback = base::Bind(&HttpCache::OnPendingOpComplete,
+                                    AsWeakPtr(), pending_op);
 
-  int rv = disk_cache_->CreateEntry(
-      key, &(pending_op->disk_entry),
-      base::Bind(&net::OldCompletionCallbackAdapter, my_callback));
+  int rv = disk_cache_->CreateEntry(key, &(pending_op->disk_entry),
+                                    pending_op->callback);
   if (rv != ERR_IO_PENDING) {
     item->ClearTransaction();
-    my_callback->Run(rv);
+    pending_op->callback.Run(rv);
   }
 
   return rv;
@@ -1146,13 +1108,26 @@ void HttpCache::OnIOComplete(int result, PendingOp* pending_op) {
   }
 }
 
+// static
+void HttpCache::OnPendingOpComplete(const base::WeakPtr<HttpCache>& cache,
+                                    PendingOp* pending_op,
+                                    int rv) {
+  if (cache.get()) {
+    cache->OnIOComplete(rv, pending_op);
+  } else {
+    // The callback was cancelled so we should delete the pending_op that
+    // was used with this callback.
+    delete pending_op;
+  }
+}
+
 void HttpCache::OnBackendCreated(int result, PendingOp* pending_op) {
   scoped_ptr<WorkItem> item(pending_op->writer);
   WorkItemOperation op = item->operation();
   DCHECK_EQ(WI_CREATE_BACKEND, op);
 
   // We don't need the callback anymore.
-  pending_op->callback = NULL;
+  pending_op->callback.Reset();
   disk_cache::Backend* backend = pending_op->backend;
 
   if (backend_factory_.get()) {
diff --git a/net/http/http_cache.h b/net/http/http_cache.h
index ab8e4ad..27c6951 100644
--- a/net/http/http_cache.h
+++ b/net/http/http_cache.h
@@ -210,7 +210,6 @@ class NET_EXPORT HttpCache : public HttpTransactionFactory,
  private:
   // Types --------------------------------------------------------------------
 
-  class BackendCallback;
   class MetadataWriter;
   class SSLHostInfoFactoryAdaptor;
   class Transaction;
@@ -353,6 +352,16 @@ class NET_EXPORT HttpCache : public HttpTransactionFactory,
   // Processes BackendCallback notifications.
   void OnIOComplete(int result, PendingOp* entry);
 
+  // Helper to conditionally delete |pending_op| if the HttpCache object it
+  // is meant for has been deleted.
+  //
+  // TODO(ajwong): The PendingOp lifetime management is very tricky.  It might
+  // be possible to simplify it using either base::Owned() or base::Passed()
+  // with the callback.
+  static void OnPendingOpComplete(const base::WeakPtr<HttpCache>& cache,
+                                  PendingOp* pending_op,
+                                  int result);
+
   // Processes the backend creation notification.
   void OnBackendCreated(int result, PendingOp* pending_op);
 
diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index 58ad27e..6a9b33f 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -122,14 +122,10 @@ HttpCache::Transaction::Transaction(HttpCache* cache)
       effective_load_flags_(0),
       write_len_(0),
       final_upload_progress_(0),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
       ALLOW_THIS_IN_INITIALIZER_LIST(io_callback_(
-          base::Bind(&Transaction::OnIOComplete, base::Unretained(this)))),
-      ALLOW_THIS_IN_INITIALIZER_LIST(
-          cache_callback_(new CancelableOldCompletionCallback<Transaction>(
-              this, &Transaction::OnIOComplete))),
-      ALLOW_THIS_IN_INITIALIZER_LIST(
-          write_headers_callback_(new CancelableOldCompletionCallback<Transaction>(
-              this, &Transaction::OnIOComplete))) {
+          base::Bind(&Transaction::OnIOComplete,
+                     weak_factory_.GetWeakPtr()))) {
   COMPILE_ASSERT(HttpCache::Transaction::kNumValidationHeaders ==
                  arraysize(kValidationHeaders),
                  Invalid_number_of_validation_headers);
@@ -157,10 +153,9 @@ HttpCache::Transaction::~Transaction() {
     }
   }
 
-  // If there is an outstanding callback, mark it as cancelled so running it
-  // does nothing.
-  cache_callback_->Cancel();
-  write_headers_callback_->Cancel();
+  // Cancel any outstanding callbacks before we drop our reference to the
+  // HttpCache.  This probably isn't strictly necessary, but might as well.
+  weak_factory_.InvalidateWeakPtrs();
 
   // We could still have a cache read or write in progress, so we just null the
   // cache_ pointer to signal that we are dead.  See DoCacheReadCompleted.
@@ -1106,16 +1101,13 @@ int HttpCache::Transaction::DoOverwriteCachedResponse() {
 
 int HttpCache::Transaction::DoTruncateCachedData() {
   next_state_ = STATE_TRUNCATE_CACHED_DATA_COMPLETE;
-  cache_callback_->AddRef();  // Balanced in DoTruncateCachedDataComplete.
   if (!entry_)
     return OK;
   if (net_log_.IsLoggingAllEvents())
     net_log_.BeginEvent(NetLog::TYPE_HTTP_CACHE_WRITE_DATA, NULL);
 
   // Truncate the stream.
-  return WriteToEntry(
-      kResponseContentIndex, 0, NULL, 0,
-      base::Bind(&net::OldCompletionCallbackAdapter, cache_callback_));
+  return WriteToEntry(kResponseContentIndex, 0, NULL, 0, io_callback_);
 }
 
 int HttpCache::Transaction::DoTruncateCachedDataComplete(int result) {
@@ -1124,23 +1116,18 @@ int HttpCache::Transaction::DoTruncateCachedDataComplete(int result) {
                                       result);
   }
 
-  // Balance the AddRef from DoTruncateCachedData.
-  cache_callback_->Release();
   next_state_ = STATE_TRUNCATE_CACHED_METADATA;
   return OK;
 }
 
 int HttpCache::Transaction::DoTruncateCachedMetadata() {
   next_state_ = STATE_TRUNCATE_CACHED_METADATA_COMPLETE;
-  cache_callback_->AddRef();  // Balanced in DoTruncateCachedMetadataComplete.
   if (!entry_)
     return OK;
 
   if (net_log_.IsLoggingAllEvents())
     net_log_.BeginEvent(NetLog::TYPE_HTTP_CACHE_WRITE_INFO, NULL);
-  return WriteToEntry(
-      kMetadataIndex, 0, NULL, 0,
-      base::Bind(&net::OldCompletionCallbackAdapter, cache_callback_));
+  return WriteToEntry(kMetadataIndex, 0, NULL, 0, io_callback_);
 }
 
 int HttpCache::Transaction::DoTruncateCachedMetadataComplete(int result) {
@@ -1149,9 +1136,6 @@ int HttpCache::Transaction::DoTruncateCachedMetadataComplete(int result) {
                                       result);
   }
 
-  // Balance the AddRef from DoTruncateCachedMetadata.
-  cache_callback_->Release();
-
   // If this response is a redirect, then we can stop writing now.  (We don't
   // need to cache the response body of a redirect.)
   if (response_.headers->IsRedirect(NULL))
@@ -1191,15 +1175,11 @@ int HttpCache::Transaction::DoCacheReadResponse() {
   read_buf_ = new IOBuffer(io_buf_len_);
 
   net_log_.BeginEvent(NetLog::TYPE_HTTP_CACHE_READ_INFO, NULL);
-  cache_callback_->AddRef();  // Balanced in DoCacheReadResponseComplete.
-  return entry_->disk_entry->ReadData(
-      kResponseInfoIndex, 0, read_buf_, io_buf_len_,
-      base::Bind(&net::OldCompletionCallbackAdapter, cache_callback_));
+  return entry_->disk_entry->ReadData(kResponseInfoIndex, 0, read_buf_,
+                                      io_buf_len_, io_callback_);
 }
 
 int HttpCache::Transaction::DoCacheReadResponseComplete(int result) {
-  cache_callback_->Release();  // Balance the AddRef from DoCacheReadResponse.
-
   net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_READ_INFO, result);
   if (result != io_buf_len_ ||
       !HttpCache::ParseResponseInfo(read_buf_->data(), io_buf_len_,
@@ -1267,7 +1247,6 @@ int HttpCache::Transaction::DoCacheWriteResponseComplete(int result) {
   }
 
   // Balance the AddRef from WriteResponseInfoToEntry.
-  write_headers_callback_->Release();
   if (result != io_buf_len_) {
     DLOG(ERROR) << "failed to write response info to cache";
     DoneWritingToEntry(false);
@@ -1284,14 +1263,12 @@ int HttpCache::Transaction::DoCacheReadMetadata() {
       new IOBufferWithSize(entry_->disk_entry->GetDataSize(kMetadataIndex));
 
   net_log_.BeginEvent(NetLog::TYPE_HTTP_CACHE_READ_INFO, NULL);
-  cache_callback_->AddRef();  // Balanced in DoCacheReadMetadataComplete.
-  return entry_->disk_entry->ReadData(
-      kMetadataIndex, 0, response_.metadata, response_.metadata->size(),
-      base::Bind(&net::OldCompletionCallbackAdapter, cache_callback_));
+  return entry_->disk_entry->ReadData(kMetadataIndex, 0, response_.metadata,
+                                      response_.metadata->size(),
+                                      io_callback_);
 }
 
 int HttpCache::Transaction::DoCacheReadMetadataComplete(int result) {
-  cache_callback_->Release();  // Balance the AddRef from DoCacheReadMetadata.
   net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_READ_INFO, result);
   if (result != response_.metadata->size()) {
     DLOG(ERROR) << "ReadData failed: " << result;
@@ -1305,15 +1282,11 @@ int HttpCache::Transaction::DoCacheQueryData() {
   next_state_ = STATE_CACHE_QUERY_DATA_COMPLETE;
 
   // Balanced in DoCacheQueryDataComplete.
-  cache_callback_->AddRef();
-  return entry_->disk_entry->ReadyForSparseIO(
-      base::Bind(&net::OldCompletionCallbackAdapter, cache_callback_));
+  return entry_->disk_entry->ReadyForSparseIO(io_callback_);
 }
 
 int HttpCache::Transaction::DoCacheQueryDataComplete(int result) {
   DCHECK_EQ(OK, result);
-  // Balance the AddRef from DoCacheQueryData.
-  cache_callback_->Release();
   if (!cache_)
     return ERR_UNEXPECTED;
 
@@ -1323,23 +1296,19 @@ int HttpCache::Transaction::DoCacheQueryDataComplete(int result) {
 int HttpCache::Transaction::DoCacheReadData() {
   DCHECK(entry_);
   next_state_ = STATE_CACHE_READ_DATA_COMPLETE;
-  cache_callback_->AddRef();  // Balanced in DoCacheReadDataComplete.
 
   if (net_log_.IsLoggingAllEvents())
     net_log_.BeginEvent(NetLog::TYPE_HTTP_CACHE_READ_DATA, NULL);
   if (partial_.get()) {
-    return partial_->CacheRead(
-        entry_->disk_entry, read_buf_, io_buf_len_,
-        base::Bind(&net::OldCompletionCallbackAdapter, cache_callback_));
+    return partial_->CacheRead(entry_->disk_entry, read_buf_, io_buf_len_,
+                               io_callback_);
   }
 
-  return entry_->disk_entry->ReadData(
-      kResponseContentIndex, read_offset_, read_buf_, io_buf_len_,
-      base::Bind(&net::OldCompletionCallbackAdapter, cache_callback_));
+  return entry_->disk_entry->ReadData(kResponseContentIndex, read_offset_,
+                                      read_buf_, io_buf_len_, io_callback_);
 }
 
 int HttpCache::Transaction::DoCacheReadDataComplete(int result) {
-  cache_callback_->Release();  // Balance the AddRef from DoCacheReadData.
   if (net_log_.IsLoggingAllEvents()) {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_READ_DATA,
                                       result);
@@ -1365,11 +1334,8 @@ int HttpCache::Transaction::DoCacheWriteData(int num_bytes) {
   write_len_ = num_bytes;
   if (net_log_.IsLoggingAllEvents() && entry_)
     net_log_.BeginEvent(NetLog::TYPE_HTTP_CACHE_WRITE_DATA, NULL);
-  cache_callback_->AddRef();  // Balanced in DoCacheWriteDataComplete.
 
-  return AppendResponseDataToEntry(
-      read_buf_, num_bytes,
-      base::Bind(&net::OldCompletionCallbackAdapter, cache_callback_));
+  return AppendResponseDataToEntry(read_buf_, num_bytes, io_callback_);
 }
 
 int HttpCache::Transaction::DoCacheWriteDataComplete(int result) {
@@ -1378,7 +1344,6 @@ int HttpCache::Transaction::DoCacheWriteDataComplete(int result) {
                                       result);
   }
   // Balance the AddRef from DoCacheWriteData.
-  cache_callback_->Release();
   if (!cache_)
     return ERR_UNEXPECTED;
 
@@ -1991,14 +1956,9 @@ int HttpCache::Transaction::WriteResponseInfoToEntry(bool truncated) {
   response_.Persist(data->pickle(), skip_transient_headers, truncated);
   data->Done();
 
-  // Balanced in DoCacheWriteResponseComplete.  We may be running from the
-  // destructor of this object so cache_callback_ may be currently in use.
-  write_headers_callback_->AddRef();
   io_buf_len_ = data->pickle()->size();
-  return entry_->disk_entry->WriteData(
-      kResponseInfoIndex, 0, data, io_buf_len_,
-      base::Bind(&net::OldCompletionCallbackAdapter, write_headers_callback_),
-      true);
+  return entry_->disk_entry->WriteData(kResponseInfoIndex, 0, data,
+                                       io_buf_len_, io_callback_, true);
 }
 
 int HttpCache::Transaction::AppendResponseDataToEntry(
diff --git a/net/http/http_cache_transaction.h b/net/http/http_cache_transaction.h
index 7629c0b..38cf781 100644
--- a/net/http/http_cache_transaction.h
+++ b/net/http/http_cache_transaction.h
@@ -12,6 +12,7 @@
 #include <string>
 
 #include "base/time.h"
+#include "net/base/completion_callback.h"
 #include "net/base/net_log.h"
 #include "net/http/http_cache.h"
 #include "net/http/http_response_info.h"
@@ -361,10 +362,8 @@ class HttpCache::Transaction : public HttpTransaction {
   int write_len_;
   scoped_ptr<PartialData> partial_;  // We are dealing with range requests.
   uint64 final_upload_progress_;
+  base::WeakPtrFactory<Transaction> weak_factory_;
   CompletionCallback io_callback_;
-  scoped_refptr<CancelableOldCompletionCallback<Transaction> > cache_callback_;
-  scoped_refptr<CancelableOldCompletionCallback<Transaction> >
-      write_headers_callback_;
 };
 
 }  // namespace net
diff --git a/net/http/http_transaction_unittest.cc b/net/http/http_transaction_unittest.cc
index 5e14521..d3854d3 100644
--- a/net/http/http_transaction_unittest.cc
+++ b/net/http/http_transaction_unittest.cc
@@ -162,7 +162,8 @@ void TestTransactionConsumer::Start(const net::HttpRequestInfo* request,
                                     const net::BoundNetLog& net_log) {
   state_ = STARTING;
   int result = trans_->Start(
-      request, base::Bind(&net::OldCompletionCallbackAdapter, this), net_log);
+      request, base::Bind(&TestTransactionConsumer::OnIOComplete,
+                          base::Unretained(this)), net_log);
   if (result != net::ERR_IO_PENDING)
     DidStart(result);
 }
@@ -194,14 +195,14 @@ void TestTransactionConsumer::DidFinish(int result) {
 void TestTransactionConsumer::Read() {
   state_ = READING;
   read_buf_ = new net::IOBuffer(1024);
-  int result = trans_->Read(
-      read_buf_, 1024, base::Bind(&net::OldCompletionCallbackAdapter, this));
+  int result = trans_->Read(read_buf_, 1024,
+                            base::Bind(&TestTransactionConsumer::OnIOComplete,
+                                       base::Unretained(this)));
   if (result != net::ERR_IO_PENDING)
     DidRead(result);
 }
 
-void TestTransactionConsumer::RunWithParams(const Tuple1<int>& params) {
-  int result = params.a;
+void TestTransactionConsumer::OnIOComplete(int result) {
   switch (state_) {
     case STARTING:
       DidStart(result);
@@ -214,7 +215,6 @@ void TestTransactionConsumer::RunWithParams(const Tuple1<int>& params) {
   }
 }
 
-
 MockNetworkTransaction::MockNetworkTransaction(MockNetworkLayer* factory)
     : ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
       data_cursor_(0),
diff --git a/net/http/http_transaction_unittest.h b/net/http/http_transaction_unittest.h
index ba24c8c..1e1e588 100644
--- a/net/http/http_transaction_unittest.h
+++ b/net/http/http_transaction_unittest.h
@@ -103,7 +103,7 @@ class MockHttpRequest : public net::HttpRequestInfo {
 //-----------------------------------------------------------------------------
 // use this class to test completely consuming a transaction
 
-class TestTransactionConsumer : public CallbackRunner< Tuple1<int> > {
+class TestTransactionConsumer {
  public:
   explicit TestTransactionConsumer(net::HttpTransactionFactory* factory);
   virtual ~TestTransactionConsumer();
@@ -132,8 +132,7 @@ class TestTransactionConsumer : public CallbackRunner< Tuple1<int> > {
   void DidFinish(int result);
   void Read();
 
-  // Callback implementation:
-  virtual void RunWithParams(const Tuple1<int>& params) OVERRIDE;
+  void OnIOComplete(int result);
 
   State state_;
   scoped_ptr<net::HttpTransaction> trans_;