Disk cache: Introduce a BlockHeader class.

Disk cache: Introduce a BlockHeader class.

This class provides a somewhat similar interface to BlockFiles but
operates exclusively with data in memory so it can be used on
any thread because it never blocks. It encapsulates the header
of a block file.

BUG=241277
TEST=net_unittests (as in BlockFiles are implemented using BlockHeader)

Committed http://src.chromium.org/viewvc/chrome?view=revision&revision=207665

[rvargas (doing something else), 2013-06-14 02:37:46]

On 2013/06/13 01:25:20, rvargas wrote:

ping

[gavinp, 2013-06-14 15:37:02]

https://codereview.chromium.org/16843007/diff/1/net/disk_cache/block_files.h
File net/disk_cache/block_files.h (right):

https://codereview.chromium.org/16843007/diff/1/net/disk_cache/block_files.h#...
net/disk_cache/block_files.h:27: class BlockHeader {
Is this class ever referenced in any translation unit other than block_files.cc?

I've gone through
https://codereview.chromium.org/download/issue15203004_15001.diff and I couldn't
find it.

https://codereview.chromium.org/16843007/diff/1/net/disk_cache/block_files.h#...
net/disk_cache/block_files.h:67: BlockFileHeader* header_;
#include "net/disk_cache/disk_format_base.h"

[rvargas (doing something else), 2013-06-14 18:32:07]

https://codereview.chromium.org/16843007/diff/1/net/disk_cache/block_files.h
File net/disk_cache/block_files.h (right):

https://codereview.chromium.org/16843007/diff/1/net/disk_cache/block_files.h#...
net/disk_cache/block_files.h:27: class BlockHeader {
On 2013/06/14 15:37:02, gavinp wrote:
> Is this class ever referenced in any translation unit other than
block_files.cc?
> 
> I've gone through
> https://codereview.chromium.org/download/issue15203004_15001.diff and I
couldn't
> find it.

Sure,
https://codereview.chromium.org/15203004/diff/15001/net/disk_cache/v3/backend...
line 334.

In other words, BackendImplV3 only uses BlockHeaders, not BlockFiles

https://codereview.chromium.org/16843007/diff/1/net/disk_cache/block_files.h#...
net/disk_cache/block_files.h:67: BlockFileHeader* header_;
On 2013/06/14 15:37:02, gavinp wrote:
> #include "net/disk_cache/disk_format_base.h"

Done.

[gavinp, 2013-06-18 14:17:14]

This class seems like a lot of machinery to hide a cast; the class is just a
pointer.

Why not just put methods in a namespace, and make methods like:

namespace block_header {

bool CreateMapBlock(BlockFileHeader* header, int index, int size, int* index);
...

}

[gavinp, 2013-06-18 14:17:45]

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

https://codereview.chromium.org/16843007/diff/1/net/disk_cache/block_files.cc...
net/disk_cache/block_files.cc:46: :
header_(reinterpret_cast<BlockFileHeader*>(file->buffer())) {
Shouldn't this be a static_cast?

[rvargas (doing something else), 2013-06-18 18:12:18]

On 2013/06/18 14:17:14, gavinp wrote:
> This class seems like a lot of machinery to hide a cast; the class is just a
> pointer.
> 
> Why not just put methods in a namespace, and make methods like:
> 
> namespace block_header {
> 
> bool CreateMapBlock(BlockFileHeader* header, int index, int size, int* index);
> ...
> 
> }

for posterity: as we talked about, this class encapsulates the concept behind a
header. It uses a pointer to a structure more as an implementation detail (and
to keep BlockFiles code clean)

[rvargas (doing something else), 2013-06-18 18:12:23]

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

https://codereview.chromium.org/16843007/diff/1/net/disk_cache/block_files.cc...
net/disk_cache/block_files.cc:46: :
header_(reinterpret_cast<BlockFileHeader*>(file->buffer())) {
On 2013/06/18 14:17:45, gavinp wrote:
> Shouldn't this be a static_cast?

This is a cast between "unrelated" pointer types, so IMO logically should be a
reinterpret.

https://codereview.chromium.org/16843007/diff/1/net/disk_cache/block_files.h
File net/disk_cache/block_files.h (right):

https://codereview.chromium.org/16843007/diff/1/net/disk_cache/block_files.h#...
net/disk_cache/block_files.h:27: class BlockHeader {
On 2013/06/14 18:32:07, rvargas wrote:
> On 2013/06/14 15:37:02, gavinp wrote:
> > Is this class ever referenced in any translation unit other than
> block_files.cc?
> > 
> > I've gone through
> > https://codereview.chromium.org/download/issue15203004_15001.diff and I
> couldn't
> > find it.
> 
> Sure,
>
https://codereview.chromium.org/15203004/diff/15001/net/disk_cache/v3/backend...
> line 334.
> 
> In other words, BackendImplV3 only uses BlockHeaders, not BlockFiles

I'm sorry, this was not completely accurate... there is a layer in between
BlockHeader and the backend: BlockBitmaps (v3/block_bitmaps.xx)... but the idea
is the same, the backend uses the same interface of BlockFiles, so there is a
class (BlockHeader) that implements that interface for an in-memory entity
(without doing IO)

[gavinp, 2013-06-18 18:14:02]

LGTM, I'll look at this more closely once consumers land.

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

https://codereview.chromium.org/16843007/diff/1/net/disk_cache/block_files.cc...
net/disk_cache/block_files.cc:220: int BlockHeader::Size() const {
This is a good candidate for an inline function, especially since it's compile
time evaluable.

[rvargas (doing something else), 2013-06-19 01:32:32]

Thanks

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

https://codereview.chromium.org/16843007/diff/1/net/disk_cache/block_files.cc...
net/disk_cache/block_files.cc:220: int BlockHeader::Size() const {
On 2013/06/18 18:14:03, gavinp wrote:
> This is a good candidate for an inline function, especially since it's compile
> time evaluable.

This method will be removed when the code to open v3 block files land.

[gavinp, 2013-07-16 18:15:57]

Note: this change was committed
http://src.chromium.org/viewvc/chrome?view=revision&revision=207665