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

Refactor our SimpleIndex file format and serialization to use Pickle instead of the previously buggy way of writing uninitialized structs and ints into the file.

BUG=230772
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194347

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194387

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194403

[Philippe, 2013-04-15 13:57:56]

Thanks Felipe! Sorry for the nits :)

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:6: 
<utility> should be added ideally for make_pair.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:9: #include "base/message_loop.h"
Nit: base/logging.h is missing.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:18: EntryMetadata::EntryMetadata() {}
You need to have an initializer list here.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:21: base::Time last_used_time,
Nit: indentation issue.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:25: entry_size_(entry_size) {}
Nit: the '}' should be on the line below in cases where the declaration is
spread across multiple lines.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:27: uint64 EntryMetadata::GetHashKey()
const {
See my comment in the header.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:39: void EntryMetadata::Serialize(Pickle*
pickle) const {
I would leverage the fact that you are using size-defined integers at least here
by using a COMPILE_ASSERT with sizeof(EntryMetadata) == 192.

This will notify you at compile time in case this function goes out of sync.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:48: EntryMetadata* out) {
Nit: lines should not be broken when they don't have to.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:51: return 
it->ReadUInt64(&(out->hash_key_)) &&
Nit: extra space after return.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:51: return 
it->ReadUInt64(&(out->hash_key_)) &&
Nit: extra parenthesis around out->hash_key_.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:31: uint64 GetHashKey() const;
Nit: I would make this an inline getter as you did with GetEntrySize().

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:43: static bool DeSerialize(PickleIterator*
it,
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);

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:61: // TODO(felipeg): This way we are
storing the hash_key twice (as the
Nit: the ')' is missing.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:66: void InsertInEntrySet(EntrySet*
entry_set,
Nit: output parameters should be last.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:66: void InsertInEntrySet(EntrySet*
entry_set,
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.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:14: #include "base/logging.h"
ref_counted.h is missing.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:38: IndexMetadata* out);
Nit: I believe this fits on the line above.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:46: uint32 version_;
FWIW, I believe you might be able to save 32 bits by making this field last. I
generally have struct fields ordered by descending size to avoid unnecessary
padding bytes.

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.cc:71: &contents)) {
Nit: this should fit on the line above.

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.cc:77: Pickle pickle (contents.data(),
contents.size());
Nit: extra space after 'pickle'.

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.cc:188: // Serialize the
|index_metadata|.
Nit: I'm not sure this comment is really useful.

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:70: scoped_ptr<Pickle> pickle);
It's not clear to me why this method is taking ownership of |pickle|. After you
call this method (i.e. |pickle| is "moved"), the callee's scoped_ptr is
invalidated which can lead to NULL pointer dereferencing. This might be ok for
now if this is the last operation you do on |pickle| but this seems less than
ideal in the long term.

I think the only sane reason to take onwership of |pickle| would be in case the
method is asynchronous but this does not seem to be the case.

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:73: struct PickleHeader : public
Pickle::Header {
Nit: I don't think you need to expose this in this header.

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:77: static uint32
CalculatePickleCRC(const Pickle& pickle);
Nit: I would move this to an anonymous namespace in the .cc file.

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:79: SimpleIndexFile() {}
I would replace this constructor and destructor with DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:82: 
Nit: extra blank line.

https://codereview.chromium.org/14263005/diff/11003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/14263005/diff/11003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:167: EntrySet::iterator it =
index_file_entries->find(hash_key);
See my comment in a previous header about the lookup + insert anti-pattern.

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

Two major requests:
1. a dedicated serialization/deserialization test
2. make layering more obvious: classes used for keeping and
serializing/deserializing data should not know about passing data around threads

and a few nits below, as usual..

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:21: base::Time last_used_time,
align

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:66: // TODO(felipeg): Use a hash_set
instead of a hash_map.
I saw this TODO in another place, no need to repeat yourself

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:40: // only pickles.
thanks for the funny comment, and please remove it ;)

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:44: EntryMetadata* out);
nit: fits in one line

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:56: int64 last_used_time_;
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.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.cc:116: base::Bind(completion_callback,
can we please make pickling un-pickling independent from callbacks? It would be
easier to test then, also easier to read when serialization/deserialization is
contained within one plain data object.

All the job of passing data across threads would be more readable to find in the
backend, not even in the SimpleIndex

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:26: // Simple Index File format is a
pickle serialized data of IndexMetadata and
makes sense to expand this a little:
The file format is as follows: one instance of serialized |IndexMetadata|
followed serialized |EntryMetadata| entries repeated |number_of_entries| amount
of times.

Also, I would prefer the name IndexFileHeader, less confusing than "metadata"

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:27: // EntryMetadata objects. To know
more about the pickle format, see
there is no need to reference what a pickled format is, just take the word
"pickle" in quotes on line 26 above

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:57: static void RestoreFromDisk(
why is this in SimpleIndexFile? Logically it belongs more to the SimpleIndex.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_util.cc (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_util.cc:7: #include "base/logging.h"
logging is not needed

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_util.h (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_util.h:13: class Time;
not needed here

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_util.h:19: const size_t
kEntryHashKeyAsHexStringSize = 2 * sizeof(uint64);
it is encouraged to take sizeof(variable) instead of sizeof(type), in is less
error prone, and in this CL requires only a couple of small changes, so please
do it

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

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:6: 
On 2013/04/15 13:57:57, Philippe wrote:
> <utility> should be added ideally for make_pair.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:9: #include "base/message_loop.h"
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: base/logging.h is missing.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:18: EntryMetadata::EntryMetadata() {}
On 2013/04/15 13:57:57, Philippe wrote:
> You need to have an initializer list here.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:21: base::Time last_used_time,
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: indentation issue.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:25: entry_size_(entry_size) {}
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: the '}' should be on the line below in cases where the declaration is
> spread across multiple lines.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:27: uint64 EntryMetadata::GetHashKey()
const {
On 2013/04/15 13:57:57, Philippe wrote:
> See my comment in the header.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:39: void EntryMetadata::Serialize(Pickle*
pickle) const {
On 2013/04/15 13:57:57, Philippe wrote:
> I would leverage the fact that you are using size-defined integers at least
here
> by using a COMPILE_ASSERT with sizeof(EntryMetadata) == 192.
> 
> This will notify you at compile time in case this function goes out of sync.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:48: EntryMetadata* out) {
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: lines should not be broken when they don't have to.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:51: return 
it->ReadUInt64(&(out->hash_key_)) &&
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: extra space after return.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:51: return 
it->ReadUInt64(&(out->hash_key_)) &&
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: extra parenthesis around out->hash_key_.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:31: uint64 GetHashKey() const;
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: I would make this an inline getter as you did with GetEntrySize().

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:43: static bool DeSerialize(PickleIterator*
it,
On 2013/04/15 13:57:57, Philippe wrote:
> 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);

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:61: // TODO(felipeg): This way we are
storing the hash_key twice (as the
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: the ')' is missing.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:66: void InsertInEntrySet(EntrySet*
entry_set,
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: output parameters should be last.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:14: #include "base/logging.h"
On 2013/04/15 13:57:57, Philippe wrote:
> ref_counted.h is missing.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:38: IndexMetadata* out);
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: I believe this fits on the line above.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:46: uint32 version_;
On 2013/04/15 13:57:57, Philippe wrote:
> FWIW, I believe you might be able to save 32 bits by making this field last. I
> generally have struct fields ordered by descending size to avoid unnecessary
> padding bytes. 

early optimization

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.cc:71: &contents)) {
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: this should fit on the line above.

Done.

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.cc:77: Pickle pickle (contents.data(),
contents.size());
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: extra space after 'pickle'.

Done.

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.cc:188: // Serialize the
|index_metadata|.
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: I'm not sure this comment is really useful.

Done.

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:70: scoped_ptr<Pickle> pickle);
On 2013/04/15 13:57:57, Philippe wrote:
> It's not clear to me why this method is taking ownership of |pickle|. After
you
> call this method (i.e. |pickle| is "moved"), the callee's scoped_ptr is
> invalidated which can lead to NULL pointer dereferencing. This might be ok for
> now if this is the last operation you do on |pickle| but this seems less than
> ideal in the long term.
> 
> I think the only sane reason to take onwership of |pickle| would be in case
the
> method is asynchronous but this does not seem to be the case.

This is used in a callback.
Discussed offline.

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:73: struct PickleHeader : public
Pickle::Header {
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: I don't think you need to expose this in this header.

Yes, but I think it is more readable this way.
PickleHeader is inside the SimpleIndexFile class namespace.

It documents the file format.

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:77: static uint32
CalculatePickleCRC(const Pickle& pickle);
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: I would move this to an anonymous namespace in the .cc file.

Done.

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:79: SimpleIndexFile() {}
On 2013/04/15 13:57:57, Philippe wrote:
> I would replace this constructor and destructor with DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/14263005/diff/1012/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:82: 
On 2013/04/15 13:57:57, Philippe wrote:
> Nit: extra blank line.

Done.

https://codereview.chromium.org/14263005/diff/11003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/14263005/diff/11003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:167: EntrySet::iterator it =
index_file_entries->find(hash_key);
On 2013/04/15 13:57:57, Philippe wrote:
> See my comment in a previous header about the lookup + insert anti-pattern.

I think this is an early optimization.
The code is more clear this way.

Once we change it to hash_set, I will revisit this decision.

[maekkamaikunnaluk, 2013-04-15 15:11:02]

0894211429

[maekkamaikunnaluk, 2013-04-15 15:25:12]

On 2013/04/15 15:11:02, maekkamaikunnaluk wrote:
> 0894211429

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

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:66: // TODO(felipeg): Use a hash_set
instead of a hash_map.
On 2013/04/15 14:23:55, pasko wrote:
> I saw this TODO in another place, no need to repeat yourself

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:40: // only pickles.
On 2013/04/15 14:23:55, pasko wrote:
> thanks for the funny comment, and please remove it ;)

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:44: EntryMetadata* out);
On 2013/04/15 14:23:55, pasko wrote:
> nit: fits in one line

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:44: EntryMetadata* out);
On 2013/04/15 14:23:55, pasko wrote:
> nit: fits in one line

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:56: int64 last_used_time_;
On 2013/04/15 14:23:55, pasko wrote:
> 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.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.cc:116: base::Bind(completion_callback,
On 2013/04/15 14:23:55, pasko wrote:
> can we please make pickling un-pickling independent from callbacks? It would
be
> easier to test then, also easier to read when serialization/deserialization is
> contained within one plain data object.
> 
> All the job of passing data across threads would be more readable to find in
the
> backend, not even in the SimpleIndex

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:26: // Simple Index File format is a
pickle serialized data of IndexMetadata and
On 2013/04/15 14:23:55, pasko wrote:
> makes sense to expand this a little:
> The file format is as follows: one instance of serialized |IndexMetadata|
> followed serialized |EntryMetadata| entries repeated |number_of_entries|
amount
> of times.
> 
> Also, I would prefer the name IndexFileHeader, less confusing than "metadata"

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:27: // EntryMetadata objects. To know
more about the pickle format, see
On 2013/04/15 14:23:55, pasko wrote:
> there is no need to reference what a pickled format is, just take the word
> "pickle" in quotes on line 26 above

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:57: static void RestoreFromDisk(
On 2013/04/15 14:23:55, pasko wrote:
> why is this in SimpleIndexFile? Logically it belongs more to the SimpleIndex.

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_util.cc (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_util.cc:7: #include "base/logging.h"
On 2013/04/15 14:23:55, pasko wrote:
> logging is not needed

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_util.h (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_util.h:13: class Time;
On 2013/04/15 14:23:55, pasko wrote:
> not needed here

Done.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_util.h:19: const size_t
kEntryHashKeyAsHexStringSize = 2 * sizeof(uint64);
On 2013/04/15 14:23:55, pasko wrote:
> it is encouraged to take sizeof(variable) instead of sizeof(type), in is less
> error prone, and in this CL requires only a couple of small changes, so please
> do it

Done.

[gavinp, 2013-04-15 15:40:54]

Mostly nits.

I'd like to see unit tests.

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_disk_format.h (right):

https://codereview.chromium.org/14263005/diff/4001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_disk_format.h:18: const uint64
kSimpleInitialMagicNumber = GG_UINT64_C(0xfcfb6d1ba7725c30);
Good to rename this file to simple_entry_format.h maybe?

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_disk_format.cc (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_disk_format.cc:6: 
We need #include <cstring>

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_disk_format.cc:7: #include "base/logging.h"
This is not needed.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_disk_format.cc:13: memset(this, 0, sizeof(*this));
std::memset is what's in <cstring>

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_disk_format.h (left):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_disk_format.h:11: #include "base/port.h"
We still need this.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_disk_format.h (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_disk_format.h:8: #include <string>
But we don't need this.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.cc:96: this->AsWeakPtr());
This fits on the previous line.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.h:14: #include "base/hash_tables.h"
needs #include "base/ref_counted.h"

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.h:41: // only pickles.
Lose this comment.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.h:44: bool DeSerialize(PickleIterator* it);
gavin@avclub:~/chrome/src$ git grep -n -e Deserialize -- * | wc
    432    1825   47688
gavin@avclub:~/chrome/src$ git grep -n -e DeSerialize -- * | wc
      3      12     290

I say we go with Deserialize.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.h:47: // The existing current valid data in
this object will prevail.
Nit: // Any existing valid data in |this| will prevail.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:26:
implicit_cast<uInt>(pickle.payload_size()));
You can lose this cast. Yes, I'm aware you cut and paste it from my code.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:16: #include "base/pickle.h"
Missing base/port.h.

Need base/memory/scoped_ptr.h.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:45: uint64 initial_magic_number_;
This may not be "initial" any more.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_util.h (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_util.h:6: #define
NET_DISK_CACHE_SIMPLE_SIMPLE_INDEX_UTIL_H_
Let's just move these into simple_util.h; if we need to comment by section, so
be it.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_synchronous_entry.cc (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_synchronous_entry.cc:23: #include
"net/disk_cache/simple/simple_index_util.h"
Really?

https://codereview.chromium.org/14263005/diff/10002/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/14263005/diff/10002/net/net.gyp#newcode381
net/net.gyp:381: 'disk_cache/simple/simple_disk_format.h',
Ordering.

[felipeg, 2013-04-15 15:56:09]

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_disk_format.cc (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_disk_format.cc:6: 
On 2013/04/15 15:40:54, gavinp wrote:
> We need #include <cstring>

Done.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_disk_format.cc:7: #include "base/logging.h"
On 2013/04/15 15:40:54, gavinp wrote:
> This is not needed.

Done.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_disk_format.cc:13: memset(this, 0, sizeof(*this));
On 2013/04/15 15:40:54, gavinp wrote:
> std::memset is what's in <cstring>

Done.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_disk_format.h (left):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_disk_format.h:11: #include "base/port.h"
On 2013/04/15 15:40:54, gavinp wrote:
> We still need this.

Done.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_disk_format.h (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_disk_format.h:8: #include <string>
On 2013/04/15 15:40:54, gavinp wrote:
> But we don't need this.

Done.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.cc:96: this->AsWeakPtr());
On 2013/04/15 15:40:54, gavinp wrote:
> This fits on the previous line.

Done.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.h:14: #include "base/hash_tables.h"
On 2013/04/15 15:40:54, gavinp wrote:
> needs #include "base/ref_counted.h"

Done.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.h:41: // only pickles.
On 2013/04/15 15:40:54, gavinp wrote:
> Lose this comment.

Done.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.h:44: bool DeSerialize(PickleIterator* it);
On 2013/04/15 15:40:54, gavinp wrote:
> gavin@avclub:~/chrome/src$ git grep -n -e Deserialize -- * | wc
>     432    1825   47688
> gavin@avclub:~/chrome/src$ git grep -n -e DeSerialize -- * | wc
>       3      12     290
> 
> I say we go with Deserialize.

Done.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.h:47: // The existing current valid data in
this object will prevail.
On 2013/04/15 15:40:54, gavinp wrote:
> Nit: // Any existing valid data in |this| will prevail.

Done.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:26:
implicit_cast<uInt>(pickle.payload_size()));
On 2013/04/15 15:40:54, gavinp wrote:
> You can lose this cast. Yes, I'm aware you cut and paste it from my code.

Done.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:16: #include "base/pickle.h"
On 2013/04/15 15:40:54, gavinp wrote:
> Missing base/port.h.
> 
> Need base/memory/scoped_ptr.h.

Done.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:45: uint64 initial_magic_number_;
On 2013/04/15 15:40:54, gavinp wrote:
> This may not be "initial" any more.

Done.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_synchronous_entry.cc (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_synchronous_entry.cc:23: #include
"net/disk_cache/simple/simple_index_util.h"
On 2013/04/15 15:40:54, gavinp wrote:
> Really?
Yes because of using GetEntryHashKeyAsHexString()

https://codereview.chromium.org/14263005/diff/10002/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/14263005/diff/10002/net/net.gyp#newcode381
net/net.gyp:381: 'disk_cache/simple/simple_disk_format.h',
On 2013/04/15 15:40:54, gavinp wrote:
> Ordering.

Done.

[gavinp, 2013-04-15 15:59:19]

This is mostly good. I'll look again once there's an upload with a unit test.

https://codereview.chromium.org/14263005/diff/1026/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/14263005/diff/1026/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:32: 
Lose blank line.

https://codereview.chromium.org/14263005/diff/1026/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:65: typedef base::hash_map<uint64,
EntryMetadata> EntrySet;
Why can't these type definitions be public: in SimpleIndex?

[gavinp, 2013-04-15 16:00:55]

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_disk_format.cc (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_disk_format.cc:13: memset(this, 0, sizeof(*this));
On 2013/04/15 15:56:09, felipeg1 wrote:
> On 2013/04/15 15:40:54, gavinp wrote:
> > std::memset is what's in <cstring>
> 
> Done.

Done?

[felipeg, 2013-04-15 18:01:59]

Added unittests.

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_disk_format.cc (right):

https://codereview.chromium.org/14263005/diff/10002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_disk_format.cc:13: memset(this, 0, sizeof(*this));
On 2013/04/15 16:00:55, gavinp wrote:
> On 2013/04/15 15:56:09, felipeg1 wrote:
> > On 2013/04/15 15:40:54, gavinp wrote:
> > > std::memset is what's in <cstring>
> > 
> > Done.
> 
> Done?
> 

Done.

[gavinp, 2013-04-15 19:27:46]

Looking good.

Have fun with your merge!

https://codereview.chromium.org/14263005/diff/1026/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/14263005/diff/1026/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:32: 
On 2013/04/15 15:59:19, gavinp wrote:
> Lose blank line.

Lose blank line.

https://codereview.chromium.org/14263005/diff/1026/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:65: typedef base::hash_map<uint64,
EntryMetadata> EntrySet;
On 2013/04/15 15:59:19, gavinp wrote:
> Why can't these type definitions be public: in SimpleIndex?

Why can't these type definitions be public: in SimpleIndex?

https://codereview.chromium.org/14263005/diff/7014/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_disk_format.h (right):

https://codereview.chromium.org/14263005/diff/7014/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_disk_format.h:18: const uint64
kSimpleInitialMagicNumber = GG_UINT64_C(0xfcfb6d1ba7725c30);
I think we should rename this file still.

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:97:
base::Bind(&SimpleIndex::MergeInitializingSet, this->AsWeakPtr());
Can just use AsWeakPtr() here.

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.cc:77: scoped_ptr<EntrySet>
SimpleIndexFile::Deserialize(const char* data,
Method ordering: methods in .cc should be in the same order as in the .h.

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:75:
FRIEND_TEST_ALL_PREFIXES(SimpleIndexFileTest, Serialize);
Blank line here.

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_util.cc (right):

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_util.cc:16: }
Blank lines at beginning and end of namespace, and }  // namespace comment.

Maybe moot as this file is dead.

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_synchronous_entry.cc (right):

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_synchronous_entry.cc:23: #include
"net/disk_cache/simple/simple_index_util.h"
I think this is not needed.

[felipeg, 2013-04-15 20:05:03]

https://codereview.chromium.org/14263005/diff/1026/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/14263005/diff/1026/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:32: 
On 2013/04/15 19:27:46, gavinp wrote:
> On 2013/04/15 15:59:19, gavinp wrote:
> > Lose blank line.
> 
> Lose blank line.

Done.

https://codereview.chromium.org/14263005/diff/1026/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:65: typedef base::hash_map<uint64,
EntryMetadata> EntrySet;
On 2013/04/15 15:59:19, gavinp wrote:
> Why can't these type definitions be public: in SimpleIndex?

Done.

https://codereview.chromium.org/14263005/diff/7014/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_disk_format.h (right):

https://codereview.chromium.org/14263005/diff/7014/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_disk_format.h:18: const uint64
kSimpleInitialMagicNumber = GG_UINT64_C(0xfcfb6d1ba7725c30);
On 2013/04/15 19:27:46, gavinp wrote:
> I think we should rename this file still.

Done.

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:97:
base::Bind(&SimpleIndex::MergeInitializingSet, this->AsWeakPtr());
On 2013/04/15 19:27:46, gavinp wrote:
> Can just use AsWeakPtr() here.

Done.

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_file.h:75:
FRIEND_TEST_ALL_PREFIXES(SimpleIndexFileTest, Serialize);
On 2013/04/15 19:27:46, gavinp wrote:
> Blank line here.

Done.

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index_util.cc (right):

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index_util.cc:16: }
On 2013/04/15 19:27:46, gavinp wrote:
> Blank lines at beginning and end of namespace, and }  // namespace comment.
> 
> Maybe moot as this file is dead.

Done.

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_synchronous_entry.cc (right):

https://codereview.chromium.org/14263005/diff/5007/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_synchronous_entry.cc:23: #include
"net/disk_cache/simple/simple_index_util.h"
On 2013/04/15 19:27:46, gavinp wrote:
> I think this is not needed.

Done.

[rvargas (doing something else), 2013-04-16 03:24:19]

rbstmp lgtm.

I'm a little surprised that the answer to "I write bugs when dealing with a
struct" is "let's not use a struct".

https://codereview.chromium.org/14263005/diff/24001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/14263005/diff/24001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:26: const uint64
kSimpleIndexMagicNumber = GG_UINT64_C(0x656e74657220796f);
Now that we're here, can somebody tell me what's the advantage of a 64 bit magic
number (over a 32 one)?

https://codereview.chromium.org/14263005/diff/24001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:28: // Simple Index File format is a
pickle serialized data of IndexMetadata and
I'm torn with this direction. Don't get me wrong, I have nothing against
pickles, they are great.

But caches is one of those things that people want to interact with. Maybe for
Android that is never going to be a problem because of separation between apps,
but for other platforms that's something to consider.

So on the one hand, we have the idea of building the simplest cache possible, in
part as a baseline for other implementations, in part as a test for a specific
platform. On the other hand, we want to spend the least amount of time writing
that cache. For the easiest and fastest to write cache, using a proven
technology is the smartest choice... and that would be delegating most of the
parsing to somebody else (like pickles here). I guess that's what we want.

Probably if someone comes and say "I want to write a quick backend that uses
sqlite for the index and files for the responses" I would be OK with that
(depending on the use case). I would not call that a "file" cache though... I
mention it because the index is the complicated part, not actually writing
responses to files.

If the simplest cache were something to consider as a long term solution for
something then playing nice with third party apps would be something relevant.
So as long as we accept that we don't want to go there (like saying later that
we want to do the same thing for, let's say Mac). then this is good enough.

[gavinp, 2013-04-16 11:22:12]

Just nits.

LGTM!

Well done.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.h:42: void Serialize(Pickle* pickle) const;
These need a forward def or an #include. I'm amazed this works!

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_unittest.cc (right):

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_unittest.cc:15: namespace {
blank line at beginning and end, and // namespace comment at end.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_unittest.cc:24: 
lose blank line

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_util.h (right):

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_util.h:19: // |key| is the regular HTTP Cache key,
which is a URL.
// |key| is the regular cache key, such as an URL.

Definitely mention the most important URL case, but it is just one case.

[felipeg, 2013-04-16 11:25:34]

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.h:42: void Serialize(Pickle* pickle) const;
On 2013/04/16 11:22:13, gavinp wrote:
> These need a forward def or an #include. I'm amazed this works!

Done.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_unittest.cc (right):

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_unittest.cc:15: namespace {
On 2013/04/16 11:22:13, gavinp wrote:
> blank line at beginning and end, and // namespace comment at end.

Done.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_unittest.cc:24: 
On 2013/04/16 11:22:13, gavinp wrote:
> lose blank line

Done.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_util.h (right):

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_util.h:19: // |key| is the regular HTTP Cache key,
which is a URL.
On 2013/04/16 11:22:13, gavinp wrote:
> // |key| is the regular cache key, such as an URL.
> 
> Definitely mention the most important URL case, but it is just one case.

Done.

[felipeg, 2013-04-16 11:58:05]

Committed patchset #18 manually as r194347 (presubmit successful).

[Philippe, 2013-04-16 11:58:47]

Thanks Felipe for the unit tests and sorry for the new nits. We are almost
there.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_entry_format.h (right):

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_format.h:7: 
Nit: extra blank line.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_format.h:19: const uint64
kSimpleInitialMagicNumber = GG_UINT64_C(0xfcfb6d1ba7725c30);
I believe this (and below) could result in multiple defined references on
certain tool chains.

If some client code uses the address of |kSimpleInitialMagicNumber| then
|kSimpleInitialMagicNumber| needs to reside in memory.
Which object file will it be stored in/what if two different compilation units
use it?

enum constants are a nice way to address this since they cannot be referenced
but 64 bit enums are not supported before C++11 unfortunately.

Does this magic number need to be 64 bits?

Or can you make these declarations extern and define the variables in
simple_entry_format.cc instead?

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_entry_format.h:26: static const int
kSimpleEntryFileCount = 3;
The 'static' here is confusing. This is a header file.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.cc:29: 
Nit: extra blank line.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.cc:15: 
Nit: extra blank line.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file.h (right):

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file.h:89: 
Nit: extra blank line.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file_unittest.cc (right):

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file_unittest.cc:4: 
base/basictypes.h for arrayzise()

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file_unittest.cc:54: bool
CompareTwoEntryMetadata(const EntryMetadata& a, const EntryMetadata& b) {
Nit: this should be a static method (or a const method in case you would feel
strongly).

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file_unittest.cc:59: 
Nit: extra blank line.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_unittest.cc (right):

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_unittest.cc:15: namespace {
I would make this anonymous namespace (and the rest of the file) part of the
disk_cache namespace as you did with the other unit test. Then you could remove
the using directive below.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_unittest.cc:19:
base::Time::FromInternalValue(kTestLastUsedTimeInternal);
Note that this will generate a static initializer. This is probably ok for
testing though.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_unittest.cc:26: class EntryMetadataTest  :
public testing::Test {
Nit: extra space before ':'.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_unittest.cc:34: void
CheckEntryMetadataValues(const EntryMetadata& entry_metadata) {
Nit: this should be static or const.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_util_unittest.cc (right):

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_util_unittest.cc:20:
ConvertEntryHashKeyToHexString(9223372036854775807UL));
I think this test should be enough. You don't need to test the same things
multiple times.

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_util_unittest.cc:29:
GetEntryHashKeyAsHexString("http://www.amazon.com/"));
Same here. I would keep a test for the empty string, a "regular" URL and a very
long one.

I would also make them more fine grained, e.g.:
TEST_F(SimpleUtilTest, GetEntryHashKeyWithEmptyString) {
...
}

TEST_F(SimpleUtilTest, GetEntryHashKeyWithVeryLongString) {
...
}

https://codereview.chromium.org/14263005/diff/32003/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_util_unittest.cc:46:
EXPECT_TRUE(GetEntryHashKeyFromHexString("0000000005f5e0ff", &hash_key));
Same here. I would have more fine grained non-duplicated unit tests, e.g.:

TEST_F(SimpleUtilTest, GetEntryHashKeyFromHexStringBasic) { ... }

TEST_F(SimpleUtilTest, GetEntryHashKeyFromHexStringWithInvalidSize) { ... }

TEST_F(SimpleUtilTest, GetEntryHashKeyFromHexStringWithInvalidCharacters) { ...
}

That way you don't need any comment.

[felipeg, 2013-04-16 17:44:57]

Committed patchset #20 manually as r194387 (presubmit successful).

[cpu_(ooo_6.6-7.5), 2013-04-16 17:57:44]

On 2013/04/16 17:44:57, felipeg1 wrote:
> Committed patchset #20 manually as r194387 (presubmit successful).

the further change given comments by Philippe need another l.gtm before commit

[felipeg, 2013-04-16 18:09:15]

On 2013/04/16 17:57:44, cpu wrote:
> On 2013/04/16 17:44:57, felipeg1 wrote:
> > Committed patchset #20 manually as r194387 (presubmit successful).
> 
> the further change given comments by Philippe need another l.gtm before commit

Which change ?
I did not change anything after Philippe's comments.

[felipeg, 2013-04-16 18:22:28]

On 2013/04/16 18:09:15, felipeg1 wrote:
> On 2013/04/16 17:57:44, cpu wrote:
> > On 2013/04/16 17:44:57, felipeg1 wrote:
> > > Committed patchset #20 manually as r194387 (presubmit successful).
> > 
> > the further change given comments by Philippe need another l.gtm before
commit
> 
> Which change ?
> I did not change anything after Philippe's comments.

FYI, the commit r194387 has failed (because of a problem in my local git client)
and only uploaded one file.
So I reverted it.
https://codereview.chromium.org/14300002

I will commit again, with the fix.

[felipeg, 2013-04-16 18:44:58]

Committed patchset #21 manually as r194403 (presubmit successful).

[cpu_(ooo_6.6-7.5), 2013-04-16 19:14:31]

On 2013/04/16 18:44:58, felipeg1 wrote:
> Committed patchset #21 manually as r194403 (presubmit successful).

Note that Philippe's made comments that you ignored broke the build
that is why I reverted
http://src.chromium.org/viewvc/chrome?revision=194373&view=revision