Add PersistenceDelegate to HostCache

Add PersistenceDelegate to HostCache

PersistenceDelegate is a new interface for persisting the contents of
the HostCache. This commit includes the interface itself, the logic in
HostCache for interacting with it, and a mock implementation of the
interface for testing. It does not include support for immediate data
removal since that won't be needed for the currently planned use case.

BUG=605149
Review-Url: https://codereview.chromium.org/2943143002
Cr-Commit-Position: refs/heads/master@{#481015}
Committed: https://chromium.googlesource.com/chromium/src/+/7c28e7988fef9bb3e03027226bd199736d99abc3

[mgersh, 2017-06-16 17:01:45]

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

[mgersh, 2017-06-16 17:01:45]

I added an explanation of where I'm going with this at the bottom of the design
doc
(https://docs.google.com/document/d/16Ru9m0T0NtzIM5rr57mbk7nzrpDrtgVTBZdfeFRDz...).

[Julia Tuttle, 2017-06-16 17:04:55]

Couple of suggestions.

https://codereview.chromium.org/2943143002/diff/1/net/dns/host_cache.cc
File net/dns/host_cache.cc (right):

https://codereview.chromium.org/2943143002/diff/1/net/dns/host_cache.cc#newco...
net/dns/host_cache.cc:254: if (delegate_)
Maybe actually check if the cache is already empty? It wouldn't surprise me if
we get multiple clear calls in a row around network changes.

https://codereview.chromium.org/2943143002/diff/1/net/dns/host_cache.cc#newco...
net/dns/host_cache.cc:279: if (delegate_)
Ditto; see if we actually remove any entries.

[mgersh, 2017-06-16 17:32:50]

https://codereview.chromium.org/2943143002/diff/1/net/dns/host_cache.cc
File net/dns/host_cache.cc (right):

https://codereview.chromium.org/2943143002/diff/1/net/dns/host_cache.cc#newco...
net/dns/host_cache.cc:254: if (delegate_)
On 2017/06/16 17:04:54, Julia Tuttle wrote:
> Maybe actually check if the cache is already empty? It wouldn't surprise me if
> we get multiple clear calls in a row around network changes.

Done.

https://codereview.chromium.org/2943143002/diff/1/net/dns/host_cache.cc#newco...
net/dns/host_cache.cc:279: if (delegate_)
On 2017/06/16 17:04:54, Julia Tuttle wrote:
> Ditto; see if we actually remove any entries.

Done.

[mgersh, 2017-06-20 15:45:51]

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

[mgersh, 2017-06-20 15:45:52]

Looks like Julia isn't here today. Misha, can you look at this?

[mef, 2017-06-20 16:18:13]

https://codereview.chromium.org/2943143002/diff/20001/net/dns/host_cache.cc
File net/dns/host_cache.cc (right):

https://codereview.chromium.org/2943143002/diff/20001/net/dns/host_cache.cc#n...
net/dns/host_cache.cc:213: bool result_changed;
nit: Maybe initialize to false?
nit: Maybe name it |address_list_changed|?

https://codereview.chromium.org/2943143002/diff/20001/net/dns/host_cache.cc#n...
net/dns/host_cache.cc:224: it->second.error() != entry.error() || delta !=
DELTA_IDENTICAL;
Would we want to schedule write if new entry.error() is not OK?

https://codereview.chromium.org/2943143002/diff/20001/net/dns/host_cache_unit...
File net/dns/host_cache_unittest.cc (right):

https://codereview.chromium.org/2943143002/diff/20001/net/dns/host_cache_unit...
net/dns/host_cache_unittest.cc:37: void ScheduleWrite() override {
num_changes_++; }
nit: I think ++num_changes_ is preferred.

https://codereview.chromium.org/2943143002/diff/20001/net/dns/host_cache_unit...
net/dns/host_cache_unittest.cc:39: int num_changes() { return num_changes_; }
nit: const

[mgersh, 2017-06-20 17:16:30]

https://codereview.chromium.org/2943143002/diff/20001/net/dns/host_cache.cc
File net/dns/host_cache.cc (right):

https://codereview.chromium.org/2943143002/diff/20001/net/dns/host_cache.cc#n...
net/dns/host_cache.cc:213: bool result_changed;
On 2017/06/20 16:18:12, mef wrote:
> nit: Maybe initialize to false?
Done.
> nit: Maybe name it |address_list_changed|?
It's not necessarily the address list that changed though.

https://codereview.chromium.org/2943143002/diff/20001/net/dns/host_cache.cc#n...
net/dns/host_cache.cc:224: it->second.error() != entry.error() || delta !=
DELTA_IDENTICAL;
On 2017/06/20 16:18:12, mef wrote:
> Would we want to schedule write if new entry.error() is not OK?

I could go either way on this, but I guess the write in that case isn't very
useful. I'll add that condition.

https://codereview.chromium.org/2943143002/diff/20001/net/dns/host_cache_unit...
File net/dns/host_cache_unittest.cc (right):

https://codereview.chromium.org/2943143002/diff/20001/net/dns/host_cache_unit...
net/dns/host_cache_unittest.cc:37: void ScheduleWrite() override {
num_changes_++; }
On 2017/06/20 16:18:12, mef wrote:
> nit: I think ++num_changes_ is preferred.

The rule is technically only for iterators, but sure.

https://codereview.chromium.org/2943143002/diff/20001/net/dns/host_cache_unit...
net/dns/host_cache_unittest.cc:39: int num_changes() { return num_changes_; }
On 2017/06/20 16:18:12, mef wrote:
> nit: const

Done.

[mef, 2017-06-20 21:36:39]

lgtm

[mgersh, 2017-06-20 21:37:04]

The CQ bit was checked by mgersh@chromium.org

[commit-bot: I haz the power, 2017-06-20 21:37:35]

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-20 23:12:32]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497994624620640,
"parent_rev": "b90a97ab99ad72ebf16aae5ee56c82b5017544ac", "commit_rev":
"ac66835af3040ebd373e617af0cc5ad1a7b22990"}

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

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497994624620640,
"parent_rev": "8b9d403eea3b3e0a6a79a2aa533b9fdb6658965e", "commit_rev":
"797c8941684924097c9ca0d2ea98ae03e2a6fed3"}

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

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497994624620640,
"parent_rev": "edf8e43c615fb02a9350a01ee4cfd0a5144fc2c2", "commit_rev":
"7c28e7988fef9bb3e03027226bd199736d99abc3"}

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

Description was changed from

==========
Add PersistenceDelegate to HostCache

PersistenceDelegate is a new interface for persisting the contents of
the HostCache. This commit includes the interface itself, the logic in
HostCache for interacting with it, and a mock implementation of the
interface for testing. It does not include support for immediate data
removal since that won't be needed for the currently planned use case.

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

to

==========
Add PersistenceDelegate to HostCache

PersistenceDelegate is a new interface for persisting the contents of
the HostCache. This commit includes the interface itself, the logic in
HostCache for interacting with it, and a mock implementation of the
interface for testing. It does not include support for immediate data
removal since that won't be needed for the currently planned use case.

BUG=605149

Review-Url: https://codereview.chromium.org/2943143002
Cr-Commit-Position: refs/heads/master@{#481015}
Committed:
https://chromium.googlesource.com/chromium/src/+/7c28e7988fef9bb3e03027226bd1...
==========

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

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7c28e7988fef9bb3e03027226bd1...