Remove void** from disk_cache interface.

net/disk_cache/disk_cache.h

Index: net/disk_cache/disk_cache.h
diff --git a/net/disk_cache/disk_cache.h b/net/disk_cache/disk_cache.h
index 4f64b9afb02ac4219532491f592d0c7b269985fb..da73433eeff3b210aa6f36496ec6c43adb8a2e31 100644
--- a/net/disk_cache/disk_cache.h
+++ b/net/disk_cache/disk_cache.h
@@ -13,6 +13,7 @@
 
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/time/time.h"
 #include "net/base/cache_type.h"
 #include "net/base/completion_callback.h"
@@ -62,6 +63,11 @@ NET_EXPORT int CreateCacheBackend(
 // The root interface for a disk cache instance.
 class NET_EXPORT Backend {
  public:
+  class EnumerationState {
+   public:
+    virtual ~EnumerationState() = 0;
+  };
+  typedef scoped_ptr<EnumerationState> Iterator;

[rvargas (doing something else), 2014/09/05 02:01:29]

I don't think we need (or want) an extra typedef to hide a scoped_ptr

[gavinp, 2014/09/11 23:57:04]

The latest upload removes the typedef.

   typedef net::CompletionCallback CompletionCallback;
 
   // If the backend is destroyed when there are operations in progress (any
@@ -70,7 +76,7 @@ class NET_EXPORT Backend {
   // half way (for instance, dooming just a few entries). Note that pending IO
   // for a given Entry (as opposed to the Backend) will still generate a
   // callback from within this method.
-  virtual ~Backend() {}
+  virtual ~Backend();
 
   // Returns the type of this cache.
   virtual net::CacheType GetCacheType() const = 0;
@@ -123,32 +129,15 @@ class NET_EXPORT Backend {
   virtual int DoomEntriesSince(base::Time initial_time,
                                const CompletionCallback& callback) = 0;
 
-  // Enumerates the cache. Initialize |iter| to NULL before calling this method
-  // the first time. That will cause the enumeration to start at the head of
-  // the cache. For subsequent calls, pass the same |iter| pointer again without
-  // changing its value. This method returns ERR_FAILED when there are no more
-  // entries to enumerate. When the entry pointer is no longer needed, its
-  // Close method should be called. The return value is a net error code. If
-  // this method returns ERR_IO_PENDING, the |callback| will be invoked when the
-  // |next_entry| is available. The pointer to receive the |next_entry| must
-  // remain valid until the operation completes.
-  //
-  // NOTE: This method does not modify the last_used field of the entry, and
-  // therefore it does not impact the eviction ranking of the entry. However,
-  // an enumeration will go through all entries on the cache only if the cache
-  // is not modified while the enumeration is taking place. Significantly
-  // altering the entry pointed by |iter| (for example, deleting the entry) will
-  // invalidate |iter|. Performing operations on an entry that modify the entry
-  // may result in loops in the iteration, skipped entries or similar.
-  virtual int OpenNextEntry(void** iter, Entry** next_entry,
+  // Enumerates the cache. Initialize |iter| to NULL for the first call, and
+  // provide it again for subsequent calls. For each successful step of
+  // enumeration, OpenNextEntry returns OK and provides |next_entry|, which is
+  // open and should be closed by the caller. This method returns ERR_FAILED at
+  // the end of enumeration. If any entry in a cache is modified during
+  // iteration, then the result of this function is thereafter undefined.
+  virtual int OpenNextEntry(Iterator* iter, Entry** next_entry,

[rvargas (doing something else), 2014/09/05 02:01:29]

So far I'm unconvinced about the API change.

The current pattern is pretty standard: the cache returns an opaque handle to
the consumer and the consumer uses it for further enumerations until it doesn't
want to enumerate anymore. The consumer cannot do any operation directly with
that handle.

Sure, it is a C-style pattern, but it is extremely common and makes sure the
cache is in control of the iteration at all times.

What you propose opens up the caller to know, and do more with the iterator than
it should. It also poses restrictions on the implementation that I don't see
fully justified: deletion of the object is tied to the consumer[1] and cleanup
when the cache goes away may be more complicated (as now it has to make sure
that anything related to an iterator can be left behind because it gave up
ownership to the consumer, who really should not care about what is tied to the
iterator).

[1] Arguably, calling a function to say that the iterator is not needed is
basically the same as directly deleting the iterator, but there is a conceptual
difference between "I delete this because I own it, and I know what to do with
it" and "I don't know what this is and I don't need it anymore so I'll tell the
owner about it". It limits what the cache can do with it, or may force an extra
layer of isolation just for the sake of it. I still think that the second
message is the one that the API should deliver.

Another way to put it is that so far I much rather let the cache cleanup all
enumerations at destruction (and leave the caller with handles that are invalid,
but there's no cache to call), than leave a bunch of enumerators lying around
after the cache is gone.

[gavinp, 2014/09/11 23:57:04]

The existing pattern of using a void** is neither standard or extremely common
in chrome APIs. This CL removes the last instance of us advertising such an API
in net/.

A caller cannot "do more with the iterator than it should."

You say you would "rather let the cache cleanup all enumerations at
destruction." Your suggestion contradicts the current documentation of the
EndEnumeration() function "... the enumeration must be ended ...".

Happily, this change reduces the need for documentation. The APIs use of
scoped_ptr<> defines ownership and forces callers to use it safely. As a result,
a lot of confusing comment text describing a complex ownership rule is removed.
Even better, many existing tests that violated those rules now only test
documented behaviour.

On 2014/09/05 02:01:29, rvargas wrote:

                             const CompletionCallback& callback) = 0;
 
-  // Releases iter without returning the next entry. Whenever OpenNextEntry()
-  // returns true, but the caller is not interested in continuing the
-  // enumeration by calling OpenNextEntry() again, the enumeration must be
-  // ended by calling this method with iter returned by OpenNextEntry().
-  virtual void EndEnumeration(void** iter) = 0;
-
   // Return a list of cache statistics.
   virtual void GetStats(
       std::vector<std::pair<std::string, std::string> >* stats) = 0;
@@ -310,7 +299,7 @@ class NET_EXPORT Entry {
   virtual int ReadyForSparseIO(const CompletionCallback& callback) = 0;
 
  protected:
-  virtual ~Entry() {}
+  virtual ~Entry();
 };
 
 struct EntryDeleter {