Remove void** from disk_cache interface.

net/disk_cache/simple/simple_backend_impl.cc

 // Copyright (c) 2013 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.
 
 #include "net/disk_cache/simple/simple_backend_impl.h"
 
 #include <algorithm>
 #include <cstdlib>
 #include <functional>
 
@@ skipping 455 matching lines @@
       base::Bind(&SimpleBackendImpl::IndexReadyForDoom, AsWeakPtr(),
                  initial_time, end_time, callback));
 }
 
 int SimpleBackendImpl::DoomEntriesSince(
     const Time initial_time,
     const CompletionCallback& callback) {
   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) {}
 
-void SimpleBackendImpl::EndEnumeration(void** iter) {
-  SimpleIndex::HashList* entry_list =
-      static_cast<SimpleIndex::HashList*>(*iter);
-  delete entry_list;
-  *iter = NULL;
+  // 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 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(
     std::vector<std::pair<std::string, std::string> >* stats) {
   std::pair<std::string, std::string> item;
   item.first = "Cache type";
   item.second = "Simple Cache";
   stats->push_back(item);
 }
 
@@ skipping 107 matching lines @@
     return active_it->second->DoomEntry(callback);
 
   // There's no pending dooms, nor any open entry. We can make a trivial
   // call to DoomEntries() to delete this entry.
   std::vector<uint64> entry_hash_vector;
   entry_hash_vector.push_back(entry_hash);
   DoomEntries(&entry_hash_vector, callback);
   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,
     const scoped_refptr<SimpleEntryImpl>& simple_entry,
     const CompletionCallback& callback,
     int error_code) {
   if (error_code != net::OK) {
     callback.Run(error_code);
     return;
   }
@@ skipping 33 matching lines @@
       simple_entry->Close();
       final_code = net::ERR_FAILED;
     } else {
       DCHECK_EQ(simple_entry->entry_hash(), simple_util::GetEntryHashKey(key));
     }
     SIMPLE_CACHE_UMA(BOOLEAN, "KeyMatchedOnOpen", cache_type_, key_matches);
   }
   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,
     int result) {
   std::for_each(
       entry_hashes->begin(), entry_hashes->end(),
       std::bind1st(std::mem_fun(&SimpleBackendImpl::OnDoomComplete),
                    this));
   callback.Run(result);
 }
 
 void SimpleBackendImpl::FlushWorkerPoolForTesting() {
   if (g_sequenced_worker_pool)
     g_sequenced_worker_pool->FlushForTesting();
 }
 
 }  // namespace disk_cache