Make the blockfile cache an optional build product for chrome target.

Make the blockfile cache an optional build product for chrome target.

This change adds a new build option to net to not build the blockfile
backend, and touches up cache_creator.cc enough to let chrome build
using this option.

Remaining to be done is to fix the disk cache tests to respect this...

R=rvargas
BUG=331062

[gavinp, 2014-01-02 22:52:50]

Here's the start of making the blockfile cache a configurable build product.
This change doesn't enable anything but default, but chrome builds and runs
without linking in the blockfile cache.

[rvargas (doing something else), 2014-01-03 19:56:13]

https://codereview.chromium.org/117533003/diff/290001/net/disk_cache/cache_cr...
File net/disk_cache/cache_creator.cc (right):

https://codereview.chromium.org/117533003/diff/290001/net/disk_cache/cache_cr...
net/disk_cache/cache_creator.cc:15: #ifdef USE_BLOCKFILE_CACHE_BACKEND
Can we use OS_ANDROID here?

Having too many knobs and levers is not a good long term strategy and it leads
to not knowing what is being compiled/supported.

I gather that your intention is to remove unit tests for Android, and that's why
I think it is better to avoid an extra configuration axis. However, if your long
term plan is to keep the code unit tested then an extra knob may be needed.

[gavinp, 2014-02-05 20:54:13]

PTAL, this is now rebased on top of the rebased upstream, and it'd be great to
settle the questions here before we move on to the test refactor.

https://codereview.chromium.org/117533003/diff/290001/net/disk_cache/cache_cr...
File net/disk_cache/cache_creator.cc (right):

https://codereview.chromium.org/117533003/diff/290001/net/disk_cache/cache_cr...
net/disk_cache/cache_creator.cc:15: #ifdef USE_BLOCKFILE_CACHE_BACKEND
On 2014/01/03 19:56:13, rvargas wrote:
> Can we use OS_ANDROID here?
> 
> Having too many knobs and levers is not a good long term strategy and it leads
> to not knowing what is being compiled/supported.

If we use OS_ANDROID here, the expression will be repeated twice in this CL, and
a third time in net.gyp (in a downstream CL). That's a lot of repetitions,
especially since this flag is very useful to set during development.

There's an argument that we don't need to conditionalise the #include here,
since the linker will catch an error if we reference a symbol. That scares me
though, as I enumerate the number of distinct ways in which a header file can
change compilation besides introducing symbols.

> 
> I gather that your intention is to remove unit tests for Android, and that's
why
> I think it is better to avoid an extra configuration axis. However, if your
long
> term plan is to keep the code unit tested then an extra knob may be needed.

https://codereview.chromium.org/117533003/diff/1720001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/117533003/diff/1720001/net/net.gyp#newcode10
net/net.gyp:10: 'use_blockfile_cache_backend%': 1,
In the upstream review, there was some discussion about naming the cache used on
non-Android platforms. I've been using blockfile, but we can change this
identifier to include anything. Any suggestions?

[rvargas (doing something else), 2014-02-06 05:09:28]

https://codereview.chromium.org/117533003/diff/290001/net/disk_cache/cache_cr...
File net/disk_cache/cache_creator.cc (right):

https://codereview.chromium.org/117533003/diff/290001/net/disk_cache/cache_cr...
net/disk_cache/cache_creator.cc:15: #ifdef USE_BLOCKFILE_CACHE_BACKEND
On 2014/02/05 20:54:13, gavinp wrote:
> On 2014/01/03 19:56:13, rvargas wrote:
> > Can we use OS_ANDROID here?
> > 
> > Having too many knobs and levers is not a good long term strategy and it
leads
> > to not knowing what is being compiled/supported.
> 
> If we use OS_ANDROID here, the expression will be repeated twice in this CL,
and
> a third time in net.gyp (in a downstream CL). That's a lot of repetitions,
> especially since this flag is very useful to set during development.

The number of references seem the same regardless of the name of the symbol.
However, even though now I don't like the way that net _seems_ to have a lot of
configuration options that are not tested (and hence not really well supported),
the fact remains that all knobs on the library are handled the same way: through
a subject-specific gyp variable. In other words, using an OS directly here would
go against the flow.

> 
> There's an argument that we don't need to conditionalise the #include here,
> since the linker will catch an error if we reference a symbol. That scares me
> though, as I enumerate the number of distinct ways in which a header file can
> change compilation besides introducing symbols.

I lean towards leaving the include in place (we shouldn't be adding code weight
in headers in the first place), but I really don't care that much one way or the
other.

> 
> > 
> > I gather that your intention is to remove unit tests for Android, and that's
> why
> > I think it is better to avoid an extra configuration axis. However, if your
> long
> > term plan is to keep the code unit tested then an extra knob may be needed.
>

https://codereview.chromium.org/117533003/diff/1920001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/117533003/diff/1920001/net/net.gyp#newcode26
net/net.gyp:26: 'posix_avoid_mmap%': 1,
I think we should either move all backends to a use_foo pattern, or if we are
making an exception to the norm use this pattern and use a negative definition
(as much as I dislike them).

I'm actually thinking about the final #ifdef in the code, not really about the
gyp variables.

https://codereview.chromium.org/117533003/diff/1920001/net/net.gyp#newcode1341
net/net.gyp:1341: 'sources/': [ ['exclude', '^disk_cache/v[23]/' ] ]
I'm not sure about excluding v3. I've been leaning towards not excluding it
lately.

Also, I don't think in general we use patterns to exclude/include files on gyp
files (other than the platform specific ones). I would have guessed we want to
be absolutely explicit about what files are part of a target. Matching patterns
from cs or similar doesn't work.

[gavinp, 2014-02-09 15:29:37]

PTAL, this is now updated to its upstream review.

https://codereview.chromium.org/117533003/diff/1920001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/117533003/diff/1920001/net/net.gyp#newcode1341
net/net.gyp:1341: 'sources/': [ ['exclude', '^disk_cache/v[23]/' ] ]
On 2014/02/06 05:09:28, rvargas wrote:
> I'm not sure about excluding v3. I've been leaning towards not excluding it
> lately.

Can you expand on that?

> 
> Also, I don't think in general we use patterns to exclude/include files on gyp
> files (other than the platform specific ones). I would have guessed we want to
> be absolutely explicit about what files are part of a target. Matching
patterns
> from cs or similar doesn't work.

What is "cs or similar"?

This pattern works here, because the directory organization was chosen
specifically to support it, though; it is absolutely explicit because placement
in a directory means exactly that.

Explicitly listing carries the downside that there's two lists to keep precisely
in sync.  Someone will get that wrong very quickly if we go to it.

[gavinp, 2014-02-13 20:18:04]

I think Ricardo meant "codesearch" when he said "cs or similar above."

I was going to remediate this CL per the review, adding an explicit list of
excluded sources, when I came across this existing pattern very close to the
call site for blockfile cache.

Is blockfile backend different from ftp? How?

If it isn't, should we fix ftp too?

https://codereview.chromium.org/117533003/diff/2820001/net/net.gyp
File net/net.gyp (right):

https://codereview.chromium.org/117533003/diff/2820001/net/net.gyp#newcode1320
net/net.gyp:1320: ['exclude', '^ftp/'],
We exclude by pattern on ftp already. Should we fix that?

[rvargas (doing something else), 2014-02-13 21:46:01]

On 2014/02/13 20:18:04, gavinp wrote:
> I think Ricardo meant "codesearch" when he said "cs or similar above."
> 
> I was going to remediate this CL per the review, adding an explicit list of
> excluded sources, when I came across this existing pattern very close to the
> call site for blockfile cache.
> 
> Is blockfile backend different from ftp? How?
> 
> If it isn't, should we fix ftp too?
> 
> https://codereview.chromium.org/117533003/diff/2820001/net/net.gyp
> File net/net.gyp (right):
> 
> https://codereview.chromium.org/117533003/diff/2820001/net/net.gyp#newcode1320
> net/net.gyp:1320: ['exclude', '^ftp/'],
> We exclude by pattern on ftp already. Should we fix that?

Yes, we should