Convert to base::Callback in safe_browsing client-side-detection code.

chrome/browser/safe_browsing/client_side_detection_service.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/safe_browsing/client_side_detection_service.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 42 matching lines @@
     base::TimeDelta::FromDays(1);
 const base::TimeDelta ClientSideDetectionService::kPositiveCacheInterval =
     base::TimeDelta::FromMinutes(30);
 
 const char ClientSideDetectionService::kClientReportPhishingUrl[] =
     "https://sb-ssl.google.com/safebrowsing/clientreport/phishing";
 const char ClientSideDetectionService::kClientModelUrl[] =
     "https://ssl.gstatic.com/safebrowsing/csd/client_model_v4.pb";
 
 struct ClientSideDetectionService::ClientReportInfo {
-  scoped_ptr<ClientReportPhishingRequestCallback> callback;
+  ClientReportPhishingRequestCallback callback;
   GURL phishing_url;
 };
 
 ClientSideDetectionService::CacheState::CacheState(bool phish, base::Time time)
     : is_phishing(phish),
       timestamp(time) {}
 
 ClientSideDetectionService::ClientSideDetectionService(
     net::URLRequestContextGetter* request_context_getter)
     : enabled_(false),
@@ skipping 37 matching lines @@
     // We delay the first model fetch to avoid slowing down browser startup.
     ScheduleFetchModel(kInitialClientModelFetchDelayMs);
   } else {
     // Cancel pending requests.
     model_fetcher_.reset();
     // Invoke pending callbacks with a false verdict.
     for (std::map<const content::URLFetcher*, ClientReportInfo*>::iterator it =
              client_phishing_reports_.begin();
          it != client_phishing_reports_.end(); ++it) {
       ClientReportInfo* info = it->second;
-      if (info->callback.get())
-        info->callback->Run(info->phishing_url, false);
+      info->callback.Run(info->phishing_url, false);
     }
     STLDeleteContainerPairPointers(client_phishing_reports_.begin(),
                                    client_phishing_reports_.end());
     client_phishing_reports_.clear();
     cache_.clear();
   }
 }
 
 void ClientSideDetectionService::SendClientReportPhishingRequest(
     ClientPhishingRequest* verdict,
-    ClientReportPhishingRequestCallback* callback) {
+    const ClientReportPhishingRequestCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   MessageLoop::current()->PostTask(
       FROM_HERE,
       base::Bind(&ClientSideDetectionService::StartClientReportPhishingRequest,
                  weak_factory_.GetWeakPtr(), verdict, callback));
 }
 
 bool ClientSideDetectionService::IsPrivateIPAddress(
     const std::string& ip_address) const {
   net::IPAddressNumber ip_number;
@@ skipping 136 matching lines @@
     delay_ms = model_max_age_->InMilliseconds();
   }
   model_max_age_.reset();
 
   // Schedule the next model reload.
   ScheduleFetchModel(delay_ms);
 }
 
 void ClientSideDetectionService::StartClientReportPhishingRequest(
     ClientPhishingRequest* verdict,
-    ClientReportPhishingRequestCallback* callback) {
+    const ClientReportPhishingRequestCallback& callback) {

[noelutz, 2011/11/21 21:46:05]

note: this callback can be null.

[Scott Hess - ex-Googler, 2011/11/21 22:15:20]

That's odd.  I'm sure I had reviewed the code and verified that a callback used
like this:

  base::Callback<void(void)> cb;
  if (SomethingIsTrue()) {
    cb = base::Bind(&myfun);
  }
  cb.Run();

would be safe to call Run() in either case.  But it doesn't look that way to me
when I look at the code, now.  I might have to sprinkle some is_null() calls in
here.

[noelutz, 2011/11/21 22:24:00]

Oh, if this code is safe then we're fine.  In other words: is it ok to call
Run() if the callback is_null()?

   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   scoped_ptr<ClientPhishingRequest> request(verdict);
-  scoped_ptr<ClientReportPhishingRequestCallback> cb(callback);
 
   if (!enabled_) {
-    if (cb.get())
-      cb->Run(GURL(request->url()), false);
+    callback.Run(GURL(request->url()), false);
     return;
   }
 
   std::string request_data;
   if (!request->SerializeToString(&request_data)) {
     UMA_HISTOGRAM_COUNTS("SBClientPhishing.RequestNotSerialized", 1);
     VLOG(1) << "Unable to serialize the CSD request. Proto file changed?";
-    if (cb.get()) {
-      cb->Run(GURL(request->url()), false);
-    }
+    callback.Run(GURL(request->url()), false);
     return;
   }
 
   content::URLFetcher* fetcher = content::URLFetcher::Create(
       0 /* ID used for testing */, GURL(kClientReportPhishingUrl),
       content::URLFetcher::POST, this);
 
   // Remember which callback and URL correspond to the current fetcher object.
   ClientReportInfo* info = new ClientReportInfo;
-  info->callback.swap(cb);  // takes ownership of the callback.
+  info->callback = callback;
   info->phishing_url = GURL(request->url());
   client_phishing_reports_[fetcher] = info;
 
   fetcher->SetLoadFlags(net::LOAD_DISABLE_CACHE);
   fetcher->SetRequestContext(request_context_getter_.get());
   fetcher->SetUploadData("application/octet-stream", request_data);
   fetcher->Start();
 
   // Record that we made a request
   phishing_report_times_.push(base::Time::Now());
@@ skipping 55 matching lines @@
     // Cache response, possibly flushing an old one.
     cache_[info->phishing_url] =
         make_linked_ptr(new CacheState(response.phishy(), base::Time::Now()));
     is_phishing = (response.phishy() &&
                    !IsFalsePositiveResponse(info->phishing_url, response));
   } else {
     DLOG(ERROR) << "Unable to get the server verdict for URL: "
                 << info->phishing_url << " status: " << status.status() << " "
                 << "response_code:" << response_code;
   }
-  if (info->callback.get()) {
-    info->callback->Run(info->phishing_url, is_phishing);
-  }
+  info->callback.Run(info->phishing_url, is_phishing);
   client_phishing_reports_.erase(source);
   delete source;
 }
 
 bool ClientSideDetectionService::IsInCache(const GURL& url) {
   UpdateCache();
 
   return cache_.find(url) != cache_.end();
 }
 
@@ skipping 169 matching lines @@
                                                 &whitelist_patterns);
     for (size_t j = 0; j < whitelist_patterns.size(); ++j) {
       if (whitelist_patterns[j] == canonical_url_as_pattern) {
         return true;
       }
     }
   }
   return false;
 }
 }  // namespace safe_browsing