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

components/password_manager/core/browser/affiliation_backend.cc

Index: components/password_manager/core/browser/affiliation_backend.cc
diff --git a/components/password_manager/core/browser/affiliation_backend.cc b/components/password_manager/core/browser/affiliation_backend.cc
index e1668444f854199aad3bc333fdf54753008f4539..2555260bb779524e7eb11d856abc4ea8fd15575d 100644
--- a/components/password_manager/core/browser/affiliation_backend.cc
+++ b/components/password_manager/core/browser/affiliation_backend.cc
@@ -4,26 +4,208 @@
 
 #include "components/password_manager/core/browser/affiliation_backend.h"
 
+#include <stdint.h>
+
+#include "base/bind.h"
+#include "base/location.h"
 #include "base/task_runner.h"
+#include "base/time/clock.h"
+#include "base/time/time.h"
+#include "components/password_manager/core/browser/affiliation_database.h"
+#include "components/password_manager/core/browser/affiliation_fetcher.h"
+#include "net/url_request/url_request_context_getter.h"
 
 namespace password_manager {
+namespace {
+
+// The duration after which cached affiliation data is considered stale and will
+// not be used to serve requests any longer.
+const int64_t kCacheHardExpiryDurationInSeconds = 24 * 3600;

[Mike West, 2015/02/02 15:19:44]

Nit: "24 * 60 * 60" seems slightly more common in `components/` than "24 *
3600". But whatever. :)

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

Done. I have replaced this with kCacheHardExpiryDurationInHours.

+
+// RequestInfo ----------------------------------------------------------------
+
+// Encapsulates the details of a pending GetAffiliations() request.
+struct RequestInfo {
+  RequestInfo();
+  ~RequestInfo();
+
+  AffiliationService::ResultCallback callback;
+  scoped_refptr<base::TaskRunner> callback_task_runner;
+};
+
+RequestInfo::RequestInfo() {
+}
+
+RequestInfo::~RequestInfo() {
+}

[Mike West, 2015/02/02 15:19:44]

Nit: Any reason not to inline these? FacetManager stuff below as well, now that
I see it...

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

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:37]

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.

[Garrett Casto, 2015/02/03 08:20:37]

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.

[engedy, 2015/02/03 10:49:22]

Done.

+
+}  // namespace
+
+// AffiliationBackend::FacetManager -------------------------------------------
+
+// Manages and performs ongoing tasks concerning a single facet.
+class AffiliationBackend::FacetManager {
+ public:
+  // The |backend| must outlive this object.
+  FacetManager(AffiliationBackend* backend, const FacetURI& facet_uri);
+  ~FacetManager();
+
+  // Called when |affiliation| information regarding this facet has just been
+  // fetched from the Affiliation API.
+  void OnFetchSucceeded(const AffiliatedFacetsWithUpdateTime& affiliation);
+
+  // Returns whether this instance has becomes redundant, that is, it has no
+  // more meaningful state than a newly created instance would have.
+  bool CanBeDiscarded() const;
+
+  // Returns whether or not affiliation information relating to this facet needs
+  // to be fetched right now.
+  bool DoesRequireFetch() const;
+
+  // Facet-specific implementations for methods in AffiliationService of the
+  // same name. See documentation in affiliation_service.h for details:
+  void GetAffiliations(bool cached_only, const RequestInfo& request_info);
+
+ private:
+  // Returns the time when cached data for this facet will become stale.
+  base::Time GetCacheHardExpiryTime() const;
+
+  // Returns whether or not the cache has fresh data for this facet.
+  bool IsCachedDataFresh() const;
+
+  // Posts the callback of the request described by |request_info| with success.
+  static void ServeRequestWithSuccess(const RequestInfo& request_info,
+                                      const AffiliatedFacets& affiliation);
 
-AffiliationBackend::AffiliationBackend() {
+  // Posts the callback of the request described by |request_info| with failure.
+  static void ServeRequestWithFailure(const RequestInfo& request_info);
+
+  AffiliationBackend* backend_;
+  FacetURI facet_uri_;
+
+  // The last time affiliation information was fetched for this facet, i.e. the
+  // freshness of the data in the database. If there is no corresponding data in
+  // in the database, this will contain something sufficiently far away in the
+  // past to be considered stale for sure. Otherwise, the last_update_time in
+  // the database should match this value; it is only cached to reduce disk I/O.
+  base::Time last_update_time_;
+
+  // Contains information about the GetAffiliations() requests that are waiting
+  // for the result of looking up this facet.
+  std::vector<RequestInfo> pending_requests_;
+
+  DISALLOW_COPY_AND_ASSIGN(FacetManager);
+};
+
+AffiliationBackend::FacetManager::FacetManager(AffiliationBackend* backend,
+                                               const FacetURI& facet_uri)
+    : backend_(backend),
+      facet_uri_(facet_uri),
+      last_update_time_(backend_->ReadLastUpdateTimeFromDatabase(facet_uri)) {
+}
+
+AffiliationBackend::FacetManager::~FacetManager() {
+  // The manager will only be destroyed while there are pending requests if the
+  // entire backend is going. Call failure on pending requests in this case.
+  for (const auto& request_info : pending_requests_)
+    ServeRequestWithFailure(request_info);
+}
+
+void AffiliationBackend::FacetManager::GetAffiliations(
+    bool cached_only,
+    const RequestInfo& request_info) {
+  if (IsCachedDataFresh()) {
+    AffiliatedFacetsWithUpdateTime affiliation;
+    if (!backend_->ReadAffiliationsFromDatabase(facet_uri_, &affiliation))
+      NOTIMPLEMENTED();
+    DCHECK_EQ(affiliation.last_update_time, last_update_time_) << facet_uri_;
+    ServeRequestWithSuccess(request_info, affiliation.facets);
+  } else if (cached_only) {
+    ServeRequestWithFailure(request_info);
+  } else {
+    pending_requests_.push_back(request_info);
+    backend_->SignalNeedNetworkRequest();
+  }
+}
+
+void AffiliationBackend::FacetManager::OnFetchSucceeded(
+    const AffiliatedFacetsWithUpdateTime& affiliation) {
+  last_update_time_ = affiliation.last_update_time;
+  DCHECK(IsCachedDataFresh()) << facet_uri_;
+  for (const auto& request_info : pending_requests_)
+    ServeRequestWithSuccess(request_info, affiliation.facets);
+  pending_requests_.clear();
+}
+
+bool AffiliationBackend::FacetManager::CanBeDiscarded() const {
+  return pending_requests_.empty();
+}
+
+bool AffiliationBackend::FacetManager::DoesRequireFetch() const {
+  return !pending_requests_.empty() && !IsCachedDataFresh();
+}
+
+base::Time AffiliationBackend::FacetManager::GetCacheHardExpiryTime() const {
+  return last_update_time_ +
+         base::TimeDelta::FromSeconds(kCacheHardExpiryDurationInSeconds);
+}
+
+bool AffiliationBackend::FacetManager::IsCachedDataFresh() const {
+  return backend_->GetCurrentTime() < GetCacheHardExpiryTime();
+}
+
+// static
+void AffiliationBackend::FacetManager::ServeRequestWithSuccess(
+    const RequestInfo& request_info,
+    const AffiliatedFacets& affiliation) {
+  request_info.callback_task_runner->PostTask(
+      FROM_HERE, base::Bind(request_info.callback, affiliation, true));
+}
+
+// static
+void AffiliationBackend::FacetManager::ServeRequestWithFailure(
+    const RequestInfo& request_info) {
+  request_info.callback_task_runner->PostTask(
+      FROM_HERE, base::Bind(request_info.callback, AffiliatedFacets(), false));
+}
+
+// AffiliationBackend ---------------------------------------------------------
+
+AffiliationBackend::AffiliationBackend(
+    const scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
+    scoped_ptr<base::Clock> time_source)
+    : request_context_getter_(request_context_getter),
+      clock_(time_source.Pass()),
+      weak_ptr_factory_(this) {
 }
 
 AffiliationBackend::~AffiliationBackend() {
 }
 
-void AffiliationBackend::Initialize() {
-  NOTIMPLEMENTED();
+void AffiliationBackend::Initialize(const base::FilePath& db_path) {

[Mike West, 2015/02/02 15:19:44]

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.

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

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

+  cache_.reset(new AffiliationDatabase());
+  if (!cache_->Init(db_path))
+    NOTIMPLEMENTED();
 }
 
 void AffiliationBackend::GetAffiliations(
     const FacetURI& facet_uri,
     bool cached_only,
     const AffiliationService::ResultCallback& callback,
-    scoped_refptr<base::TaskRunner> callback_task_runner) {
-  NOTIMPLEMENTED();
+    const scoped_refptr<base::TaskRunner>& callback_task_runner) {
+  if (!facet_managers_.contains(facet_uri)) {
+    scoped_ptr<FacetManager> new_manager(new FacetManager(this, facet_uri));
+    facet_managers_.add(facet_uri, new_manager.Pass());
+  }
+
+  FacetManager* facet_manager = facet_managers_.get(facet_uri);

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

`.add()` can fail, can't it? OOM, etc. Perhaps `DCHECK(facet_manager)`?

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

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.

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

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

+  RequestInfo request_info;
+  request_info.callback = callback;
+  request_info.callback_task_runner = callback_task_runner;
+  facet_manager->GetAffiliations(cached_only, request_info);
+
+  if (facet_manager->CanBeDiscarded())
+    facet_managers_.erase(facet_uri);
 }
 
 void AffiliationBackend::Prefetch(const FacetURI& facet_uri,
@@ -40,4 +222,88 @@ void AffiliationBackend::TrimCache() {
   NOTIMPLEMENTED();
 }
 
+void AffiliationBackend::SendNetworkRequest() {
+  DCHECK(!fetcher_);

[Mike West, 2015/02/02 15:19:44]

DCHECK that you have more than 0 managers? I guess that's covered by the empty
check on #233.

[engedy, 2015/02/02 17:00:11]

Yes, I'd prefer not checking that separately.

+
+  std::vector<FacetURI> requested_facet_uris;
+  for (const auto& facet_manager_pair : facet_managers_) {
+    if (facet_manager_pair.second->DoesRequireFetch())
+      requested_facet_uris.push_back(facet_manager_pair.first);
+  }
+  DCHECK(!requested_facet_uris.empty());
+  fetcher_.reset(AffiliationFetcher::Create(request_context_getter_.get(),
+                                            requested_facet_uris, this));
+  fetcher_->StartRequest();
+}
+
+base::Time AffiliationBackend::GetCurrentTime() {
+  return clock_->Now();
+}
+
+base::Time AffiliationBackend::ReadLastUpdateTimeFromDatabase(
+    const FacetURI& facet_uri) {
+  AffiliatedFacetsWithUpdateTime affiliation;
+  if (cache_->GetAffiliationsForFacet(facet_uri, &affiliation))

[Mike West, 2015/02/02 15:19:44]

You could spell this `ReadAffiliationsFromDatabase`.

[engedy, 2015/02/02 17:00:11]

Done.

+    return affiliation.last_update_time;
+  else
+    return clock_->Now() -
+           base::TimeDelta::FromSeconds(kCacheHardExpiryDurationInSeconds);

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

Nit: I think a ternary if would be prettier here. But whatever. :)

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

[engedy, 2015/02/02 17:00:11]

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

+}
+
+bool AffiliationBackend::ReadAffiliationsFromDatabase(
+    const FacetURI& facet_uri,
+    AffiliatedFacetsWithUpdateTime* affiliations) {
+  return cache_->GetAffiliationsForFacet(facet_uri, affiliations);
+}
+
+void AffiliationBackend::SignalNeedNetworkRequest() {
+  // TODO(engedy): Integrate more sophisticated throttling logic here.
+  if (fetcher_)
+    return;
+  SendNetworkRequest();
+}
+
+void AffiliationBackend::OnFetchSucceeded(
+    scoped_ptr<AffiliationFetcherDelegate::Result> result) {
+  fetcher_.reset();
+
+  for (const AffiliatedFacets& affiliated_facets : *result) {
+    AffiliatedFacetsWithUpdateTime affiliation;
+    affiliation.facets = affiliated_facets;
+    affiliation.last_update_time = clock_->Now();
+
+    std::vector<AffiliatedFacetsWithUpdateTime> obsoleted_affiliations;
+    cache_->StoreAndRemoveConflicting(affiliation, &obsoleted_affiliations);
+
+    if (!obsoleted_affiliations.empty())
+      NOTIMPLEMENTED();

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

Can you add a comment here? It's not clear to me what this is supposed to be
checking.

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

Done.

+
+    for (const auto& facet_uri : affiliated_facets) {
+      if (!facet_managers_.contains(facet_uri))

[Mike West, 2015/02/02 15:19:44]

It feels like there can be a race here if requests come in during a fetch.

[engedy, 2015/02/02 17:00:11]

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?

+        continue;
+      FacetManager* facet_manager = facet_managers_.get(facet_uri);
+      facet_manager->OnFetchSucceeded(affiliation);
+      if (facet_manager->CanBeDiscarded())
+        facet_managers_.erase(facet_uri);
+    }
+  }
+
+  // A subsequent fetch is needed if additional GetAffiliations() request came

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

Nit: s/request came/requests come/ or s/if additional/if an additional/

[engedy, 2015/02/02 17:00:11]

Done.

+  // in while the current fetch was in flight.
+  for (const auto& facet_manager_pair : facet_managers_) {
+    if (facet_manager_pair.second->DoesRequireFetch()) {
+      SendNetworkRequest();
+      return;
+    }
+  }
+}
+
+void AffiliationBackend::OnFetchFailed() {
+  NOTIMPLEMENTED();

[Mike West, 2015/02/02 15:19:44]

Please add TODO comments pointing to a bug for this and other NOTIMPLEMENTED
calls. At least the ones you added in this CL.

[engedy, 2015/02/02 17:00:11]

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

+}
+
+void AffiliationBackend::OnMalformedResponse() {
+  NOTIMPLEMENTED();
+}
+
 }  // namespace password_manager