base::Bind: Implement a 1-arity CancelableCallback and use this to implement

chrome/browser/renderer_host/offline_resource_handler.cc

Index: chrome/browser/renderer_host/offline_resource_handler.cc
diff --git a/chrome/browser/renderer_host/offline_resource_handler.cc b/chrome/browser/renderer_host/offline_resource_handler.cc
index 536e27cc800110f3a89d906560d34cb4b2e9f054..2ee3cdb329b0d24c6568fd1b18b0f901f545bda5 100644
--- a/chrome/browser/renderer_host/offline_resource_handler.cc
+++ b/chrome/browser/renderer_host/offline_resource_handler.cc
@@ -45,7 +45,7 @@ OfflineResourceHandler::OfflineResourceHandler(
 
 OfflineResourceHandler::~OfflineResourceHandler() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  DCHECK(!appcache_completion_callback_.get());
+  DCHECK(appcache_completion_callback_.IsCancelled());
 }
 
 bool OfflineResourceHandler::OnUploadProgress(int request_id,
@@ -77,26 +77,24 @@ bool OfflineResourceHandler::OnResponseCompleted(
 }
 
 void OfflineResourceHandler::OnRequestClosed() {
-  if (appcache_completion_callback_) {
-    appcache_completion_callback_->Cancel();
-    appcache_completion_callback_.release();
-    Release();  // Balanced with OnWillStart
+  if (!appcache_completion_callback_.IsCancelled()) {
+    appcache_completion_callback_.Cancel();
   }
+
   next_handler_->OnRequestClosed();
 }
 
 void OfflineResourceHandler::OnCanHandleOfflineComplete(int rv) {
-  CHECK(appcache_completion_callback_);
-  appcache_completion_callback_ = NULL;
+  appcache_completion_callback_.Cancel();

[michaeln, 2011/11/30 20:04:22]

The call to Cancel() here is odd, your doing this to break reference cycle i
suppose, a small comment here would be good. Also is there is a different method
that would also work (like Reset() or Clear())?

[James Hawkins, 2011/11/30 20:07:12]

I'll add a comment.

There is a Reset(callback), but that is for setting the held callback to a
different callback.

[awong, 2011/11/30 20:28:17]

This again makes me want to figure out how to rename these functions so
"Reset()". Maybe "Replace" versus CancelAndReset()?  I don't know.  Anyways,
another CL.

+
   if (deferred_request_id_ == -1) {
-    LOG(WARNING) << "OnCanHandleOfflineComplete called after completion: "
-                 << " this=" << this;
-    NOTREACHED();
+    DLOG(FATAL) << "OnCanHandleOfflineComplete called after completion: "
+                << " this=" << this;
     return;
   }
+
   if (rv == net::OK) {
     Resume();
-    Release();  // Balanced with OnWillStart
   } else {
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
@@ -107,24 +105,29 @@ void OfflineResourceHandler::OnCanHandleOfflineComplete(int rv) {
 bool OfflineResourceHandler::OnWillStart(int request_id,
                                          const GURL& url,
                                          bool* defer) {
-  if (ShouldShowOfflinePage(url)) {
-    deferred_request_id_ = request_id;
-    deferred_url_ = url;
-    DVLOG(1) << "OnWillStart: this=" << this << ", request id=" << request_id
-             << ", url=" << url;
-    AddRef();  //  Balanced with OnCanHandleOfflineComplete
-    DCHECK(!appcache_completion_callback_);
-    appcache_completion_callback_ =
-        new net::CancelableOldCompletionCallback<OfflineResourceHandler>(
-            this, &OfflineResourceHandler::OnCanHandleOfflineComplete);
-    appcache_service_->CanHandleMainResourceOffline(
-        url, request_->first_party_for_cookies(),
-        appcache_completion_callback_);
-
-    *defer = true;
-    return true;
-  }
-  return next_handler_->OnWillStart(request_id, url, defer);
+  if (!ShouldShowOfflinePage(url))
+    return next_handler_->OnWillStart(request_id, url, defer);
+
+  deferred_request_id_ = request_id;
+  deferred_url_ = url;
+  DVLOG(1) << "OnWillStart: this=" << this << ", request id=" << request_id
+           << ", url=" << url;
+
+  DCHECK(appcache_completion_callback_.IsCancelled());
+
+  // |appcache_completion_callback_| holds a reference to |this|, so there is a
+  // circular reference; however,

[awong, 2011/11/30 20:28:17]

however, -> however, either 

[James Hawkins, 2011/11/30 20:31:05]

Done.

+  // OfflineResourceHandler::OnCanHandleOfflineComplete cancels the callback
+  // (thus dropping the reference), or CanHandleMainResourceOffline calls the
+  // callback which Resets it.
+  appcache_completion_callback_.Reset(
+      base::Bind(&OfflineResourceHandler::OnCanHandleOfflineComplete, this));
+  appcache_service_->CanHandleMainResourceOffline(
+      url, request_->first_party_for_cookies(),
+      appcache_completion_callback_.callback());
+
+  *defer = true;
+  return true;
 }
 
 // We'll let the original event handler provide a buffer, and reuse it for
@@ -161,7 +164,6 @@ void OfflineResourceHandler::OnBlockingPageComplete(bool proceed) {
     ClearRequestInfo();
     rdh_->CancelRequest(process_host_id_, request_id, false);
   }
-  Release();  // Balanced with OnWillStart
 }
 
 void OfflineResourceHandler::ClearRequestInfo() {