Reorganize net/disk_cache into backend specific directories.

Reorganize net/disk_cache into backend specific directories.

We want to unlink the blockfile cache on Android, and it seems that
we'll want more platforms with more varieties of backend in the
future. So let's move the backend into its own subdirectory as we
start to untangle the dependencies from tests/etc... into their
respective backends.

This initial checkin isn't perfect; in particular the tests have a lot
of v2 specific dependencies still.

R=rvargas@chromium.org, cbentzel, rvargas
TBR=mmenke@chromium.org
BUG=331062
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253581

[gavinp, 2013-12-27 20:39:29]

Ricardo,

I'm going to take another look over this myself before Monday, so you might want
to (or not) hold off on a detailed look through this.

The intention here is to be a mostly mechanical move in preparation for more
changes to let net be built without linking in the blockfile backend for
android.

[rvargas (doing something else), 2013-12-28 02:43:58]

On 2013/12/27 20:39:29, gavinp wrote:
> Ricardo,
> 
> I'm going to take another look over this myself before Monday, so you might
want
> to (or not) hold off on a detailed look through this.
> 
> The intention here is to be a mostly mechanical move in preparation for more
> changes to let net be built without linking in the blockfile backend for
> android.

May I suggest using /V2/ instead of /Blockfile/, and not moving /v3/ ? I'd like
to avoid extra directory nesting.

BTW, moving files into a particular directory doesn't really have to do with
being able to compile a given backend on a given platform, right?

[gavinp, 2013-12-30 16:18:44]

On 2013/12/28 02:43:58, rvargas wrote:
> On 2013/12/27 20:39:29, gavinp wrote:
> > Ricardo,
> > 
> > I'm going to take another look over this myself before Monday, so you might
> want
> > to (or not) hold off on a detailed look through this.
> > 
> > The intention here is to be a mostly mechanical move in preparation for more
> > changes to let net be built without linking in the blockfile backend for
> > android.
> 
> May I suggest using /V2/ instead of /Blockfile/, and not moving /v3/ ? I'd
like
> to avoid extra directory nesting.
> 
> BTW, moving files into a particular directory doesn't really have to do with
> being able to compile a given backend on a given platform, right?

I think /blockfile/ is better, as it makes it much more clear which backend is
where. What is "v2" in contrast to "mem_*", for instance. For the /v3/ stuff,
that seems minor: many of the files in there aren't even built, and it's my
understanding that the intention is for it to replace the existing blockfile
cache backend at some point, making that directory short lived.

I've created a new bug, bug 331062, and put this issue on it, as you're right,
this isn't directly about "Separate cache backend detection from backend
instances", the previous bug. Instead this is about modularizing backends, so
that we can build chrome with different backends without having to depend on the
backend we are not using.

[gavinp, 2014-01-22 00:31:29]

Do you think we can advance this review? Rebasing this, and the two downstream
reviews is getting to be difficult.

The second downstream is https://codereview.chromium.org/117533003/ .

The third downstream ( https://codereview.chromium.org/127083002/ ) which splits
out the tests, is converging and should be ready for review soon, if you want to
wait for it before continuing.

[rvargas (doing something else), 2014-01-22 01:21:32]

It was not clear to me that this was blocked in my part. Please ping me more
often if you're feeling blocked.

From the discussion on the bug, I think it was clear that moving files around
was not really needed in order to disable some files to be compiled for a given
platform.

I still don't want to have more levels of directories than needed, and I would
be much happier moving the v2 files into a v2 folder if you _really_ want to
move them somewhere.

[gavinp, 2014-01-27 11:58:22]

On 2014/01/22 01:21:32, rvargas wrote:
> It was not clear to me that this was blocked in my part. Please ping me more
> often if you're feeling blocked.
> 
> From the discussion on the bug, I think it was clear that moving files around
> was not really needed in order to disable some files to be compiled for a
given
> platform.

That's true. The argument in favour of using directories to control conditional
compilation is that it's a system that's comprehensible. You've repeated a few
times that you don't want directories. What's the problem caused by directories?

> I still don't want to have more levels of directories than needed, and I would
> be much happier moving the v2 files into a v2 folder if you _really_ want to
> move them somewhere.

The blockfile cache is compiled as one unit; there's no suggestion that we'll
ever compile blockfile_v2 but not blockfile_v3, etc...; removing blockfile from
the name seems like a loss (v2 of what?), and having separate directories for v2
and v3 breaks the proposed "each backend has its own directory" rule, which is a
really easy to understand rule.

[rvargas (doing something else), 2014-01-28 21:15:38]

On 2014/01/27 11:58:22, gavinp wrote:
> On 2014/01/22 01:21:32, rvargas wrote:
> > It was not clear to me that this was blocked in my part. Please ping me more
> > often if you're feeling blocked.
> > 
> > From the discussion on the bug, I think it was clear that moving files
around
> > was not really needed in order to disable some files to be compiled for a
> given
> > platform.
> 
> That's true. The argument in favour of using directories to control
conditional
> compilation is that it's a system that's comprehensible. You've repeated a few

Are you saying that excluding individual files by name (which whatever the
directory structure we still have to do) is incomprehensible?


> times that you don't want directories. What's the problem caused by
directories?

I have not said I don't want directories. I said I don't want gratuitous levels
of directory hierarchies.

<sarcasm>
Hey, why don't we do
src\net\disk_cache\android\simple
src\net\disk_cache\android\flash
</sarcasm>

> 
> > I still don't want to have more levels of directories than needed, and I
would
> > be much happier moving the v2 files into a v2 folder if you _really_ want to
> > move them somewhere.
> 
> The blockfile cache is compiled as one unit; there's no suggestion that we'll
> ever compile blockfile_v2 but not blockfile_v3, etc...; removing blockfile
from

Why not?. Except from testing / experiments, there should be no "real" user of
both caches.

> the name seems like a loss (v2 of what?), and having separate directories for
v2
> and v3 breaks the proposed "each backend has its own directory" rule, which is
a
> really easy to understand rule.

disk_cache\v2, the directory says "v2 of what". The code has never had the
official name of "blockfile".

V2 is not the same as V3, they are two different backends, and having them on
two different directories make sense... from the point of view of "what backends
can I have that implement the disk cache interface", the answer is memory, v2,
v3, simple and flash[1], and they are at the same logical level, both in terms
of "direct implementations of the interface", and in terms of code sharing:
anyone can use more or less what they want from the others, as code in net\http
can use code from net\disk_cache or net\url_request or net\spdy (subject to
specific logical layering that is not embedded in the directory structure).

[1]: yes, I would not create a directory to hold two files of a
meta-implementation like the tracing backend.

[gavinp, 2014-02-04 19:35:38]

+cbentzel

Ricardo, I'm inviting Chris to come take a look here; it's getting a bit
cumbersome to rebase this for me, and I think at this point any comments by
either of us would just be us repeating ourselves. Hopefully a third perspective
will help.

Chris, I was hoping to land this first CL of three in prep for unlinking the
blockfile backend on Android. The second CL is at
https://codereview.chromium.org/117533003/ , also ready for review, and the
third CL is in development (mostly finished, but some renames, cleanup, and the
last few tests need to be added) at https://codereview.chromium.org/127083002/

The thread documents the tension between Ricardo and I; there's three
disagreements as far as I can tell.

The first is whether to name the current backend used by default on non-Android
platforms; it currently is unnamed, and included in that question is what to
name if it should have a name. "blockfile" is what this CL proposes.

The second is about the utility of using directories to group files by backend.
There's a disadvantage to using too many directories. This CL proposes an extra
layer of directories grouping the files from each backend together.

The third, and the one I'm the least able to speak to, is about grouping of the
v3 backend together with v2.

Thanks!

[rvargas (doing something else), 2014-02-04 20:11:25]

Keep in mind that there was a concrete proposal more than a month ago to move
this CL forward. See
https://code.google.com/p/chromium/issues/detail?id=331062#c2

[cbentzel, 2014-02-05 01:26:36]

On 2014/02/04 20:11:25, rvargas wrote:
> Keep in mind that there was a concrete proposal more than a month ago to move
> this CL forward. See
> https://code.google.com/p/chromium/issues/detail?id=331062#c2

I'll take a look and provide feedback tomorrow.

[cbentzel, 2014-02-05 01:26:43]

On 2014/02/04 20:11:25, rvargas wrote:
> Keep in mind that there was a concrete proposal more than a month ago to move
> this CL forward. See
> https://code.google.com/p/chromium/issues/detail?id=331062#c2

I'll take a look and provide feedback tomorrow.

[cbentzel, 2014-02-05 11:14:29]

On 2014/02/05 01:26:43, cbentzel wrote:
> On 2014/02/04 20:11:25, rvargas wrote:
> > Keep in mind that there was a concrete proposal more than a month ago to
move
> > this CL forward. See
> > https://code.google.com/p/chromium/issues/detail?id=331062#c2
> 
> I'll take a look and provide feedback tomorrow.

- I don't care too much about blockfile versus v2 name decision. If being a
tiebreaker helps I'd choose blockfile - v2 already shows up in codesearch under
UI, whereas blockfile shows up cache items (and will in the future).

- I like the directory structure proposed by Ricardo in
https://code.google.com/p/chromium/issues/detail?id=331062#c2 (and generally
showing up here), as well as GYP configs and/or compile-time flags to
enable/disable different backends rather than depend on linker dead-code
elimination. The reason for preferring GYP is that it makes it harder for net
stack consumers to inadvertently add uses of a presumed-unsupported backend on a
platform and it will at least require a net/OWNERS review since net.gyp would
need to be changed.

[cbentzel, 2014-02-05 13:45:13]

On 2014/02/05 11:14:29, cbentzel wrote:
> On 2014/02/05 01:26:43, cbentzel wrote:
> > On 2014/02/04 20:11:25, rvargas wrote:
> > > Keep in mind that there was a concrete proposal more than a month ago to
> move
> > > this CL forward. See
> > > https://code.google.com/p/chromium/issues/detail?id=331062#c2
> > 
> > I'll take a look and provide feedback tomorrow.
> 
> - I don't care too much about blockfile versus v2 name decision. If being a
> tiebreaker helps I'd choose blockfile - v2 already shows up in codesearch
under
> UI, whereas blockfile shows up cache items (and will in the future).
> 
> - I like the directory structure proposed by Ricardo in
> https://code.google.com/p/chromium/issues/detail?id=331062#c2 (and generally
> showing up here), as well as GYP configs and/or compile-time flags to
> enable/disable different backends rather than depend on linker dead-code
> elimination. The reason for preferring GYP is that it makes it harder for net
> stack consumers to inadvertently add uses of a presumed-unsupported backend on
a
> platform and it will at least require a net/OWNERS review since net.gyp would
> need to be changed.

And I'd also follow whatever Ricardo thinks is best for grouping v2+v3 backends.

[gavinp, 2014-02-05 14:05:42]

On 2014/02/05 13:45:13, cbentzel wrote:
> On 2014/02/05 11:14:29, cbentzel wrote:
> > On 2014/02/05 01:26:43, cbentzel wrote:
> > > On 2014/02/04 20:11:25, rvargas wrote:
> > > > Keep in mind that there was a concrete proposal more than a month ago to
> > move
> > > > this CL forward. See
> > > > https://code.google.com/p/chromium/issues/detail?id=331062#c2
> > > 
> > > I'll take a look and provide feedback tomorrow.
> > 
> > - I don't care too much about blockfile versus v2 name decision. If being a
> > tiebreaker helps I'd choose blockfile - v2 already shows up in codesearch
> under
> > UI, whereas blockfile shows up cache items (and will in the future).
> > 
> > - I like the directory structure proposed by Ricardo in
> > https://code.google.com/p/chromium/issues/detail?id=331062#c2 (and generally
> > showing up here), as well as GYP configs and/or compile-time flags to
> > enable/disable different backends rather than depend on linker dead-code
> > elimination. The reason for preferring GYP is that it makes it harder for
net
> > stack consumers to inadvertently add uses of a presumed-unsupported backend
on
> a
> > platform and it will at least require a net/OWNERS review since net.gyp
would
> > need to be changed.
> 
> And I'd also follow whatever Ricardo thinks is best for grouping v2+v3
backends.

Great, this is basically what's already uploaded. I believe the only steps
required are to:

mv net/disk_cache/blockfile/v3 net/disk_cache/v3
mv net/disk_cache/blockfile net/disk_cache/v2

And we're done. I'll upload that remediation shortly.

[gavinp, 2014-02-05 17:10:02]

On 2014/02/05 11:14:29, cbentzel wrote:
> On 2014/02/05 01:26:43, cbentzel wrote:
> > On 2014/02/04 20:11:25, rvargas wrote:
> > > Keep in mind that there was a concrete proposal more than a month ago to
> move
> > > this CL forward. See
> > > https://code.google.com/p/chromium/issues/detail?id=331062#c2
> > 
> > I'll take a look and provide feedback tomorrow.
> 
> - I don't care too much about blockfile versus v2 name decision. If being a
> tiebreaker helps I'd choose blockfile - v2 already shows up in codesearch
under
> UI, whereas blockfile shows up cache items (and will in the future).
> 
> - I like the directory structure proposed by Ricardo in
> https://code.google.com/p/chromium/issues/detail?id=331062#c2 (and generally
> showing up here), as well as GYP configs and/or compile-time flags to
> enable/disable different backends rather than depend on linker dead-code
> elimination. The reason for preferring GYP is that it makes it harder for net
> stack consumers to inadvertently add uses of a presumed-unsupported backend on
a
> platform and it will at least require a net/OWNERS review since net.gyp would
> need to be changed.

This is now done. Patch set 6 from mid December was this, except for
s/blockfile/v2, which is now uploaded as patch set 16.

Can everyone PTAL?

[gavinp, 2014-02-05 21:40:52]

https://codereview.chromium.org/121643003/diff/2820001/net/disk_cache/v3/back...
File net/disk_cache/v3/backend_impl_v3.cc (right):

https://codereview.chromium.org/121643003/diff/2820001/net/disk_cache/v3/back...
net/disk_cache/v3/backend_impl_v3.cc:34: #define
NET_DISK_CACHE_BLOCKFILE_BACKEND_IMPL_CC_
Oopsie. I'll upload a fix to this one on Thursday.

[rvargas (doing something else), 2014-02-06 04:21:21]

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/memory/...
File net/disk_cache/memory/mem_backend_impl.h (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/memory/...
net/disk_cache/memory/mem_backend_impl.h:7: #ifndef
NET_DISK_CACHE_MEMORY_MEM_BACKEND_IMPL_H__
NET_DISK_CACHE_MEMORY_MEM_BACKEND_IMPL_H_

(and elsewhere)

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v2/hist...
File net/disk_cache/v2/histogram_macros.h (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v2/hist...
net/disk_cache/v2/histogram_macros.h:87: #ifdef
NET_DISK_CACHE_V2_BACKEND_IMPL_CC_
no version prefix

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/back...
File net/disk_cache/v3/backend_impl_v3.h (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/back...
net/disk_cache/v3/backend_impl_v3.h:7: #ifndef NET_DISK_CACHE_V3_BACKEND_IMPL_H_
needs v3 at the end

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/back...
File net/disk_cache/v3/backend_worker.cc (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/back...
net/disk_cache/v3/backend_worker.cc:31: #define
NET_DISK_CACHE_V2_BACKEND_IMPL_CC_
no version prefix here

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/entr...
File net/disk_cache/v3/entry_impl_v3.h (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/entr...
net/disk_cache/v3/entry_impl_v3.h:5: #ifndef NET_DISK_CACHE_V3_ENTRY_IMPL_H_
needs v3 at the end

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/evic...
File net/disk_cache/v3/eviction_v3.h (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/evic...
net/disk_cache/v3/eviction_v3.h:5: #ifndef NET_DISK_CACHE_V3_EVICTION_H_
needs v3 at the end

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/hist...
File net/disk_cache/v3/histogram_macros.h (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/hist...
net/disk_cache/v3/histogram_macros.h:87: #ifdef
NET_DISK_CACHE_V2_BACKEND_IMPL_CC_
no version prefix

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/spar...
File net/disk_cache/v3/sparse_control_v3.h (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/spar...
net/disk_cache/v3/sparse_control_v3.h:5: #ifndef
NET_DISK_CACHE_V3_SPARSE_CONTROL_H_
needs v3 at the end

https://codereview.chromium.org/121643003/diff/3020001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/121643003/diff/3020001/net/net.gyp#newcode401
net/net.gyp:401: 'disk_cache/tracing/tracing_cache_backend.cc',
Having two files on a directory seems overkill. We should leave them at the
root.

[cbentzel, 2014-02-07 01:52:37]

So, my responses to the questions Gavin proposed were fairly muddled.

Here's my proposed directory structure:

net/disk_cache: The common test functions as this shows.
.... /blockfile
.... /flash
...  /memory
.... /simple
.... /tracing

Some details on why:

  - I still want to use compile-time flags/gyp defines rather than rely on the
linker for reasons mentioned in the bug, as well as to help with bot cycle time
by removing a few compiled files. Not a huge impact on cycle-time relative to
other things, but may as well. 

  - Keeping the common functions in the top-level directory is fine, consistent
with other places in the code, and I don't think we need a separate base/ or
common/ directory under disk_cache.
  
  - blockfile is a clearer and easier name to search for than v2. I also think a
single directory makes sense for both v2+v3. v3 code is dependent on v2 code,
and the v3-specific files already have _v3 suffixes so it is easy to distinguish
by filename rather than path. It seems like there is violent agreement that we
do not want to copy-and-paste common code and have two separate directories.
This leaves either peer directories (like disk_cache/blockfile_v2 and
disk_cache/blockfile_v3) which can call into each other which is a bit
confusing, or something like disk_cache/blockfile/v3 which can call into v2, but
still has the redundant _v3 filenames. It should also be easy to filter out the
_v3 files from being built for most configurations with GYP rules based on file
name.

  - flash also makes sense as a separate directory, and it should not be built
at this point except for people who have it on with GYP flags.

  - memory is a very separate backend. 
  
  - simple is already a separate directory and backend that we only build on
some platforms.

  - tracing is only two files, but I think it helps for consistency to put the
backend in its own directory

Please provide feedback, but in the absence of really compelling arguments
either way I'd like to just stick with this so we can move forward with the CL.
Being able to unlink blockfile cache on Android will provide some user benefit
as well as some minor developer benefit by reducing build cycle times.

[cbentzel, 2014-02-07 01:52:43]

So, my responses to the questions Gavin proposed were fairly muddled.

Here's my proposed directory structure:

net/disk_cache: The common test functions as this shows.
.... /blockfile
.... /flash
...  /memory
.... /simple
.... /tracing

Some details on why:

  - I still want to use compile-time flags/gyp defines rather than rely on the
linker for reasons mentioned in the bug, as well as to help with bot cycle time
by removing a few compiled files. Not a huge impact on cycle-time relative to
other things, but may as well. 

  - Keeping the common functions in the top-level directory is fine, consistent
with other places in the code, and I don't think we need a separate base/ or
common/ directory under disk_cache.
  
  - blockfile is a clearer and easier name to search for than v2. I also think a
single directory makes sense for both v2+v3. v3 code is dependent on v2 code,
and the v3-specific files already have _v3 suffixes so it is easy to distinguish
by filename rather than path. It seems like there is violent agreement that we
do not want to copy-and-paste common code and have two separate directories.
This leaves either peer directories (like disk_cache/blockfile_v2 and
disk_cache/blockfile_v3) which can call into each other which is a bit
confusing, or something like disk_cache/blockfile/v3 which can call into v2, but
still has the redundant _v3 filenames. It should also be easy to filter out the
_v3 files from being built for most configurations with GYP rules based on file
name.

  - flash also makes sense as a separate directory, and it should not be built
at this point except for people who have it on with GYP flags.

  - memory is a very separate backend. 
  
  - simple is already a separate directory and backend that we only build on
some platforms.

  - tracing is only two files, but I think it helps for consistency to put the
backend in its own directory

Please provide feedback, but in the absence of really compelling arguments
either way I'd like to just stick with this so we can move forward with the CL.
Being able to unlink blockfile cache on Android will provide some user benefit
as well as some minor developer benefit by reducing build cycle times.

[rvargas (doing something else), 2014-02-08 04:06:44]

On 2014/02/07 01:52:43, cbentzel wrote:
> So, my responses to the questions Gavin proposed were fairly muddled.
> 
> Here's my proposed directory structure:
> 
> net/disk_cache: The common test functions as this shows.
> .... /blockfile
> .... /flash
> ...  /memory
> .... /simple
> .... /tracing
> 
> Some details on why:
> 
>   - I still want to use compile-time flags/gyp defines rather than rely on the
> linker for reasons mentioned in the bug, as well as to help with bot cycle
time
> by removing a few compiled files. Not a huge impact on cycle-time relative to
> other things, but may as well. 
> 
>   - Keeping the common functions in the top-level directory is fine,
consistent
> with other places in the code, and I don't think we need a separate base/ or
> common/ directory under disk_cache.
>   
>   - blockfile is a clearer and easier name to search for than v2. I also think
a
> single directory makes sense for both v2+v3. v3 code is dependent on v2 code,
> and the v3-specific files already have _v3 suffixes so it is easy to
distinguish
> by filename rather than path. It seems like there is violent agreement that we
> do not want to copy-and-paste common code and have two separate directories.
> This leaves either peer directories (like disk_cache/blockfile_v2 and
> disk_cache/blockfile_v3) which can call into each other which is a bit
> confusing, or something like disk_cache/blockfile/v3 which can call into v2,
but
> still has the redundant _v3 filenames. It should also be easy to filter out
the
> _v3 files from being built for most configurations with GYP rules based on
file
> name.
> 
>   - flash also makes sense as a separate directory, and it should not be built
> at this point except for people who have it on with GYP flags.
> 
>   - memory is a very separate backend. 
>   
>   - simple is already a separate directory and backend that we only build on
> some platforms.
> 
>   - tracing is only two files, but I think it helps for consistency to put the
> backend in its own directory
> 
> Please provide feedback, but in the absence of really compelling arguments
> either way I'd like to just stick with this so we can move forward with the
CL.
> Being able to unlink blockfile cache on Android will provide some user benefit
> as well as some minor developer benefit by reducing build cycle times.

I have no objections to a single directory.

I still consider having one cc file living under its own directory going
overboard in the name of consistency, but that's certainly no blocker.

I'm sad that I'll have to review everything again, but at least I'm not the one
who has to rewrite the cl one more time.

I still see compile time flags as an orthogonal issue to this cl, but then again
I have never opposed that (just for completeness).

[gavinp, 2014-02-09 15:11:43]

On 2014/02/07 01:52:43, cbentzel wrote:
> So, my responses to the questions Gavin proposed were fairly muddled.
> 
> Here's my proposed directory structure:
> 
> net/disk_cache: The common test functions as this shows.
> .... /blockfile
> .... /flash
> ...  /memory
> .... /simple
> .... /tracing
> 
> Some details on why:
> 
>   - I still want to use compile-time flags/gyp defines rather than rely on the
> linker for reasons mentioned in the bug, as well as to help with bot cycle
time
> by removing a few compiled files. Not a huge impact on cycle-time relative to
> other things, but may as well. 
> 
>   - Keeping the common functions in the top-level directory is fine,
consistent
> with other places in the code, and I don't think we need a separate base/ or
> common/ directory under disk_cache.
>   
>   - blockfile is a clearer and easier name to search for than v2. I also think
a
> single directory makes sense for both v2+v3. v3 code is dependent on v2 code,
> and the v3-specific files already have _v3 suffixes so it is easy to
distinguish
> by filename rather than path. It seems like there is violent agreement that we
> do not want to copy-and-paste common code and have two separate directories.
> This leaves either peer directories (like disk_cache/blockfile_v2 and
> disk_cache/blockfile_v3) which can call into each other which is a bit
> confusing, or something like disk_cache/blockfile/v3 which can call into v2,
but
> still has the redundant _v3 filenames. It should also be easy to filter out
the
> _v3 files from being built for most configurations with GYP rules based on
file
> name.
> 
>   - flash also makes sense as a separate directory, and it should not be built
> at this point except for people who have it on with GYP flags.
> 
>   - memory is a very separate backend. 
>   
>   - simple is already a separate directory and backend that we only build on
> some platforms.

This turns out to not be the case. We build simple on all platforms, with some
tests disabled on non-POSIX platforms.

> 
>   - tracing is only two files, but I think it helps for consistency to put the
> backend in its own directory
> 
> Please provide feedback, but in the absence of really compelling arguments
> either way I'd like to just stick with this so we can move forward with the
CL.
> Being able to unlink blockfile cache on Android will provide some user benefit
> as well as some minor developer benefit by reducing build cycle times.

Thanks for the detailed reply, I'm glad to have a comprehensive status in this
bug so I can remediate the code and we can move forward.

[gavinp, 2014-02-09 15:13:30]

PTAL.

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/memory/...
File net/disk_cache/memory/mem_backend_impl.h (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/memory/...
net/disk_cache/memory/mem_backend_impl.h:7: #ifndef
NET_DISK_CACHE_MEMORY_MEM_BACKEND_IMPL_H__
On 2014/02/06 04:21:23, rvargas wrote:
> NET_DISK_CACHE_MEMORY_MEM_BACKEND_IMPL_H_
> 
> (and elsewhere)

Done.

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v2/hist...
File net/disk_cache/v2/histogram_macros.h (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v2/hist...
net/disk_cache/v2/histogram_macros.h:87: #ifdef
NET_DISK_CACHE_V2_BACKEND_IMPL_CC_
On 2014/02/06 04:21:23, rvargas wrote:
> no version prefix

This problematic #define is being dealt with in an upstream review now, see
https://codereview.chromium.org/140893013/

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/back...
File net/disk_cache/v3/backend_impl_v3.h (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/back...
net/disk_cache/v3/backend_impl_v3.h:7: #ifndef NET_DISK_CACHE_V3_BACKEND_IMPL_H_
On 2014/02/06 04:21:23, rvargas wrote:
> needs v3 at the end

Done.

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/back...
File net/disk_cache/v3/backend_worker.cc (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/back...
net/disk_cache/v3/backend_worker.cc:31: #define
NET_DISK_CACHE_V2_BACKEND_IMPL_CC_
On 2014/02/06 04:21:23, rvargas wrote:
> no version prefix here

See https://codereview.chromium.org/140893013/

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/entr...
File net/disk_cache/v3/entry_impl_v3.h (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/entr...
net/disk_cache/v3/entry_impl_v3.h:5: #ifndef NET_DISK_CACHE_V3_ENTRY_IMPL_H_
On 2014/02/06 04:21:23, rvargas wrote:
> needs v3 at the end

Done.

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/evic...
File net/disk_cache/v3/eviction_v3.h (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/evic...
net/disk_cache/v3/eviction_v3.h:5: #ifndef NET_DISK_CACHE_V3_EVICTION_H_
On 2014/02/06 04:21:23, rvargas wrote:
> needs v3 at the end

Done.

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/hist...
File net/disk_cache/v3/histogram_macros.h (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/hist...
net/disk_cache/v3/histogram_macros.h:87: #ifdef
NET_DISK_CACHE_V2_BACKEND_IMPL_CC_
On 2014/02/06 04:21:23, rvargas wrote:
> no version prefix

See https://codereview.chromium.org/140893013/

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/spar...
File net/disk_cache/v3/sparse_control_v3.h (right):

https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v3/spar...
net/disk_cache/v3/sparse_control_v3.h:5: #ifndef
NET_DISK_CACHE_V3_SPARSE_CONTROL_H_
On 2014/02/06 04:21:23, rvargas wrote:
> needs v3 at the end

Done.

[rvargas (doing something else), 2014-02-11 03:21:26]

https://codereview.chromium.org/121643003/diff/3710001/chrome/browser/net/htt...
File chrome/browser/net/http_pipelining_compatibility_client.cc (left):

https://codereview.chromium.org/121643003/diff/3710001/chrome/browser/net/htt...
chrome/browser/net/http_pipelining_compatibility_client.cc:27: 
I guess this belongs to another CL

https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/blockfi...
File net/disk_cache/blockfile/backend_worker_v3.h (right):

https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/blockfi...
net/disk_cache/blockfile/backend_worker_v3.h:7: #ifndef
NET_DISK_CACHE_BLOCKFILE_BACKEND_WORKER_V3_H_
no V3 postfix

https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/blockfi...
File net/disk_cache/blockfile/block_bitmaps_v3.h (right):

https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/blockfi...
net/disk_cache/blockfile/block_bitmaps_v3.h:7: #ifndef
NET_DISK_CACHE_BLOCKFILE_BLOCK_BITMAPS_V3_H_
no v3

https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/blockfi...
File net/disk_cache/blockfile/index_table_v3.h (right):

https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/blockfi...
net/disk_cache/blockfile/index_table_v3.h:5: #ifndef
NET_DISK_CACHE_BLOCKFILE_INDEX_TABLE_V3_H_
no v3

https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/disk_ca...
File net/disk_cache/disk_cache_perftest.cc (right):

https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/disk_ca...
net/disk_cache/disk_cache_perftest.cc:4: 
This file seems to be block file only.

https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/v3/back...
File net/disk_cache/v3/backend_impl_v3.h (left):

https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/v3/back...
net/disk_cache/v3/backend_impl_v3.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Funny that these files are marked as M and not D

[gavinp, 2014-02-11 16:37:16]

PTAL.

Ricardo, you left a few comments suggesting that v3 specific files being moved
from disk_cache/v3 --> disk_cache/blockfile should not have _v3.cc or _v3.h
postfixes on their name. Can you explain why this is undesirable?

In favour of it, I see readability; this code is already quite tightly linked,
it's good to know what's part of what version. Note that previously v3 files
were distinguished by their subdirectory, now they are not, so this postfix is
really maintaining a status quo.

As well, I thought this was implied by
https://codereview.chromium.org/121643003/#msg18 , though it is a bit ambiguous.

https://codereview.chromium.org/121643003/diff/3710001/chrome/browser/net/htt...
File chrome/browser/net/http_pipelining_compatibility_client.cc (left):

https://codereview.chromium.org/121643003/diff/3710001/chrome/browser/net/htt...
chrome/browser/net/http_pipelining_compatibility_client.cc:27: 
On 2014/02/11 03:21:28, rvargas wrote:
> I guess this belongs to another CL

Done.

https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/disk_ca...
File net/disk_cache/disk_cache_perftest.cc (right):

https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/disk_ca...
net/disk_cache/disk_cache_perftest.cc:4: 
On 2014/02/11 03:21:28, rvargas wrote:
> This file seems to be block file only.

It is. Now it's moved into the blockfile directory.

https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/v3/back...
File net/disk_cache/v3/backend_impl_v3.h (left):

https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/v3/back...
net/disk_cache/v3/backend_impl_v3.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2014/02/11 03:21:28, rvargas wrote:
> Funny that these files are marked as M and not D

Yes it is.

[rvargas (doing something else), 2014-02-11 19:52:40]

On 2014/02/11 16:37:16, gavinp wrote:
> PTAL.
> 
> Ricardo, you left a few comments suggesting that v3 specific files being moved
> from disk_cache/v3 --> disk_cache/blockfile should not have _v3.cc or _v3.h
> postfixes on their name. Can you explain why this is undesirable?
> 
> In favour of it, I see readability; this code is already quite tightly linked,
> it's good to know what's part of what version. Note that previously v3 files
> were distinguished by their subdirectory, now they are not, so this postfix is
> really maintaining a status quo.

The guard is a direct consequence of the name of the file. The name of the file
most of the time is a direct consequence of the name of the class that it
implements. Directory structure is not really part of that picture, and having
V2 and V3 in the same directory works as well as having them in independent
directories but should not set the naming convention of everything under it.

BackendImpl is the V2 class. BackendImplV3 is the V3 class because it does
exactly the same, it is an evolution of V2 and has to coexist for some time on
the same executable with V2 (for the purpose of experiments) so it has to have a
different name. IndexTable is a class that implements a V3 feature but there is
no equivalent class for V2 (or at least it is not called IndexTable), so there
is no need to use a version postfix. Remember that the postfix is a temporal
measure that will be removed once V2 code goes away.


> 
> As well, I thought this was implied by
> https://codereview.chromium.org/121643003/#msg18 , though it is a bit
ambiguous.
> 
>
https://codereview.chromium.org/121643003/diff/3710001/chrome/browser/net/htt...
> File chrome/browser/net/http_pipelining_compatibility_client.cc (left):
> 
>
https://codereview.chromium.org/121643003/diff/3710001/chrome/browser/net/htt...
> chrome/browser/net/http_pipelining_compatibility_client.cc:27: 
> On 2014/02/11 03:21:28, rvargas wrote:
> > I guess this belongs to another CL
> 
> Done.
> 
>
https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/disk_ca...
> File net/disk_cache/disk_cache_perftest.cc (right):
> 
>
https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/disk_ca...
> net/disk_cache/disk_cache_perftest.cc:4: 
> On 2014/02/11 03:21:28, rvargas wrote:
> > This file seems to be block file only.
> 
> It is. Now it's moved into the blockfile directory.
> 
>
https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/v3/back...
> File net/disk_cache/v3/backend_impl_v3.h (left):
> 
>
https://codereview.chromium.org/121643003/diff/3710001/net/disk_cache/v3/back...
> net/disk_cache/v3/backend_impl_v3.h:1: // Copyright (c) 2012 The Chromium
> Authors. All rights reserved.
> On 2014/02/11 03:21:28, rvargas wrote:
> > Funny that these files are marked as M and not D
> 
> Yes it is.

[rvargas (doing something else), 2014-02-11 20:15:09]

https://codereview.chromium.org/121643003/diff/3890001/net/disk_cache/blockfi...
File net/disk_cache/blockfile/eviction_v3.cc (right):

https://codereview.chromium.org/121643003/diff/3890001/net/disk_cache/blockfi...
net/disk_cache/blockfile/eviction_v3.cc:44: #define BLOCKFILE_BACKEND_IMPL_OBJ
backend_
I know I've mentioned it before, but seeing the same changes on multiple CLs is
a pain. CLs should be independent, always diffed against trunk, not intermixed.

Now it is not just a matter of seeing what changed between PS 22 and 23

[gavinp, 2014-02-11 20:39:27]

On 2014/02/11 20:15:09, rvargas wrote:
>
https://codereview.chromium.org/121643003/diff/3890001/net/disk_cache/blockfi...
> File net/disk_cache/blockfile/eviction_v3.cc (right):
> 
>
https://codereview.chromium.org/121643003/diff/3890001/net/disk_cache/blockfi...
> net/disk_cache/blockfile/eviction_v3.cc:44: #define BLOCKFILE_BACKEND_IMPL_OBJ
> backend_
> I know I've mentioned it before, but seeing the same changes on multiple CLs
is
> a pain. CLs should be independent, always diffed against trunk, not
intermixed.

Please recall that in
https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v2/hist...
, you asked for a change that I felt was out of scope for this CL; I responded
by creating an upstream CL, now at https://codereview.chromium.org/140893013/ .
Starting at PS 19, this CL has been expressed downstream of that CL.

I did this to reduce, not cause confusion. The changes around how histogram
macros were controlled were out of scope, and needed to be done first or else
this CL would have confusing out of scope changes included in it. It had to
think hard how to respond to your comment at
https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v2/hist...
, and my judgement, together with a peer I consulted with before responding, was
that creating an upstream CL was less confusing.

After https://codereview.chromium.org/140893013/ lands, this CL will be based on
trunk.

> Now it is not just a matter of seeing what changed between PS 22 and 23

Actually, it is. PS 22 is also based on
https://codereview.chromium.org/140893013/ , the last PS which was not is PS 18.

[rvargas (doing something else), 2014-02-11 21:16:39]

On 2014/02/11 20:39:27, gavinp wrote:
> On 2014/02/11 20:15:09, rvargas wrote:
> >
>
https://codereview.chromium.org/121643003/diff/3890001/net/disk_cache/blockfi...
> > File net/disk_cache/blockfile/eviction_v3.cc (right):
> > 
> >
>
https://codereview.chromium.org/121643003/diff/3890001/net/disk_cache/blockfi...
> > net/disk_cache/blockfile/eviction_v3.cc:44: #define
BLOCKFILE_BACKEND_IMPL_OBJ
> > backend_
> > I know I've mentioned it before, but seeing the same changes on multiple CLs
> is
> > a pain. CLs should be independent, always diffed against trunk, not
> intermixed.
> 
> Please recall that in
>
https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v2/hist...
> , you asked for a change that I felt was out of scope for this CL; I responded
> by creating an upstream CL, now at https://codereview.chromium.org/140893013/
.
> Starting at PS 19, this CL has been expressed downstream of that CL.
> 
> I did this to reduce, not cause confusion. The changes around how histogram
> macros were controlled were out of scope, and needed to be done first or else
> this CL would have confusing out of scope changes included in it. It had to
> think hard how to respond to your comment at
>
https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v2/hist...
> , and my judgement, together with a peer I consulted with before responding,
was
> that creating an upstream CL was less confusing.
> 
> After https://codereview.chromium.org/140893013/ lands, this CL will be based
on
> trunk.
> 
> > Now it is not just a matter of seeing what changed between PS 22 and 23
> 
> Actually, it is. PS 22 is also based on
> https://codereview.chromium.org/140893013/ , the last PS which was not is PS
18.

I understand what you are doing and why. I appreciate you taking the time to fix
something on an independent CL, but really, there is no concept of "downstream"
or "upstream" for code reviews or CLs (other than the existence of multiple
repos for projects hosted somewhere else, like what was WebKit).

Fixing the way that uma macros are generated can be done before or after this
CL, and in any case, the only relevant thing is what is ToT at the time of the
review and the change landing. If you take it out of this CL, I should not see
it as part of this CL :(

If I compare changes between 22 and 23 for eviction_v3.cc (the first file that
says it was modified between 22 and 23), I see changes that are not logically
part of this CL, they are part of the code review taking place to change uma
generation. That's what I mean by the two CLs being intermixed when they should
not. To be more explicit: a CL should not be diffed against another CL in
progress, it should be diffed against ToT.

[gavinp, 2014-02-11 22:28:48]

On 2014/02/11 21:16:39, rvargas wrote:
> On 2014/02/11 20:39:27, gavinp wrote:
> > On 2014/02/11 20:15:09, rvargas wrote:
> > >
> >
>
https://codereview.chromium.org/121643003/diff/3890001/net/disk_cache/blockfi...
> > > File net/disk_cache/blockfile/eviction_v3.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/121643003/diff/3890001/net/disk_cache/blockfi...
> > > net/disk_cache/blockfile/eviction_v3.cc:44: #define
> BLOCKFILE_BACKEND_IMPL_OBJ
> > > backend_
> > > I know I've mentioned it before, but seeing the same changes on multiple
CLs
> > is
> > > a pain. CLs should be independent, always diffed against trunk, not
> > intermixed.
> > 
> > Please recall that in
> >
>
https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v2/hist...
> > , you asked for a change that I felt was out of scope for this CL; I
responded
> > by creating an upstream CL, now at
https://codereview.chromium.org/140893013/
> .
> > Starting at PS 19, this CL has been expressed downstream of that CL.
> > 
> > I did this to reduce, not cause confusion. The changes around how histogram
> > macros were controlled were out of scope, and needed to be done first or
else
> > this CL would have confusing out of scope changes included in it. It had to
> > think hard how to respond to your comment at
> >
>
https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v2/hist...
> > , and my judgement, together with a peer I consulted with before responding,
> was
> > that creating an upstream CL was less confusing.
> > 
> > After https://codereview.chromium.org/140893013/ lands, this CL will be
based
> on
> > trunk.
> > 
> > > Now it is not just a matter of seeing what changed between PS 22 and 23
> > 
> > Actually, it is. PS 22 is also based on
> > https://codereview.chromium.org/140893013/ , the last PS which was not is PS
> 18.
> 
> I understand what you are doing and why. I appreciate you taking the time to
fix
> something on an independent CL, but really, there is no concept of
"downstream"
> or "upstream" for code reviews or CLs (other than the existence of multiple
> repos for projects hosted somewhere else, like what was WebKit).
> 
> Fixing the way that uma macros are generated can be done before or after this
> CL, and in any case, the only relevant thing is what is ToT at the time of the
> review and the change landing. If you take it out of this CL, I should not see
> it as part of this CL :(
> 
> If I compare changes between 22 and 23 for eviction_v3.cc (the first file that
> says it was modified between 22 and 23), I see changes that are not logically
> part of this CL, they are part of the code review taking place to change uma
> generation. That's what I mean by the two CLs being intermixed when they
should
> not. To be more explicit: a CL should not be diffed against another CL in
> progress, it should be diffed against ToT.

I have uploaded two new patchsets which you may find useful.

Patchset 24 is patchset 22, rebased to upstream. Upstream is
https://codereview.chromium.org/140893013/#ps700001 (patch set 7). 

Patchset 25 is a re-upload of patchset 23.

I think the diff you're looking for is patchset 24 --> patchset 25.

[gavinp, 2014-02-11 22:29:23]

On 2014/02/11 22:28:48, gavinp wrote:
> On 2014/02/11 21:16:39, rvargas wrote:
> > On 2014/02/11 20:39:27, gavinp wrote:
> > > On 2014/02/11 20:15:09, rvargas wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/121643003/diff/3890001/net/disk_cache/blockfi...
> > > > File net/disk_cache/blockfile/eviction_v3.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/121643003/diff/3890001/net/disk_cache/blockfi...
> > > > net/disk_cache/blockfile/eviction_v3.cc:44: #define
> > BLOCKFILE_BACKEND_IMPL_OBJ
> > > > backend_
> > > > I know I've mentioned it before, but seeing the same changes on multiple
> CLs
> > > is
> > > > a pain. CLs should be independent, always diffed against trunk, not
> > > intermixed.
> > > 
> > > Please recall that in
> > >
> >
>
https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v2/hist...
> > > , you asked for a change that I felt was out of scope for this CL; I
> responded
> > > by creating an upstream CL, now at
> https://codereview.chromium.org/140893013/
> > .
> > > Starting at PS 19, this CL has been expressed downstream of that CL.
> > > 
> > > I did this to reduce, not cause confusion. The changes around how
histogram
> > > macros were controlled were out of scope, and needed to be done first or
> else
> > > this CL would have confusing out of scope changes included in it. It had
to
> > > think hard how to respond to your comment at
> > >
> >
>
https://codereview.chromium.org/121643003/diff/3020001/net/disk_cache/v2/hist...
> > > , and my judgement, together with a peer I consulted with before
responding,
> > was
> > > that creating an upstream CL was less confusing.
> > > 
> > > After https://codereview.chromium.org/140893013/ lands, this CL will be
> based
> > on
> > > trunk.
> > > 
> > > > Now it is not just a matter of seeing what changed between PS 22 and 23
> > > 
> > > Actually, it is. PS 22 is also based on
> > > https://codereview.chromium.org/140893013/ , the last PS which was not is
PS
> > 18.
> > 
> > I understand what you are doing and why. I appreciate you taking the time to
> fix
> > something on an independent CL, but really, there is no concept of
> "downstream"
> > or "upstream" for code reviews or CLs (other than the existence of multiple
> > repos for projects hosted somewhere else, like what was WebKit).
> > 
> > Fixing the way that uma macros are generated can be done before or after
this
> > CL, and in any case, the only relevant thing is what is ToT at the time of
the
> > review and the change landing. If you take it out of this CL, I should not
see
> > it as part of this CL :(
> > 
> > If I compare changes between 22 and 23 for eviction_v3.cc (the first file
that
> > says it was modified between 22 and 23), I see changes that are not
logically
> > part of this CL, they are part of the code review taking place to change uma
> > generation. That's what I mean by the two CLs being intermixed when they
> should
> > not. To be more explicit: a CL should not be diffed against another CL in
> > progress, it should be diffed against ToT.
> 
> I have uploaded two new patchsets which you may find useful.
> 
> Patchset 24 is patchset 22, rebased to upstream. Upstream is
> https://codereview.chromium.org/140893013/#ps700001 (patch set 7). 
> 
> Patchset 25 is a re-upload of patchset 23.
> 
> I think the diff you're looking for is patchset 24 --> patchset 25.

[gavinp, 2014-02-11 22:38:27]

If you want to see this review based on ToT, let's focus on landing
https://codereview.chromium.org/140893013 .

It is, at times, confusing that patchsets can be rebases, remediations, or both.
I'm sorry for mixing them up periodically, I'll pay special attention here to
split them up.

Even basing on ToT, it's worth noting that rebases are required and a normal
part of reviews, particularly for long lived CLs like this one. For instance,
consider https://src.chromium.org/viewvc/chrome?revision=246089&view=revision ,
a change which landed in mid-January. It shows up in a rebase on this CL as
diffs between patchset 10 and patchset 11. No amount of basing on ToT would keep
that from showing up in inter-patchset diffs.

[rvargas (doing something else), 2014-02-12 19:00:16]

On 2014/02/11 22:38:27, gavinp wrote:
> If you want to see this review based on ToT, let's focus on landing
> https://codereview.chromium.org/140893013 .
> 
> It is, at times, confusing that patchsets can be rebases, remediations, or
both.
> I'm sorry for mixing them up periodically, I'll pay special attention here to
> split them up.
> 
> Even basing on ToT, it's worth noting that rebases are required and a normal
> part of reviews, particularly for long lived CLs like this one. For instance,
> consider https://src.chromium.org/viewvc/chrome?revision=246089&view=revision
,
> a change which landed in mid-January. It shows up in a rebase on this CL as
> diffs between patchset 10 and patchset 11. No amount of basing on ToT would
keep
> that from showing up in inter-patchset diffs.

Sure, rebasing against ToT is expected to happen. There's not much to do about
it. Seeing intermixed CLs is a different issue.

BTW, I didn't expect you to attempt to correct this CL to fix that issue (we are
very close to be done with it), but _please_ don't do that in the future. I
mention it again because this is not the first time it happens.

As for this CL, I'm still waiting for the final file rename for index_table &
Co. (in case that is not clear).

[rvargas (doing something else), 2014-02-12 19:42:37]

lgtm

[gavinp, 2014-02-26 16:32:09]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-02-26 16:32:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/121643003/5770001

[commit-bot: I haz the power, 2014-02-26 16:33:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-26 16:33:30]

Failed to apply patch for net/net.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file net/net.gyp
  Hunk #1 succeeded at 351 (offset 6 lines).
  Hunk #2 succeeded at 464 (offset 6 lines).
  Hunk #3 succeeded at 1319 (offset 12 lines).
  Hunk #4 succeeded at 1717 (offset 14 lines).
  Hunk #5 FAILED at 2135.
  Hunk #6 succeeded at 2400 (offset 18 lines).
  Hunk #7 succeeded at 2828 (offset 18 lines).
  1 out of 7 hunks FAILED -- saving rejects to file net/net.gyp.rej

Patch:       net/net.gyp
Index: net/net.gyp
diff --git a/net/net.gyp b/net/net.gyp
index
348c134af6d7c80c8aff55fce3955cd444806ce5..f4c1dec8e61cb8b18d1a43daeb2c35969c374527
100644
--- a/net/net.gyp
+++ b/net/net.gyp
@@ -345,70 +345,93 @@
         'cookies/cookie_util.h',
         'cookies/parsed_cookie.cc',
         'cookies/parsed_cookie.h',
-        'disk_cache/addr.cc',
-        'disk_cache/addr.h',
-        'disk_cache/backend_impl.cc',
-        'disk_cache/backend_impl.h',
-        'disk_cache/bitmap.cc',
-        'disk_cache/bitmap.h',
-        'disk_cache/block_files.cc',
-        'disk_cache/block_files.h',
+        'disk_cache/blockfile/addr.cc',
+        'disk_cache/blockfile/addr.h',
+        'disk_cache/blockfile/backend_impl.cc',
+        'disk_cache/blockfile/backend_impl.h',
+        'disk_cache/blockfile/backend_impl_v3.cc',
+        'disk_cache/blockfile/backend_impl_v3.h',
+        'disk_cache/blockfile/bitmap.cc',
+        'disk_cache/blockfile/bitmap.h',
+        'disk_cache/blockfile/block_bitmaps_v3.cc',
+        'disk_cache/blockfile/block_bitmaps_v3.h',
+        'disk_cache/blockfile/block_files.cc',
+        'disk_cache/blockfile/block_files.h',
+        'disk_cache/blockfile/disk_format.cc',
+        'disk_cache/blockfile/disk_format.h',
+        'disk_cache/blockfile/disk_format_base.h',
+        'disk_cache/blockfile/disk_format_v3.h',
+        'disk_cache/blockfile/entry_impl.cc',
+        'disk_cache/blockfile/entry_impl.h',
+        'disk_cache/blockfile/entry_impl_v3.cc',
+        'disk_cache/blockfile/entry_impl_v3.h',
+        'disk_cache/blockfile/errors.h',
+        'disk_cache/blockfile/eviction.cc',
+        'disk_cache/blockfile/eviction.h',
+        'disk_cache/blockfile/eviction_v3.cc',
+        'disk_cache/blockfile/eviction_v3.h',
+        'disk_cache/blockfile/experiments.h',
+        'disk_cache/blockfile/file.cc',
+        'disk_cache/blockfile/file.h',
+        'disk_cache/blockfile/file_block.h',
+        'disk_cache/blockfile/file_ios.cc',
+        'disk_cache/blockfile/file_lock.cc',
+        'disk_cache/blockfile/file_lock.h',
+        'disk_cache/blockfile/file_posix.cc',
+        'disk_cache/blockfile/file_win.cc',
+        'disk_cache/blockfile/histogram_macros.h',
+        'disk_cache/blockfile/histogram_macros_v3.h',
+        'disk_cache/blockfile/in_flight_backend_io.cc',
+        'disk_cache/blockfile/in_flight_backend_io.h',
+        'disk_cache/blockfile/in_flight_io.cc',
+        'disk_cache/blockfile/in_flight_io.h',
+        'disk_cache/blockfile/index_table_v3.cc',
+        'disk_cache/blockfile/index_table_v3.h',
+        'disk_cache/blockfile/mapped_file.cc',
+        'disk_cache/blockfile/mapped_file.h',
+        'disk_cache/blockfile/mapped_file_avoid_mmap_posix.cc',
+        'disk_cache/blockfile/mapped_file_posix.cc',
+        'disk_cache/blockfile/mapped_file_win.cc',
+        'disk_cache/blockfile/rankings.cc',
+        'disk_cache/blockfile/rankings.h',
+        'disk_cache/blockfile/sparse_control.cc',
+        'disk_cache/blockfile/sparse_control.h',
+        'disk_cache/blockfile/stats.cc',
+        'disk_cache/blockfile/stats.h',
+        'disk_cache/blockfile/stats_histogram.cc',
+        'disk_cache/blockfile/stats_histogram.h',
+        'disk_cache/blockfile/storage_block-inl.h',
+        'disk_cache/blockfile/storage_block.h',
+        'disk_cache/blockfile/stress_support.h',
+        'disk_cache/blockfile/trace.cc',
+        'disk_cache/blockfile/trace.h',
         'disk_cache/cache_creator.cc',
-        'disk_cache/cache_util.h',
         'disk_cache/cache_util.cc',
+        'disk_cache/cache_util.h',
         'disk_cache/cache_util_posix.cc',
         'disk_cache/cache_util_win.cc',
         'disk_cache/disk_cache.h',
-        'disk_cache/disk_format.cc',
-        'disk_cache/disk_format.h',
-        'disk_cache/disk_format_base.h',
-        'disk_cache/entry_impl.cc',
-        'disk_cache/entry_impl.h',
-        'disk_cache/errors.h',
-        'disk_cache/eviction.cc',
-        'disk_cache/eviction.h',
-        'disk_cache/experiments.h',
-        'disk_cache/file.cc',
-        'disk_cache/file.h',
-        'disk_cache/file_block.h',
-        'disk_cache/file_ios.cc',
-        'disk_cache/file_lock.cc',
-        'disk_cache/file_lock.h',
-        'disk_cache/file_posix.cc',
-        'disk_cache/file_win.cc',
-        'disk_cache/histogram_macros.h',
-        'disk_cache/in_flight_backend_io.cc',
-        'disk_cache/in_flight_backend_io.h',
-        'disk_cache/in_flight_io.cc',
-        'disk_cache/in_flight_io.h',
-        'disk_cache/mapped_file.cc',
-        'disk_cache/mapped_file.h',
-        'disk_cache/mapped_file_posix.cc',
-        'disk_cache/mapped_file_avoid_mmap_posix.cc',
-        'disk_cache/mapped_file_win.cc',
-        'disk_cache/mem_backend_impl.cc',
-        'disk_cache/mem_backend_impl.h',
-        'disk_cache/mem_entry_impl.cc',
-        'disk_cache/mem_entry_impl.h',
-        'disk_cache/mem_rankings.cc',
-        'disk_cache/mem_rankings.h',
+        'disk_cache/flash/flash_entry_impl.cc',
+        'disk_cache/flash/flash_entry_impl.h',
+        'disk_cache/flash/format.h',
+        'disk_cache/flash/internal_entry.cc',
+        'disk_cache/flash/internal_entry.h',
+        'disk_cache/flash/log_store.cc',
+        'disk_cache/flash/log_store.h',
+        'disk_cache/flash/log_store_entry.cc',
+        'disk_cache/flash/log_store_entry.h',
+        'disk_cache/flash/segment.cc',
+        'disk_cache/flash/segment.h',
+        'disk_cache/flash/storage.cc',
+        'disk_cache/flash/storage.h',
+        'disk_cache/memory/mem_backend_impl.cc',
+        'disk_cache/memory/mem_backend_impl.h',
+        'disk_cache/memory/mem_entry_impl.cc',
+        'disk_cache/memory/mem_entry_impl.h',
+        'disk_cache/memory/mem_rankings.cc',
+        'disk_cache/memory/mem_rankings.h',
         'disk_cache/net_log_parameters.cc',
         'disk_cache/net_log_parameters.h',
-        'disk_cache/rankings.cc',
-        'disk_cache/rankings.h',
-        'disk_cache/sparse_control.cc',
-        'disk_cache/sparse_control.h',
-        'disk_cache/stats.cc',
-        'disk_cache/stats.h',
-        'disk_cache/stats_histogram.cc',
-        'disk_cache/stats_histogram.h',
-        'disk_cache/storage_block-inl.h',
-        'disk_cache/storage_block.h',
-        'disk_cache/stress_support.h',
-        'disk_cache/trace.cc',
-        'disk_cache/trace.h',
-        'disk_cache/tracing_cache_backend.cc',
-        'disk_cache/tracing_cache_backend.h',
         'disk_cache/simple/simple_backend_impl.cc',
         'disk_cache/simple/simple_backend_impl.h',
         'disk_cache/simple/simple_backend_version.h',
@@ -435,31 +458,8 @@
         'disk_cache/simple/simple_util.h',
         'disk_cache/simple/simple_version_upgrade.cc',
         'disk_cache/simple/simple_version_upgrade.h',
-        'disk_cache/flash/flash_entry_impl.cc',
-        'disk_cache/flash/flash_entry_impl.h',
-        'disk_cache/flash/format.h',
-        'disk_cache/flash/internal_entry.cc',
-        'disk_cache/flash/internal_entry.h',
-        'disk_cache/flash/log_store.cc',
-        'disk_cache/flash/log_store.h',
-        'disk_cache/flash/log_store_entry.cc',
-        'disk_cache/flash/log_store_entry.h',
-        'disk_cache/flash/segment.cc',
-        'disk_cache/flash/segment.h',
-        'disk_cache/flash/storage.cc',
-        'disk_cache/flash/storage.h',
-        'disk_cache/v3/backend_impl_v3.cc',
-        'disk_cache/v3/backend_impl_v3.h',
-        'disk_cache/v3/block_bitmaps.cc',
-        'disk_cache/v3/block_bitmaps.h',
-        'disk_cache/v3/disk_format_v3.h',
-        'disk_cache/v3/entry_impl_v3.cc',
-        'disk_cache/v3/entry_impl_v3.h',
-        'disk_cache/v3/eviction_v3.cc',
-        'disk_cache/v3/eviction_v3.h',
-        'disk_cache/v3/histogram_macros.h',
-        'disk_cache/v3/index_table.cc',
-        'disk_cache/v3/index_table.h',
+        'disk_cache/tracing/tracing_cache_backend.cc',
+        'disk_cache/tracing/tracing_cache_backend.h',
         'dns/address_sorter.h',
         'dns/address_sorter_posix.cc',
         'dns/address_sorter_posix.h',
@@ -1307,11 +1307,11 @@
             ],
           },
           'sources!': [
-            'disk_cache/mapped_file_posix.cc',
+            'disk_cache/blockfile/mapped_file_posix.cc',
           ],
         }, { # else
           'sources!': [
-            'disk_cache/mapped_file_avoid_mmap_posix.cc',
+            'disk_cache/blockfile/mapped_file_avoid_mmap_posix.cc',
           ],
         }],
         ['disable_ftp_support==1', {
@@ -1703,26 +1703,27 @@
         'cookies/cookie_store_unittest.h',
         'cookies/cookie_util_unittest.cc',
         'cookies/parsed_cookie_unittest.cc',
-        'disk_cache/addr_unittest.cc',
         'disk_cache/backend_unittest.cc',
-        'disk_cache/bitmap_unittest.cc',
-        'disk_cache/block_files_unittest.cc',
+        'disk_cache/blockfile/addr_unittest.cc',
+        'disk_cache/blockfile/bitmap_unittest.cc',
+        'disk_cache/blockfile/block_bitmaps_v3_unittest.cc',
+        'disk_cache/blockfile/block_bitmaps_v3_unittest.cc',
+        'disk_cache/blockfile/block_files_unittest.cc',
+        'disk_cache/blockfile/index_table_v3_unittest.cc',
+        'disk_cache/blockfile/mapped_file_unittest.cc',
+        'disk_cache/blockfile/storage_block_unittest.cc',
         'disk_cache/cache_util_unittest.cc',
        …
(message too large)

[gavinp, 2014-02-26 19:31:57]

OK, I have given up on making a rietveld upload that doesn't 500, after all the
tries here. I've rebased, going to CQ again.

[gavinp, 2014-02-26 19:34:20]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-02-26 19:43:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/121643003/8450002

[commit-bot: I haz the power, 2014-02-26 20:18:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-26 20:18:15]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[gavinp, 2014-02-26 20:23:54]

Committed patchset #38 manually as r253581.

[mmenke, 2014-02-26 21:07:59]

On 2014/02/26 20:23:54, gavinp wrote:
> Committed patchset #38 manually as r253581.

Gavin:  Build failure looks like it may be yours.

[gavinp, 2014-02-26 21:23:24]

On 2014/02/26 21:07:59, mmenke wrote:
> On 2014/02/26 20:23:54, gavinp wrote:
> > Committed patchset #38 manually as r253581.
> 
> Gavin:  Build failure looks like it may be yours.

It was. I've reverted this patch in r253598, and I'll fix it here and reland.

[gavinp, 2014-02-26 21:35:49]

This is now based on trunk. I've added mmenke as a TBR for the #include change
in include/net. I'm letting try jobs mature before I land, as this can't go
through CQ due to the disk cache not passing presubmit.

[gavinp, 2014-03-03 16:39:06]

See https://src.chromium.org/viewvc/chrome?view=rev&revision=254474 for the
reland.