Some easy NewRunnableMethod conversions in safe_browsing.

chrome/browser/safe_browsing/safe_browsing_service.h

Index: chrome/browser/safe_browsing/safe_browsing_service.h
diff --git a/chrome/browser/safe_browsing/safe_browsing_service.h b/chrome/browser/safe_browsing/safe_browsing_service.h
index 8844792e8151d8976b3c34e83b698857fae2eaf4..252911421d7b1c810942b88e85467d269c1ebae2 100644
--- a/chrome/browser/safe_browsing/safe_browsing_service.h
+++ b/chrome/browser/safe_browsing/safe_browsing_service.h
@@ -102,13 +102,15 @@ class SafeBrowsingService
     std::vector<SBPrefix> prefix_hits;
     std::vector<SBFullHashResult> full_hits;
 
-    // Task to make the callback to safebrowsing clients in case
-    // safebrowsing check takes too long to finish. Not owned by
-    // this class.
+    // Flag to cancel the check if it takes too long.  This is owned
+    // by the timeout task.  If the timeout runs first, the check's
+    // client will be cleared, as will this pointer.  If the check is
+    // completed before the timeout, the flag is set and the timeout
+    // will not fire.

[awong, 2011/11/14 20:13:37]

Wow. complicated.

So the flag is owned by the timeout closure only, and the "non-timeout" paths
know to disable themselves because "client" is gone?

I'm tempted to favor the more baroque style of embedding the factory in the
Check object, if only because people understand the lifetimes.  If you embedded
the factory, it'd be clear that on delete of |check|, the timeout callback would
just not fire.

As is, I actually read through the patch multiple times before I could satisfy
myself that I understood where each of these objects was expected to be deleted
with respect to the callbacks that might be outstanding for them.

[Scott Hess - ex-Googler, 2011/11/14 23:21:54]

I think so.  Author has left the project ...
I've made that change.

My initial approach was to have the SafeBrowsingCheck constructor take the
pointer to put in the factory.  This lead to having to modify some unit tests,
which lead to having to read up on gmock ACTION_P macros, which lead to wailing
and teeth-gnashing, but even once all of that was fixed I ended up with an
insane error message (constructing w/o a parameter gave the expected error,
constructing with the appropriate parameter gave an error "Declaration has no
effect").  So I backed it out to this version.

Another option would be to have a default-to-NULL constructor (or
with-and-without constructors), though I understand that to be against our style
guidelines.

YET ANOTHER OPTION would be to leave it alone and hope that someone finds time
to refactor this code.

     // TODO(lzheng): We should consider to use this time out check
     // for browsing too (instead of implementin in
     // safe_browsing_resource_handler.cc).
-    CancelableTask* timeout_task;
+    bool* cancel_timeout;
 
    private:
     DISALLOW_COPY_AND_ASSIGN(SafeBrowsingCheck);
@@ -442,7 +444,7 @@ class SafeBrowsingService
 
   // The callback function when a safebrowsing check is timed out. Client will
   // be notified that the safebrowsing check is SAFE when this happens.
-  void TimeoutCallback(SafeBrowsingCheck* check);
+  void TimeoutCallback(SafeBrowsingCheck* check, bool* cancel_timeout);
 
   // Calls the Client's callback on IO thread after CheckDownloadUrl finishes.
   void CheckDownloadUrlDone(SafeBrowsingCheck* check);
@@ -458,7 +460,7 @@ class SafeBrowsingService
   // success, otherwise TimeoutCallback will be called.
   void StartDownloadCheck(SafeBrowsingCheck* check,
                           Client* client,
-                          CancelableTask* task,
+                          const base::Closure& task,
                           int64 timeout_ms);
 
   // Adds the given entry to the whitelist.  Called on the UI thread.