Expand the functionality of the BudgetDatabase.

chrome/browser/budget_service/budget_database.cc

Index: chrome/browser/budget_service/budget_database.cc
diff --git a/chrome/browser/budget_service/budget_database.cc b/chrome/browser/budget_service/budget_database.cc
index c786ea34058e2982275890be0abcac5606b6ff9d..5e98166f15600eb17d1ba1369d45ec1e063b2896 100644
--- a/chrome/browser/budget_service/budget_database.cc
+++ b/chrome/browser/budget_service/budget_database.cc
@@ -31,10 +31,7 @@ BudgetDatabase::BudgetDatabase(
 
 BudgetDatabase::~BudgetDatabase() {}
 
-void BudgetDatabase::OnDatabaseInit(bool success) {
-  // TODO(harkness): Consider caching the budget database now?
-}
-
+// TODO(harkness): Either remove this method or add caching.

[Peter Beverloo, 2016/07/22 16:14:51]

Based on what will you make this decision? Dito re: the new TODO on line 46.

Together with the TODO on line 81, it seems to me like the three options you're
considering are:

  (1) Never cache, always cause file I/O (current impl)
  (2) Lazily cache, cause file I/O on the first request
  (3) Eagerly cache, cause file I/O on database load and writes

Right now there's a mixture of (1) and (2), which is not great. In respect to
(2), we're not expiring the cached entries either, which means memory usage may
approximate (3) over time.

This is probably a discussion worth having in person :-).

[harkness, 2016/07/26 16:42:07]

Discussed in person, this will be a lazy system. The methods which are still
using the direct I/O will be removed once their replacement is available.

 void BudgetDatabase::GetValue(const GURL& origin,
                               const GetValueCallback& callback) {
   DCHECK_EQ(origin.GetOrigin(), origin);
@@ -46,6 +43,9 @@ void BudgetDatabase::SetValue(const GURL& origin,
                               const SetValueCallback& callback) {
   DCHECK_EQ(origin.GetOrigin(), origin);
 
+  // TODO(harkness) Either remove this method or add the updated value to
+  // the cache.
+
   // Build structures to hold the updated values.
   std::unique_ptr<
       leveldb_proto::ProtoDatabase<budget_service::Budget>::KeyEntryVector>
@@ -58,3 +58,81 @@ void BudgetDatabase::SetValue(const GURL& origin,
   // Send the updates to the database.
   db_->UpdateEntries(std::move(entries), std::move(keys_to_remove), callback);
 }
+
+void BudgetDatabase::GetBudgetDetails(
+    const GURL& origin,
+    const GetBudgetDetailsCallback& callback) {
+  DCHECK_EQ(origin.GetOrigin(), origin);
+
+  // If this origin is already in the cache, immediately return the data.
+  if (budget_map_.find(origin.spec()) != budget_map_.end())
+    ReturnBudgetDetails(origin, callback, true);

[Peter Beverloo, 2016/07/22 16:14:51]

This is a risk of prefixing your method names with "Return" - control flow in
the method will continue after this, so |callback| will actually be invoked
twice.

[harkness, 2016/07/26 16:42:07]

Nope, this is just me being stupid because I have to keep reminding myself that
we're working with callbacks.

+
+  // Otherwise, query for the data, add it to the cache, then return the result.
+  AddToCacheCallback cache_callback =
+      base::Bind(&BudgetDatabase::ReturnBudgetDetails,
+                 weak_ptr_factory_.GetWeakPtr(), origin, callback);
+  db_->GetEntry(origin.spec(), base::Bind(&BudgetDatabase::AddToCache,
+                                          weak_ptr_factory_.GetWeakPtr(),
+                                          origin, cache_callback));
+}
+
+void BudgetDatabase::OnDatabaseInit(bool success) {
+  // TODO(harkness): Consider caching the budget database now?
+}
+
+void BudgetDatabase::AddToCache(
+    const GURL& origin,
+    const AddToCacheCallback& callback,
+    bool success,
+    std::unique_ptr<budget_service::Budget> budget_proto) {
+  // If the database read failed, there's nothing to add to the cache.
+  if (!success)
+    callback.Run(success);

[Peter Beverloo, 2016/07/22 16:14:51]

Control flow still continues after this check, so we may theoretically be
working with corrupt information in the |budget_proto|, access a null pointer
or...?

[harkness, 2016/07/26 16:42:07]

or just being stupid.  :)

+
+  // Add the data to the cache.
+  BudgetDatabase::BudgetInfo& info = budget_map_[origin.spec()];
+  info.second.clear();
+  info.first = budget_proto->debt();
+
+  // Convert from the proto format to an STL format which is better for removing
+  // things from the front of the list.
+  for (int i = 0; i < budget_proto->budget_size(); i++) {
+    const budget_service::BudgetChunk& chunk = budget_proto->budget(i);
+    info.second.push_back(
+        std::pair<double, double>(chunk.amount(), chunk.expiration()));
+  }

[Peter Beverloo, 2016/07/22 16:14:52]

Since you're clearing the |info| after getting it, wouldn't this code (:93 until
:104) be more elegant when written as follows?

=====
BudgetChunks chunks;
for (const auto& chunk : budget_proto->budget())
  chunks.push_back(std::make_pair(chunk.amount(), chunk.expiration()));

budget_map_[origin.spec()] =
    std::make_pair(budget_proto->debt(), std::move(chunks));
=====

[harkness, 2016/07/26 16:42:07]

Yes, the two have equivalent effects.

In terms of elegance, I have a preference for seeing things broken out in a more
explicit style, but I recognize that's a more old-fashioned approach, so I'm
willing to change it if you prefer.

[Peter Beverloo, 2016/07/26 17:26:20]

Semantically you're replacing the entry in |budget_map_|.

In your code, you do this manually by overriding the values (leading to the
{first/second} obscurity, also because |info| is a non-const reference which is
less obvious than a pointer). In my code I do this by creating a new value
entirely, then overriding the previous one.

I personally find my suggestion much clearer, but up to you.

+  callback.Run(success);
+}
+
+void BudgetDatabase::ReturnBudgetDetails(

[Peter Beverloo, 2016/07/22 16:14:51]

suggestion: DidGetBudgetChunks?

[harkness, 2016/07/26 16:42:07]

I went with DidGetBudget

+    const GURL& origin,
+    const GetBudgetDetailsCallback& callback,
+    bool success) {
+  // If the database wasn't able to read the information, return the
+  // failure and an empty BudgetExpectation.
+  if (!success)
+    callback.Run(success, 0.0, BudgetExpectation());

[Peter Beverloo, 2016/07/22 16:14:51]

nit: s/0.0/0/, elsewhere too

[Peter Beverloo, 2016/07/22 16:14:51]

dito - control flow still continues...

[harkness, 2016/07/26 16:42:07]

Acknowledged.

[harkness, 2016/07/26 16:42:07]

Done.

+
+  // Otherwise, build up the BudgetExpection. This is different from the format
+  // in which the cache stores the data. The cache stores chunks of budget and
+  // when that budget expires. The BudgetExpectation describes a set of times
+  // and the budget at those times.
+  const BudgetInfo& info = budget_map_[origin.spec()];
+  BudgetExpectation expectation;
+  double total = 0.0;
+
+  // Starting with the chunks that expire the farthest in the future, build up
+  // the budget expectations for those future times.
+  for (BudgetChunks::const_reverse_iterator iter = info.second.rbegin();
+       iter != info.second.rend(); iter++) {
+    expectation.push_front(std::pair<double, double>(total, iter->second));

[Peter Beverloo, 2016/07/22 16:14:51]

std::make_pair

[harkness, 2016/07/26 16:42:07]

Done.

+    total += iter->first;
+  }
+
+  // Always add one entry at the front of the list for the total budget right
+  // now.
+  expectation.push_front(std::pair<double, double>(total, 0));

[Peter Beverloo, 2016/07/22 16:14:51]

std::make_pair

[harkness, 2016/07/26 16:42:07]

Done.

+
+  callback.Run(success, info.first, expectation);
+}