Remove void** from disk_cache interface.

net/disk_cache/simple/simple_backend_impl.cc

Index: net/disk_cache/simple/simple_backend_impl.cc
diff --git a/net/disk_cache/simple/simple_backend_impl.cc b/net/disk_cache/simple/simple_backend_impl.cc
index ffdc9b185b366ef95a2f1b6ca91a89714e6423f5..a211c634ca095cc4b1bb466b990a298bedb64f1f 100644
--- a/net/disk_cache/simple/simple_backend_impl.cc
+++ b/net/disk_cache/simple/simple_backend_impl.cc
@@ -473,20 +473,77 @@ int SimpleBackendImpl::DoomEntriesSince(
   return DoomEntriesBetween(initial_time, Time(), callback);
 }
 
-int SimpleBackendImpl::OpenNextEntry(void** iter,
-                                     Entry** next_entry,
-                                     const CompletionCallback& callback) {
-  CompletionCallback get_next_entry =
-      base::Bind(&SimpleBackendImpl::GetNextEntryInIterator, AsWeakPtr(), iter,
-                 next_entry, callback);
-  return index_->ExecuteWhenReady(get_next_entry);
-}
+class SimpleBackendImpl::SimpleIterator : public Iterator {
+ public:
+  explicit SimpleIterator(base::WeakPtr<SimpleBackendImpl> backend)
+      : backend_(backend),
+        weak_factory_(this) {}
+
+  // From Backend::Iterator:
+  virtual int OpenNextEntry(Entry** next_entry,
+                            const CompletionCallback& callback) OVERRIDE {
+    CompletionCallback open_next_entry_impl =
+        base::Bind(&SimpleIterator::OpenNextEntryImpl,
+                   weak_factory_.GetWeakPtr(), next_entry, callback);
+    return backend_->index_->ExecuteWhenReady(open_next_entry_impl);
+  }
+
+  void OpenNextEntryImpl(Entry** next_entry,

[pasko, 2014/09/12 08:10:26]

This has a lot in common with SimpleBackendImpl::GetNextEntryInIterator, but
it's hard to see what changed. In cases like this I would prefer reading it in
two changes: the first preserves the order of functions as it was, the second
(optional) brings the order in accordance with the order as it is in the header
by only moving the code without change.

FYI, not necessary now.

[gavinp, 2014/09/12 14:15:32]

You're right, that would have been a good way to present the change. My
apologies for making your review harder than it should have been.

As it happens, the ordering situation is relatively free here; the function
could have been defined in the same location the old one was: There's no rule
about where class definitions should appear in .cc, nor a rule about where the
definitions of methods for a class defined in .cc file. This location was chosen
so that all the relevant functions would be defined right above
SimpleBackendImpl::CreateIterator().

Perhaps more controversially, I chose inline function bodies because I felt the
functions were short enough that it was an easier read this way. Because the
class is defined in a .cc file, there's no concerns about linker explosion or
object file explosion from generated code.

+                         const CompletionCallback& callback,
+                         int index_initialization_error_code) {
+    if (!backend_) {
+      callback.Run(net::ERR_FAILED);
+      return;
+    }
+    if (index_initialization_error_code != net::OK) {
+      callback.Run(index_initialization_error_code);
+      return;
+    }
+    if (!hashes_to_enumerate_)
+      hashes_to_enumerate_ = backend_->index()->GetAllHashes().Pass();
+
+    while (!hashes_to_enumerate_->empty()) {
+      uint64 entry_hash = hashes_to_enumerate_->back();
+      hashes_to_enumerate_->pop_back();
+      if (backend_->index()->Has(entry_hash)) {
+        *next_entry = NULL;
+        CompletionCallback continue_iteration = base::Bind(
+            &SimpleIterator::CheckIterationReturnValue,
+            weak_factory_.GetWeakPtr(),
+            next_entry,
+            callback);
+        int error_code_open = backend_->OpenEntryFromHash(entry_hash,
+                                                          next_entry,
+                                                          continue_iteration);
+        if (error_code_open == net::ERR_IO_PENDING)
+          return;
+        if (error_code_open != net::ERR_FAILED) {
+          callback.Run(error_code_open);
+          return;
+        }
+      }
+    }
+    callback.Run(net::ERR_FAILED);
+  }
 
-void SimpleBackendImpl::EndEnumeration(void** iter) {
-  SimpleIndex::HashList* entry_list =
-      static_cast<SimpleIndex::HashList*>(*iter);
-  delete entry_list;
-  *iter = NULL;
+  void CheckIterationReturnValue(Entry** entry,
+                                 const CompletionCallback& callback,
+                                 int error_code) {
+    if (error_code == net::ERR_FAILED) {
+      OpenNextEntry(entry, callback);
+      return;
+    }
+    callback.Run(error_code);
+  }
+
+ private:
+  base::WeakPtr<SimpleBackendImpl> backend_;
+  scoped_ptr<std::vector<uint64> > hashes_to_enumerate_;
+  base::WeakPtrFactory<SimpleIterator> weak_factory_;
+};
+
+scoped_ptr<Backend::Iterator> SimpleBackendImpl::CreateIterator() {
+  return scoped_ptr<Iterator>(new SimpleIterator(AsWeakPtr()));
 }
 
 void SimpleBackendImpl::GetStats(
@@ -614,45 +671,6 @@ int SimpleBackendImpl::DoomEntryFromHash(uint64 entry_hash,
   return net::ERR_IO_PENDING;
 }
 
-void SimpleBackendImpl::GetNextEntryInIterator(
-    void** iter,
-    Entry** next_entry,
-    const CompletionCallback& callback,
-    int error_code) {
-  if (error_code != net::OK) {
-    callback.Run(error_code);
-    return;
-  }
-  if (*iter == NULL) {
-    *iter = index()->GetAllHashes().release();
-  }
-  SimpleIndex::HashList* entry_list =
-      static_cast<SimpleIndex::HashList*>(*iter);
-  while (entry_list->size() > 0) {
-    uint64 entry_hash = entry_list->back();
-    entry_list->pop_back();
-    if (index()->Has(entry_hash)) {
-      *next_entry = NULL;
-      CompletionCallback continue_iteration = base::Bind(
-          &SimpleBackendImpl::CheckIterationReturnValue,
-          AsWeakPtr(),
-          iter,
-          next_entry,
-          callback);
-      int error_code_open = OpenEntryFromHash(entry_hash,
-                                              next_entry,
-                                              continue_iteration);
-      if (error_code_open == net::ERR_IO_PENDING)
-        return;
-      if (error_code_open != net::ERR_FAILED) {
-        callback.Run(error_code_open);
-        return;
-      }
-    }
-  }
-  callback.Run(net::ERR_FAILED);
-}
-
 void SimpleBackendImpl::OnEntryOpenedFromHash(
     uint64 hash,
     Entry** entry,
@@ -706,18 +724,6 @@ void SimpleBackendImpl::OnEntryOpenedFromKey(
   callback.Run(final_code);
 }
 
-void SimpleBackendImpl::CheckIterationReturnValue(
-    void** iter,
-    Entry** entry,
-    const CompletionCallback& callback,
-    int error_code) {
-  if (error_code == net::ERR_FAILED) {
-    OpenNextEntry(iter, entry, callback);
-    return;
-  }
-  callback.Run(error_code);
-}
-
 void SimpleBackendImpl::DoomEntriesComplete(
     scoped_ptr<std::vector<uint64> > entry_hashes,
     const net::CompletionCallback& callback,