Make SimpleEntryImpl ref counted.

net/disk_cache/simple/simple_entry_impl.h

Index: net/disk_cache/simple/simple_entry_impl.h
diff --git a/net/disk_cache/simple/simple_entry_impl.h b/net/disk_cache/simple/simple_entry_impl.h
index 7b898cf4881f68f2c019ccd14c065c7261b18796..f1161ce0646e94a895da4c70c0ed3860e9a40285 100644
--- a/net/disk_cache/simple/simple_entry_impl.h
+++ b/net/disk_cache/simple/simple_entry_impl.h
@@ -8,7 +8,7 @@
 #include <string>
 
 #include "base/files/file_path.h"
-#include "base/memory/weak_ptr.h"
+#include "base/memory/ref_counted.h"
 #include "base/threading/thread_checker.h"
 #include "net/disk_cache/disk_cache.h"
 #include "net/disk_cache/simple/simple_entry_format.h"
@@ -26,21 +26,23 @@ class SimpleSynchronousEntry;
 // SimpleEntryImpl is the IO thread interface to an entry in the very simple
 // disk cache. It proxies for the SimpleSynchronousEntry, which performs IO
 // on the worker thread.
-class SimpleEntryImpl : public Entry {
+class SimpleEntryImpl : public Entry,
+                        public base::RefCountedThreadSafe<SimpleEntryImpl> {
+  friend class base::RefCountedThreadSafe<SimpleEntryImpl>;
  public:
-  static int OpenEntry(base::WeakPtr<SimpleIndex> index,
+  static int OpenEntry(scoped_refptr<SimpleIndex> index,

[Philippe, 2013/04/17 10:19:22]

I guess passing ref-counted objects around by copying scoped_refptr's is
probably the general consensus in simple cache backend. This brings extra safety
but causes unnecessary AddRef/Release (translating to atomicops).

I believe the "recommended" practice in Chromium is to pass ref-counted objects
around through raw pointers and store them (in long-lived data structures) using
scoped_refptr.

What do you think? Should this be const scoped_refptr<>& at least?

[gavinp, 2013/04/17 10:56:28]

const scoped_refptr<> seems the right way to go. This came about because of
copy-replace of the WeakPtrs, (which also should perhaps have been references).

Your point about raw pointers is well said though; but we are IO blocked and
have massive correctness concerns because of the racing of IO latency; I like
having the safe passage.

[rvargas (doing something else), 2013/04/17 19:56:38]

Note that the recommended practice (Google C++ premier) is to use raw pointers
on arguments (as much as possible), and it is not so much about performance
improvements. So my preference is also to pass a pointer to the object and not a
smart pointer, unless there is a very strong reason to do otherwise. This is a
general comment, without looking at the specifics of this particular method.

                        const base::FilePath& path,
                        const std::string& key,
                        Entry** entry,
                        const CompletionCallback& callback);
 
-  static int CreateEntry(base::WeakPtr<SimpleIndex> index,
+  static int CreateEntry(scoped_refptr<SimpleIndex> index,
                          const base::FilePath& path,
                          const std::string& key,
                          Entry** entry,
                          const CompletionCallback& callback);
 
-  static int DoomEntry(base::WeakPtr<SimpleIndex> index,
+  static int DoomEntry(scoped_refptr<SimpleIndex> index,
                        const base::FilePath& path,
                        const std::string& key,
                        const CompletionCallback& callback);
@@ -80,34 +82,30 @@ class SimpleEntryImpl : public Entry {
   virtual int ReadyForSparseIO(const CompletionCallback& callback) OVERRIDE;
 
  private:
-  SimpleEntryImpl(SimpleSynchronousEntry* synchronous_entry,
-                  base::WeakPtr<SimpleIndex> index);
+  SimpleEntryImpl(scoped_refptr<SimpleIndex> index,
+                  const base::FilePath& path,
+                  const std::string& key);
+

[Philippe, 2013/04/17 10:19:22]

Nit: extra blank line.

[gavinp, 2013/04/17 10:56:28]

Done.

 
   virtual ~SimpleEntryImpl();
 
   // Called after a SimpleSynchronousEntry has completed CreateEntry() or
-  // OpenEntry(). Constructs the new SimpleEntryImpl (if |result| is net::OK)
-  // and passes it back to the caller via |out_entry|. Also runs
+  // OpenEntry(). If |sync_entry| is non-NULL, creation is successful and we
+  // can return |this| SimpleEntryImpl to |*out_entry|. Runs
   // |completion_callback|.
-  static void CreationOperationComplete(
-      base::WeakPtr<SimpleIndex> index,
-      const CompletionCallback& completion_callback,
-      const std::string& key,
+  void CreationOperationComplete(
       Entry** out_entry,
+      const CompletionCallback& completion_callback,
       SimpleSynchronousEntry* sync_entry);
 
   // Called after a SimpleSynchronousEntry has completed an asynchronous IO
   // operation, such as ReadData() or WriteData(). Calls |completion_callback|.
-  // If |entry| no longer exists, then it ensures |sync_entry| is closed.
-  static void EntryOperationComplete(
-      base::WeakPtr<SimpleIndex> index,
+  void EntryOperationComplete(
       const CompletionCallback& completion_callback,
-      base::WeakPtr<SimpleEntryImpl> entry,
-      SimpleSynchronousEntry* sync_entry,
       int result);
 
-  // Called on construction and also after the completion of asynchronous IO to
-  // initialize the IO thread copies of data returned by synchronous accessor
+  // Called on initialization and also after the completion of asynchronous IO
+  // to initialize the IO thread copies of data returned by synchronous accessor
   // functions. Copies data from |synchronous_entry_| into |this|, so that
   // values can be returned during our next IO operation.
   void SetSynchronousData();
@@ -116,10 +114,7 @@ class SimpleEntryImpl : public Entry {
   // thread, in all cases. |io_thread_checker_| documents and enforces this.
   base::ThreadChecker io_thread_checker_;
 
-  base::WeakPtrFactory<SimpleEntryImpl> weak_ptr_factory_;
-
-  // |path_| and |key_| are copied from the synchronous entry on construction,
-  // and never updated as they are const.
+  const scoped_refptr<SimpleIndex> index_;
   const base::FilePath path_;
   const std::string key_;
 
@@ -140,8 +135,6 @@ class SimpleEntryImpl : public Entry {
   // and false after. Used to ensure thread safety by not allowing multiple
   // threads to access the |synchronous_entry_| simultaneously.
   bool synchronous_entry_in_use_by_worker_;
-
-  base::WeakPtr<SimpleIndex> index_;
 };
 
 }  // namespace disk_cache