base::Bind: Remove OldCompletionCallback.

webkit/appcache/appcache_response.cc

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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 "webkit/appcache/appcache_response.h"
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/compiler_specific.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/pickle.h"
 #include "base/string_util.h"
 #include "net/base/completion_callback.h"
 #include "net/base/net_errors.h"
 #include "net/base/io_buffer.h"
 #include "webkit/appcache/appcache_service.h"
 
 namespace appcache {
@@ skipping 112 matching lines @@
   DCHECK_NE(net::ERR_IO_PENDING, result);
   OnIOComplete(result);
 }
 
 
 // AppCacheResponseReader ----------------------------------------------
 
 AppCacheResponseReader::AppCacheResponseReader(
     int64 response_id, int64 group_id, AppCacheDiskCacheInterface* disk_cache)
     : AppCacheResponseIO(response_id, group_id, disk_cache),
-      range_offset_(0), range_length_(kint32max),
-      read_position_(0) {
+      range_offset_(0),
+      range_length_(kint32max),
+      read_position_(0),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
 }
 
 AppCacheResponseReader::~AppCacheResponseReader() {

[michaeln, 2012/01/04 02:07:17]

Is disk_cache_->OpenEntry() 'copy' of the callback still valid  after this dtor
has returned? 

[James Hawkins, 2012/01/05 22:44:56]

No, because of the WeakPtr.

[michaeln, 2012/01/05 23:34:54]

Great, so AppCacheResponseReader::OnOpenEntryComplete won't get called.

More pointedly, is the heap allocated 'entry_ptr' param held via base::OwnPtr by
the disk_cache's copy/ref of the bind closure still valid after this dtor? I
expect it is, just checking?

-  if (open_callback_)
-    open_callback_.release()->Cancel();
 }
 
 void AppCacheResponseReader::ReadInfo(HttpResponseInfoIOBuffer* info_buf,
                                       const net::CompletionCallback& callback) {
   DCHECK(!callback.is_null());
   DCHECK(!IsReadPending());
   DCHECK(info_buf);
   DCHECK(!info_buf->http_info.get());
   DCHECK(!buffer_.get());
   DCHECK(!info_buffer_.get());
@@ skipping 76 matching lines @@
           entry_->GetSize(kResponseContentIndex);
     } else {
       read_position_ += result;
     }
   }
   InvokeUserCompletionCallback(result);
 }
 
 void AppCacheResponseReader::OpenEntryIfNeededAndContinue() {
   int rv;
+  AppCacheDiskCacheInterface::Entry** entry_ptr = NULL;
   if (entry_) {
     rv = net::OK;
   } else if (!disk_cache_) {
     rv = net::ERR_FAILED;
   } else {
-    open_callback_ = new EntryCallback<AppCacheResponseReader>(
-        this, &AppCacheResponseReader::OnOpenEntryComplete);
-    rv = disk_cache_->OpenEntry(
-        response_id_, &open_callback_->entry_ptr_,
-        base::Bind(&net::OldCompletionCallbackAdapter, open_callback_));
+    entry_ptr = new(AppCacheDiskCacheInterface::Entry*);
+    open_callback_ =
+        base::Bind(&AppCacheResponseReader::OnOpenEntryComplete,
+                   weak_factory_.GetWeakPtr(), base::Owned(entry_ptr));
+    rv = disk_cache_->OpenEntry(response_id_, entry_ptr, open_callback_);
   }
 
   if (rv != net::ERR_IO_PENDING)
-    OnOpenEntryComplete(rv);
+    OnOpenEntryComplete(entry_ptr, rv);
 }
 
-void AppCacheResponseReader::OnOpenEntryComplete(int rv) {
+void AppCacheResponseReader::OnOpenEntryComplete(
+    AppCacheDiskCacheInterface::Entry** entry, int rv) {
   DCHECK(info_buffer_.get() || buffer_.get());
 
-  if (open_callback_) {
+  if (!open_callback_.is_null()) {
     if (rv == net::OK) {
-      entry_ = open_callback_->entry_ptr_;
-      open_callback_->entry_ptr_ = NULL;
+      DCHECK(entry);
+      entry_ = *entry;
     }
-    open_callback_ = NULL;
+    open_callback_.Reset();
   }
 
   if (info_buffer_)
     ContinueReadInfo();
   else
     ContinueReadData();
 }
 
 // AppCacheResponseWriter ----------------------------------------------
 
 AppCacheResponseWriter::AppCacheResponseWriter(
     int64 response_id, int64 group_id, AppCacheDiskCacheInterface* disk_cache)
     : AppCacheResponseIO(response_id, group_id, disk_cache),
-      info_size_(0), write_position_(0), write_amount_(0),
-      creation_phase_(INITIAL_ATTEMPT) {
+      info_size_(0),
+      write_position_(0),
+      write_amount_(0),
+      creation_phase_(INITIAL_ATTEMPT),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
 }
 
 AppCacheResponseWriter::~AppCacheResponseWriter() {
-  if (create_callback_)
-    create_callback_.release()->Cancel();
 }
 
 void AppCacheResponseWriter::WriteInfo(
     HttpResponseInfoIOBuffer* info_buf,
     const net::CompletionCallback& callback) {
   DCHECK(!callback.is_null());
   DCHECK(!IsWritePending());
   DCHECK(info_buf);
   DCHECK(info_buf->http_info.get());
   DCHECK(!buffer_.get());
@@ skipping 49 matching lines @@
     if (!info_buffer_.get())
       write_position_ += result;
     else
       info_size_ = result;
   }
   InvokeUserCompletionCallback(result);
 }
 
 void AppCacheResponseWriter::CreateEntryIfNeededAndContinue() {
   int rv;
+  AppCacheDiskCacheInterface::Entry** entry_ptr = NULL;

[awong, 2012/01/04 01:50:28]

Move into else cause.

[James Hawkins, 2012/01/05 22:44:56]

See michaeln's comment below as to why that's not possible.

   if (entry_) {
     creation_phase_ = NO_ATTEMPT;
     rv = net::OK;
   } else if (!disk_cache_) {
     creation_phase_ = NO_ATTEMPT;
     rv = net::ERR_FAILED;
   } else {
     creation_phase_ = INITIAL_ATTEMPT;
-    create_callback_ = new EntryCallback<AppCacheResponseWriter>(
-        this, &AppCacheResponseWriter::OnCreateEntryComplete);
-    rv = disk_cache_->CreateEntry(
-        response_id_, &create_callback_->entry_ptr_,
-        base::Bind(&net::OldCompletionCallbackAdapter, create_callback_));
+    entry_ptr = new(AppCacheDiskCacheInterface::Entry*);
+    create_callback_ =
+        base::Bind(&AppCacheResponseWriter::OnCreateEntryComplete,
+                   base::Unretained(this), base::Owned(entry_ptr));

[michaeln, 2012/01/04 02:07:17]

use weak_factory_.GetWeakPtr() here?

[James Hawkins, 2012/01/05 22:44:56]

Done.

+    rv = disk_cache_->CreateEntry(response_id_, entry_ptr, create_callback_);
   }
   if (rv != net::ERR_IO_PENDING)
-    OnCreateEntryComplete(rv);
+    OnCreateEntryComplete(entry_ptr, rv);

[awong, 2012/01/04 01:50:28]

Call with NULL explicitly.

Same with the pattern above.

Doing this makes it so there's less code tracing when tyring to understand what
the value in OnCreateEntryComplete() is.

[michaeln, 2012/01/04 02:07:17]

It's not necessarily a NULL value at this point, rv can be OK.

 }
 
-void AppCacheResponseWriter::OnCreateEntryComplete(int rv) {
+void AppCacheResponseWriter::OnCreateEntryComplete(
+    AppCacheDiskCacheInterface::Entry** entry, int rv) {
   DCHECK(info_buffer_.get() || buffer_.get());
 
+  AppCacheDiskCacheInterface::Entry** entry_ptr = NULL;

[awong, 2012/01/04 01:50:28]

Move this into the "else if" clause?

[James Hawkins, 2012/01/05 22:44:56]

Done.

+
   if (creation_phase_ == INITIAL_ATTEMPT) {
     if (rv != net::OK) {
       // We may try to overwrite existing entries.
       creation_phase_ = DOOM_EXISTING;
-      rv = disk_cache_->DoomEntry(
-          response_id_, base::Bind(&net::OldCompletionCallbackAdapter,
-                                   create_callback_));
+      rv = disk_cache_->DoomEntry(response_id_, create_callback_);
       if (rv != net::ERR_IO_PENDING)
-        OnCreateEntryComplete(rv);
+        OnCreateEntryComplete(NULL, rv);
       return;
     }
   } else if (creation_phase_ == DOOM_EXISTING) {
     creation_phase_ = SECOND_ATTEMPT;
-    rv = disk_cache_->CreateEntry(
-        response_id_, &create_callback_->entry_ptr_,
-        base::Bind(&net::OldCompletionCallbackAdapter, create_callback_));
+    entry_ptr = new(AppCacheDiskCacheInterface::Entry*);

[awong, 2012/01/04 01:50:28]

nit: personally I prefer 
  new AppCacheDiskCacheInterface::Entry*
or
  new AppCacheDiskCacheInterface::Entry*()

cause this looks like a function invocation to me.  But your choice.

[michaeln, 2012/01/04 02:07:17]

new(T) style looked odd to me too... prefer new T

[James Hawkins, 2012/01/05 22:44:56]

Done.

[James Hawkins, 2012/01/05 22:44:56]

Done.

+    create_callback_ =
+        base::Bind(&AppCacheResponseWriter::OnCreateEntryComplete,
+                   base::Unretained(this), base::Owned(entry_ptr));

[michaeln, 2012/01/04 02:07:17]

use weak_factory_.GetWeakPtr() here?

[James Hawkins, 2012/01/05 22:44:56]

Done.

+    rv = disk_cache_->CreateEntry(response_id_, entry_ptr, create_callback_);
     if (rv != net::ERR_IO_PENDING)
-      OnCreateEntryComplete(rv);
+      OnCreateEntryComplete(entry_ptr, rv);
     return;
   }
 
-  if (create_callback_) {
-    if (rv == net::OK) {
-      entry_ = create_callback_->entry_ptr_;
-      create_callback_->entry_ptr_ = NULL;
-    }
-    create_callback_ = NULL;
+  if (!create_callback_.is_null()) {
+    if (rv == net::OK)
+      entry_ = *entry;
+
+    create_callback_.Reset();
   }
 
   if (info_buffer_)
     ContinueWriteInfo();
   else
     ContinueWriteData();
 }
 
 }  // namespace appcache