Add prefetch support to AffiliationBackend.

Add prefetch support to AffiliationBackend.

BUG=437865
Committed: https://crrev.com/bce04057f25553eb353f167fa0f578947c4cf397
Cr-Commit-Position: refs/heads/master@{#319941}

[engedy, 2015-02-24 11:56:11]

Patchset #2 (id:20001) has been deleted

[engedy, 2015-02-24 11:59:14]

engedy@chromium.org changed reviewers:
+ mkwst@chromium.org

[engedy, 2015-02-24 11:59:14]

Mike, could you please take a look.

I would also welcome comments regarding how to make the tests more
understandable for humans.

[Mike West, 2015-02-24 13:27:01]

https://codereview.chromium.org/947563002/diff/60001/components/password_mana...
File components/password_manager/core/browser/affiliation_backend_unittest.cc
(right):

https://codereview.chromium.org/947563002/diff/60001/components/password_mana...
components/password_manager/core/browser/affiliation_backend_unittest.cc:120:
GetAffiliations(mock_consumer(), facet_uri, true /* cached_only */);
Nit: In a future CL, it might be worthwhile to replace this bool with an enum
throughout.

https://codereview.chromium.org/947563002/diff/60001/components/password_mana...
components/password_manager/core/browser/affiliation_backend_unittest.cc:126:
backend_->Prefetch(facet_uri, keep_fresh_until);
You can just call the `Prefetch()` helper, right?

https://codereview.chromium.org/947563002/diff/60001/components/password_mana...
components/password_manager/core/browser/affiliation_backend_unittest.cc:221:
backend_task_runner()->Now());
Is "Now" guaranteed to be in the past?

https://codereview.chromium.org/947563002/diff/60001/components/password_mana...
components/password_manager/core/browser/affiliation_backend_unittest.cc:224: //
backend_task_runner()->RunUntilIdle();
Nit: Don't leave commented-out code in the CL.

[Mike West, 2015-02-24 13:27:23]

(Sorry, hit send accidentally... not done reviewing at all)

[Mike West, 2015-02-25 10:33:06]

The code, generally, looks great.

I don't think the tests are unreadable, but I think they're significantly
complicated by the date math you're doing to move through the various states of
the cache. I suspect you could improve the readability by adding a public
accessor for the cache's state (fresh, soft, hard), and adding assertions to the
tests to show what state you expect the cache to be in after moving X hours
forward. It would also be lovely to stop using magic numbers, and to instead
have named TimeDeltas to make your intent clear.

Other niggley bits in comments below:

https://codereview.chromium.org/947563002/diff/60001/components/password_mana...
File components/password_manager/core/browser/affiliation_backend_unittest.cc
(right):

https://codereview.chromium.org/947563002/diff/60001/components/password_mana...
components/password_manager/core/browser/affiliation_backend_unittest.cc:253:
EXPECT_GE(1u, backend()->facet_manager_count());
Why is this 1? Shouldn't you have results for alpha, beta, and gamma at this
point?

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
File components/password_manager/core/browser/affiliation_backend.cc (right):

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:148: time -
clock_->Now());
Are prefetches meant to be stored across restarts? If so, what's responsible for
doing so? If not, it should be documented somewhere (maybe it is?).

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
File components/password_manager/core/browser/affiliation_backend_unittest.cc
(right):

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend_unittest.cc:875:
AdvanceTime(base::TimeDelta::FromMicroseconds(1));
See the magic number discussion below, but there's a lot of date math in this
test (and other similar tests that makes them complex to follow.

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend_unittest.cc:907:
AdvanceTime(base::TimeDelta::FromHours(21) -
Throughout: I don't like magic numbers. Why is 1 microsecond less than 21 hours
relevant? Why is 9 hours relevant? Why is 24 hours in the future relevant? It
would be wonderful to extract these into named deltas.

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
File components/password_manager/core/browser/facet_manager.cc (right):

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/facet_manager.cc:161: auto
iter_first_non_expired = keep_fresh_untils_.upper_bound(clock_->Now());
Shouldn't you clear out the list of expired "keep-untils" before triggering the
network request, or asking for required fetch times?

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/facet_manager.cc:183: return
!last_update_time_.is_null() ? last_update_time_ + expiry
Do you need to distinguish between the first fetch and subsequent fetches? If
not, you could simplify this by always returning `last_update_time_ + expiry`,
and relying on the fact that `0 + a day` === THE DISTANT PAST.

Ditto for the soft expire.

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/facet_manager.cc:192: return
!keep_fresh_untils_.empty() ? *keep_fresh_untils_.rbegin()
Nit: My brain refuses to believe that 'untils' is spelled correctly. How about
`keep_fresh_requests_`?

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/facet_manager.cc:200: if (clock_->Now()
< GetMaximumKeepFreshUntil() &&
`<=`?

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/facet_manager.cc:201:
GetCacheHardExpiryTime() < GetMaximumKeepFreshUntil()) {
`<=`?

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
File components/password_manager/core/browser/facet_manager.h (right):

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/facet_manager.h:68: // The data is
considered stale already in the time quantum.
What is a "time quantum"?

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/facet_manager.h:76: bool
IsCachedDataFresh() const;
It might be worthwhile to add a public accessor for the state of the cache, and
unittests specifically around that

[engedy, 2015-03-10 11:01:01]

I have reworked the unit tests completely.

I have uploaded the whole to this CL, but I will split this up so that it is not
a 2k line CL.

https://codereview.chromium.org/947563002/diff/60001/components/password_mana...
File components/password_manager/core/browser/affiliation_backend_unittest.cc
(right):

https://codereview.chromium.org/947563002/diff/60001/components/password_mana...
components/password_manager/core/browser/affiliation_backend_unittest.cc:120:
GetAffiliations(mock_consumer(), facet_uri, true /* cached_only */);
On 2015/02/24 13:27:01, Mike West (OOO) wrote:
> Nit: In a future CL, it might be worthwhile to replace this bool with an enum
> throughout.

Acknowledged.

https://codereview.chromium.org/947563002/diff/60001/components/password_mana...
components/password_manager/core/browser/affiliation_backend_unittest.cc:126:
backend_->Prefetch(facet_uri, keep_fresh_until);
On 2015/02/24 13:27:01, Mike West (OOO) wrote:
> You can just call the `Prefetch()` helper, right?

Done.

https://codereview.chromium.org/947563002/diff/60001/components/password_mana...
components/password_manager/core/browser/affiliation_backend_unittest.cc:221:
backend_task_runner()->Now());
On 2015/02/24 13:27:01, Mike West (OOO) wrote:
> Is "Now" guaranteed to be in the past?

Yes, I have added a comment to clarify this.

https://codereview.chromium.org/947563002/diff/60001/components/password_mana...
components/password_manager/core/browser/affiliation_backend_unittest.cc:224: //
backend_task_runner()->RunUntilIdle();
On 2015/02/24 13:27:01, Mike West (OOO) wrote:
> Nit: Don't leave commented-out code in the CL.

Wops, sorry. Removed.

https://codereview.chromium.org/947563002/diff/60001/components/password_mana...
components/password_manager/core/browser/affiliation_backend_unittest.cc:253:
EXPECT_GE(1u, backend()->facet_manager_count());
On 2015/02/25 10:33:05, Mike West (OOO) wrote:
> Why is this 1? Shouldn't you have results for alpha, beta, and gamma at this
> point?

I have clarified in a comment.

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
File components/password_manager/core/browser/affiliation_backend.cc (right):

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:148: time -
clock_->Now());
On 2015/02/25 10:33:05, Mike West (OOO) wrote:
> Are prefetches meant to be stored across restarts? If so, what's responsible
for
> doing so? If not, it should be documented somewhere (maybe it is?).

Indeed, they will not survive a restart. This is documented in
affiliation_service.h.

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
File components/password_manager/core/browser/affiliation_backend_unittest.cc
(right):

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend_unittest.cc:875:
AdvanceTime(base::TimeDelta::FromMicroseconds(1));
On 2015/02/25 10:33:06, Mike West (OOO) wrote:
> See the magic number discussion below, but there's a lot of date math in this
> test (and other similar tests that makes them complex to follow.

I have completely reworked the unittests. I am no longer using magic constants,
and the vast majority of the temporal logic testing have been moved to
facet_manager_unittest.cc.

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend_unittest.cc:907:
AdvanceTime(base::TimeDelta::FromHours(21) -
On 2015/02/25 10:33:06, Mike West (OOO) wrote:
> Throughout: I don't like magic numbers. Why is 1 microsecond less than 21
hours
> relevant? Why is 9 hours relevant? Why is 24 hours in the future relevant? It
> would be wonderful to extract these into named deltas.

Done.

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
File components/password_manager/core/browser/facet_manager.cc (right):

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/facet_manager.cc:161: auto
iter_first_non_expired = keep_fresh_untils_.upper_bound(clock_->Now());
On 2015/02/25 10:33:06, Mike West wrote:
> Shouldn't you clear out the list of expired "keep-untils" before triggering
the
> network request, or asking for required fetch times?

Normally they should never expire at those time, but only when this callback is
called. In any case, the maximum of the set is always calculated and compared to
the current time, so there is no harm in having out-of-date values.

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/facet_manager.cc:183: return
!last_update_time_.is_null() ? last_update_time_ + expiry
On 2015/02/25 10:33:06, Mike West (OOO) wrote:
> Do you need to distinguish between the first fetch and subsequent fetches? If
> not, you could simplify this by always returning `last_update_time_ + expiry`,
> and relying on the fact that `0 + a day` === THE DISTANT PAST.
> 
> Ditto for the soft expire.

Done.

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/facet_manager.cc:192: return
!keep_fresh_untils_.empty() ? *keep_fresh_untils_.rbegin()
On 2015/02/25 10:33:06, Mike West (OOO) wrote:
> Nit: My brain refuses to believe that 'untils' is spelled correctly. How about
> `keep_fresh_requests_`?

I have renamed to |keep_fresh_until_thresholds_|.

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/facet_manager.cc:200: if (clock_->Now()
< GetMaximumKeepFreshUntil() &&
On 2015/02/25 10:33:06, Mike West wrote:
> `<=`?

I have clarified in the comment in affiliation_service.h that prefetch intervals
are open from the right.

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/facet_manager.cc:201:
GetCacheHardExpiryTime() < GetMaximumKeepFreshUntil()) {
On 2015/02/25 10:33:06, Mike West wrote:
> `<=`?

Same here.

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
File components/password_manager/core/browser/facet_manager.h (right):

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/facet_manager.h:68: // The data is
considered stale already in the time quantum.
On 2015/02/25 10:33:06, Mike West (OOO) wrote:
> What is a "time quantum"?

That was a typo. Rephrased entirely.

https://codereview.chromium.org/947563002/diff/100001/components/password_man...
components/password_manager/core/browser/facet_manager.h:76: bool
IsCachedDataFresh() const;
On 2015/02/25 10:33:06, Mike West (OOO) wrote:
> It might be worthwhile to add a public accessor for the state of the cache,
and
> unittests specifically around that

Done.

[Mike West, 2015-03-10 11:10:28]

At first glance, the diff between patchset 5 and 6 looks totally reasonable, and
the ~2k lines of unittests look better than they did in patchset 5. I'll read
the tests in more detail after lunch. Thanks for taking another pass!

[Mike West, 2015-03-10 11:58:29]

Got this far before meeting. More in 30m.

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
File components/password_manager/core/browser/facet_manager_unittest.cc (right):

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
components/password_manager/core/browser/facet_manager_unittest.cc:47: } else if
(accuracy_ == NotificationAccuracy::TOO_EARLY) {
Nit: you can drop {} from these statements, as they're all one-liners.

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
components/password_manager/core/browser/facet_manager_unittest.cc:48: delay =
std::min(delay, delay / 2 + base::TimeDelta::FromSeconds(1));
Why half of delay + a second? (I'm not saying it's wrong, I'm saying I don't
understand why this specific point. :) )

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
components/password_manager/core/browser/facet_manager_unittest.cc:415:
GetAffiliations(true /* cached_only */);
In Blink, I'd suggest that you change this to a readable enum rather than adding
comments at the callsite. Too bad this isn't Blink. :(

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
components/password_manager/core/browser/facet_manager_unittest.cc:427:
EXPECT_TRUE(facet_manager()->IsCachedDataStale());
Nit: Do you need both `IsCachedDataFresh` and `IsCachedDataStale`?

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
components/password_manager/core/browser/facet_manager_unittest.cc:521: //     
t=0                        S       H               2*S    S+H
Nit: It's not really 2*S, is it? It's H + S?

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
components/password_manager/core/browser/facet_manager_unittest.cc:544:
{GetCacheSoftExpiryPeriod() + GetCacheSoftExpiryPeriod(), 2},
This only works because 2S happens to be in between H and 2H. Go far enough out
and that will no longer hold.

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
components/password_manager/core/browser/facet_manager_unittest.cc:551: 2 *
GetCacheSoftExpiryPeriod()};
I don't understand why this passes.

[Mike West, 2015-03-10 13:32:36]

You're a super methodical coder, Balzas. Thank you. :) The code changes
certainly LGTM, this is a solid improvement over the previous patchsets.

The unit tests look great (I very much prefer the array-of-test-cases approach
you're taking here, and the date math is SIGNIFICANTLY easier to follow), but
I'd be lying if I said I read every line. After our conversation, I'm satisfied
that the diagrams are saying things that make sense, and I'm convinced that the
tests you've written are in fact testing the things you've said you're testing.

I think the right thing to do is to land this to clear the way for the other
patch and get a first pass of the feature in place.

[engedy, 2015-03-10 15:05:28]

Patchset #8 (id:160001) has been deleted

[engedy, 2015-03-10 15:07:25]

Patchset #8 (id:180001) has been deleted

[engedy, 2015-03-10 15:09:47]

Thanks for reviewing! I have addressed your comments as per offline discussion,
and I think I could fix the labeling on the diagrams pretty well too.

I will wait until https://codereview.chromium.org/994903002/ lands, then I will
land this too.

To summarize, things to address in the future:
  * Fix naming of 'prefetch'.
  * Introduce enum instead of |cached_only|.
  * Consider dropping the requirement that 2*S > H.

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
File components/password_manager/core/browser/facet_manager_unittest.cc (right):

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
components/password_manager/core/browser/facet_manager_unittest.cc:47: } else if
(accuracy_ == NotificationAccuracy::TOO_EARLY) {
On 2015/03/10 11:58:29, Mike West (OOO) wrote:
> Nit: you can drop {} from these statements, as they're all one-liners.

I have added a comment now, so N/A.

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
components/password_manager/core/browser/facet_manager_unittest.cc:48: delay =
std::min(delay, delay / 2 + base::TimeDelta::FromSeconds(1));
On 2015/03/10 11:58:29, Mike West (OOO) wrote:
> Why half of delay + a second? (I'm not saying it's wrong, I'm saying I don't
> understand why this specific point. :) )

Added comment to clarify.

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
components/password_manager/core/browser/facet_manager_unittest.cc:415:
GetAffiliations(true /* cached_only */);
On 2015/03/10 11:58:29, Mike West (OOO) wrote:
> In Blink, I'd suggest that you change this to a readable enum rather than
adding
> comments at the callsite. Too bad this isn't Blink. :(

Yes, I have already made a note of that, and I will address it in a follow-up
CL.

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
components/password_manager/core/browser/facet_manager_unittest.cc:427:
EXPECT_TRUE(facet_manager()->IsCachedDataStale());
On 2015/03/10 11:58:29, Mike West (OOO) wrote:
> Nit: Do you need both `IsCachedDataFresh` and `IsCachedDataStale`?

Not really. Done.

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
components/password_manager/core/browser/facet_manager_unittest.cc:521: //     
t=0                        S       H               2*S    S+H
On 2015/03/10 11:58:29, Mike West (OOO) wrote:
> Nit: It's not really 2*S, is it? It's H + S?

Yes, it is H+S.

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
components/password_manager/core/browser/facet_manager_unittest.cc:544:
{GetCacheSoftExpiryPeriod() + GetCacheSoftExpiryPeriod(), 2},
On 2015/03/10 11:58:29, Mike West (OOO) wrote:
> This only works because 2S happens to be in between H and 2H. Go far enough
out
> and that will no longer hold.

Yes, that is currently a design requirement, I have added static_asserts and
additional comments to clarify this.

I *think* that the production code could handle 2S < H as well, but not entirely
certain.

https://codereview.chromium.org/947563002/diff/120001/components/password_man...
components/password_manager/core/browser/facet_manager_unittest.cc:551: 2 *
GetCacheSoftExpiryPeriod()};
On 2015/03/10 11:58:29, Mike West (OOO) wrote:
> I don't understand why this passes.

Not entirely sure what you mean. Could you please elaborate?

[engedy, 2015-03-10 17:52:57]

The CQ bit was checked by engedy@chromium.org

[engedy, 2015-03-10 17:52:58]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/947563002/#ps200001
(title: "Remove 'stale' accessor, cleaned up timelines for tests, extended/fixed
some tests.")

[commit-bot: I haz the power, 2015-03-10 17:53:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/947563002/200001

[commit-bot: I haz the power, 2015-03-10 19:24:52]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2015-03-10 19:25:47]

Patchset 8 (id:??) landed as
https://crrev.com/bce04057f25553eb353f167fa0f578947c4cf397
Cr-Commit-Position: refs/heads/master@{#319941}