Removed key member from ValueStore::Error.

The key member was rarely set and removal simplifies the code.

Review URL: https://codereview.chromium.org/1460553002

Cr-Commit-Position: refs/heads/master@{#362257}

8 files changed, 26 insertions, 51 deletions

diff --git a/extensions/browser/value_store/leveldb_value_store.cc b/extensions/browser/value_store/leveldb_value_store.cc
index 3bdbeef..cf476fc 100644
--- a/extensions/browser/value_store/leveldb_value_store.cc
+++ b/extensions/browser/value_store/leveldb_value_store.cc
@@ -177,10 +177,8 @@ ValueStore::ReadResult LeveldbValueStore::Get() {
     std::string key = it->key().ToString();
     scoped_ptr<base::Value> value =
         json_reader.ReadToValue(it->value().ToString());
-    if (!value) {
-      return MakeReadResult(
-          Error::Create(CORRUPTION, kInvalidJson, util::NewKey(key)));
-    }
+    if (!value)
+      return MakeReadResult(Error::Create(CORRUPTION, kInvalidJson));
     settings->SetWithoutPathExpansion(key, value.Pass());
   }
 
@@ -190,7 +188,7 @@ ValueStore::ReadResult LeveldbValueStore::Get() {
   }
 
   if (!it->status().ok())
-    return MakeReadResult(ToValueStoreError(it->status(), util::NoKey()));
+    return MakeReadResult(ToValueStoreError(it->status()));
 
   return MakeReadResult(settings.Pass());
 }
@@ -271,7 +269,7 @@ ValueStore::WriteResult LeveldbValueStore::Remove(
 
   leveldb::Status status = db_->Write(leveldb::WriteOptions(), &batch);
   if (!status.ok() && !status.IsNotFound())
-    return MakeWriteResult(ToValueStoreError(status, util::NoKey()));
+    return MakeWriteResult(ToValueStoreError(status));
   return MakeWriteResult(changes.Pass());
 }
 
@@ -400,7 +398,7 @@ scoped_ptr<ValueStore::Error> LeveldbValueStore::EnsureDbIsOpen() {
     Restore();
   }
   if (!status.ok())
-    return ToValueStoreError(status, util::NoKey());
+    return ToValueStoreError(status);
 
   CHECK(db);
   db_.reset(db);
@@ -424,11 +422,11 @@ scoped_ptr<ValueStore::Error> LeveldbValueStore::ReadFromDb(
   }
 
   if (!s.ok())
-    return ToValueStoreError(s, util::NewKey(key));
+    return ToValueStoreError(s);
 
   scoped_ptr<base::Value> value = base::JSONReader().ReadToValue(value_as_json);
   if (!value)
-    return Error::Create(CORRUPTION, kInvalidJson, util::NewKey(key));
+    return Error::Create(CORRUPTION, kInvalidJson);
 
   *setting = value.Pass();
   return util::NoError();
@@ -446,6 +444,10 @@ scoped_ptr<ValueStore::Error> LeveldbValueStore::AddToBatch(
     scoped_ptr<base::Value> old_value;
     scoped_ptr<Error> read_error =
         ReadFromDb(leveldb::ReadOptions(), key, &old_value);
+    if (read_error && read_error->code == CORRUPTION) {
+      batch->Delete(key);
+      read_error.reset();
+    }
     if (read_error)
       return read_error.Pass();
     if (!old_value || !old_value->Equals(&value)) {
@@ -459,7 +461,7 @@ scoped_ptr<ValueStore::Error> LeveldbValueStore::AddToBatch(
   if (write_new_value) {
     std::string value_as_json;
     if (!base::JSONWriter::Write(value, &value_as_json))
-      return Error::Create(OTHER_ERROR, kCannotSerialize, util::NewKey(key));
+      return Error::Create(OTHER_ERROR, kCannotSerialize);
     batch->Put(key, value_as_json);
   }
 
@@ -469,8 +471,7 @@ scoped_ptr<ValueStore::Error> LeveldbValueStore::AddToBatch(
 scoped_ptr<ValueStore::Error> LeveldbValueStore::WriteToDb(
     leveldb::WriteBatch* batch) {
   leveldb::Status status = db_->Write(leveldb::WriteOptions(), batch);
-  return status.ok() ? util::NoError()
-                     : ToValueStoreError(status, util::NoKey());
+  return status.ok() ? util::NoError() : ToValueStoreError(status);
 }
 
 bool LeveldbValueStore::IsEmpty() {
@@ -497,8 +498,7 @@ bool LeveldbValueStore::DeleteDbFile() {
 }
 
 scoped_ptr<ValueStore::Error> LeveldbValueStore::ToValueStoreError(
-    const leveldb::Status& status,
-    scoped_ptr<std::string> key) {
+    const leveldb::Status& status) {
   CHECK(!status.ok());
   CHECK(!status.IsNotFound());  // not an error
 
@@ -508,5 +508,5 @@ scoped_ptr<ValueStore::Error> LeveldbValueStore::ToValueStoreError(
   base::ReplaceSubstringsAfterOffset(
       &message, 0u, db_path_.AsUTF8Unsafe(), "...");
 
-  return Error::Create(CORRUPTION, message, key.Pass());
+  return Error::Create(CORRUPTION, message);
 }
diff --git a/extensions/browser/value_store/leveldb_value_store.h b/extensions/browser/value_store/leveldb_value_store.h
index d486283..d32be6e 100644
--- a/extensions/browser/value_store/leveldb_value_store.h
+++ b/extensions/browser/value_store/leveldb_value_store.h
@@ -87,8 +87,7 @@ class LeveldbValueStore : public ValueStore,
   // Converts an error leveldb::Status to a ValueStore::Error. Returns a
   // scoped_ptr for convenience; the result will always be non-empty.
   scoped_ptr<ValueStore::Error> ToValueStoreError(
-      const leveldb::Status& status,
-      scoped_ptr<std::string> key);
+      const leveldb::Status& status);
 
   // Removes the on-disk database at |db_path_|. Any file system locks should
   // be released before calling this method.
diff --git a/extensions/browser/value_store/testing_value_store.cc b/extensions/browser/value_store/testing_value_store.cc
index f4002de..2629538 100644
--- a/extensions/browser/value_store/testing_value_store.cc
+++ b/extensions/browser/value_store/testing_value_store.cc
@@ -133,6 +133,5 @@ bool TestingValueStore::RestoreKey(const std::string& key) {
 }
 
 scoped_ptr<ValueStore::Error> TestingValueStore::TestingError() {
-  return make_scoped_ptr(new ValueStore::Error(
-      error_code_, kGenericErrorMessage, scoped_ptr<std::string>()));
+  return ValueStore::Error::Create(error_code_, kGenericErrorMessage);
 }
diff --git a/extensions/browser/value_store/testing_value_store.h b/extensions/browser/value_store/testing_value_store.h
index 27df928..38e5258 100644
--- a/extensions/browser/value_store/testing_value_store.h
+++ b/extensions/browser/value_store/testing_value_store.h
@@ -5,6 +5,9 @@
 #ifndef EXTENSIONS_BROWSER_VALUE_STORE_TESTING_VALUE_STORE_H_
 #define EXTENSIONS_BROWSER_VALUE_STORE_TESTING_VALUE_STORE_H_
 
+#include <string>
+#include <vector>
+
 #include "base/compiler_specific.h"
 #include "extensions/browser/value_store/value_store.h"
 
diff --git a/extensions/browser/value_store/value_store.cc b/extensions/browser/value_store/value_store.cc
index 0b87b5f..bd340fb 100644
--- a/extensions/browser/value_store/value_store.cc
+++ b/extensions/browser/value_store/value_store.cc
@@ -8,10 +8,8 @@
 
 // Implementation of Error.
 
-ValueStore::Error::Error(ErrorCode code,
-                         const std::string& message,
-                         scoped_ptr<std::string> key)
-    : code(code), message(message), key(key.Pass()) {}
+ValueStore::Error::Error(ErrorCode code, const std::string& message)
+    : code(code), message(message) {}
 
 ValueStore::Error::~Error() {}
 
diff --git a/extensions/browser/value_store/value_store.h b/extensions/browser/value_store/value_store.h
index f0d5cc0..711b327 100644
--- a/extensions/browser/value_store/value_store.h
+++ b/extensions/browser/value_store/value_store.h
@@ -41,15 +41,11 @@ class ValueStore {
 
   // Bundles an ErrorCode with further metadata.
   struct Error {
-    Error(ErrorCode code,
-          const std::string& message,
-          scoped_ptr<std::string> key);
     ~Error();
 
     static scoped_ptr<Error> Create(ErrorCode code,
-                                    const std::string& message,
-                                    scoped_ptr<std::string> key) {
-      return make_scoped_ptr(new Error(code, message, key.Pass()));
+                                    const std::string& message) {
+      return make_scoped_ptr(new Error(code, message));
     }
 
     // The error code.
@@ -58,13 +54,9 @@ class ValueStore {
     // Message associated with the error.
     const std::string message;
 
-    // The key associated with the error, if any. Use a scoped_ptr here
-    // because empty-string is a valid key.
-    //
-    // TODO(kalman): add test(s) for an empty key.
-    const scoped_ptr<std::string> key;
-
    private:
+    Error(ErrorCode code, const std::string& message);
+
     DISALLOW_COPY_AND_ASSIGN(Error);
   };
 
diff --git a/extensions/browser/value_store/value_store_util.cc b/extensions/browser/value_store/value_store_util.cc
index 67a3a00..b7cdff1 100644
--- a/extensions/browser/value_store/value_store_util.cc
+++ b/extensions/browser/value_store/value_store_util.cc
@@ -6,14 +6,6 @@
 
 namespace value_store_util {
 
-scoped_ptr<std::string> NewKey(const std::string& key) {
-  return make_scoped_ptr(new std::string(key));
-}
-
-scoped_ptr<std::string> NoKey() {
-  return scoped_ptr<std::string>();
-}
-
 scoped_ptr<ValueStore::Error> NoError() {
   return scoped_ptr<ValueStore::Error>();
 }
diff --git a/extensions/browser/value_store/value_store_util.h b/extensions/browser/value_store/value_store_util.h
index ae3b37f..6ee22aa 100644
--- a/extensions/browser/value_store/value_store_util.h
+++ b/extensions/browser/value_store/value_store_util.h
@@ -12,14 +12,6 @@
 
 namespace value_store_util {
 
-// Returns a copy of |key| as a scoped_ptr. Useful for creating keys for
-// ValueStore::Error.
-scoped_ptr<std::string> NewKey(const std::string& key);
-
-// Returns an empty scoped_ptr. Useful for creating empty keys for
-// ValueStore::Error.
-scoped_ptr<std::string> NoKey();
-
 // Return an empty Error. Useful for creating ValueStore::Error-less
 // ValueStore::Read/WriteResults.
 scoped_ptr<ValueStore::Error> NoError();