Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13907009:
Support optimistic Create and Write operations on the SimpleCache. (Closed)
|
Created:
7 years, 7 months ago by felipeg Modified:
7 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSupport optimistic Create and Write operations on the SimpleCache.
On CreateEntry we return net::OK immediately while issuing a WorkerPool thread to create the files.
If the file creation fails, we will fail every next operation in the same entry.
If another (read or write) operation is called in that entry before the completion of the creation, that entry is enqueued and we return IO_PENDING.
The same happens to the WriteData operation.
If the WriteData is called in an entry that does not have any operation in progress, we return buf_len immediately and issue a WorkerPool to write the data in the background.
If the WriteData operation fails every next operation will fail.
If we receive another Read/Write operation while the WriteData background operation did not complete, we always enqueue the operation and return IO_PENDING.
BUG=173400
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198286
Patch Set 1 #Patch Set 2 : #Patch Set 3 : rebase #Patch Set 4 : Unittests #Patch Set 5 : bugfix on an unittest #Patch Set 6 : another bugfix #Patch Set 7 : make linker happy #
Total comments: 34
Patch Set 8 : gavin comments #Patch Set 9 : Debug the unittest 4 on android #Patch Set 10 : gavin last comment #Patch Set 11 : sync #Patch Set 12 : fix SimpleCacheOptimistic4 #
Total comments: 26
Patch Set 13 : gavin comments #Patch Set 14 : egor comments #Patch Set 15 : Run browser tests with our simple cache #
Total comments: 3
Patch Set 16 : fix the http_cache DCHECK(FindActiveEntry) on Create #
Total comments: 4
Patch Set 17 : gavin comments #
Total comments: 5
Patch Set 18 : #
Total comments: 5
Patch Set 19 : #Patch Set 20 : #
Messages
Total messages: 45 (0 generated)
Here you go, folks! All tests passes continuously (its been running on a loop for half an hour in my machine and no flakes where found yet).
Can you please run try jobs on this?
On 2013/04/29 14:28:07, gavinp wrote: > Can you please run try jobs on this? (note: I can't do this myself, because this CL is not based on trunk; but it's a big enough CL that I really want to see try jobs as I review it)
Felipe, can you rebase this on top of the version of Close deflake in trunk right now, or perhaps wait until that lands, and rebase on trunk? Thanks.
Initial thoughts on testing: I think the existing unit tests aren't adequate for this, we need tests that intentionally put multiple items in flight and confirm correctness.
ptal
One quick pass. Hopefully some questions we can figure out. I'm excited! https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:2859: // To make sure the file creation completed we need to call open again so that Good catch on this. I think it should be moved out into simple_test_util.h, so that when it's used, it's clear that we're using a hack to force syncing to filesystem files. https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/entry_unit... File net/disk_cache/entry_unittest.cc (right): https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/entry_unit... net/disk_cache/entry_unittest.cc:2429: TEST_F(DiskCacheEntryTest, SimpleCacheOptmisitc) { speeling https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/entry_unit... net/disk_cache/entry_unittest.cc:2456: EXPECT_EQ(net::OK, This should be an ASSERT_EQ, so the crash stops when create fails. Otherwise we could crash the unittest with dereferences of entry later. This probably occurs in a few other places too. https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/entry_unit... net/disk_cache/entry_unittest.cc:2464: int ret = entry->WriteData( This seems wise. If we did an optimistic write after an optimistic create, we could lead the cache into thinking it had a valid entry when it did not. https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/entry_unit... net/disk_cache/entry_unittest.cc:2739: EXPECT_EQ(net::ERR_IO_PENDING, entry->WriteData( Could we just use something like: EXPECT_EQ(kSize1, WriteData(0, 0, buffer1, kSize1, false)); here? i.e. can we mix the disk_cache_test_base.h with your way more asynchronous API? https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:203: DCHECK(data_size_[stream_index] >= 0); DCHECK_LE please. https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:356: if (!out_entry) This seems like a belt and suspenders; why not make this a DCHECK() and trust callers to get this right? Alternatively, rename it, but I think I prefer the DCHECK with this clear name. https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:391: Entry** out_entry) { I'm confused by the value of optimistic open. Can you explain what the benefit of it is? https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:422: if (state_ != STATE_UNINITIALIZED) { This seems wrong. What if we're in STATE_IO_PENDING, and the pending IO operation is a Close? Why doesn't this make a test fail? https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:589: if (!completion_callback.is_null()) completion_callback.is_null() is another, redundant, signal of an optimistic create. That's a bit confusing. https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:596: // |this| may now be deleted. !!! if this comment is correct, then we are in grave danger, right? https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:242: bool optimistic_; I think we should either test with this flag (since i expect other cache users, like APP_CACHE might want it off), or we should remove this flag. WDYT?
https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/bac... File net/disk_cache/backend_unittest.cc (right): https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/bac... net/disk_cache/backend_unittest.cc:2859: // To make sure the file creation completed we need to call open again so that On 2013/05/01 13:11:22, gavinp wrote: > Good catch on this. > > I think it should be moved out into simple_test_util.h, so that when it's used, > it's clear that we're using a hack to force syncing to filesystem files. Done. https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/ent... File net/disk_cache/entry_unittest.cc (right): https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/ent... net/disk_cache/entry_unittest.cc:2429: TEST_F(DiskCacheEntryTest, SimpleCacheOptmisitc) { On 2013/05/01 13:11:22, gavinp wrote: > speeling Done. https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/ent... net/disk_cache/entry_unittest.cc:2456: EXPECT_EQ(net::OK, On 2013/05/01 13:11:22, gavinp wrote: > This should be an ASSERT_EQ, so the crash stops when create fails. Otherwise we > could crash the unittest with dereferences of entry later. This probably occurs > in a few other places too. Done. https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/ent... net/disk_cache/entry_unittest.cc:2464: int ret = entry->WriteData( On 2013/05/01 13:11:22, gavinp wrote: > This seems wise. If we did an optimistic write after an optimistic create, we > could lead the cache into thinking it had a valid entry when it did not. Done. https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/ent... net/disk_cache/entry_unittest.cc:2739: EXPECT_EQ(net::ERR_IO_PENDING, entry->WriteData( On 2013/05/01 13:11:22, gavinp wrote: > Could we just use something like: > > EXPECT_EQ(kSize1, WriteData(0, 0, buffer1, kSize1, false)); > > here? i.e. can we mix the disk_cache_test_base.h with your way more asynchronous > API? I would prefer not do that, because I want to make sure that we are returning net::ERR_IO_PENDING first, which means we don't run it opmistically, and then succeeding later in the callback. If we call EXPECT_EQ(kSize1, WriteData(0, 0, buffer1, kSize1, false)); The WriteData helper method also returns the correct value if the Write operation returns immediately (optmistically). There is no way to distinguish. https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:203: DCHECK(data_size_[stream_index] >= 0); On 2013/05/01 13:11:22, gavinp wrote: > DCHECK_LE please. Done. https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:356: if (!out_entry) On 2013/05/01 13:11:22, gavinp wrote: > This seems like a belt and suspenders; why not make this a DCHECK() and trust > callers to get this right? > > Alternatively, rename it, but I think I prefer the DCHECK with this clear name. Done. https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:391: Entry** out_entry) { On 2013/05/01 13:11:22, gavinp wrote: > I'm confused by the value of optimistic open. Can you explain what the benefit > of it is? This CL doesnt have optimistic open. Only Create and Write. https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:422: if (state_ != STATE_UNINITIALIZED) { On 2013/05/01 13:11:22, gavinp wrote: > This seems wrong. What if we're in STATE_IO_PENDING, and the pending IO > operation is a Close? Why doesn't this make a test fail? Because CreateInternal is pushed in the operations queue and is only called after whatever operation (such as Close) has finished. That is true to all XXInternal calls. We could put a DCHECK( state_ == READY || == FAILURE) in every XXInternal call ... but I don't think it is necessary. https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:589: if (!completion_callback.is_null()) On 2013/05/01 13:11:22, gavinp wrote: > completion_callback.is_null() is another, redundant, signal of an optimistic > create. That's a bit confusing. It is not, it is just a side-effect, we should not rely in this "signal". In fact the API requires us to check that the callback is not null before calling. It is allowed to call operations with a null callback. And Actually if the callback is null we should be synchronous (which we don't support yet). https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.cc:596: // |this| may now be deleted. On 2013/05/01 13:11:22, gavinp wrote: > !!! if this comment is correct, then we are in grave danger, right? Done. https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://chromiumcodereview.appspot.com/13907009/diff/37004/net/disk_cache/sim... net/disk_cache/simple/simple_entry_impl.h:242: bool optimistic_; On 2013/05/01 13:11:22, gavinp wrote: > I think we should either test with this flag (since i expect other cache users, > like APP_CACHE might want it off), or we should remove this flag. WDYT? I will remove it and we add it later if we need it.
https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/entry_unit... File net/disk_cache/entry_unittest.cc (right): https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/entry_unit... net/disk_cache/entry_unittest.cc:2734: simple_entry->DoomEntry(cb.callback()); This won't link right. Instead: backend_->DoomEntry(key, cb.callback());
https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/entry_unit... File net/disk_cache/entry_unittest.cc (right): https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/entry_unit... net/disk_cache/entry_unittest.cc:2734: simple_entry->DoomEntry(cb.callback()); On 2013/05/02 09:49:34, gavinp wrote: > This won't link right. Instead: > > backend_->DoomEntry(key, cb.callback()); > Done.
http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/123201 Looks like the failure we discussed is happening on Linux, too. Did you confirm the same-address hypothesis?
Yes On Thu, May 2, 2013 at 12:57 PM, <gavinp@chromium.org> wrote: > http://build.chromium.org/p/**tryserver.chromium/builders/** > linux_rel/builds/123201<http://build.chromium.org/p/tryserver.chromium/builders/linux_rel/builds/123201> > > Looks like the failure we discussed is happening on Linux, too. Did you > confirm > the same-address hypothesis? > > https://codereview.chromium.**org/13907009/<https://codereview.chromium.org/1... >
Looking really good! https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:2859: // To make sure the file creation completed we need to call open again so that On 2013/05/02 09:49:27, felipeg wrote: > On 2013/05/01 13:11:22, gavinp wrote: > > Good catch on this. > > > > I think it should be moved out into simple_test_util.h, so that when it's > used, > > it's clear that we're using a hack to force syncing to filesystem files. > > Done. Not done, right? https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/entry_unit... File net/disk_cache/entry_unittest.cc (right): https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/entry_unit... net/disk_cache/entry_unittest.cc:2739: EXPECT_EQ(net::ERR_IO_PENDING, entry->WriteData( On 2013/05/02 09:49:27, felipeg wrote: > On 2013/05/01 13:11:22, gavinp wrote: > > Could we just use something like: > > > > EXPECT_EQ(kSize1, WriteData(0, 0, buffer1, kSize1, false)); > > > > here? i.e. can we mix the disk_cache_test_base.h with your way more > asynchronous > > API? > > I would prefer not do that, because I want to make sure that we are returning > net::ERR_IO_PENDING first, which means we don't run it opmistically, and then > succeeding later in the callback. > If we call > EXPECT_EQ(kSize1, WriteData(0, 0, buffer1, kSize1, false)); > > The WriteData helper method also returns the correct value if the Write > operation returns immediately (optmistically). > There is no way to distinguish. Good point. Can you call out that this write can't be optimistic and you're checking that? https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:589: if (!completion_callback.is_null()) On 2013/05/02 09:49:27, felipeg wrote: > On 2013/05/01 13:11:22, gavinp wrote: > > completion_callback.is_null() is another, redundant, signal of an optimistic > > create. That's a bit confusing. > > It is not, it is just a side-effect, we should not rely in this "signal". Hrm, confusing. > > In fact the API requires us to check that the callback is not null before > calling. It is allowed to call operations with a null callback. > And Actually if the callback is null we should be synchronous (which we don't > support yet). That second point, the synchronous interface, has to do with how sparse entries are implemented and for testing, it's not vital for us unless we want to reuse the sparse entry support, but maybe we want the test coverage? I'm not sure. https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/entry_unit... File net/disk_cache/entry_unittest.cc (right): https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/entry_unit... net/disk_cache/entry_unittest.cc:2500: reinterpret_cast<disk_cache::SimpleEntryImpl*>(entry)->HasOneRef()); This isn't a safe use of reinterpret_cast; static_cast<> is correct here. This is particularly scary since there's multiple inheritance involved. If someone changed simple_cache_entry to inherit from RefCounted first instead of second, I think this would start crashing. https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:252: this, Minor nit: I really think these look better all bunched up on a few lines. https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:345: Lose this blank line. https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:478: synchronous_entry_ = NULL; We probably don't need this line. https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:492: if (!callback.is_null()) Multi-line if statements should have braces. There's a few of these. https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:590: if (!completion_callback.is_null()) I'm not convinced we need these PostTasks if we are religious about always running the callback immediately before return, and never earlier. WDYT? https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:137: void OpenEntryInternal(const CompletionCallback& callback, Entry** out_entry); This change of parameter ordering is a bit confusing; the order was the same as in disk_cache::Backend, and now it's reversed. Why?
https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:2859: // To make sure the file creation completed we need to call open again so that On 2013/05/02 12:47:39, gavinp wrote: > On 2013/05/02 09:49:27, felipeg wrote: > > On 2013/05/01 13:11:22, gavinp wrote: > > > Good catch on this. > > > > > > I think it should be moved out into simple_test_util.h, so that when it's > > used, > > > it's clear that we're using a hack to force syncing to filesystem files. > > > > Done. > > Not done, right? I thought we agreed to leave it as a comment. https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/entry_unit... File net/disk_cache/entry_unittest.cc (right): https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/entry_unit... net/disk_cache/entry_unittest.cc:2739: EXPECT_EQ(net::ERR_IO_PENDING, entry->WriteData( On 2013/05/02 12:47:39, gavinp wrote: > On 2013/05/02 09:49:27, felipeg wrote: > > On 2013/05/01 13:11:22, gavinp wrote: > > > Could we just use something like: > > > > > > EXPECT_EQ(kSize1, WriteData(0, 0, buffer1, kSize1, false)); > > > > > > here? i.e. can we mix the disk_cache_test_base.h with your way more > > asynchronous > > > API? > > > > I would prefer not do that, because I want to make sure that we are returning > > net::ERR_IO_PENDING first, which means we don't run it opmistically, and then > > succeeding later in the callback. > > If we call > > EXPECT_EQ(kSize1, WriteData(0, 0, buffer1, kSize1, false)); > > > > The WriteData helper method also returns the correct value if the Write > > operation returns immediately (optmistically). > > There is no way to distinguish. > > Good point. Can you call out that this write can't be optimistic and you're > checking that? > Done. https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:589: if (!completion_callback.is_null()) On 2013/05/02 12:47:39, gavinp wrote: > On 2013/05/02 09:49:27, felipeg wrote: > > On 2013/05/01 13:11:22, gavinp wrote: > > > completion_callback.is_null() is another, redundant, signal of an optimistic > > > create. That's a bit confusing. > > > > It is not, it is just a side-effect, we should not rely in this "signal". > > Hrm, confusing. > > > > In fact the API requires us to check that the callback is not null before > > calling. It is allowed to call operations with a null callback. > > And Actually if the callback is null we should be synchronous (which we don't > > support yet). > > That second point, the synchronous interface, has to do with how sparse entries > are implemented and for testing, it's not vital for us unless we want to reuse > the sparse entry support, but maybe we want the test coverage? I'm not sure. I see. But I still don't see a problem with checking is_null() But I can change that if you want. What is your suggestion to avoid that ? https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/entry_unit... File net/disk_cache/entry_unittest.cc (right): https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/entry_unit... net/disk_cache/entry_unittest.cc:2500: reinterpret_cast<disk_cache::SimpleEntryImpl*>(entry)->HasOneRef()); On 2013/05/02 12:47:39, gavinp wrote: > This isn't a safe use of reinterpret_cast; static_cast<> is correct here. This > is particularly scary since there's multiple inheritance involved. > > If someone changed simple_cache_entry to inherit from RefCounted first instead > of second, I think this would start crashing. Done. https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:252: this, On 2013/05/02 12:47:39, gavinp wrote: > Minor nit: I really think these look better all bunched up on a few lines. Done. https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:345: On 2013/05/02 12:47:39, gavinp wrote: > Lose this blank line. Done. https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:478: synchronous_entry_ = NULL; On 2013/05/02 12:47:39, gavinp wrote: > We probably don't need this line. Yes we need. we have a dcheck in CloseOperationComplete https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:492: if (!callback.is_null()) On 2013/05/02 12:47:39, gavinp wrote: > Multi-line if statements should have braces. There's a few of these. Done. https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:590: if (!completion_callback.is_null()) On 2013/05/02 12:47:39, gavinp wrote: > I'm not convinced we need these PostTasks if we are religious about always > running the callback immediately before return, and never earlier. > > WDYT? We do need. In case of failure of an operation, when you call Callback.Run() the http_cach_transaction immediately calls Write() which makes us to run that Write completion_callback before we return here (in the first operation that failed). That squence causes a DCHECK to fail in the http_cache_transaction. We need the PostTasks in most places, but not every place (for example in case of success we don't need it). But I thought it is better to be consistent. Posttask is cheap https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:137: void OpenEntryInternal(const CompletionCallback& callback, Entry** out_entry); On 2013/05/02 12:47:39, gavinp wrote: > This change of parameter ordering is a bit confusing; the order was the same as > in disk_cache::Backend, and now it's reversed. Why? Output parameters should come last. That is not in the style guide but is a common practice. Do you want me to change back ?
I find it difficult to follow the logic of state transitions in the simple entry. Can we (maybe in some later CL?) make the transitions explicit and local in one dispatch loop and make the rest of work in helper functions (that would not touch the state themselves)? This would require significant changes, like adding an Operation abstraction instead of keeping everything bound in the callbacks. But I think in the end it would be significantly easier to read. Bonus points: * explicit state transitions -> easier to read * later can analyse the queue of operations, reorder, throwaway quickly without posting/executing tasks * faster than base::Bind, often would avoid heap allocations https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (left): https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. BUG=173400 https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:2910: an empty line preceding the comment looked better https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:539: // |this| may be destroyed after return here. actually no, |this| is still alive https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:109: // after that must fail too. this is not strictly true because we can still do optimistic creates, also Close succeeds.
BTW, please run browser_tests with it
I have a followup CL that gets rid of the operations_ queue. I wonder how clear the transitions will be after it? https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (right): https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:2859: // To make sure the file creation completed we need to call open again so that On 2013/05/02 13:55:58, felipeg wrote: > On 2013/05/02 12:47:39, gavinp wrote: > > On 2013/05/02 09:49:27, felipeg wrote: > > > On 2013/05/01 13:11:22, gavinp wrote: > > > > Good catch on this. > > > > > > > > I think it should be moved out into simple_test_util.h, so that when it's > > > used, > > > > it's clear that we're using a hack to force syncing to filesystem files. > > > > > > Done. > > > > Not done, right? > > I thought we agreed to leave it as a comment. Yeah. I am sorry for being unclear; I just want to make sure the discussion here reflects that so future readers, if there ever are any, know what's up. https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/13907009/diff/37004/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:589: if (!completion_callback.is_null()) On 2013/05/02 13:55:58, felipeg wrote: > On 2013/05/02 12:47:39, gavinp wrote: > > On 2013/05/02 09:49:27, felipeg wrote: > > > On 2013/05/01 13:11:22, gavinp wrote: > > > > completion_callback.is_null() is another, redundant, signal of an > optimistic > > > > create. That's a bit confusing. > > > > > > It is not, it is just a side-effect, we should not rely in this "signal". > > > > Hrm, confusing. > > > > > > In fact the API requires us to check that the callback is not null before > > > calling. It is allowed to call operations with a null callback. > > > And Actually if the callback is null we should be synchronous (which we > don't > > > support yet). > > > > That second point, the synchronous interface, has to do with how sparse > entries > > are implemented and for testing, it's not vital for us unless we want to reuse > > the sparse entry support, but maybe we want the test coverage? I'm not sure. > > I see. > But I still don't see a problem with checking is_null() > But I can change that if you want. What is your suggestion to avoid that ? Ugh, I was unclear again. I meant to say: OK, yeah, we do have to check it, I agree now. I was just expanding on why sync mode might or might not make sense in the future.
https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/backend_un... File net/disk_cache/backend_unittest.cc (left): https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/05/02 14:01:43, pasko wrote: > BUG=173400 Done. https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/backend_un... net/disk_cache/backend_unittest.cc:2910: On 2013/05/02 14:01:43, pasko wrote: > an empty line preceding the comment looked better Done. https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:539: // |this| may be destroyed after return here. On 2013/05/02 14:01:43, pasko wrote: > actually no, |this| is still alive Nope, It can be destroyed after we return of the ref counted ptr doesnt have any other reference. ACtually, that is true to any return. I just added this comment here because it was particularly intriguing to understand the behavior of one debugging session I was doing. I can remove the comment if you think it is not necessary. https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.h (right): https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.h:109: // after that must fail too. On 2013/05/02 14:01:43, pasko wrote: > this is not strictly true because we can still do optimistic creates, also Close > succeeds. No we can't do optimistic creates on a failed entry. We must be in STATE_UNINITIALIZED. About the close operation: That is true, but I didn't meant that. Let me be more clear.
https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:539: // |this| may be destroyed after return here. On 2013/05/02 14:10:05, felipeg wrote: > I can remove the comment if you think it is not necessary. That's fine to be, the word 'here' in the end made me confused, so I thought you meant 'before return', my bad. Please remove the 'here'. https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:590: if (!completion_callback.is_null()) On 2013/05/02 13:55:58, felipeg wrote: > On 2013/05/02 12:47:39, gavinp wrote: > > I'm not convinced we need these PostTasks if we are religious about always > > running the callback immediately before return, and never earlier. > > > > WDYT? > > We do need. > In case of failure of an operation, when you call Callback.Run() the > http_cach_transaction immediately calls Write() which makes us to run that Write > completion_callback before we return here (in the first operation that failed). > That squence causes a DCHECK to fail in the http_cache_transaction. > > We need the PostTasks in most places, but not every place (for example in case > of success we don't need it). > But I thought it is better to be consistent. > > Posttask is cheap I think if you move completion_callback.Run(...) _after_ MakeUninitialized(), it should work. But I did not test, of course. Is there a unittest that could catch this?
https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:590: if (!completion_callback.is_null()) On 2013/05/02 14:39:57, pasko wrote: > On 2013/05/02 13:55:58, felipeg wrote: > > On 2013/05/02 12:47:39, gavinp wrote: > > > I'm not convinced we need these PostTasks if we are religious about always > > > running the callback immediately before return, and never earlier. > > > > > > WDYT? > > > > We do need. > > In case of failure of an operation, when you call Callback.Run() the > > http_cach_transaction immediately calls Write() which makes us to run that > Write > > completion_callback before we return here (in the first operation that > failed). > > That squence causes a DCHECK to fail in the http_cache_transaction. > > > > We need the PostTasks in most places, but not every place (for example in case > > of success we don't need it). > > But I thought it is better to be consistent. > > > > Posttask is cheap > > I think if you move completion_callback.Run(...) _after_ MakeUninitialized(), it > should work. But I did not test, of course. Is there a unittest that could catch > this? It doesnt have anything to do with MakeUninitialized() it has to do with an intenral state of the http_cache_transaction. And there is not way to fix that by our side, besides posttask. We have to return before calling the callback. I think any unittest that do a write operation (for example) should catch it.
https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/13907009/diff/78002/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:590: if (!completion_callback.is_null()) On 2013/05/02 14:48:02, felipeg wrote: > On 2013/05/02 14:39:57, pasko wrote: > > On 2013/05/02 13:55:58, felipeg wrote: > > > On 2013/05/02 12:47:39, gavinp wrote: > > > > I'm not convinced we need these PostTasks if we are religious about always > > > > running the callback immediately before return, and never earlier. > > > > > > > > WDYT? > > > > > > We do need. > > > In case of failure of an operation, when you call Callback.Run() the > > > http_cach_transaction immediately calls Write() which makes us to run that > > Write > > > completion_callback before we return here (in the first operation that > > failed). > > > That squence causes a DCHECK to fail in the http_cache_transaction. > > > > > > We need the PostTasks in most places, but not every place (for example in > case > > > of success we don't need it). > > > But I thought it is better to be consistent. > > > > > > Posttask is cheap > > > > I think if you move completion_callback.Run(...) _after_ MakeUninitialized(), > it > > should work. But I did not test, of course. Is there a unittest that could > catch > > this? > > It doesnt have anything to do with MakeUninitialized() it has to do with an > intenral state of the http_cache_transaction. > And there is not way to fix that by our side, besides posttask. > We have to return before calling the callback. > > I think any unittest that do a write operation (for example) should catch it. I just ran DiskCacheBackendTest.* and DiskCacheEntryTest.* with this change and saw no new failures: ================================================================================ --- a/net/disk_cache/simple/simple_entry_impl.cc +++ b/net/disk_cache/simple/simple_entry_impl.cc @@ -580,12 +580,11 @@ void SimpleEntryImpl::CreationOperationComplete( if (*in_result != net::OK) { if (*in_result!= net::ERR_FILE_EXISTS) MarkAsDoomed(); - if (!completion_callback.is_null()) { - MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind( - completion_callback, net::ERR_FAILED)); - } MakeUninitialized(); state_ = STATE_FAILURE; + if (!completion_callback.is_null()) { + completion_callback.Run(net::ERR_FAILED); + } return; } // If out_entry is NULL, it means we already called ReturnEntryToCaller from ================================================================================ So if you claim that there is a tricky problem revealed in http_cache_transaction by not doing a PostTask here, it would be important to add a unittest to reproduce this kind of failure.
https://codereview.chromium.org/13907009/diff/99007/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/13907009/diff/99007/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:85: if (type_ == net::DISK_CACHE) { I thought you would change this in a separate CL, I just ran tests with this CL patched in, and could not figure out why some of the tests failed. In future please do not add changes to the CL that reviewers should not be agreeing with. This makes things complicated and unexpected.
https://codereview.chromium.org/13907009/diff/99007/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/13907009/diff/99007/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:85: if (type_ == net::DISK_CACHE) { On 2013/05/02 15:29:35, pasko wrote: > I thought you would change this in a separate CL, I just ran tests with this CL > patched in, and could not figure out why some of the tests failed. > > In future please do not add changes to the CL that reviewers should not be > agreeing with. This makes things complicated and unexpected. I recommended he do this; I thought it would be nice to share the results with the reviewers. I know I'm interested in them. Is it so bad if the patch set is named appropriately?
https://codereview.chromium.org/13907009/diff/99007/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/13907009/diff/99007/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:85: if (type_ == net::DISK_CACHE) { On 2013/05/02 15:32:40, gavinp wrote: > On 2013/05/02 15:29:35, pasko wrote: > > I thought you would change this in a separate CL, I just ran tests with this > CL > > patched in, and could not figure out why some of the tests failed. > > > > In future please do not add changes to the CL that reviewers should not be > > agreeing with. This makes things complicated and unexpected. > > I recommended he do this; I thought it would be nice to share the results with > the reviewers. I know I'm interested in them. > > Is it so bad if the patch set is named appropriately? sry, I normally do not look at patch set names, probably a good time to start doing that
OK this is scary: One browser test fails like this: shell> gdb --args browser_tests --gtest_filter=WebViewTest.IndexedDBIsolation --user-data-dir=/tmp/u --allow-file-access --gtest_also_run_disabled_tests --single_process --use-simple-cache-backend=on #0 base::debug::BreakDebugger () at ../../base/debug/debugger_posix.cc:262 #1 0x0000000003b6af3a in logging::LogMessage::~LogMessage (this=0x7fffdea1ed70, __in_chrg=<optimized out>) at ../../base/logging.cc:649 #2 0x00000000022aa475 in net::HttpCache::CreateEntry (this=0x10bf78dcd520, key="http://localhost:38474/files/extensions/platform_apps/web_view/isolation/storage1.html", entry=0x10bf781412a0, trans=0x10bf78141220) at ../../net/http/http_cache.cc:725 #3 0x00000000022b9f91 in net::HttpCache::Transaction::DoCreateEntry (this=0x10bf78141220) at ../../net/http/http_cache_transaction.cc:1063 #4 0x00000000022b7b66 in net::HttpCache::Transaction::DoLoop (this=0x10bf78141220, result=-2) at ../../net/http/http_cache_transaction.cc:635 #5 0x00000000022c0c0b in net::HttpCache::Transaction::OnIOComplete (this=0x10bf78141220, result=-2) at ../../net/http/http_cache_transaction.cc:2418 #6 0x00000000022c4d70 in base::internal::RunnableAdapter<void (net::HttpCache::Transaction::*)(int)>::Run (this=0x7fffdea21880, object=0x10bf78141220, a1=@0x7fffdea21944: -2) at ../../base/bind_internal.h:190 #7 0x00000000022c4bb7 in base::internal::InvokeHelper<true, void, base::internal::RunnableAdapter<void (net::HttpCache::Transaction::*)(int)>, void (base::WeakPtr<net::HttpCache::Transaction> const&, int const&)>::MakeItSo(base::internal::RunnableAdapter<void (net::HttpCache::Transaction::*)(int)>, base::WeakPtr<net::HttpCache::Transaction> const&, int const&) (runnable=..., a1=..., a2=@0x7fffdea21944: -2) at ../../base/bind_internal.h:911 #8 0x00000000022c4976 in base::internal::Invoker<1, base::internal::BindState<base::internal::RunnableAdapter<void (net::HttpCache::Transaction::*)(int)>, void (net::HttpCache::Transaction*, int), void (base::WeakPtr<net::HttpCache::Transaction>)>, void (net::HttpCache::Transaction*, int)>::Run(base::internal::BindStateBase*, int const&) (base=0x10bf78d9d5d0, x2=@0x7fffdea21944: -2) at ../../base/bind_internal.h:1228 #9 0x000000000082a20c in base::Callback<void (int)>::Run(int const&) const (this=0x10bf78141478, a1=@0x7fffdea21944: -2) at ../../base/callback.h:436 #10 0x00000000022ac67a in net::HttpCache::WorkItem::NotifyTransaction (this=0x10bf78e27870, result=-2, entry=0x0) at ../../net/http/http_cache.cc:144 #11 0x00000000022abe94 in net::HttpCache::OnIOComplete (this=0x10bf78dcd520, result=-2, pending_op=0x10bf78f3c800) at ../../net/http/http_cache.cc:1076 #12 0x00000000022abf19 in net::HttpCache::OnPendingOpComplete (cache=..., pending_op=0x10bf78f3c800, rv=-2) at ../../net/http/http_cache.cc:1087 DCHECK(!FindActiveEntry(key));
Fixed thanks to Gavin: https://codereview.chromium.org/11787007/diff/26002/net/http/http_cache.cc On 2013/05/02 15:56:19, pasko wrote: > OK this is scary: > > One browser test fails like this: > shell> gdb --args browser_tests --gtest_filter=WebViewTest.IndexedDBIsolation > --user-data-dir=/tmp/u --allow-file-access --gtest_also_run_disabled_tests > --single_process --use-simple-cache-backend=on > > #0 base::debug::BreakDebugger () at ../../base/debug/debugger_posix.cc:262 > #1 0x0000000003b6af3a in logging::LogMessage::~LogMessage (this=0x7fffdea1ed70, > __in_chrg=<optimized out>) at ../../base/logging.cc:649 > #2 0x00000000022aa475 in net::HttpCache::CreateEntry (this=0x10bf78dcd520, > key="http://localhost:38474/files/extensions/platform_apps/web_view/isolation/storage1.html", > entry=0x10bf781412a0, trans=0x10bf78141220) at ../../net/http/http_cache.cc:725 > #3 0x00000000022b9f91 in net::HttpCache::Transaction::DoCreateEntry > (this=0x10bf78141220) at ../../net/http/http_cache_transaction.cc:1063 > #4 0x00000000022b7b66 in net::HttpCache::Transaction::DoLoop > (this=0x10bf78141220, result=-2) at ../../net/http/http_cache_transaction.cc:635 > #5 0x00000000022c0c0b in net::HttpCache::Transaction::OnIOComplete > (this=0x10bf78141220, result=-2) at > ../../net/http/http_cache_transaction.cc:2418 > #6 0x00000000022c4d70 in base::internal::RunnableAdapter<void > (net::HttpCache::Transaction::*)(int)>::Run (this=0x7fffdea21880, > object=0x10bf78141220, a1=@0x7fffdea21944: -2) at ../../base/bind_internal.h:190 > #7 0x00000000022c4bb7 in base::internal::InvokeHelper<true, void, > base::internal::RunnableAdapter<void (net::HttpCache::Transaction::*)(int)>, > void (base::WeakPtr<net::HttpCache::Transaction> const&, int > const&)>::MakeItSo(base::internal::RunnableAdapter<void > (net::HttpCache::Transaction::*)(int)>, > base::WeakPtr<net::HttpCache::Transaction> const&, int const&) (runnable=..., > a1=..., a2=@0x7fffdea21944: -2) at ../../base/bind_internal.h:911 > #8 0x00000000022c4976 in base::internal::Invoker<1, > base::internal::BindState<base::internal::RunnableAdapter<void > (net::HttpCache::Transaction::*)(int)>, void (net::HttpCache::Transaction*, > int), void (base::WeakPtr<net::HttpCache::Transaction>)>, void > (net::HttpCache::Transaction*, int)>::Run(base::internal::BindStateBase*, int > const&) (base=0x10bf78d9d5d0, x2=@0x7fffdea21944: -2) at > ../../base/bind_internal.h:1228 > #9 0x000000000082a20c in base::Callback<void (int)>::Run(int const&) const > (this=0x10bf78141478, a1=@0x7fffdea21944: -2) at ../../base/callback.h:436 > #10 0x00000000022ac67a in net::HttpCache::WorkItem::NotifyTransaction > (this=0x10bf78e27870, result=-2, entry=0x0) at ../../net/http/http_cache.cc:144 > #11 0x00000000022abe94 in net::HttpCache::OnIOComplete (this=0x10bf78dcd520, > result=-2, pending_op=0x10bf78f3c800) at ../../net/http/http_cache.cc:1076 > #12 0x00000000022abf19 in net::HttpCache::OnPendingOpComplete (cache=..., > pending_op=0x10bf78f3c800, rv=-2) at ../../net/http/http_cache.cc:1087 > > DCHECK(!FindActiveEntry(key));
This is looking really good, and very exciting that it passes browser tests. https://codereview.chromium.org/13907009/diff/94011/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/13907009/diff/94011/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:85: if (type_ == net::DISK_CACHE) { When you do this, probably worth putting a really explicit comment about what it is. https://codereview.chromium.org/13907009/diff/94011/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/13907009/diff/94011/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:380: if (!callback.is_null()) { When does this happen?
https://codereview.chromium.org/13907009/diff/94011/net/disk_cache/cache_crea... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/13907009/diff/94011/net/disk_cache/cache_crea... net/disk_cache/cache_creator.cc:85: if (type_ == net::DISK_CACHE) { On 2013/05/03 08:19:12, gavinp wrote: > When you do this, probably worth putting a really explicit comment about what it > is. sorry this is for test only done https://codereview.chromium.org/13907009/diff/94011/net/disk_cache/simple/sim... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/13907009/diff/94011/net/disk_cache/simple/sim... net/disk_cache/simple/simple_entry_impl.cc:380: if (!callback.is_null()) { On 2013/05/03 08:19:12, gavinp wrote: > When does this happen? Done.
https://codereview.chromium.org/13907009/diff/108001/net/disk_cache/cache_cre... File net/disk_cache/cache_creator.cc (right): https://codereview.chromium.org/13907009/diff/108001/net/disk_cache/cache_cre... net/disk_cache/cache_creator.cc:85: if (type_ == net::DISK_CACHE) { This will need to be removed before landing. https://codereview.chromium.org/13907009/diff/108001/net/disk_cache/simple/si... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/13907009/diff/108001/net/disk_cache/simple/si... net/disk_cache/simple/simple_entry_impl.cc:1: g// Copyright (c) 2013 The Chromium Authors. All rights reserved. g// !!! https://codereview.chromium.org/13907009/diff/108001/net/disk_cache/simple/si... net/disk_cache/simple/simple_entry_impl.cc:263: if (truncate) Nit: needs braces. https://codereview.chromium.org/13907009/diff/108001/net/disk_cache/simple/si... net/disk_cache/simple/simple_entry_impl.cc:384: if (!callback.is_null()) { When is this used? https://codereview.chromium.org/13907009/diff/115003/net/http/http_cache.cc File net/http/http_cache.cc (right): https://codereview.chromium.org/13907009/diff/115003/net/http/http_cache.cc#n... net/http/http_cache.cc:728: Nit: extra blank line.
LGTM with that last nit of mine about braces addressed!
https://chromiumcodereview.appspot.com/13907009/diff/108001/net/disk_cache/si... File net/disk_cache/simple/simple_entry_impl.cc (right): https://chromiumcodereview.appspot.com/13907009/diff/108001/net/disk_cache/si... net/disk_cache/simple/simple_entry_impl.cc:263: if (truncate) On 2013/05/03 11:19:27, gavinp wrote: > Nit: needs braces. Done. https://chromiumcodereview.appspot.com/13907009/diff/115003/net/http/http_cac... File net/http/http_cache.cc (right): https://chromiumcodereview.appspot.com/13907009/diff/115003/net/http/http_cac... net/http/http_cache.cc:728: On 2013/05/03 11:19:27, gavinp wrote: > Nit: extra blank line. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/13907009/111013
Retried try job too often on ios_dbg_simulator for step(s) googleurl_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Looks like a flake to me.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/13907009/111013
Retried try job too often on ios_dbg_simulator for step(s) googleurl_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/13907009/111013
Retried try job too often on ios_dbg_simulator for step(s) googleurl_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
this looks good to me with small nits and removed browser_tests-related hack from cache_creator.cc. Thanks, Felipe! Exciting! https://codereview.chromium.org/13907009/diff/115003/net/disk_cache/simple/si... File net/disk_cache/simple/simple_entry_impl.cc (right): https://codereview.chromium.org/13907009/diff/115003/net/disk_cache/simple/si... net/disk_cache/simple/simple_entry_impl.cc:120: pending_operations_.size() == 0) { this would fit in one line https://codereview.chromium.org/13907009/diff/115003/net/disk_cache/simple/si... net/disk_cache/simple/simple_entry_impl.cc:495: callback, 0)); nit: this may be a little more readable, but probably depends on who is reading: s/0/net::OK/ https://codereview.chromium.org/13907009/diff/115003/net/disk_cache/simple/si... net/disk_cache/simple/simple_entry_impl.cc:530: // |this| may be destroyed after return here. please remove the word 'here'
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/13907009/111013
Retried try job too often on ios_dbg_simulator for step(s) googleurl_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/13907009/111013
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/13907009/111013
Message was sent while issue was closed.
Change committed as 198286 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
