Add a Restore() method to ValueStore and make StorageAPI use it

chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc

Index: chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc
diff --git a/chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc b/chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc
index 25162d53964d0fa4bc42e665d684ac371734e2a1..7a607a9302692ff8ab576ce6a673dc2981f9abbd 100644
--- a/chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc
+++ b/chrome/browser/extensions/api/storage/settings_storage_quota_enforcer.cc
@@ -9,6 +9,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
+#include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/value_store/value_store_util.h"
 #include "extensions/common/extension_api.h"
@@ -87,17 +88,7 @@ scoped_ptr<ValueStore::Error> QuotaExceededError(Resource resource,
 SettingsStorageQuotaEnforcer::SettingsStorageQuotaEnforcer(
     const Limits& limits, ValueStore* delegate)
     : limits_(limits), delegate_(delegate), used_total_(0) {
-  ReadResult maybe_settings = delegate_->Get();
-  if (maybe_settings->HasError()) {
-    LOG(WARNING) << "Failed to get initial settings for quota: " <<
-        maybe_settings->error().message;
-    return;
-  }
-
-  for (base::DictionaryValue::Iterator it(maybe_settings->settings());
-       !it.IsAtEnd(); it.Advance()) {
-    Allocate(it.key(), it.value(), &used_total_, &used_per_setting_);
-  }
+  CalculateUsage();
 }
 
 SettingsStorageQuotaEnforcer::~SettingsStorageQuotaEnforcer() {}
@@ -229,10 +220,46 @@ ValueStore::WriteResult SettingsStorageQuotaEnforcer::Clear() {
     return result.Pass();
   }
 
-  while (!used_per_setting_.empty()) {
-    Free(&used_total_, &used_per_setting_, used_per_setting_.begin()->first);
-  }
+  used_per_setting_.clear();
+  used_total_ = 0;
   return result.Pass();
 }
 
+bool SettingsStorageQuotaEnforcer::Restore() {
+  if (delegate_->Restore()) {
+    // If we failed, we can't calculate the usage.

[not at google - send to devlin, 2014/02/14 19:35:56]

nit: comment seems like it's in the wrong place, but if you reverse the
condition it would look right. It might also be nice to say like "That's ok
because next Restore(), if it succeeds, will need to recalculate the storage
anyway".

Which might imply that we should free the usage entirely (i.e. reset the usage
counter) just to free up resources and put ourselves in a predictable state.

[Devlin, 2014/02/18 23:55:22]

Done.

+    CalculateUsage();
+    return true;
+  }
+  return false;
+}
+
+bool SettingsStorageQuotaEnforcer::RestoreKey(const std::string& key) {
+  if (delegate_->RestoreKey(key)) {

[not at google - send to devlin, 2014/02/14 19:35:56]

nit: this would also look neater if you reversed the condition

[Devlin, 2014/02/18 23:55:22]

Done.

+    ReadResult result = Get(key);
+    // If the key was deleted as a result of the Restore() call, free it.
+    if (!result->settings().HasKey(key) &&
+        ContainsKey(used_per_setting_, key)) {
+      Free(&used_total_, &used_per_setting_, key);
+    }
+    return true;
+  }
+  return false;
+}
+
+void SettingsStorageQuotaEnforcer::CalculateUsage() {
+  ReadResult maybe_settings = delegate_->Get();
+  if (maybe_settings->HasError()) {
+    LOG(WARNING) << "Failed to get initial settings for quota: "
+                 << maybe_settings->error().message;
+    return;
+  }
+
+  for (base::DictionaryValue::Iterator it(maybe_settings->settings());
+       !it.IsAtEnd();
+       it.Advance()) {
+    Allocate(it.key(), it.value(), &used_total_, &used_per_setting_);
+  }
+}
+
 }  // namespace extensions