Add HostCachePersistenceManager for Cronet

Add HostCachePersistenceManager for Cronet

HostCachePersistenceManager handles the interaction between HostCache
and prefs for persistence. It batches writes to avoid writing to prefs
too often. This implementation is Cronet-specific because it uses the
same sequence for interactions with network and prefs code.

BUG=605149
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2953483003
Cr-Commit-Position: refs/heads/master@{#482001}
Committed: https://chromium.googlesource.com/chromium/src/+/983dbf87d4b115593d648e58f86b336401945647

[mgersh, 2017-06-20 22:48:03]

Description was changed from

==========
Add HostCachePersistenceManager for Cronet

HostCachePersistenceManager handles the interaction between HostCache
and prefs for persistence. It batches writes to avoid writing to prefs
too often. This implementation is Cronet-specific because it uses the
same sequence for interactions with network and prefs code.

BUG=605149
==========

to

==========
Add HostCachePersistenceManager for Cronet

HostCachePersistenceManager handles the interaction between HostCache
and prefs for persistence. It batches writes to avoid writing to prefs
too often. This implementation is Cronet-specific because it uses the
same sequence for interactions with network and prefs code.

BUG=605149
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[mgersh, 2017-06-21 12:24:58]

Patchset #1 (id:1) has been deleted

[mgersh, 2017-06-21 12:33:10]

mgersh@chromium.org changed reviewers:
+ xunjieli@chromium.org

[mgersh, 2017-06-21 12:33:11]

PTAL

[mgersh, 2017-06-22 14:25:09]

mgersh@chromium.org changed reviewers:
+ pauljensen@chromium.org

[mgersh, 2017-06-22 14:25:10]

Paul, PTAL. Helen suggested that I add you. There's a public copy of the design
doc at
https://docs.google.com/document/d/16Ru9m0T0NtzIM5rr57mbk7nzrpDrtgVTBZdfeFRDz....

[pauljensen, 2017-06-22 19:03:51]

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
File components/cronet/host_cache_persistence_manager.cc (right):

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.cc:50:
cache_->set_persistence_delegate(nullptr);
I feel like either there should be a DCHECK_EQ(cache_->persistence_delectate(),
this) here, or in set_persistence_delegate() there should be a
DCHECK_NE(delegate == nullptr, delegate_ == nullptr), to prevent inadvertent
overwriting of the persistence delegate.

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.cc:53: void
HostCachePersistenceManager::OnRead() {
It looks like this is the counterpart to DoWrite() as DoWrite() copies from
cache_ to prefs, and this function copies from prefs to cache_.  Can we name it
DoRead()?  or perhaps we could rename both functions to Save() and Restore(), or
Save() and Reload(), or WriteToDisk() and ReadFromDisk()?

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
File components/cronet/host_cache_persistence_manager.h (right):

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:35:
HostCachePersistenceManager(net::HostCache* cache,
Can you add a comment here explaining the arguments?

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:50: net::HostCache* cache_;
add const, then we can remove all but the constructor's DCHECK(cache_)

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:52:
std::unique_ptr<PrefChangeRegistrar> registrar_;
remove the unique_ptr, so just PrefChangeRegistrar

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:53: PrefService*
pref_service_;
add const, then we can remove all but the constructor's DCHECK(cache_)

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:57: base::TimeDelta delay_;
const

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:58:
std::unique_ptr<base::OneShotTimer> timer_;
remove the unique_ptr, so just base::OneShotTimer

[mgersh, 2017-06-22 21:27:33]

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
File components/cronet/host_cache_persistence_manager.cc (right):

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.cc:50:
cache_->set_persistence_delegate(nullptr);
On 2017/06/22 19:03:50, pauljensen wrote:
> I feel like either there should be a
DCHECK_EQ(cache_->persistence_delectate(),
> this) here, or in set_persistence_delegate() there should be a
> DCHECK_NE(delegate == nullptr, delegate_ == nullptr), to prevent inadvertent
> overwriting of the persistence delegate.
>   

Done. Picked the second because it'll give a more useful stack trace if
something tries to add a second PersistenceDelegate.

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.cc:53: void
HostCachePersistenceManager::OnRead() {
On 2017/06/22 19:03:50, pauljensen wrote:
> It looks like this is the counterpart to DoWrite() as DoWrite() copies from
> cache_ to prefs, and this function copies from prefs to cache_.  Can we name
it
> DoRead()?  or perhaps we could rename both functions to Save() and Restore(),
or
> Save() and Reload(), or WriteToDisk() and ReadFromDisk()?

Done.

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
File components/cronet/host_cache_persistence_manager.h (right):

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:35:
HostCachePersistenceManager(net::HostCache* cache,
On 2017/06/22 19:03:51, pauljensen wrote:
> Can you add a comment here explaining the arguments?

Done.

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:50: net::HostCache* cache_;
On 2017/06/22 19:03:50, pauljensen wrote:
> add const, then we can remove all but the constructor's DCHECK(cache_)

Done.

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:52:
std::unique_ptr<PrefChangeRegistrar> registrar_;
On 2017/06/22 19:03:51, pauljensen wrote:
> remove the unique_ptr, so just PrefChangeRegistrar

Done.

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:53: PrefService*
pref_service_;
On 2017/06/22 19:03:51, pauljensen wrote:
> add const, then we can remove all but the constructor's DCHECK(cache_)

Done.

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:57: base::TimeDelta delay_;
On 2017/06/22 19:03:50, pauljensen wrote:
> const

Done.

https://codereview.chromium.org/2953483003/diff/20001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:58:
std::unique_ptr<base::OneShotTimer> timer_;
On 2017/06/22 19:03:50, pauljensen wrote:
> remove the unique_ptr, so just base::OneShotTimer

Done.

[pauljensen, 2017-06-23 14:10:31]

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
File components/cronet/host_cache_persistence_manager.cc (right):

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.cc:71:
std::unique_ptr<base::ListValue> value = cache_->GetAsListValue(false);
I'd prefer if GetAsListValue() took a ListValue* rather than returned a
unique_ptr; it saves a memory allocation and deallocation in cases like this
function.  I'm not sure what the precedent for this is; net::GetNetworkList()
and URLRequest::GetFullHeaders() take pointer to classes that they fill in.

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
File components/cronet/host_cache_persistence_manager.h (right):

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:23: // On write, starts a
timer, or ignores if the timer is already running. When
I'm a little confused by "On write", perhaps "Upon a write to the HostCache"
might clarify?

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:43:
~HostCachePersistenceManager();
should be virtual.
"If your class has virtual methods, its destructor should be virtual."
https://google.github.io/styleguide/cppguide.html#Inheritance

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:45: //
HostCache::PersistenceDelegate implementation
nit: add "net::"

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
File components/cronet/host_cache_persistence_manager_unittest.cc (right):

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
components/cronet/host_cache_persistence_manager_unittest.cc:19: const char
kPrefName[] = "net.test";
how about moving this into HostCachePersistenceManagerTest as a static?

[mgersh, 2017-06-23 16:02:06]

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
File components/cronet/host_cache_persistence_manager.cc (right):

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.cc:71:
std::unique_ptr<base::ListValue> value = cache_->GetAsListValue(false);
On 2017/06/23 14:10:31, pauljensen wrote:
> I'd prefer if GetAsListValue() took a ListValue* rather than returned a
> unique_ptr; it saves a memory allocation and deallocation in cases like this
> function.  I'm not sure what the precedent for this is; net::GetNetworkList()
> and URLRequest::GetFullHeaders() take pointer to classes that they fill in.

Done.

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
File components/cronet/host_cache_persistence_manager.h (right):

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:23: // On write, starts a
timer, or ignores if the timer is already running. When
On 2017/06/23 14:10:31, pauljensen wrote:
> I'm a little confused by "On write", perhaps "Upon a write to the HostCache"
> might clarify?

Done.

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:43:
~HostCachePersistenceManager();
On 2017/06/23 14:10:31, pauljensen wrote:
> should be virtual.
> "If your class has virtual methods, its destructor should be virtual."
> https://google.github.io/styleguide/cppguide.html#Inheritance

Done.

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
components/cronet/host_cache_persistence_manager.h:45: //
HostCache::PersistenceDelegate implementation
On 2017/06/23 14:10:31, pauljensen wrote:
> nit: add "net::"

Done.

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
File components/cronet/host_cache_persistence_manager_unittest.cc (right):

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
components/cronet/host_cache_persistence_manager_unittest.cc:19: const char
kPrefName[] = "net.test";
On 2017/06/23 14:10:31, pauljensen wrote:
> how about moving this into HostCachePersistenceManagerTest as a static?

The compiler wasn't happy with that, and all the other tests I've found use an
anonymous namespace like I did here.

[pauljensen, 2017-06-23 17:39:31]

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
File components/cronet/host_cache_persistence_manager_unittest.cc (right):

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
components/cronet/host_cache_persistence_manager_unittest.cc:19: const char
kPrefName[] = "net.test";
On 2017/06/23 16:02:06, mgersh wrote:
> On 2017/06/23 14:10:31, pauljensen wrote:
> > how about moving this into HostCachePersistenceManagerTest as a static?
> 
> The compiler wasn't happy with that, and all the other tests I've found use an
> anonymous namespace like I did here.

What was the compiler error?

[mgersh, 2017-06-23 18:08:09]

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
File components/cronet/host_cache_persistence_manager_unittest.cc (right):

https://codereview.chromium.org/2953483003/diff/40001/components/cronet/host_...
components/cronet/host_cache_persistence_manager_unittest.cc:19: const char
kPrefName[] = "net.test";
On 2017/06/23 17:39:31, pauljensen wrote:
> On 2017/06/23 16:02:06, mgersh wrote:
> > On 2017/06/23 14:10:31, pauljensen wrote:
> > > how about moving this into HostCachePersistenceManagerTest as a static?
> > 
> > The compiler wasn't happy with that, and all the other tests I've found use
an
> > anonymous namespace like I did here.
> 
> What was the compiler error?

(Changed it.)

[pauljensen, 2017-06-23 18:23:52]

lgtm

[xunjieli, 2017-06-23 18:30:25]

Description was changed from

==========
Add HostCachePersistenceManager for Cronet

HostCachePersistenceManager handles the interaction between HostCache
and prefs for persistence. It batches writes to avoid writing to prefs
too often. This implementation is Cronet-specific because it uses the
same sequence for interactions with network and prefs code.

BUG=605149
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Add HostCachePersistenceManager for Cronet

HostCachePersistenceManager handles the interaction between HostCache
and prefs for persistence. It batches writes to avoid writing to prefs
too often. This implementation is Cronet-specific because it uses the
same sequence for interactions with network and prefs code.

BUG=605149
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[xunjieli, 2017-06-23 18:30:25]

xunjieli@chromium.org changed reviewers:
- xunjieli@chromium.org

[xunjieli, 2017-06-23 18:34:17]

Removing myself. Sorry, I am a bit overloaded with other things atm. Paul has
more experience with DNS in general. His stamp is enough :)

[mgersh, 2017-06-23 18:36:07]

Thanks for the review!

[mgersh, 2017-06-23 18:36:10]

The CQ bit was checked by mgersh@chromium.org

[commit-bot: I haz the power, 2017-06-23 18:36:27]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-23 20:17:08]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498242970908890,
"parent_rev": "25d708b7e24d70becb747d3863a959c239159117", "commit_rev":
"983dbf87d4b115593d648e58f86b336401945647"}

[commit-bot: I haz the power, 2017-06-23 20:17:16]

Description was changed from

==========
Add HostCachePersistenceManager for Cronet

HostCachePersistenceManager handles the interaction between HostCache
and prefs for persistence. It batches writes to avoid writing to prefs
too often. This implementation is Cronet-specific because it uses the
same sequence for interactions with network and prefs code.

BUG=605149
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Add HostCachePersistenceManager for Cronet

HostCachePersistenceManager handles the interaction between HostCache
and prefs for persistence. It batches writes to avoid writing to prefs
too often. This implementation is Cronet-specific because it uses the
same sequence for interactions with network and prefs code.

BUG=605149
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2953483003
Cr-Commit-Position: refs/heads/master@{#482001}
Committed:
https://chromium.googlesource.com/chromium/src/+/983dbf87d4b115593d648e58f86b...
==========

[commit-bot: I haz the power, 2017-06-23 20:17:17]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/983dbf87d4b115593d648e58f86b...