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

chrome/browser/renderer_host/offline_resource_handler.cc

 // 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 "chrome/browser/renderer_host/offline_resource_handler.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 27 matching lines @@
       render_view_id_(route_id),
       rdh_(rdh),
       request_(request),
       appcache_service_(appcache_service),
       deferred_request_id_(-1) {
   DCHECK(appcache_service_);
 }
 
 OfflineResourceHandler::~OfflineResourceHandler() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  DCHECK(!appcache_completion_callback_.get());
+  DCHECK(appcache_completion_callback_.IsCancelled());
 }
 
 bool OfflineResourceHandler::OnUploadProgress(int request_id,
                                               uint64 position,
                                               uint64 size) {
   return next_handler_->OnUploadProgress(request_id, position, size);
 }
 
 bool OfflineResourceHandler::OnRequestRedirected(
     int request_id,
@@ skipping 11 matching lines @@
 }
 
 bool OfflineResourceHandler::OnResponseCompleted(
     int request_id,
     const net::URLRequestStatus& status,
     const std::string& security_info) {
   return next_handler_->OnResponseCompleted(request_id, status, security_info);
 }
 
 void OfflineResourceHandler::OnRequestClosed() {
-  if (appcache_completion_callback_) {
-    appcache_completion_callback_->Cancel();
-    appcache_completion_callback_.release();
+  if (!appcache_completion_callback_.IsCancelled()) {
+    appcache_completion_callback_.Cancel();
     Release();  // Balanced with OnWillStart
   }
+
   next_handler_->OnRequestClosed();
 }
 
 void OfflineResourceHandler::OnCanHandleOfflineComplete(int rv) {
-  CHECK(appcache_completion_callback_);

[awong, 2011/11/30 00:52:55]

Do we no longer need an equivalent check?  Maybe
CHECK(appcache_completion_callback_.IsCancelled()) and then reset?

[James Hawkins, 2011/11/30 01:44:58]

IMO the check is superfluous (we're in the callback afterall.  I'll add the
Cancel though.

-  appcache_completion_callback_ = NULL;
   if (deferred_request_id_ == -1) {
-    LOG(WARNING) << "OnCanHandleOfflineComplete called after completion: "
-                 << " this=" << this;
-    NOTREACHED();
-    return;
+    DLOG(FATAL) << "OnCanHandleOfflineComplete called after completion: "
+                << " this=" << this;

[awong, 2011/11/30 00:52:55]

You need a return; still.

[James Hawkins, 2011/11/30 01:44:58]

Done.

   }
+
   if (rv == net::OK) {
     Resume();
-    Release();  // Balanced with OnWillStart
   } else {
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
         base::Bind(&OfflineResourceHandler::ShowOfflinePage, this));
   }
 }
 
 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);
+
+    DCHECK(appcache_completion_callback_.IsCancelled());

[awong, 2011/11/30 00:52:55]

I think you still need to retain the manual AddRef() and Relaese() calls.  As
is, I think we introduce a refcount cycle on this since
appcache_completion_callback_ takes a reference onto this.

[James Hawkins, 2011/11/30 01:44:58]

Per-offline, made the callback own |this|.

+    appcache_completion_callback_.Reset(
+        base::Bind(&OfflineResourceHandler::OnCanHandleOfflineComplete, this));
     appcache_service_->CanHandleMainResourceOffline(
         url, request_->first_party_for_cookies(),
-        appcache_completion_callback_);
+        appcache_completion_callback_.callback());
 
     *defer = true;
     return true;
   }
   return next_handler_->OnWillStart(request_id, url, defer);
 }
 
 // We'll let the original event handler provide a buffer, and reuse it for
 // subsequent reads until we're done buffering.
 bool OfflineResourceHandler::OnWillRead(int request_id, net::IOBuffer** buf,
@@ skipping 74 matching lines @@
     TabContents* tab_contents = render_view_host ?
         render_view_host->delegate()->GetAsTabContents() : NULL;
     // There is a chance that the tab closed after we decided to show
     // the offline page on the IO thread and before we actually show the
     // offline page here on the UI thread.
     if (tab_contents)
       (new chromeos::OfflineLoadPage(tab_contents, deferred_url_, this))->
           Show();
   }
 }