Adding disk_cache::Entry implementation.

Adding disk_cache::Entry implementation.

BUG=157187
TEST=net_unittests --gtest_filter="FlashCacheTest.*" --gtest_repeat=10 --shuffle


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

[agayev, 2013-03-22 00:54:14]

PTAL.  I hope to add features to this CL and iterating until it is complete.

[rvargas (doing something else), 2013-03-22 16:45:00]

looks good so far

[agayev, 2013-03-26 17:23:53]

Added Init logic, please review.  The rest is not implemented yet.  Also, the
test fails, I'm looking into that as well.

[rvargas (doing something else), 2013-03-28 18:36:25]

Looks fine. Having the code that creates entries would be much better because
there is no way to tell if there's something funky going on.

https://codereview.chromium.org/12847012/diff/8001/net/disk_cache/flash/flash...
File net/disk_cache/flash/flash_entry_impl.cc (right):

https://codereview.chromium.org/12847012/diff/8001/net/disk_cache/flash/flash...
net/disk_cache/flash/flash_entry_impl.cc:41: Bind(&InternalEntry::Init,
old_internal_entry_),
This requires InternalEntry to be RefCountedThreadSafe

https://codereview.chromium.org/12847012/diff/8001/net/disk_cache/flash/flash...
net/disk_cache/flash/flash_entry_impl.cc:42:
Bind(&FlashEntryImpl::OnInitComplete, this));
Ditto

https://codereview.chromium.org/12847012/diff/8001/net/disk_cache/flash/flash...
net/disk_cache/flash/flash_entry_impl.cc:129: void
FlashEntryImpl::OnInitComplete(scoped_ptr<KeyAndStreamSizes> rv) {
nit: result? use the same name you used on the declaration.

[agayev, 2013-04-04 22:07:57]

PTAL.

https://codereview.chromium.org/12847012/diff/8001/net/disk_cache/flash/flash...
File net/disk_cache/flash/flash_entry_impl.cc (right):

https://codereview.chromium.org/12847012/diff/8001/net/disk_cache/flash/flash...
net/disk_cache/flash/flash_entry_impl.cc:41: Bind(&InternalEntry::Init,
old_internal_entry_),
On 2013/03/28 18:36:25, rvargas wrote:
> This requires InternalEntry to be RefCountedThreadSafe

Done.

https://codereview.chromium.org/12847012/diff/8001/net/disk_cache/flash/flash...
net/disk_cache/flash/flash_entry_impl.cc:42:
Bind(&FlashEntryImpl::OnInitComplete, this));
On 2013/03/28 18:36:25, rvargas wrote:
> Ditto

Done.

https://codereview.chromium.org/12847012/diff/8001/net/disk_cache/flash/flash...
net/disk_cache/flash/flash_entry_impl.cc:129: void
FlashEntryImpl::OnInitComplete(scoped_ptr<KeyAndStreamSizes> rv) {
On 2013/03/28 18:36:25, rvargas wrote:
> nit: result? use the same name you used on the declaration.

Done.

[rvargas (doing something else), 2013-04-05 18:15:40]

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
File net/disk_cache/flash/flash_entry_impl.h (right):

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:28: // Context switches are slow on
Android.  In order to minimize the context
nit: Remove the first sentence?. It is vague and even if true it may become
stale without notice.

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:29: // switches between IOThread and
CacheThread, this disk_cache::Entry
This layer doesn't know anything about IO or Cache threads.

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:31: // creating a new entry.
creating a new entry is the most common case? that is... surprising :)

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:33: // All asynchronous function calls
on a new entry are served from IOThread
nit: All calls are served synchronously.

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:89: // Used if |this| is an existing
entry.  It is initialized once during Init(),
So... it is not used for new entries?. It is initialized more than once? (it's
not a good idea to describe when and how a member is used... this is just the
declaration, not the implementation)

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
File net/disk_cache/flash/flash_entry_unittest.cc (right):

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_unittest.cc:27: entry->AddRef();
why?

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_unittest.cc:38: 0, 0, buffer1, kSize1,
net::CompletionCallback()));
nit: indent under first arg

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/inte...
File net/disk_cache/flash/internal_entry.h (right):

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/inte...
net/disk_cache/flash/internal_entry.h:33: class NET_EXPORT_PRIVATE InternalEntry
nit: Add a class description. One sentence would be fine...

[agayev, 2013-04-05 22:26:45]

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
File net/disk_cache/flash/flash_entry_impl.h (right):

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:28: // Context switches are slow on
Android.  In order to minimize the context
On 2013/04/05 18:15:41, rvargas wrote:
> nit: Remove the first sentence?. It is vague and even if true it may become
> stale without notice.

Done.

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:29: // switches between IOThread and
CacheThread, this disk_cache::Entry
On 2013/04/05 18:15:41, rvargas wrote:
> This layer doesn't know anything about IO or Cache threads.

Done.

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:31: // creating a new entry.
On 2013/04/05 18:15:41, rvargas wrote:
> creating a new entry is the most common case? that is... surprising :)

Done, I think :)

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:33: // All asynchronous function calls
on a new entry are served from IOThread
On 2013/04/05 18:15:41, rvargas wrote:
> nit: All calls are served synchronously.

Done.

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:89: // Used if |this| is an existing
entry.  It is initialized once during Init(),
On 2013/04/05 18:15:41, rvargas wrote:
> So... it is not used for new entries?. It is initialized more than once? (it's
> not a good idea to describe when and how a member is used... this is just the
> declaration, not the implementation)

Done.  I was going to use it for old entry only, but since they take space
anyway in both cases, decided to use it for both cases; will update accordingly.

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
File net/disk_cache/flash/flash_entry_unittest.cc (right):

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_unittest.cc:27: entry->AddRef();
On 2013/04/05 18:15:41, rvargas wrote:
> why?

If I put it inside scoped_refptr, there are 2 Release() calls, one by the
scoped_refptr destructor and another by Close() which isn't right.

This will disappear in the next CL, when backend is in place.  Or if you have a
suggestion, I can do that too.

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_unittest.cc:38: 0, 0, buffer1, kSize1,
net::CompletionCallback()));
On 2013/04/05 18:15:41, rvargas wrote:
> nit: indent under first arg

Done.

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/inte...
File net/disk_cache/flash/internal_entry.h (right):

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/inte...
net/disk_cache/flash/internal_entry.h:33: class NET_EXPORT_PRIVATE InternalEntry
On 2013/04/05 18:15:41, rvargas wrote:
> nit: Add a class description. One sentence would be fine...

Done.

[rvargas (doing something else), 2013-04-06 01:25:23]

almost

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
File net/disk_cache/flash/flash_entry_unittest.cc (right):

https://codereview.chromium.org/12847012/diff/20001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_unittest.cc:27: entry->AddRef();
On 2013/04/05 22:26:45, agayev wrote:
> On 2013/04/05 18:15:41, rvargas wrote:
> > why?
> 
> If I put it inside scoped_refptr, there are 2 Release() calls, one by the
> scoped_refptr destructor and another by Close() which isn't right.
> 
> This will disappear in the next CL, when backend is in place.  Or if you have
a
> suggestion, I can do that too.

I miss'd this was a direct new, ok.

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/flas...
File net/disk_cache/flash/flash_entry_impl.h (right):

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:31: // All asynchronous calls on a new
entry are served synchronously.  When an
nit: All calls on a new entry

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:32: // object is destructed (via final
Close() call), a message is posted to cache
nit: to the cache

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:35: // When an entry is not new, every
asynchronous call is posted to cache thread,
nit: to the cache

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/flas...
File net/disk_cache/flash/flash_entry_unittest.cc (right):

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_unittest.cc:41: EXPECT_EQ(kSize1,
entry->WriteData(
nit: indent under the first arg

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/inte...
File net/disk_cache/flash/internal_entry.h (right):

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/inte...
net/disk_cache/flash/internal_entry.h:34: // storing data.  FlashEntryImpl
contains a pair of InternalEntry objects, each
nit: don't document the caller of this code... comments should refer to what the
code being commented does.

[agayev, 2013-04-06 03:24:28]

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/flas...
File net/disk_cache/flash/flash_entry_impl.h (right):

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:31: // All asynchronous calls on a new
entry are served synchronously.  When an
On 2013/04/06 01:25:23, rvargas wrote:
> nit: All calls on a new entry

Done.

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:32: // object is destructed (via final
Close() call), a message is posted to cache
On 2013/04/06 01:25:23, rvargas wrote:
> nit: to the cache

Done.

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:35: // When an entry is not new, every
asynchronous call is posted to cache thread,
On 2013/04/06 01:25:23, rvargas wrote:
> nit: to the cache

Done.

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_impl.h:35: // When an entry is not new, every
asynchronous call is posted to cache thread,
On 2013/04/06 01:25:23, rvargas wrote:
> nit: to the cache

Done.

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/flas...
File net/disk_cache/flash/flash_entry_unittest.cc (right):

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/flas...
net/disk_cache/flash/flash_entry_unittest.cc:41: EXPECT_EQ(kSize1,
entry->WriteData(
On 2013/04/06 01:25:23, rvargas wrote:
> nit: indent under the first arg

Done.

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/inte...
File net/disk_cache/flash/internal_entry.h (right):

https://codereview.chromium.org/12847012/diff/30001/net/disk_cache/flash/inte...
net/disk_cache/flash/internal_entry.h:34: // storing data.  FlashEntryImpl
contains a pair of InternalEntry objects, each
On 2013/04/06 01:25:23, rvargas wrote:
> nit: don't document the caller of this code... comments should refer to what
the
> code being commented does.

Done.

[rvargas (doing something else), 2013-04-08 17:51:22]

lgtm

[commit-bot: I haz the power, 2013-04-08 19:53:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agayev@chromium.org/12847012/46001

[commit-bot: I haz the power, 2013-04-08 21:16:54]

Change committed as 192896