Refactor our SimpleIndex file format and serialization to use Pickle instead of the previously bugg…

net/disk_cache/simple/simple_index.h

 // 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.
 
 #ifndef NET_DISK_CACHE_SIMPLE_SIMPLE_INDEX_H_
 #define NET_DISK_CACHE_SIMPLE_SIMPLE_INDEX_H_
 
 #include <map>
 #include <string>
 
 #include "base/basictypes.h"
-#include "base/file_util.h"
+#include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/hash_tables.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
-#include "net/disk_cache/disk_cache.h"
-#include "net/disk_cache/simple/simple_disk_format.h"
 
 namespace base {
 class TaskRunner;
 }
 
 namespace disk_cache {
 
+class EntryMetadata {
+ public:
+  EntryMetadata();
+  EntryMetadata(uint64 hash_key,
+                base::Time last_used_time,
+                uint64 entry_size);
+
+  uint64 GetHashKey() const;

[Philippe, 2013/04/15 13:57:57]

Nit: I would make this an inline getter as you did with GetEntrySize().

[felipeg, 2013/04/15 14:39:07]

Done.

+
+  base::Time GetLastUsedTime() const;
+  void SetLastUsedTime(const base::Time& last_used_time);
+
+  uint64 GetEntrySize() const { return entry_size_; }
+  void SetEntrySize(uint64 entry_size) { entry_size_ = entry_size; }
+
+  // If we had cheese we could make a sandwitch if we had bread. But we got
+  // only pickles.

[pasko-google - do not use, 2013/04/15 14:23:55]

thanks for the funny comment, and please remove it ;)

[felipeg, 2013/04/15 15:37:04]

Done.

+  // Serialize the data into the provided pickle.
+  void Serialize(Pickle* pickle) const;
+  static bool DeSerialize(PickleIterator* it,

[Philippe, 2013/04/15 13:57:57]

I would make DeSerialize() an instance method to make it more symmetric with
Serialize() (given that you are already providing a default constructor).

EntryMetadata metadata;
metadata.DeSerialize(&pickle_it);

[felipeg, 2013/04/15 14:39:07]

Done.

+                          EntryMetadata* out);

[pasko-google - do not use, 2013/04/15 14:23:55]

nit: fits in one line

[felipeg, 2013/04/15 15:37:04]

Done.

[felipeg, 2013/04/15 15:37:04]

Done.

+
+  // Merge two EntryMetadata instances.
+  // The existing current valid data in this object will prevail.
+  void MergeWith(const EntryMetadata& entry_metadata);
+
+ private:
+  uint64 hash_key_;
+
+  // This is the serialized format from Time::ToInternalValue().
+  // If you want to make calculations/comparisons, you should use the
+  // base::Time() class. Use the GetLastUsedTime() method above.
+  int64 last_used_time_;

[pasko-google - do not use, 2013/04/15 14:23:55]

since we serialize now without writing bare structures to buffers, this one
should be a base::Time as encouraged in its header file. If for a later change,
add a TODO plz.

[felipeg, 2013/04/15 15:37:04]

Done.

+
+  uint64 entry_size_;  // Storage size in bytes.
+};
+
+// TODO(felipeg): This way we are storing the hash_key twice (as the

[Philippe, 2013/04/15 13:57:57]

Nit: the ')' is missing.

[felipeg, 2013/04/15 14:39:07]

Done.

+// hash_map::key and as a member of EntryMetadata. We could save space if we
+// use a hash_set.
+typedef base::hash_map<uint64, EntryMetadata> EntrySet;
+
+void InsertInEntrySet(EntrySet* entry_set,

[Philippe, 2013/04/15 13:57:57]

Nit: output parameters should be last.

[Philippe, 2013/04/15 13:57:57]

I would make this function return whether the insertion happened (hash_{map,set}
do return this information). This might let you avoid this famous anti-pattern
in some cases: lookup + insertion in case the key did not exist. I'm not sure
this applies to your case.

[felipeg, 2013/04/15 14:39:07]

Done.

+                      const EntryMetadata& entry_metadata);
+
+typedef base::Callback<void(scoped_ptr<EntrySet>)> IndexCompletionCallback;
+
 // This class is not Thread-safe.
 class SimpleIndex
     : public base::SupportsWeakPtr<SimpleIndex> {
  public:
   SimpleIndex(
       const scoped_refptr<base::TaskRunner>& cache_thread,
       const scoped_refptr<base::TaskRunner>& io_thread,
       const base::FilePath& path);
 
   virtual ~SimpleIndex();
@@ skipping 10 matching lines @@
   bool UseIfExists(const std::string& key);
 
   void WriteToDisk();
 
   // Update the size (in bytes) of an entry, in the metadata stored in the
   // index. This should be the total disk-file size including all streams of the
   // entry.
   bool UpdateEntrySize(const std::string& key, uint64 entry_size);
 
  private:
-  // TODO(felipeg): This way we are storing the hash_key twice (as the
-  // hash_map::key and as a member of EntryMetadata. We could save space if we
-  // use a hash_set.
-  typedef base::hash_map<uint64, SimpleIndexFile::EntryMetadata> EntrySet;
-
-  typedef base::Callback<void(scoped_ptr<EntrySet>)> MergeCallback;
-
-  static void InsertInternal(
-      EntrySet* entry_set,
-      const SimpleIndexFile::EntryMetadata& entry_metadata);
-
-  // Load index from disk. If it is corrupted, call RestoreFromDisk().
-  static void LoadFromDisk(
-      const base::FilePath& index_filename,
-      const scoped_refptr<base::TaskRunner>& io_thread,
-      const MergeCallback& merge_callback);
-
-  // Enumerates all entries' files on disk and regenerates the index.
-  static void RestoreFromDisk(
-      const base::FilePath& index_filename,
-      const scoped_refptr<base::TaskRunner>& io_thread,
-      const MergeCallback& merge_callback);
-
   // Must run on IO Thread.
   void MergeInitializingSet(scoped_ptr<EntrySet> index_file_entries);
 
   // |out_buffer| needs to be pre-allocated. The serialized index is stored in
   // |out_buffer|.
   void Serialize(std::string* out_buffer);
 
-  bool OpenIndexFile();
-  bool CloseIndexFile();
-
-  static void UpdateFile(const base::FilePath& index_filename,
-                         const base::FilePath& temp_filename,
-                         scoped_ptr<std::string> buffer);
-
   EntrySet entries_set_;
   uint64 cache_size_;  // Total cache storage size in bytes.
 
   // This stores all the hash_key of entries that are removed during
   // initialization.
   base::hash_set<uint64> removed_entries_;
   bool initialized_;
 
   base::FilePath index_filename_;
 
   scoped_refptr<base::TaskRunner> cache_thread_;
   scoped_refptr<base::TaskRunner> io_thread_;
 
   // All nonstatic SimpleEntryImpl methods should always be called on the IO
   // thread, in all cases. |io_thread_checker_| documents and enforces this.
   base::ThreadChecker io_thread_checker_;
 };
 
 }  // namespace disk_cache
 
 #endif  // NET_DISK_CACHE_SIMPLE_SIMPLE_INDEX_H_