Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13839011:
Asynchronous initialization in Simple Index. (Closed)
|
Created:
7 years, 8 months ago by felipeg Modified:
7 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, dmazzoni Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAsynchronous initialization in Simple Index.
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193665
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=193944
Patch Set 1 #Patch Set 2 : #
Total comments: 11
Patch Set 3 : #Patch Set 4 : #
Total comments: 6
Patch Set 5 : wainting for pasko CL to be committed #Patch Set 6 : syncing #Patch Set 7 : egor's change is in #
Total comments: 46
Patch Set 8 : egors nits #Patch Set 9 : gavins comments #Patch Set 10 : #Patch Set 11 : #
Total comments: 4
Patch Set 12 : #
Messages
Total messages: 17 (0 generated)
Sending these comments out right away, since I'm 3 uploads behind and want to start my review again. https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_disk_format.cc (right): https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_disk_format.cc:79: if (out_entry_metadata->entry_size == 0) So 0 is both an invalid size, and the size of a 0 length datum? https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_index.cc (right): https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.cc:53: io_thread_(io_thread) { } I think I'm more used to {} or { } A quick grepfight on the former shows: gavin@avclub:~/chrome/src$ git grep -n -e '{}' -- '*' | wc 26694 182935 18669027 gavin@avclub:~/chrome/src$ git grep -n -e '{ }' -- '*' | wc 1437 7904 125774 https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.cc:62: void SimpleIndex::LoadFromDisk() { Method ordering in the .cc file should match the .h file. https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.cc:133: base::Time::Now(), 0)); indent. https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_index.h (right): https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.h:69: // Must run on IO Thread. Can you add a base::ThreadChecker to this class? https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.h:101: // the Cache Thread. Is this comment right anymore? I think these come from the constructor (which is nicer anyway).
I think threading gets too confusing when nonstatic methods on the same class
are called on different threads. I can't follow SimpleIndex very well now. I'd
like to really guard it a lot more carefully. Here's one suggestion:
Can we make SimpleIndex::LoadFromDisk() and SimpleIndex::RestoreFromDisk() into
static methods? Perhaps like:
typedef base::Callback<void(scoped_ptr<EntrySet>)> MergeCallback;
static void LoadFromDisk(scoped_refptr<base::TaskRunner>& callback_thread,
const MergeCallback& merge_callback);
Then instead of mutating this->initializing_set, they can gather up all of their
index entries into an EntrySet which is then passed in the PostTask through a
scoped_ptr<>. So we change the signature of MergeInitializingSet to this: (note
it's nonstatic)
void MergeInitializingSet(scoped_ptr<EntrySet> index_file_entries);
Then Initialize looks like this:
void SimpleIndex::Initialize() {
MergeCallback merge_callback =
base::Bind(&SimpleIndex::MergeInitializingSet, GetWeakPtr());
base::WorkerPool::PostTask(FROM_HERE,
base::Bind(&SimpleIndex::LoadFromDisk,
io_thread_, merge_callback),
true);
}
In this way, we have no nonstatic methods called on different threads; it's a
lot easier for me to verify this is safe.
https://codereview.chromium.org/13839011/diff/3001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_disk_format.h (right):
https://codereview.chromium.org/13839011/diff/3001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_disk_format.h:85: // Merge two EntryMetada
instances.
typo: EntryMetada -> EntryMetadata
https://codereview.chromium.org/13839011/diff/3001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.cc (right):
https://codereview.chromium.org/13839011/diff/3001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.cc:58: base::Unretained(this)),
I don't think this use of base::Unretained is safe. Can't the destructor be
called before the cache is done initializing?
https://codereview.chromium.org/13839011/diff/3001/net/disk_cache/simple/simp...
File net/disk_cache/simple/simple_index.h (right):
https://codereview.chromium.org/13839011/diff/3001/net/disk_cache/simple/simp...
net/disk_cache/simple/simple_index.h:63: // Load index file from disk, if file
is corrupted it calls RestoreFromDisk().
Suggestion: // Load index from disk. If it is corrupted, call RestoreFromDisk().
https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_disk_format.cc (right): https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_disk_format.cc:79: if (out_entry_metadata->entry_size == 0) On 2013/04/10 10:51:56, gavinp wrote: > So 0 is both an invalid size, and the size of a 0 length datum? You said there should never be a valid entry with a 0 lenght datum Since the size also includes the header, metadata, etc... https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_index.cc (right): https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.cc:53: io_thread_(io_thread) { } On 2013/04/10 10:51:56, gavinp wrote: > I think I'm more used to {} or { > } > > A quick grepfight on the former shows: > > gavin@avclub:~/chrome/src$ git grep -n -e '{}' -- '*' | wc > 26694 182935 18669027 > gavin@avclub:~/chrome/src$ git grep -n -e '{ }' -- '*' | wc > 1437 7904 125774 Done. https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.cc:133: base::Time::Now(), 0)); On 2013/04/10 10:51:56, gavinp wrote: > indent. Done. https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_index.h (right): https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.h:69: // Must run on IO Thread. On 2013/04/10 10:51:56, gavinp wrote: > Can you add a base::ThreadChecker to this class? Done. https://codereview.chromium.org/13839011/diff/1001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.h:101: // the Cache Thread. On 2013/04/10 10:51:56, gavinp wrote: > Is this comment right anymore? I think these come from the constructor (which is > nicer anyway). Done. https://codereview.chromium.org/13839011/diff/3001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_disk_format.h (right): https://codereview.chromium.org/13839011/diff/3001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_disk_format.h:85: // Merge two EntryMetada instances. On 2013/04/10 11:45:44, gavinp wrote: > typo: EntryMetada -> EntryMetadata Done. https://codereview.chromium.org/13839011/diff/3001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_index.cc (right): https://codereview.chromium.org/13839011/diff/3001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.cc:58: base::Unretained(this)), On 2013/04/10 11:45:44, gavinp wrote: > I don't think this use of base::Unretained is safe. Can't the destructor be > called before the cache is done initializing? Done. https://codereview.chromium.org/13839011/diff/3001/net/disk_cache/simple/simp... File net/disk_cache/simple/simple_index.h (right): https://codereview.chromium.org/13839011/diff/3001/net/disk_cache/simple/simp... net/disk_cache/simple/simple_index.h:63: // Load index file from disk, if file is corrupted it calls RestoreFromDisk(). On 2013/04/10 11:45:44, gavinp wrote: > Suggestion: // Load index from disk. If it is corrupted, call RestoreFromDisk(). Done.
initial naming nits, will look at it more tomorrow https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... File net/disk_cache/simple/simple_backend_impl.cc (right): https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_backend_impl.cc:58: completition_callback); s/completition_callback/completion_callback/ https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... File net/disk_cache/simple/simple_backend_impl.h (right): https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_backend_impl.h:70: void InitializeIndex(const CompletionCallback& callback, int rv); Should probably comment that this should run on the IO thread. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_backend_impl.h:72: static void CreateDirectory( I would rather call it EnsureDirectoryConsistent(), there will be a little more checking shortly.. Should comment that this runs on the cache thread. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... File net/disk_cache/simple/simple_disk_format.cc (right): https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_disk_format.cc:75: void EntryMetadata::Merge(const EntryMetadata& entry_metadata, I would have just called the parameters: from, to
https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... File net/disk_cache/simple/simple_backend_impl.cc (right): https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_backend_impl.cc:58: completition_callback); On 2013/04/10 17:51:01, pasko wrote: > s/completition_callback/completion_callback/ Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... File net/disk_cache/simple/simple_backend_impl.h (right): https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_backend_impl.h:70: void InitializeIndex(const CompletionCallback& callback, int rv); On 2013/04/10 17:51:01, pasko wrote: > Should probably comment that this should run on the IO thread. Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_backend_impl.h:72: static void CreateDirectory( On 2013/04/10 17:51:01, pasko wrote: > I would rather call it EnsureDirectoryConsistent(), there will be a little more > checking shortly.. > > Should comment that this runs on the cache thread. Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... File net/disk_cache/simple/simple_disk_format.cc (right): https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_disk_format.cc:75: void EntryMetadata::Merge(const EntryMetadata& entry_metadata, On 2013/04/10 17:51:01, pasko wrote: > I would have just called the parameters: > from, to Done.
Very close! https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_backend_impl.cc (right): https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.cc:48: index_.reset(new SimpleIndex(cache_thread, Move this into the ctor list. https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_backend_impl.h (right): https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.h:41: int Init(const CompletionCallback& completition_callback); Nit: spelling (in the method too). https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.h:68: typedef base::Callback<void(int)> InitializeIndexCallback; How about naming the int? Call it "result" or io_result. https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.h:70: void InitializeIndex(const CompletionCallback& callback, int rv); Does every method on this class now run on the IO thread, or are there still cache thread holdovers? https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.h:70: void InitializeIndex(const CompletionCallback& callback, int rv); rv isn't a good name for an argument; it's more typically an automatic that is going to be returned. Call it "result" or "io_result" instead? https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_disk_format.cc (right): https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_disk_format.cc:75: void EntryMetadata::Merge(const EntryMetadata& entry_metadata, On 2013/04/10 17:51:01, pasko wrote: > I would have just called the parameters: > from, to +1 https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_index.cc (right): https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:42: explicit FileAutoCloser(base::PlatformFile* file) : file_(file) { } No need for base::PlatformFile*. You can just use base::PlatformFile here. https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:44: if (file_) Can this ever be null? https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:45: base::ClosePlatformFile(*file_); No error checking? https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:47: private: DISALLOW_COPY_AND_ASSIGN(FileAutoCloser); https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:90: scoped_ptr<FileAutoCloser> auto_close_index_file( Why is this a scoped_ptr? https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:104: return RestoreFromDisk(index_filename, io_thread, merge_callback); Should we delete the index file here, too? https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:143: scoped_ptr<EntrySet> initializing_set(new EntrySet()); This name is a bit confusing to me. How about index_file_entries ? https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:162: // UpdateEntrySize will be called. I liked this comment better when it was: // It will be updated later when the SimpleEntryImpl finishes opening or // creating the new entry, and then UpdateEntrySize will be called. https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:210: void SimpleIndex::InsertInternal( Do we really want this? Just call insert maybe? https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:287: if (current_entry != entries_set_.end()) { Why handle the case of inserting and merging differently? How about instead: EntrySet::iterator current_entry = entries_set_.insert(SimpleIndexFile::EntryMetadata()).first; SimpleIndexFile::EntryMetadata::Merge(it->second, &(current_entry->second)); cache_size_ += current_entry->second.entry_size; 11 lines becomes 3! https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:332: void SimpleIndex::WriteToDisk() { Can you add a thread checker call on EVERY method? It's much clearer. https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_index.h (right): https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.h:99: base::hash_set<std::string> removals_set_; I like: removed_entries_ better for a name.
https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... File net/disk_cache/simple/simple_backend_impl.cc (right): https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_backend_impl.cc:48: index_.reset(new SimpleIndex(cache_thread, On 2013/04/11 09:56:32, gavinp wrote: > Move this into the ctor list. Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_backend_impl.cc:48: index_.reset(new SimpleIndex(cache_thread, On 2013/04/11 09:56:32, gavinp wrote: > Move this into the ctor list. Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... File net/disk_cache/simple/simple_backend_impl.h (right): https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_backend_impl.h:41: int Init(const CompletionCallback& completition_callback); On 2013/04/11 09:56:32, gavinp wrote: > Nit: spelling (in the method too). Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_backend_impl.h:68: typedef base::Callback<void(int)> InitializeIndexCallback; On 2013/04/11 09:56:32, gavinp wrote: > How about naming the int? Call it "result" or io_result. > Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_backend_impl.h:70: void InitializeIndex(const CompletionCallback& callback, int rv); On 2013/04/11 09:56:32, gavinp wrote: > Does every method on this class now run on the IO thread, or are there still > cache thread holdovers? Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... File net/disk_cache/simple/simple_index.cc (right): https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:42: explicit FileAutoCloser(base::PlatformFile* file) : file_(file) { } On 2013/04/11 09:56:32, gavinp wrote: > No need for base::PlatformFile*. You can just use base::PlatformFile here. Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:42: explicit FileAutoCloser(base::PlatformFile* file) : file_(file) { } On 2013/04/11 09:56:32, gavinp wrote: > No need for base::PlatformFile*. You can just use base::PlatformFile here. Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:44: if (file_) On 2013/04/11 09:56:32, gavinp wrote: > Can this ever be null? Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:45: base::ClosePlatformFile(*file_); On 2013/04/11 09:56:32, gavinp wrote: > No error checking? Nope the only error that can happen is that the file is not opened. And that can happen as not an error, since the destructor of this class will be called even if we can't open the file on the LoadFromDisk function. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:90: scoped_ptr<FileAutoCloser> auto_close_index_file( On 2013/04/11 09:56:32, gavinp wrote: > Why is this a scoped_ptr? Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:90: scoped_ptr<FileAutoCloser> auto_close_index_file( On 2013/04/11 09:56:32, gavinp wrote: > Why is this a scoped_ptr? Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:104: return RestoreFromDisk(index_filename, io_thread, merge_callback); On 2013/04/11 09:56:32, gavinp wrote: > Should we delete the index file here, too? we don't need to. RestoreFromDisk and Merge will take care of creating the new file and deleting the old one. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:143: scoped_ptr<EntrySet> initializing_set(new EntrySet()); On 2013/04/11 09:56:32, gavinp wrote: > This name is a bit confusing to me. How about index_file_entries ? Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:162: // UpdateEntrySize will be called. On 2013/04/11 09:56:32, gavinp wrote: > I liked this comment better when it was: > > // It will be updated later when the SimpleEntryImpl finishes opening or > // creating the new entry, and then UpdateEntrySize will be called. Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:210: void SimpleIndex::InsertInternal( On 2013/04/11 09:56:32, gavinp wrote: > Do we really want this? Just call insert maybe? I want to avoid having this long line through the code: make_pair(entry_metadata.GetHashKey(), entry_metadata) When we change the hash_map to be a hash_set we will probably change it here. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:287: if (current_entry != entries_set_.end()) { On 2013/04/11 09:56:32, gavinp wrote: > Why handle the case of inserting and merging differently? How about instead: > > EntrySet::iterator current_entry = > entries_set_.insert(SimpleIndexFile::EntryMetadata()).first; > SimpleIndexFile::EntryMetadata::Merge(it->second, &(current_entry->second)); > cache_size_ += current_entry->second.entry_size; > > 11 lines becomes 3! > > > Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_index.cc:332: void SimpleIndex::WriteToDisk() { On 2013/04/11 09:56:32, gavinp wrote: > Can you add a thread checker call on EVERY method? It's much clearer. Done. https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... File net/disk_cache/simple/simple_index.h (right): https://chromiumcodereview.appspot.com/13839011/diff/23001/net/disk_cache/sim... net/disk_cache/simple/simple_index.h:99: base::hash_set<std::string> removals_set_; On 2013/04/11 09:56:32, gavinp wrote: > I like: removed_entries_ better for a name. Done.
https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_index.cc (right): https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:143: scoped_ptr<EntrySet> initializing_set(new EntrySet()); On 2013/04/11 11:25:45, felipeg1 wrote: > On 2013/04/11 09:56:32, gavinp wrote: > > This name is a bit confusing to me. How about index_file_entries ? > > Done. Done? https://codereview.chromium.org/13839011/diff/23001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:332: void SimpleIndex::WriteToDisk() { On 2013/04/11 11:25:45, felipeg1 wrote: > On 2013/04/11 09:56:32, gavinp wrote: > > Can you add a thread checker call on EVERY method? It's much clearer. > > Done. Done?
https://codereview.chromium.org/13839011/diff/43001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_backend_impl.h (right): https://codereview.chromium.org/13839011/diff/43001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.h:44: virtual ~SimpleBackendImpl(); Method ordering: Constructor, Destructor, then other methods. Same in the .cc file. https://codereview.chromium.org/13839011/diff/43001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_index.cc (right): https://codereview.chromium.org/13839011/diff/43001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:352: SimpleIndex::~SimpleIndex() { Missed one for the DCHECK. Also see the ordering.
https://codereview.chromium.org/13839011/diff/43001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_backend_impl.h (right): https://codereview.chromium.org/13839011/diff/43001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_backend_impl.h:44: virtual ~SimpleBackendImpl(); On 2013/04/11 11:59:26, gavinp wrote: > Method ordering: Constructor, Destructor, then other methods. Same in the .cc > file. Done. https://codereview.chromium.org/13839011/diff/43001/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_index.cc (right): https://codereview.chromium.org/13839011/diff/43001/net/disk_cache/simple/sim... net/disk_cache/simple/simple_index.cc:352: SimpleIndex::~SimpleIndex() { On 2013/04/11 11:59:26, gavinp wrote: > Missed one for the DCHECK. Also see the ordering. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/13839011/43003
Message was sent while issue was closed.
Change committed as 193665
We will re-land this as soon as Egor land the fix on the unittest.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/13839011/43003
Message was sent while issue was closed.
Change committed as 193944 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
