Some easy NewRunnableMethod conversions in safe_browsing.

chrome/browser/safe_browsing/safe_browsing_service.h

 // 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.
 //
 // The Safe Browsing service is responsible for downloading anti-phishing and
 // anti-malware tables and checking urls against them.
 
 #ifndef CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_SERVICE_H_
 #define CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_SERVICE_H_
 #pragma once
@@ skipping 84 matching lines @@
     scoped_ptr<SBFullHash> full_hash;
 
     Client* client;
     bool need_get_hash;
     base::TimeTicks start;  // When check was sent to SB service.
     UrlCheckResult result;
     bool is_download;  // If this check for download url or hash.
     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);
   };
 
   // Observer class can be used to get notified when a SafeBrowsing hit
   // was found.
   class Observer {
    public:
     // The |resource| must not be accessed after OnSafeBrowsingHit returns.
@@ skipping 313 matching lines @@
 
   // Checks the download hash on safe_browsing_thread_.
   void CheckDownloadHashOnSBThread(SafeBrowsingCheck* check);
 
   // Invoked by CheckDownloadUrl. It checks the download URL on
   // safe_browsing_thread_.
   void CheckDownloadUrlOnSBThread(SafeBrowsingCheck* check);
 
   // 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);
 
   // Calls the Client's callback on IO thread after CheckDownloadHash finishes.
   void CheckDownloadHashDone(SafeBrowsingCheck* check);
 
   // Helper function that calls safe browsing client and cleans up |checks_|.
   void SafeBrowsingCheckDone(SafeBrowsingCheck* check);
 
   // Helper function to set |check| with default values and start a safe
   // browsing check with timeout of |timeout_ms|. |task| will be called upon
   // 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.
   void UpdateWhitelist(const UnsafeResource& resource);
 
   // content::NotificationObserver override
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
@@ skipping 100 matching lines @@
 class SafeBrowsingServiceFactory {
  public:
   SafeBrowsingServiceFactory() { }
   virtual ~SafeBrowsingServiceFactory() { }
   virtual SafeBrowsingService* CreateSafeBrowsingService() = 0;
  private:
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingServiceFactory);
 };
 
 #endif  // CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_SERVICE_H_