AffiliationBackend: Implement the better part of on-demand fetching.

AffiliationBackend: Implement the better part of on-demand fetching.

This CL implements AffiliationBackend::GetAffiliation(), including support for caching, but without support for some error scenarios.

The missing parts, viz. Prefetch() functionality and error handling will be added later to reduce CL size. Given that the code is not yet exercised outside of tests, this should not create any problems.

BUG=437865
TBR=gcasto@chromium.org

Committed: https://crrev.com/45920ca9cb490d8f1969a3d9bc543a457f94ec6b
Cr-Commit-Position: refs/heads/master@{#314320}

[engedy, 2015-01-30 16:29:42]

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

[engedy, 2015-01-30 19:35:30]

@Garrett, please take a look.

I have tried to reduce the CL size as much as I could, there are several parts
still marked NOTIMPLEMENTED(), these will follow in a subsequent CL. Still, the
CL is pretty large, sorry about that.

If you prefer, I can factor out the test support classes in a separate CL, but
they are only about 200 lines, so do not make much of a difference.

[engedy, 2015-01-30 22:42:40]

Oh, the leak. Dunno what I was expecting there... I will fix that.

[engedy, 2015-02-02 09:34:27]

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

[engedy, 2015-02-02 09:34:28]

@Mike, given the size of the CL, Garrett suggested that a second pair of eyes
taking a look may be useful. As discussed, could you PTAL?

[Mike West, 2015-02-02 13:22:28]

I'll look at the rest in a moment, but first I'll note that the ASAN bot failure
looks relevant.

[engedy, 2015-02-02 13:37:09]

On 2015/02/02 13:22:28, Mike West wrote:
> I'll look at the rest in a moment, but first I'll note that the ASAN bot
failure
> looks relevant.

Fixed.

[Mike West, 2015-02-02 15:19:45]

Overall, this looks like the right structure for what you want to do. I want to
take a closer look at the tests, but I don't think I'll be able to do so until
the morning. :/

Here's an initial round of mostly benign comments.

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

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:23: const
int64_t kCacheHardExpiryDurationInSeconds = 24 * 3600;
Nit: "24 * 60 * 60" seems slightly more common in `components/` than "24 *
3600". But whatever. :)

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:40: }
Nit: Any reason not to inline these? FacetManager stuff below as well, now that
I see it...

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:185: void
AffiliationBackend::Initialize(const base::FilePath& db_path) {
Should initialize be called on the same thread as the network requests? It seems
like it, so it might be a good idea to add some checks.

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:201:
FacetManager* facet_manager = facet_managers_.get(facet_uri);
`.add()` can fail, can't it? OOM, etc. Perhaps `DCHECK(facet_manager)`?

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:226:
DCHECK(!fetcher_);
DCHECK that you have more than 0 managers? I guess that's covered by the empty
check on #233.

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:246: if
(cache_->GetAffiliationsForFacet(facet_uri, &affiliation))
You could spell this `ReadAffiliationsFromDatabase`.

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:250:
base::TimeDelta::FromSeconds(kCacheHardExpiryDurationInSeconds);
Nit: I think a ternary if would be prettier here. But whatever. :)

Nit2: `base::Time::UnixEpoch()` seems safer to rely upon.

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:279:
NOTIMPLEMENTED();
Can you add a comment here? It's not clear to me what this is supposed to be
checking.

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:282: if
(!facet_managers_.contains(facet_uri))
It feels like there can be a race here if requests come in during a fetch.

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:291: // A
subsequent fetch is needed if additional GetAffiliations() request came
Nit: s/request came/requests come/ or s/if additional/if an additional/

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:302:
NOTIMPLEMENTED();
Please add TODO comments pointing to a bug for this and other NOTIMPLEMENTED
calls. At least the ones you added in this CL.

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

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.h:39: // This class
is not thread-safe, but it is fine to construct it one on thread
Nit: s/one on/on one/g

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.h:54: void
Initialize(const base::FilePath& db_path);
It worries me that we're passing in a path here. Is this just for testing
purposes, or do we need the ability to inject a path at runtime?

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.h:109:
base::ScopedPtrHashMap<FacetURI, FacetManager> facet_managers_;
Do we have one manager per URL? That is, will Amazon initially create one
manager per TLD? How do they merge into one?

[engedy, 2015-02-02 15:33:15]

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

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:40: }
On 2015/02/02 15:19:44, Mike West wrote:
> Nit: Any reason not to inline these? FacetManager stuff below as well, now
that
> I see it...

Vaclav prefers we do not inline in production code, mostly for readability. I'll
let him explain his motivations in more detail. :-)

[vabr (Chromium), 2015-02-02 15:37:36]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2015-02-02 15:37:37]

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

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:40: }
On 2015/02/02 15:19:44, Mike West wrote:
> Nit: Any reason not to inline these? FacetManager stuff below as well, now
that
> I see it...

http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...
sounds to be a good reason not to inline this.

If I understand the style correctly, as soon as the constructor/destructor is
not trivial, it should not be inlined. It is preferred to call it to copying
it's body in the code over and over. Here the constructor contains the
construction of the data members. If they were just POD, inlining sounds fine.
But they are not.

[Mike West, 2015-02-02 15:40:35]

On 2015/02/02 15:37:37, vabr (Chromium) wrote:
>
https://codereview.chromium.org/868253005/diff/100001/components/password_man...
> File components/password_manager/core/browser/affiliation_backend.cc (right):
> 
>
https://codereview.chromium.org/868253005/diff/100001/components/password_man...
> components/password_manager/core/browser/affiliation_backend.cc:40: }
> On 2015/02/02 15:19:44, Mike West wrote:
> > Nit: Any reason not to inline these? FacetManager stuff below as well, now
> that
> > I see it...
> 
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...
> sounds to be a good reason not to inline this.
> 
> If I understand the style correctly, as soon as the constructor/destructor is
> not trivial, it should not be inlined. It is preferred to call it to copying
> it's body in the code over and over. Here the constructor contains the
> construction of the data members. If they were just POD, inlining sounds fine.
> But they are not.

Hrm. This Vaclav guy approved
https://codereview.chromium.org/886793002/diff/80001/components/password_mana...
(twice!) which is all inlined, all the time.

[vabr (Chromium), 2015-02-02 16:45:55]

On 2015/02/02 15:40:35, Mike West wrote:
> On 2015/02/02 15:37:37, vabr (Chromium) wrote:
> >
>
https://codereview.chromium.org/868253005/diff/100001/components/password_man...
> > File components/password_manager/core/browser/affiliation_backend.cc
(right):
> > 
> >
>
https://codereview.chromium.org/868253005/diff/100001/components/password_man...
> > components/password_manager/core/browser/affiliation_backend.cc:40: }
> > On 2015/02/02 15:19:44, Mike West wrote:
> > > Nit: Any reason not to inline these? FacetManager stuff below as well, now
> > that
> > > I see it...
> > 
> >
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...
> > sounds to be a good reason not to inline this.
> > 
> > If I understand the style correctly, as soon as the constructor/destructor
is
> > not trivial, it should not be inlined. It is preferred to call it to copying
> > it's body in the code over and over. Here the constructor contains the
> > construction of the data members. If they were just POD, inlining sounds
fine.
> > But they are not.
> 
> Hrm. This Vaclav guy approved
>
https://codereview.chromium.org/886793002/diff/80001/components/password_mana...
> (twice!) which is all inlined, all the time.

Good catch, Mike. I should have had a better look. However, rather than using it
as a precedent here, I believe this is rather a TODO for
PendingRequestParameters, for the reasons I described above.

[engedy, 2015-02-02 17:00:12]

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

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:23: const
int64_t kCacheHardExpiryDurationInSeconds = 24 * 3600;
On 2015/02/02 15:19:44, Mike West wrote:
> Nit: "24 * 60 * 60" seems slightly more common in `components/` than "24 *
> 3600". But whatever. :)

Done. I have replaced this with kCacheHardExpiryDurationInHours.

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:185: void
AffiliationBackend::Initialize(const base::FilePath& db_path) {
On 2015/02/02 15:19:44, Mike West wrote:
> Should initialize be called on the same thread as the network requests? It
seems
> like it, so it might be a good idea to add some checks.

As discussed, I have added a thread checker in a scoped_ptr.

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:201:
FacetManager* facet_manager = facet_managers_.get(facet_uri);
On 2015/02/02 15:19:45, Mike West wrote:
> `.add()` can fail, can't it? OOM, etc. Perhaps `DCHECK(facet_manager)`?

Do I understand correctly that you prefer this because in case of a NULL
pointer, we will crash here with an easy to understand stack trace, as opposed
to somewhere deeper in the call stack with a slightly more obscure stack trace?

Done.

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:226:
DCHECK(!fetcher_);
On 2015/02/02 15:19:44, Mike West wrote:
> DCHECK that you have more than 0 managers? I guess that's covered by the empty
> check on #233.

Yes, I'd prefer not checking that separately.

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:246: if
(cache_->GetAffiliationsForFacet(facet_uri, &affiliation))
On 2015/02/02 15:19:44, Mike West wrote:
> You could spell this `ReadAffiliationsFromDatabase`.

Done.

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:250:
base::TimeDelta::FromSeconds(kCacheHardExpiryDurationInSeconds);
On 2015/02/02 15:19:45, Mike West wrote:
> Nit: I think a ternary if would be prettier here. But whatever. :)

Done.
 
> Nit2: `base::Time::UnixEpoch()` seems safer to rely upon.

In tests, we mock out time and start form 0 for readability. How about 0 - 24
hrs?

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:279:
NOTIMPLEMENTED();
On 2015/02/02 15:19:45, Mike West wrote:
> Can you add a comment here? It's not clear to me what this is supposed to be
> checking.

Done.

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:282: if
(!facet_managers_.contains(facet_uri))
On 2015/02/02 15:19:44, Mike West wrote:
> It feels like there can be a race here if requests come in during a fetch.

Well, if the response happens to contain the data needed by the second request
already, that's just even better. There is even a test for this.

I don't see other contingencies here. I might be missing something, though.
Could you please elaborate?

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:291: // A
subsequent fetch is needed if additional GetAffiliations() request came
On 2015/02/02 15:19:45, Mike West wrote:
> Nit: s/request came/requests come/ or s/if additional/if an additional/


Done.

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:302:
NOTIMPLEMENTED();
On 2015/02/02 15:19:44, Mike West wrote:
> Please add TODO comments pointing to a bug for this and other NOTIMPLEMENTED
> calls. At least the ones you added in this CL.

Done. I have mentioned the very same bug that this CL references. Let me know if
you would prefer otherwise.

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

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.h:39: // This class
is not thread-safe, but it is fine to construct it one on thread
On 2015/02/02 15:19:45, Mike West wrote:
> Nit: s/one on/on one/g

Done.

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.h:54: void
Initialize(const base::FilePath& db_path);
On 2015/02/02 15:19:45, Mike West wrote:
> It worries me that we're passing in a path here. Is this just for testing
> purposes, or do we need the ability to inject a path at runtime?

Hmm, both, actually. The path is browser-layer specific, and is therefore
injected by the factories in src/chrome/browser/password_manager. Please let me
know if you see a better solution.

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.h:109:
base::ScopedPtrHashMap<FacetURI, FacetManager> facet_managers_;
On 2015/02/02 15:19:45, Mike West wrote:
> Do we have one manager per URL? That is, will Amazon initially create one
> manager per TLD? How do they merge into one?

Well, one manager for each origin (facet) that needs a password lookup. They
will never be merged.

Instead, if they are affiliated, they will happen to read the same data from the
DB, serve pending requests, and then probably get destroyed.

[Garrett Casto, 2015-02-03 08:20:38]

Still haven't been able to completely digest this CL, but here are a few
preliminary comments.

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

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:40: }
On 2015/02/02 15:37:37, vabr (Chromium) wrote:
> On 2015/02/02 15:19:44, Mike West wrote:
> > Nit: Any reason not to inline these? FacetManager stuff below as well, now
> that
> > I see it...
> 
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...
> sounds to be a good reason not to inline this.
> 
> If I understand the style correctly, as soon as the constructor/destructor is
> not trivial, it should not be inlined. It is preferred to call it to copying
> it's body in the code over and over. Here the constructor contains the
> construction of the data members. If they were just POD, inlining sounds fine.
> But they are not.

Further down on that page it talks about how in the case of classes in .cc files
that you can do whichever is most readable. I would just not declare a
constructor or destructor here since that's the equivalent of inlining the
current empty body constructor and destructor and it saved a few extra lines.

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

https://codereview.chromium.org/868253005/diff/140001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:24: const
int64_t kCacheHardExpiryDurationInHours = 24;
Why is this int64_t? Seems like overkill, especially since this is only used as
a param to a function that takes int.

https://codereview.chromium.org/868253005/diff/140001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:72: base::Time
GetCacheHardExpiryTime() const;
Why do you call this HardExpiry instead of just Expiry? The current naming makes
me think that there is a hard/soft distinction that doesn't exist.

https://codereview.chromium.org/868253005/diff/140001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:266:
base::TimeDelta::FromHours(kCacheHardExpiryDurationInHours);
From reading the other comments it sounds like you are doing this for testing
purposes, but without any context the fact that you are returning negative time
here looks like a bug. I would think that it's worth making the tests slightly
less clean (start time from something other than 0) in order to make the
production code cleaner. Failing that, perhaps make 0 a special value or comment
the current behavior.

https://codereview.chromium.org/868253005/diff/140001/components/password_man...
File components/password_manager/core/browser/affiliation_backend.h (right):

https://codereview.chromium.org/868253005/diff/140001/components/password_man...
components/password_manager/core/browser/affiliation_backend.h:58: // are not
documented here again, see affiliation_service.h for details:
Nit: This colon seems like it should be a period.

[Mike West, 2015-02-03 09:59:02]

LGTM.

I'm happy with this, modulo Garrett's comments. Next time I'll start the review
with the tests, which I think were nicely commented to make the expected
behavior clearer.

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

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:201:
FacetManager* facet_manager = facet_managers_.get(facet_uri);
On 2015/02/02 at 17:00:12, engedy wrote:
> On 2015/02/02 15:19:45, Mike West wrote:
> > `.add()` can fail, can't it? OOM, etc. Perhaps `DCHECK(facet_manager)`?
> 
> Do I understand correctly that you prefer this because in case of a NULL
pointer, we will crash here with an easy to understand stack trace, as opposed
to somewhere deeper in the call stack with a slightly more obscure stack trace?

Right. The value of a DCHECK is that you crash predictably and know why you
crashed.

[engedy, 2015-02-03 10:48:57]

Patchset #9 (id:160001) has been deleted

[engedy, 2015-02-03 10:49:23]

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

https://codereview.chromium.org/868253005/diff/100001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:40: }
On 2015/02/03 08:20:37, Garrett Casto wrote:
> On 2015/02/02 15:37:37, vabr (Chromium) wrote:
> > On 2015/02/02 15:19:44, Mike West wrote:
> > > Nit: Any reason not to inline these? FacetManager stuff below as well, now
> > that
> > > I see it...
> > 
> >
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...
> > sounds to be a good reason not to inline this.
> > 
> > If I understand the style correctly, as soon as the constructor/destructor
is
> > not trivial, it should not be inlined. It is preferred to call it to copying
> > it's body in the code over and over. Here the constructor contains the
> > construction of the data members. If they were just POD, inlining sounds
fine.
> > But they are not.
> 
> Further down on that page it talks about how in the case of classes in .cc
files
> that you can do whichever is most readable. I would just not declare a
> constructor or destructor here since that's the equivalent of inlining the
> current empty body constructor and destructor and it saved a few extra lines.

Done.

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

https://codereview.chromium.org/868253005/diff/140001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:24: const
int64_t kCacheHardExpiryDurationInHours = 24;
On 2015/02/03 08:20:37, Garrett Casto wrote:
> Why is this int64_t? Seems like overkill, especially since this is only used
as
> a param to a function that takes int.

It used to be in seconds, and I did not notice TimeDelta::FromHours takes an
int. Fixed.

https://codereview.chromium.org/868253005/diff/140001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:72: base::Time
GetCacheHardExpiryTime() const;
On 2015/02/03 08:20:38, Garrett Casto wrote:
> Why do you call this HardExpiry instead of just Expiry? The current naming
makes
> me think that there is a hard/soft distinction that doesn't exist.

Yeah, there will be such a distinction, introduced by a follow-up CL I have
carved that out but forgot to rename this. Done.

https://codereview.chromium.org/868253005/diff/140001/components/password_man...
components/password_manager/core/browser/affiliation_backend.cc:266:
base::TimeDelta::FromHours(kCacheHardExpiryDurationInHours);
On 2015/02/03 08:20:37, Garrett Casto wrote:
> From reading the other comments it sounds like you are doing this for testing
> purposes, but without any context the fact that you are returning negative
time
> here looks like a bug. I would think that it's worth making the tests slightly
> less clean (start time from something other than 0) in order to make the
> production code cleaner. Failing that, perhaps make 0 a special value or
comment
> the current behavior.

Done.

https://codereview.chromium.org/868253005/diff/140001/components/password_man...
File components/password_manager/core/browser/affiliation_backend.h (right):

https://codereview.chromium.org/868253005/diff/140001/components/password_man...
components/password_manager/core/browser/affiliation_backend.h:58: // are not
documented here again, see affiliation_service.h for details:
On 2015/02/03 08:20:38, Garrett Casto wrote:
> Nit: This colon seems like it should be a period.

Done.

[engedy, 2015-02-03 11:55:56]

engedy@chromium.org changed reviewers:
- vabr@chromium.org

[engedy, 2015-02-03 11:57:34]

Garrett, given this CL is on the critical path, and not yet exercised in
production, I will land this with Mike's LGTM.

I would be more than happy to address any comments you may have in a follow-up
CL.

[engedy, 2015-02-03 11:57:55]

The CQ bit was checked by engedy@chromium.org

[commit-bot: I haz the power, 2015-02-03 11:58:09]

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

[commit-bot: I haz the power, 2015-02-03 12:43:11]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2015-02-03 12:44:00]

Patchset 10 (id:??) landed as
https://crrev.com/45920ca9cb490d8f1969a3d9bc543a457f94ec6b
Cr-Commit-Position: refs/heads/master@{#314320}