Separate Simple Backend creation from initialization.

Separate Simple Backend creation from initialization.

Also make both backends use the same directory for files, and the same name for
the index file. Using the index file each backend will detect if the index
belongs to it. Hence, easier to make sure that both cache trees on disk are
mutually exclusive.

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

[gavinp, 2013-04-09 16:03:49]

What happens if the cache directory doesn't exist?

https://codereview.chromium.org/13813015/diff/1/net/disk_cache/cache_creator.cc
File net/disk_cache/cache_creator.cc (right):

https://codereview.chromium.org/13813015/diff/1/net/disk_cache/cache_creator....
net/disk_cache/cache_creator.cc:87: VLOG(1) << "Using the Simple Cache
Backend.";
Maybe lose this VLOG?

https://codereview.chromium.org/13813015/diff/1/net/disk_cache/cache_creator....
net/disk_cache/cache_creator.cc:113: *backend_ = new
disk_cache::TracingCacheBackend(created_cache_);
This change looks like a NOP. Is it a NOP?

https://codereview.chromium.org/13813015/diff/1/net/disk_cache/simple/simple_...
File net/disk_cache/simple/simple_backend_impl.h (right):

https://codereview.chromium.org/13813015/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_backend_impl.h:62: virtual ~SimpleBackendImpl();
This is weird: why redeclare the destructor as private in a subclass, but leave
the rest of the PVC public?

[pasko-google - do not use, 2013-04-09 16:20:48]

On 2013/04/09 16:03:49, gavinp wrote:
> What happens if the cache directory doesn't exist?

Each backend is responsible for creating the directory, and I added code to
execute on the cache thread to do that.

It is slightly inconvenient that there is a common interface for cleaning up the
cache directory, but there is none for creating. However, the function is
trivial, not a big deal.

https://codereview.chromium.org/13813015/diff/1/net/disk_cache/cache_creator.cc
File net/disk_cache/cache_creator.cc (right):

https://codereview.chromium.org/13813015/diff/1/net/disk_cache/cache_creator....
net/disk_cache/cache_creator.cc:87: VLOG(1) << "Using the Simple Cache
Backend.";
On 2013/04/09 16:03:49, gavinp wrote:
> Maybe lose this VLOG?

Done.

https://codereview.chromium.org/13813015/diff/1/net/disk_cache/cache_creator....
net/disk_cache/cache_creator.cc:113: *backend_ = new
disk_cache::TracingCacheBackend(created_cache_);
On 2013/04/09 16:03:49, gavinp wrote:
> This change looks like a NOP. Is it a NOP?

It is based on another change (https://codereview.chromium.org/13731002/), where
this may not be a no-op, if I happen to commit this change before, I will rebase
and this line will be gone.

https://codereview.chromium.org/13813015/diff/1/net/disk_cache/simple/simple_...
File net/disk_cache/simple/simple_backend_impl.h (right):

https://codereview.chromium.org/13813015/diff/1/net/disk_cache/simple/simple_...
net/disk_cache/simple/simple_backend_impl.h:62: virtual ~SimpleBackendImpl();
On 2013/04/09 16:03:49, gavinp wrote:
> This is weird: why redeclare the destructor as private in a subclass, but
leave
> the rest of the PVC public?

Moved back.

[rvargas (doing something else), 2013-04-09 19:16:07]

https://codereview.chromium.org/13813015/diff/1006/net/disk_cache/cache_creat...
File net/disk_cache/cache_creator.cc (left):

https://codereview.chromium.org/13813015/diff/1006/net/disk_cache/cache_creat...
net/disk_cache/cache_creator.cc:118: #ifdef USE_TRACING_CACHE_BACKEND
I should not be seeing this!

[gavinp, 2013-04-10 10:04:39]

Egor,

Can you please make another upload of this based onto trunk? I'd like to review
that version of it. Thanks.

https://codereview.chromium.org/13813015/diff/1006/net/disk_cache/cache_creat...
File net/disk_cache/cache_creator.cc (left):

https://codereview.chromium.org/13813015/diff/1006/net/disk_cache/cache_creat...
net/disk_cache/cache_creator.cc:118: #ifdef USE_TRACING_CACHE_BACKEND
On 2013/04/09 19:16:07, rvargas wrote:
> I should not be seeing this!

I'm confused. Since this CL is downstream from
https://codereview.chromium.org/13731002 , isn't the line you're commenting on
actually https://codereview.chromium.org/13731002#pair-118 ? Isn't CL 13731002
the place to comment on this line?

I just spoke to Egor, he says this CL rebases onto trunk fine.

[pasko-google - do not use, 2013-04-10 12:20:25]

On 2013/04/10 10:04:39, gavinp wrote:
> Egor,
> 
> Can you please make another upload of this based onto trunk? I'd like to
review
> that version of it. Thanks.

I rebased it on top of master. PTAL.

[pasko-google - do not use, 2013-04-10 12:25:21]

On 2013/04/10 12:20:25, pasko wrote:
> On 2013/04/10 10:04:39, gavinp wrote:
> > Egor,
> > 
> > Can you please make another upload of this based onto trunk? I'd like to
> review
> > that version of it. Thanks.
> 
> I rebased it on top of master. PTAL.

oh, not yet, bots are legitimately failing.

[pasko-google - do not use, 2013-04-10 13:54:31]

On 2013/04/10 12:25:21, pasko wrote:
> On 2013/04/10 12:20:25, pasko wrote:
> > On 2013/04/10 10:04:39, gavinp wrote:
> > > Egor,
> > > 
> > > Can you please make another upload of this based onto trunk? I'd like to
> > review
> > > that version of it. Thanks.
> > 
> > I rebased it on top of master. PTAL.
> 
> oh, not yet, bots are legitimately failing.

please take a look

[gavinp, 2013-04-10 14:20:53]

If the index open fails, does each cache delete all the rest of the other
cache's files?

https://codereview.chromium.org/13813015/diff/19001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_backend_impl.cc (right):

https://codereview.chromium.org/13813015/diff/19001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_backend_impl.cc:46: : path_(path),
    index_(new SimpleIndex(cache_thread, path)),

and lose the .reset() below.

[gavinp, 2013-04-10 14:23:34]

I just spoke offline and Egor reminded me of the failure semantics.

LGTM.

[commit-bot: I haz the power, 2013-04-10 14:23:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pasko@google.com/13813015/19001

[commit-bot: I haz the power, 2013-04-10 15:37:13]

Change committed as 193391

[rvargas (doing something else), 2013-04-10 18:50:11]

https://codereview.chromium.org/13813015/diff/1006/net/disk_cache/cache_creat...
File net/disk_cache/cache_creator.cc (left):

https://codereview.chromium.org/13813015/diff/1006/net/disk_cache/cache_creat...
net/disk_cache/cache_creator.cc:118: #ifdef USE_TRACING_CACHE_BACKEND
On 2013/04/10 10:04:39, gavinp wrote:
> On 2013/04/09 19:16:07, rvargas wrote:
> > I should not be seeing this!
> 
> I'm confused. Since this CL is downstream from
> https://codereview.chromium.org/13731002 , isn't the line you're commenting on
> actually https://codereview.chromium.org/13731002#pair-118 ? Isn't CL 13731002
> the place to comment on this line?
> 
> I just spoke to Egor, he says this CL rebases onto trunk fine.
> 

No, I'm not saying that I don't understand what this line is about. I'm saying
that this CL doesn't depend on anything else to land, and a code review should
be performed against the _current_ state of the tree, not against the state of
the code on a developer's machine.

So I should not be seeing lines in the "old" code that don't exist on TOT, that
simple. It's just a request to follow regular code review practices.

[pasko-google - do not use, 2013-04-11 12:37:59]

On 2013/04/10 18:50:11, rvargas wrote:
>
https://codereview.chromium.org/13813015/diff/1006/net/disk_cache/cache_creat...
> File net/disk_cache/cache_creator.cc (left):
> 
>
https://codereview.chromium.org/13813015/diff/1006/net/disk_cache/cache_creat...
> net/disk_cache/cache_creator.cc:118: #ifdef USE_TRACING_CACHE_BACKEND
> On 2013/04/10 10:04:39, gavinp wrote:
> > On 2013/04/09 19:16:07, rvargas wrote:
> > > I should not be seeing this!
> > 
> > I'm confused. Since this CL is downstream from
> > https://codereview.chromium.org/13731002 , isn't the line you're commenting
on
> > actually https://codereview.chromium.org/13731002#pair-118 ? Isn't CL
13731002
> > the place to comment on this line?
> > 
> > I just spoke to Egor, he says this CL rebases onto trunk fine.
> > 
> 
> No, I'm not saying that I don't understand what this line is about. I'm saying
> that this CL doesn't depend on anything else to land, and a code review should
> be performed against the _current_ state of the tree, not against the state of
> the code on a developer's machine.
> 
> So I should not be seeing lines in the "old" code that don't exist on TOT,
that
> simple. It's just a request to follow regular code review practices.

I added the lines in attempt to show how it should look in the end, in case you
are interested. The change was not hard to rebase either way, so I assumed
looking at the final result is more comfortable.

OK, good to know about the common practice here. I am going to follow it :)

[gavinp, 2013-04-11 12:41:30]

On 2013/04/10 18:50:11, rvargas wrote:
>
https://codereview.chromium.org/13813015/diff/1006/net/disk_cache/cache_creat...
> File net/disk_cache/cache_creator.cc (left):
> 
>
https://codereview.chromium.org/13813015/diff/1006/net/disk_cache/cache_creat...
> net/disk_cache/cache_creator.cc:118: #ifdef USE_TRACING_CACHE_BACKEND
> On 2013/04/10 10:04:39, gavinp wrote:
> > On 2013/04/09 19:16:07, rvargas wrote:
> > > I should not be seeing this!
> > 
> > I'm confused. Since this CL is downstream from
> > https://codereview.chromium.org/13731002 , isn't the line you're commenting
on
> > actually https://codereview.chromium.org/13731002#pair-118 ? Isn't CL
13731002
> > the place to comment on this line?
> > 
> > I just spoke to Egor, he says this CL rebases onto trunk fine.
> > 
> 
> No, I'm not saying that I don't understand what this line is about. I'm saying
> that this CL doesn't depend on anything else to land, and a code review should
> be performed against the _current_ state of the tree, not against the state of
> the code on a developer's machine.
> 
> So I should not be seeing lines in the "old" code that don't exist on TOT,
that
> simple. It's just a request to follow regular code review practices.

"Please rebase this CL onto trunk, I can review it then." says that I lot more
clearly than "I should not be seeing this!"