Change HistoryService::QueryURL to use CancelableTaskTracker

chrome/browser/safe_browsing/browser_feature_extractor.cc

 // 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 "chrome/browser/safe_browsing/browser_feature_extractor.h"
 
 #include <map>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 305 matching lines @@
     ClientPhishingRequest* request,
     const DoneCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   size_t removed = pending_extractions_.erase(request);
   DCHECK_EQ(1U, removed);
   HistoryService* history;
   if (!request || !request->IsInitialized() || !GetHistoryService(&history)) {
     callback.Run(false, request);
     return;
   }
-  CancelableRequestProvider::Handle handle = history->QueryURL(
-      GURL(request->url()),
-      true /* wants_visits */,
-      &request_consumer_,
-      base::Bind(&BrowserFeatureExtractor::QueryUrlHistoryDone,
-                 base::Unretained(this)));
-
-  StorePendingQuery(handle, request, callback);

[blundell, 2014/06/11 11:08:40]

Can StorePendingQuery() and GetPendingQuery() go away now?

[sdefresne, 2014/06/11 16:46:26]

No, they are still used by other functions that post tasks using
CancelableRequestConsumer/CancelableRequestProvider.

+  scoped_ptr<ClientPhishingRequest> owned_request(request);
+  history->QueryURL(GURL(request->url()),
+                    true /* wants_visits */,
+                    base::Bind(&BrowserFeatureExtractor::QueryUrlHistoryDone,
+                               base::Unretained(this),
+                               base::Passed(&owned_request),

[blundell, 2014/06/11 11:08:40]

Is there a reason this can't just be base::Unretained() as well?

[sdefresne, 2014/06/11 16:46:26]

The description of ExtractFeatures explain why:
  // Begins extraction of the browser features.  We take ownership
  // of the request object until |callback| is called (see DoneCallback above)
  // and will write the extracted features to the feature map.  Once the
  // feature extraction is complete, |callback| is run on the UI thread.  We
  // take ownership of the |callback| object.  |info| may not be valid after
  // ExtractFeatures returns.  This method must run on the UI thread.

The ExtractFeatures function launch a succession of asynchronous request
(StartExtractFeatures is just one such step). When an asynchronous request is in
progress, the ClientPhishingRequest value was associated to the request Handle
(this is what StorePendingQuery/GetPendingQuery do). When the
BrowserFeatureExtractor is killed, all the objects in that map were deleted.

With the new code, we just pass the pointer with base::Passed which means that
the object will be owned by the callback until it is called, or deleted when the
callback is deleted. Since all the pending requests are killed when the
CancelableTaskTracker is deleted, then the lifetime of the ClientPhishingRequest
is the same with old code and new code.

Do you want me to put this in a comment somewhere? This will most likely be the
pattern used by all the functions at the end of the migration. If you want me to
add the comment and this is your only nit, could you lgtm with nit?

+                               callback),
+                    &cancelable_task_tracker_);
 }
 
 void BrowserFeatureExtractor::QueryUrlHistoryDone(
-    CancelableRequestProvider::Handle handle,
+    scoped_ptr<ClientPhishingRequest> owned_request,
+    const DoneCallback& callback,
     bool success,
-    const history::URLRow* row,
-    history::VisitVector* visits) {
+    const history::URLRow& row,
+    const history::VisitVector& visits) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  ClientPhishingRequest* request;
-  DoneCallback callback;
-  if (!GetPendingQuery(handle, &request, &callback)) {
-    DLOG(FATAL) << "No pending history query found";
-    return;
-  }
-  DCHECK(request);
+  DCHECK(owned_request);
   DCHECK(!callback.is_null());
+  ClientPhishingRequest* request = owned_request.release();
   if (!success) {
     // URL is not found in the history.  In practice this should not
     // happen (unless there is a real error) because we just visited
     // that URL.
     callback.Run(false, request);
     return;
   }
   AddFeature(features::kUrlHistoryVisitCount,
-             static_cast<double>(row->visit_count()),
+             static_cast<double>(row.visit_count()),
              request);
 
   base::Time threshold = base::Time::Now() - base::TimeDelta::FromDays(1);
   int num_visits_24h_ago = 0;
   int num_visits_typed = 0;
   int num_visits_link = 0;
-  for (history::VisitVector::const_iterator it = visits->begin();
-       it != visits->end(); ++it) {
+  for (history::VisitVector::const_iterator it = visits.begin();
+       it != visits.end();
+       ++it) {
     if (!content::PageTransitionIsMainFrame(it->transition)) {
       continue;
     }
     if (it->visit_time < threshold) {
       ++num_visits_24h_ago;
     }
     content::PageTransition transition = content::PageTransitionStripQualifier(
         it->transition);
     if (transition == content::PAGE_TRANSITION_TYPED) {
       ++num_visits_typed;
@@ skipping 155 matching lines @@
     // Limit the number of matched bad IPs in one request to control
     // the request's size
     if (matched_bad_ips >= kMaxMalwareIPPerRequest) {
       break;
     }
   }
   callback.Run(true, request.Pass());
 }
 
 }  // namespace safe_browsing