IndexedDB: Convert decoding functions to pass StringPieces vs. pointers

content/browser/indexed_db/indexed_db_backing_store.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/indexed_db/indexed_db_backing_store.h"
 
 #include <string>
 
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
+#include "base/strings/string_piece.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/browser/indexed_db/indexed_db_leveldb_coding.h"
 #include "content/browser/indexed_db/indexed_db_metadata.h"
 #include "content/browser/indexed_db/indexed_db_tracing.h"
 #include "content/browser/indexed_db/leveldb/leveldb_comparator.h"
 #include "content/browser/indexed_db/leveldb/leveldb_database.h"
 #include "content/browser/indexed_db/leveldb/leveldb_iterator.h"
 #include "content/browser/indexed_db/leveldb/leveldb_slice.h"
 #include "content/browser/indexed_db/leveldb/leveldb_transaction.h"
 #include "content/common/indexed_db/indexed_db_key.h"
 #include "content/common/indexed_db/indexed_db_key_path.h"
 #include "content/common/indexed_db/indexed_db_key_range.h"
 #include "third_party/WebKit/public/platform/WebIDBKey.h"
 #include "third_party/WebKit/public/platform/WebIDBKeyPath.h"
 
+using base::StringPiece;
+
 // TODO(jsbell): Make blink push the version during the open() call.
 static const uint32 kWireVersion = 2;
 
 namespace content {
 
 static const int64 kKeyGeneratorInitialNumber =
     1;  // From the IndexedDB specification.
 
 enum IndexedDBBackingStoreErrorSource {
   // 0 - 2 are no longer used.
@@ skipping 53 matching lines @@
   std::vector<char> buffer;
   EncodeBool(value, &buffer);
   transaction->Put(key, buffer);
 }
 
 template <typename DBOrTransaction>
 static bool GetInt(DBOrTransaction* db,
                    const LevelDBSlice& key,
                    int64& found_int,
                    bool& found) {
-  std::vector<char> result;
-  bool ok = db->Get(key, result, found);
+  std::string result;
+  bool ok = db->Get(key, &result, found);
   if (!ok)
     return false;
   if (!found)
     return true;
-
-  found_int = DecodeInt(result.begin(), result.end());
-  return true;
+  StringPiece slice(result);
+  return DecodeInt(&slice, &found_int) && slice.empty();
 }
 
 static void PutInt(LevelDBTransaction* transaction,
                    const LevelDBSlice& key,
                    int64 value) {
   DCHECK_GE(value, 0);
   std::vector<char> buffer;
   EncodeInt(value, &buffer);
   transaction->Put(key, buffer);
 }
 
 template <typename DBOrTransaction>
 WARN_UNUSED_RESULT static bool GetVarInt(DBOrTransaction* db,
                                          const LevelDBSlice& key,
                                          int64& found_int,
                                          bool& found) {
-  std::vector<char> result;
-  bool ok = db->Get(key, result, found);
+  std::string result;
+  bool ok = db->Get(key, &result, found);
   if (!ok)
     return false;
   if (!found)
     return true;
-  if (!result.size())
-    return false;
-
-  found = DecodeVarInt(&*result.begin(), &*result.rbegin() + 1, found_int) ==
-          &*result.rbegin() + 1;
-  return true;
+  StringPiece slice(result);
+  return DecodeVarInt(&slice, &found_int) && slice.empty();
 }
 
 static void PutVarInt(LevelDBTransaction* transaction,
                       const LevelDBSlice& key,
                       int64 value) {
   std::vector<char> buffer;
   EncodeVarInt(value, &buffer);
   transaction->Put(key, buffer);
 }
 
 template <typename DBOrTransaction>
 WARN_UNUSED_RESULT static bool GetString(DBOrTransaction* db,
                                          const LevelDBSlice& key,
                                          string16& found_string,
                                          bool& found) {
-  std::vector<char> result;
+  std::string result;
   found = false;
-  bool ok = db->Get(key, result, found);
+  bool ok = db->Get(key, &result, found);
   if (!ok)
     return false;
   if (!found)
     return true;
-  if (!result.size()) {
-    found_string.clear();
-    return true;
-  }
-
-  found_string = DecodeString(&*result.begin(), &*result.rbegin() + 1);
-  return true;
+  StringPiece slice(result);
+  return DecodeString(&slice, &found_string) && slice.empty();
 }
 
 static void PutString(LevelDBTransaction* transaction,
                       const LevelDBSlice& key,
                       const string16& value) {
   std::vector<char> buffer;
   EncodeString(value, &buffer);
   transaction->Put(key, buffer);
 }
 
@@ skipping 688 matching lines @@
       INTERNAL_CONSISTENCY_ERROR(GET_OBJECT_STORES);
       // Possible stale metadata, but don't fail the load.
       it->Next();
       continue;
     }
 
     int64 object_store_id = meta_data_key.ObjectStoreId();
 
     // TODO(jsbell): Do this by direct key lookup rather than iteration, to
     // simplify.
-    string16 object_store_name =
-        DecodeString(it->Value().begin(), it->Value().end());
+    string16 object_store_name;
+    {
+      StringPiece slice(it->Value());
+      if (!DecodeString(&slice, &object_store_name) || !slice.empty())
+        INTERNAL_CONSISTENCY_ERROR(GET_OBJECT_STORES);
+    }
 
     it->Next();
     if (!CheckObjectStoreAndMetaDataType(it.get(),
                                          stop_key,
                                          object_store_id,
                                          ObjectStoreMetaDataKey::KEY_PATH)) {
       INTERNAL_CONSISTENCY_ERROR(GET_OBJECT_STORES);
       break;
     }
-    IndexedDBKeyPath key_path =
-        DecodeIDBKeyPath(it->Value().begin(), it->Value().end());
+    IndexedDBKeyPath key_path;
+    {
+      StringPiece slice(it->Value());
+      if (!DecodeIDBKeyPath(&slice, &key_path) || !slice.empty())
+        INTERNAL_CONSISTENCY_ERROR(GET_OBJECT_STORES);
+    }
 
     it->Next();
     if (!CheckObjectStoreAndMetaDataType(
             it.get(),
             stop_key,
             object_store_id,
             ObjectStoreMetaDataKey::AUTO_INCREMENT)) {
       INTERNAL_CONSISTENCY_ERROR(GET_OBJECT_STORES);
       break;
     }
-    bool auto_increment = DecodeBool(it->Value().begin(), it->Value().end());
+    bool auto_increment;
+    {
+      StringPiece slice(it->Value());
+      if (!DecodeBool(&slice, &auto_increment) || !slice.empty())
+        INTERNAL_CONSISTENCY_ERROR(GET_OBJECT_STORES);
+    }
 
     it->Next();  // Is evicatble.
     if (!CheckObjectStoreAndMetaDataType(it.get(),
                                          stop_key,
                                          object_store_id,
                                          ObjectStoreMetaDataKey::EVICTABLE)) {
       INTERNAL_CONSISTENCY_ERROR(GET_OBJECT_STORES);
       break;
     }
 
     it->Next();  // Last version.
     if (!CheckObjectStoreAndMetaDataType(
             it.get(),
             stop_key,
             object_store_id,
             ObjectStoreMetaDataKey::LAST_VERSION)) {
       INTERNAL_CONSISTENCY_ERROR(GET_OBJECT_STORES);
       break;
     }
 
     it->Next();  // Maximum index id allocated.
     if (!CheckObjectStoreAndMetaDataType(
             it.get(),
             stop_key,
             object_store_id,
             ObjectStoreMetaDataKey::MAX_INDEX_ID)) {
       INTERNAL_CONSISTENCY_ERROR(GET_OBJECT_STORES);
       break;
     }
-    int64 max_index_id = DecodeInt(it->Value().begin(), it->Value().end());
+    int64 max_index_id;
+    {
+      StringPiece slice(it->Value());
+      if (!DecodeInt(&slice, &max_index_id) || !slice.empty())
+        INTERNAL_CONSISTENCY_ERROR(GET_OBJECT_STORES);
+    }
 
     it->Next();  // [optional] has key path (is not null)
     if (CheckObjectStoreAndMetaDataType(it.get(),
                                         stop_key,
                                         object_store_id,
                                         ObjectStoreMetaDataKey::HAS_KEY_PATH)) {
-      bool has_key_path = DecodeBool(it->Value().begin(), it->Value().end());
+      bool has_key_path;
+      {
+        StringPiece slice(it->Value());
+        if (!DecodeBool(&slice, &has_key_path))
+          INTERNAL_CONSISTENCY_ERROR(GET_OBJECT_STORES);
+      }
       // This check accounts for two layers of legacy coding:
       // (1) Initially, has_key_path was added to distinguish null vs. string.
       // (2) Later, null vs. string vs. array was stored in the key_path itself.
       // So this check is only relevant for string-type key_paths.
       if (!has_key_path &&
           (key_path.type() == WebKit::WebIDBKeyPath::StringType &&
            !key_path.string().empty())) {
         INTERNAL_CONSISTENCY_ERROR(GET_OBJECT_STORES);
         break;
       }
       if (!has_key_path)
         key_path = IndexedDBKeyPath();
       it->Next();
     }
 
     int64 key_generator_current_number = -1;
     if (CheckObjectStoreAndMetaDataType(
             it.get(),
             stop_key,
             object_store_id,
             ObjectStoreMetaDataKey::KEY_GENERATOR_CURRENT_NUMBER)) {
-      key_generator_current_number =
-          DecodeInt(it->Value().begin(), it->Value().end());
+      StringPiece slice(it->Value());
+      if (!DecodeInt(&slice, &key_generator_current_number) || !slice.empty())
+        INTERNAL_CONSISTENCY_ERROR(GET_OBJECT_STORES);
+
       // TODO(jsbell): Return key_generator_current_number, cache in
       // object store, and write lazily to backing store.  For now,
       // just assert that if it was written it was valid.
       DCHECK_GE(key_generator_current_number, kKeyGeneratorInitialNumber);
       it->Next();
     }
 
     IndexedDBObjectStoreMetadata metadata(object_store_name,
                                           object_store_id,
                                           key_path,
@@ skipping 132 matching lines @@
     const IndexedDBKey& key,
     std::vector<char>& record) {
   IDB_TRACE("IndexedDBBackingStore::get_record");
   if (!KeyPrefix::ValidIds(database_id, object_store_id))
     return false;
   LevelDBTransaction* leveldb_transaction =
       IndexedDBBackingStore::Transaction::LevelDBTransactionFrom(transaction);
 
   const std::vector<char> leveldb_key =
       ObjectStoreDataKey::Encode(database_id, object_store_id, key);
-  std::vector<char> data;
+  std::string data;
 
   record.clear();
 
   bool found = false;
-  bool ok = leveldb_transaction->Get(LevelDBSlice(leveldb_key), data, found);
+  bool ok = leveldb_transaction->Get(LevelDBSlice(leveldb_key), &data, found);
   if (!ok) {
     INTERNAL_READ_ERROR(GET_RECORD);
     return false;
   }
   if (!found)
     return true;
-  if (!data.size()) {
+  if (data.empty()) {
     INTERNAL_READ_ERROR(GET_RECORD);
     return false;
   }
 
   int64 version;
-  const char* p = DecodeVarInt(&*data.begin(), &*data.rbegin() + 1, version);
-  if (!p) {
+  StringPiece slice(data);
+  if (!DecodeVarInt(&slice, &version)) {
     INTERNAL_READ_ERROR(GET_RECORD);
     return false;
   }
 
-  record.insert(record.end(), p, static_cast<const char*>(&*data.rbegin()) + 1);
+  record.insert(record.end(), slice.begin(), slice.end());
   return true;
 }
 
 WARN_UNUSED_RESULT static bool GetNewVersionNumber(
     LevelDBTransaction* transaction,
     int64 database_id,
     int64 object_store_id,
     int64& new_version_number) {
   const std::vector<char> last_version_key = ObjectStoreMetaDataKey::Encode(
       database_id, object_store_id, ObjectStoreMetaDataKey::LAST_VERSION);
@@ skipping 51 matching lines @@
   v.insert(v.end(), value.begin(), value.end());
 
   leveldb_transaction->Put(LevelDBSlice(object_storedata_key), v);
 
   const std::vector<char> exists_entry_key =
       ExistsEntryKey::Encode(database_id, object_store_id, key);
   std::vector<char> version_encoded;
   EncodeInt(version, &version_encoded);
   leveldb_transaction->Put(LevelDBSlice(exists_entry_key), version_encoded);
 
-  record_identifier->Reset(EncodeIDBKey(key), version);
+  std::vector<char> key_encoded;
+  EncodeIDBKey(key, &key_encoded);
+  record_identifier->Reset(key_encoded, version);
   return true;
 }
 
 bool IndexedDBBackingStore::ClearObjectStore(
     IndexedDBBackingStore::Transaction* transaction,
     int64 database_id,
     int64 object_store_id) {
   IDB_TRACE("IndexedDBBackingStore::clear_object_store");
   if (!KeyPrefix::ValidIds(database_id, object_store_id))
     return false;
@@ skipping 39 matching lines @@
   LevelDBTransaction* leveldb_transaction =
       IndexedDBBackingStore::Transaction::LevelDBTransactionFrom(transaction);
 
   const std::vector<char> key_generator_current_number_key =
       ObjectStoreMetaDataKey::Encode(
           database_id,
           object_store_id,
           ObjectStoreMetaDataKey::KEY_GENERATOR_CURRENT_NUMBER);
 
   key_generator_current_number = -1;
-  std::vector<char> data;
+  std::string data;
 
   bool found = false;
   bool ok = leveldb_transaction->Get(
-      LevelDBSlice(key_generator_current_number_key), data, found);
+      LevelDBSlice(key_generator_current_number_key), &data, found);
   if (!ok) {
     INTERNAL_READ_ERROR(GET_KEY_GENERATOR_CURRENT_NUMBER);
     return false;
   }
-  if (found) {
-    key_generator_current_number = DecodeInt(data.begin(), data.end());
-  } else {
-    // Previously, the key generator state was not stored explicitly
-    // but derived from the maximum numeric key present in existing
-    // data. This violates the spec as the data may be cleared but the
-    // key generator state must be preserved.
-    // TODO(jsbell): Fix this for all stores on database open?
-    const std::vector<char> start_key =
-        ObjectStoreDataKey::Encode(database_id, object_store_id, MinIDBKey());
-    const std::vector<char> stop_key =
-        ObjectStoreDataKey::Encode(database_id, object_store_id, MaxIDBKey());
-
-    scoped_ptr<LevelDBIterator> it = leveldb_transaction->CreateIterator();
-    int64 max_numeric_key = 0;
-
-    for (it->Seek(LevelDBSlice(start_key));
-         it->IsValid() && CompareKeys(it->Key(), LevelDBSlice(stop_key)) < 0;
-         it->Next()) {
-      const char* p = it->Key().begin();
-      const char* limit = it->Key().end();
-
-      ObjectStoreDataKey data_key;
-      p = ObjectStoreDataKey::Decode(p, limit, &data_key);
-      DCHECK(p);
-
-      scoped_ptr<IndexedDBKey> user_key = data_key.user_key();
-      if (user_key->type() == WebKit::WebIDBKey::NumberType) {
-        int64 n = static_cast<int64>(user_key->number());
-        if (n > max_numeric_key)
-          max_numeric_key = n;
-      }
+  if (found && !data.empty()) {
+    StringPiece slice(data);
+    if (!DecodeInt(&slice, &key_generator_current_number) || !slice.empty()) {
+      INTERNAL_READ_ERROR(GET_KEY_GENERATOR_CURRENT_NUMBER);
+      return false;
     }
-
-    key_generator_current_number = max_numeric_key + 1;
+    return true;
   }
 
+  // Previously, the key generator state was not stored explicitly
+  // but derived from the maximum numeric key present in existing
+  // data. This violates the spec as the data may be cleared but the
+  // key generator state must be preserved.
+  // TODO(jsbell): Fix this for all stores on database open?
+  const std::vector<char> start_key =
+      ObjectStoreDataKey::Encode(database_id, object_store_id, MinIDBKey());
+  const std::vector<char> stop_key =
+      ObjectStoreDataKey::Encode(database_id, object_store_id, MaxIDBKey());
+
+  scoped_ptr<LevelDBIterator> it = leveldb_transaction->CreateIterator();
+  int64 max_numeric_key = 0;
+
+  for (it->Seek(LevelDBSlice(start_key));
+       it->IsValid() && CompareKeys(it->Key(), LevelDBSlice(stop_key)) < 0;
+       it->Next()) {
+    const char* p = it->Key().begin();
+    const char* limit = it->Key().end();
+
+    ObjectStoreDataKey data_key;
+    p = ObjectStoreDataKey::Decode(p, limit, &data_key);
+    DCHECK(p);
+
+    scoped_ptr<IndexedDBKey> user_key = data_key.user_key();
+    if (user_key->type() == WebKit::WebIDBKey::NumberType) {
+      int64 n = static_cast<int64>(user_key->number());
+      if (n > max_numeric_key)
+        max_numeric_key = n;
+    }
+  }
+
+  key_generator_current_number = max_numeric_key + 1;
   return true;
 }
 
 bool IndexedDBBackingStore::MaybeUpdateKeyGeneratorCurrentNumber(
     IndexedDBBackingStore::Transaction* transaction,
     int64 database_id,
     int64 object_store_id,
     int64 new_number,
     bool check_current) {
   if (!KeyPrefix::ValidIds(database_id, object_store_id))
@@ skipping 30 matching lines @@
     RecordIdentifier* found_record_identifier,
     bool& found) {
   IDB_TRACE("IndexedDBBackingStore::key_exists_in_object_store");
   if (!KeyPrefix::ValidIds(database_id, object_store_id))
     return false;
   found = false;
   LevelDBTransaction* leveldb_transaction =
       IndexedDBBackingStore::Transaction::LevelDBTransactionFrom(transaction);
   const std::vector<char> leveldb_key =
       ObjectStoreDataKey::Encode(database_id, object_store_id, key);
-  std::vector<char> data;
+  std::string data;
 
-  bool ok = leveldb_transaction->Get(LevelDBSlice(leveldb_key), data, found);
+  bool ok = leveldb_transaction->Get(LevelDBSlice(leveldb_key), &data, found);
   if (!ok) {
     INTERNAL_READ_ERROR(KEY_EXISTS_IN_OBJECT_STORE);
     return false;
   }
   if (!found)
     return true;
   if (!data.size()) {
     INTERNAL_READ_ERROR(KEY_EXISTS_IN_OBJECT_STORE);
     return false;
   }
 
   int64 version;
-  if (DecodeVarInt(&*data.begin(), &*data.rbegin() + 1, version) == 0)
+  StringPiece slice(data);
+  if (!DecodeVarInt(&slice, &version))
     return false;
 
-  found_record_identifier->Reset(EncodeIDBKey(key), version);
+  std::vector<char> encoded_key;
+  EncodeIDBKey(key, &encoded_key);
+  found_record_identifier->Reset(encoded_key, version);
   return true;
 }
 
 static bool CheckIndexAndMetaDataKey(const LevelDBIterator* it,
                                      const std::vector<char>& stop_key,
                                      int64 index_id,
                                      unsigned char meta_data_type) {
   if (!it->IsValid() || CompareKeys(it->Key(), LevelDBSlice(stop_key)) >= 0)
     return false;
 
@@ skipping 39 matching lines @@
       INTERNAL_CONSISTENCY_ERROR(GET_INDEXES);
       // Possible stale metadata due to http://webkit.org/b/85557 but don't fail
       // the load.
       it->Next();
       continue;
     }
 
     // TODO(jsbell): Do this by direct key lookup rather than iteration, to
     // simplify.
     int64 index_id = meta_data_key.IndexId();
-    string16 index_name = DecodeString(it->Value().begin(), it->Value().end());
+    string16 index_name;
+    {
+      StringPiece slice(it->Value());
+      if (!DecodeString(&slice, &index_name) || !slice.empty())
+        INTERNAL_CONSISTENCY_ERROR(GET_INDEXES);
+    }
 
     it->Next();  // unique flag
     if (!CheckIndexAndMetaDataKey(
             it.get(), stop_key, index_id, IndexMetaDataKey::UNIQUE)) {
       INTERNAL_CONSISTENCY_ERROR(GET_INDEXES);
       break;
     }
-    bool index_unique = DecodeBool(it->Value().begin(), it->Value().end());
+    bool index_unique;
+    {
+      StringPiece slice(it->Value());
+      if (!DecodeBool(&slice, &index_unique) || !slice.empty())
+        INTERNAL_CONSISTENCY_ERROR(GET_INDEXES);
+    }
 
     it->Next();  // key_path
     if (!CheckIndexAndMetaDataKey(
             it.get(), stop_key, index_id, IndexMetaDataKey::KEY_PATH)) {
       INTERNAL_CONSISTENCY_ERROR(GET_INDEXES);
       break;
     }
-    IndexedDBKeyPath key_path =
-        DecodeIDBKeyPath(it->Value().begin(), it->Value().end());
+    IndexedDBKeyPath key_path;
+    {
+      StringPiece slice(it->Value());
+      if (!DecodeIDBKeyPath(&slice, &key_path) || !slice.empty())
+        INTERNAL_CONSISTENCY_ERROR(GET_INDEXES);
+    }
 
     it->Next();  // [optional] multi_entry flag
     bool index_multi_entry = false;
     if (CheckIndexAndMetaDataKey(
             it.get(), stop_key, index_id, IndexMetaDataKey::MULTI_ENTRY)) {
-      index_multi_entry = DecodeBool(it->Value().begin(), it->Value().end());
+      StringPiece slice(it->Value());
+      if (!DecodeBool(&slice, &index_multi_entry) || !slice.empty())
+        INTERNAL_CONSISTENCY_ERROR(GET_INDEXES);
+
       it->Next();
     }
 
     (*indexes)[index_id] = IndexedDBIndexMetadata(
         index_name, index_id, key_path, index_unique, index_multi_entry);
   }
   return true;
 }
 
 WARN_UNUSED_RESULT static bool SetMaxIndexId(LevelDBTransaction* transaction,
@@ skipping 88 matching lines @@
     int64 index_id,
     const IndexedDBKey& key,
     const RecordIdentifier& record_identifier) {
   IDB_TRACE("IndexedDBBackingStore::put_index_data_for_record");
   DCHECK(key.IsValid());
   if (!KeyPrefix::ValidIds(database_id, object_store_id, index_id))
     return false;
 
   LevelDBTransaction* leveldb_transaction =
       IndexedDBBackingStore::Transaction::LevelDBTransactionFrom(transaction);
+
+  std::vector<char> encoded_key;
+  EncodeIDBKey(key, &encoded_key);
+
   const std::vector<char> index_data_key =
       IndexDataKey::Encode(database_id,
                            object_store_id,
                            index_id,
-                           EncodeIDBKey(key),
+                           encoded_key,
                            record_identifier.primary_key());
 
   std::vector<char> data;
   EncodeVarInt(record_identifier.version(), &data);
   const std::vector<char>& primary_key = record_identifier.primary_key();
   data.insert(data.end(), primary_key.begin(), primary_key.end());
 
   leveldb_transaction->Put(LevelDBSlice(index_data_key), data);
   return true;
 }
@@ skipping 27 matching lines @@
 }
 
 static bool VersionExists(LevelDBTransaction* transaction,
                           int64 database_id,
                           int64 object_store_id,
                           int64 version,
                           const std::vector<char>& encoded_primary_key,
                           bool& exists) {
   const std::vector<char> key =
       ExistsEntryKey::Encode(database_id, object_store_id, encoded_primary_key);
-  std::vector<char> data;
+  std::string data;
 
-  bool ok = transaction->Get(LevelDBSlice(key), data, exists);
+  bool ok = transaction->Get(LevelDBSlice(key), &data, exists);
   if (!ok) {
     INTERNAL_READ_ERROR(VERSION_EXISTS);
     return false;
   }
   if (!exists)
     return true;
 
-  exists = (DecodeInt(data.begin(), data.end()) == version);
+  StringPiece slice(data);
+  int64 decoded;
+  if (!DecodeInt(&slice, &decoded) || !slice.empty())
+    return false;
+  exists = (decoded == version);
   return true;
 }
 
 bool IndexedDBBackingStore::FindKeyInIndex(
     IndexedDBBackingStore::Transaction* transaction,
     int64 database_id,
     int64 object_store_id,
     int64 index_id,
     const IndexedDBKey& key,
     std::vector<char>& found_encoded_primary_key,
@@ skipping 10 matching lines @@
       IndexDataKey::Encode(database_id, object_store_id, index_id, key);
   scoped_ptr<LevelDBIterator> it = leveldb_transaction->CreateIterator();
   it->Seek(LevelDBSlice(leveldb_key));
 
   for (;;) {
     if (!it->IsValid())
       return true;
     if (CompareIndexKeys(it->Key(), LevelDBSlice(leveldb_key)) > 0)
       return true;
 
+    StringPiece slice(it->Value());
+
     int64 version;
-    const char* p =
-        DecodeVarInt(it->Value().begin(), it->Value().end(), version);
-    if (!p) {
+    if (!DecodeVarInt(&slice, &version)) {
       INTERNAL_READ_ERROR(FIND_KEY_IN_INDEX);
       return false;
     }
     found_encoded_primary_key.insert(
-        found_encoded_primary_key.end(), p, it->Value().end());
+        found_encoded_primary_key.end(), slice.begin(), slice.end());
 
     bool exists = false;
     bool ok = VersionExists(leveldb_transaction,
                             database_id,
                             object_store_id,
                             version,
                             found_encoded_primary_key,
                             exists);
     if (!ok)
       return false;
@@ skipping 32 matching lines @@
     INTERNAL_READ_ERROR(GET_PRIMARY_KEY_VIA_INDEX);
     return false;
   }
   if (!found)
     return true;
   if (!found_encoded_primary_key.size()) {
     INTERNAL_READ_ERROR(GET_PRIMARY_KEY_VIA_INDEX);
     return false;
   }
 
-  DecodeIDBKey(&*found_encoded_primary_key.begin(),
-               &*found_encoded_primary_key.rbegin() + 1,
-               primary_key);
-  return true;
+  StringPiece slice(&*found_encoded_primary_key.begin(),
+                    found_encoded_primary_key.size());
+  return DecodeIDBKey(&slice, primary_key) && slice.empty();
 }
 
 bool IndexedDBBackingStore::KeyExistsInIndex(
     IndexedDBBackingStore::Transaction* transaction,
     int64 database_id,
     int64 object_store_id,
     int64 index_id,
     const IndexedDBKey& index_key,
     scoped_ptr<IndexedDBKey>* found_primary_key,
     bool& exists) {
@@ skipping 14 matching lines @@
     INTERNAL_READ_ERROR(KEY_EXISTS_IN_INDEX);
     return false;
   }
   if (!exists)
     return true;
   if (!found_encoded_primary_key.size()) {
     INTERNAL_READ_ERROR(KEY_EXISTS_IN_INDEX);
     return false;
   }
 
-  DecodeIDBKey(&*found_encoded_primary_key.begin(),
-               &*found_encoded_primary_key.rbegin() + 1,
-               found_primary_key);
-  return true;
+  StringPiece slice(&*found_encoded_primary_key.begin(),
+                    found_encoded_primary_key.size());
+  return DecodeIDBKey(&slice, found_primary_key) && slice.empty();

[alecflett, 2013/06/03 21:22:53]

This pattern makes me think we might also want something that takes a 'const
StringPiece&' so we don't have to make so many explicit temporaries that we'll
never use.

[jsbell, 2013/06/03 21:37:09]

Here there is the && slice.empty() at the end that is confirming that the decode
consumed the entire slice, so it's explicitly non-const.

The initialization using &* is gross. If we can convert this from
std::vector<char> to std::string then that initialization is cleaner.

(Fundamentally, the StringPiece usage here is like a stream. We may actually
want to make it a proper stream at some point, but getting consistent is an
incremental step.)

 }
 
 IndexedDBBackingStore::Cursor::Cursor(
     const IndexedDBBackingStore::Cursor* other)
     : transaction_(other->transaction_),
       cursor_options_(other->cursor_options_),
       current_key_(new IndexedDBKey(*other->current_key_)) {
   if (other->iterator_) {
     iterator_ = transaction_->CreateIterator();
 
@@ skipping 208 matching lines @@
   key_position = ObjectStoreDataKey::Decode(
       key_position, key_limit, &object_store_data_key);
   if (!key_position) {
     INTERNAL_READ_ERROR(LOAD_CURRENT_ROW);
     return false;
   }
 
   current_key_ = object_store_data_key.user_key();
 
   int64 version;
-  const char* value_position = DecodeVarInt(
-      iterator_->Value().begin(), iterator_->Value().end(), version);
-  if (!value_position) {
+  StringPiece slice(iterator_->Value());
+  if (!DecodeVarInt(&slice, &version)) {
     INTERNAL_READ_ERROR(LOAD_CURRENT_ROW);
     return false;
   }
 
   // TODO(jsbell): This re-encodes what was just decoded; try and optimize.
-  record_identifier_.Reset(EncodeIDBKey(*current_key_), version);
+  std::vector<char> encoded_key;
+  EncodeIDBKey(*current_key_, &encoded_key);
+  record_identifier_.Reset(encoded_key, version);
 
   return true;
 }
 
 class ObjectStoreCursorImpl : public IndexedDBBackingStore::Cursor {
  public:
   ObjectStoreCursorImpl(
       LevelDBTransaction* transaction,
       const IndexedDBBackingStore::Cursor::CursorOptions& cursor_options)
       : IndexedDBBackingStore::Cursor(transaction, cursor_options) {}
@@ skipping 26 matching lines @@
   key_position = ObjectStoreDataKey::Decode(
       key_position, key_limit, &object_store_data_key);
   if (!key_position) {
     INTERNAL_READ_ERROR(LOAD_CURRENT_ROW);
     return false;
   }
 
   current_key_ = object_store_data_key.user_key();
 
   int64 version;
-  const char* value_position = DecodeVarInt(
-      iterator_->Value().begin(), iterator_->Value().end(), version);
-  if (!value_position) {
+  StringPiece slice(iterator_->Value());
+  if (!DecodeVarInt(&slice, &version)) {
     INTERNAL_READ_ERROR(LOAD_CURRENT_ROW);
     return false;
   }
 
   // TODO(jsbell): This re-encodes what was just decoded; try and optimize.
-  record_identifier_.Reset(EncodeIDBKey(*current_key_), version);
+  std::vector<char> encoded_key;
+  EncodeIDBKey(*current_key_, &encoded_key);
+  record_identifier_.Reset(encoded_key, version);
 
   std::vector<char> value;
-  value.insert(value.end(), value_position, iterator_->Value().end());
+  value.insert(value.end(), slice.begin(), slice.end());
   current_value_.swap(value);
   return true;
 }
 
 class IndexKeyCursorImpl : public IndexedDBBackingStore::Cursor {
  public:
   IndexKeyCursorImpl(
       LevelDBTransaction* transaction,
       const IndexedDBBackingStore::Cursor::CursorOptions& cursor_options)
       : IndexedDBBackingStore::Cursor(transaction, cursor_options) {}
@@ skipping 34 matching lines @@
 bool IndexKeyCursorImpl::LoadCurrentRow() {
   const char* key_position = iterator_->Key().begin();
   const char* key_limit = iterator_->Key().end();
 
   IndexDataKey index_data_key;
   key_position = IndexDataKey::Decode(key_position, key_limit, &index_data_key);
 
   current_key_ = index_data_key.user_key();
   DCHECK(current_key_);
 
+  StringPiece slice(iterator_->Value());
   int64 index_data_version;
-  const char* value_position = DecodeVarInt(
-      iterator_->Value().begin(), iterator_->Value().end(), index_data_version);
-  if (!value_position) {
+  if (!DecodeVarInt(&slice, &index_data_version)) {
     INTERNAL_READ_ERROR(LOAD_CURRENT_ROW);
     return false;
   }
 
-  value_position =
-      DecodeIDBKey(value_position, iterator_->Value().end(), &primary_key_);
-  if (!value_position) {
+  if (!DecodeIDBKey(&slice, &primary_key_) || !slice.empty()) {
     INTERNAL_READ_ERROR(LOAD_CURRENT_ROW);
     return false;
   }
 
   std::vector<char> primary_leveldb_key =
       ObjectStoreDataKey::Encode(index_data_key.DatabaseId(),
                                  index_data_key.ObjectStoreId(),
                                  *primary_key_);
 
-  std::vector<char> result;
+  std::string result;
   bool found = false;
-  bool ok = transaction_->Get(LevelDBSlice(primary_leveldb_key), result, found);
+  bool ok =
+      transaction_->Get(LevelDBSlice(primary_leveldb_key), &result, found);
   if (!ok) {
     INTERNAL_READ_ERROR(LOAD_CURRENT_ROW);
     return false;
   }
   if (!found) {
     transaction_->Remove(iterator_->Key());
     return false;
   }
   if (!result.size()) {
     INTERNAL_READ_ERROR(LOAD_CURRENT_ROW);
     return false;
   }
 
   int64 object_store_data_version;
-  const char* t = DecodeVarInt(
-      &*result.begin(), &*result.rbegin() + 1, object_store_data_version);
-  if (!t) {
+  slice = StringPiece(result);
+  if (!DecodeVarInt(&slice, &object_store_data_version)) {
     INTERNAL_READ_ERROR(LOAD_CURRENT_ROW);
     return false;
   }
 
   if (object_store_data_version != index_data_version) {
     transaction_->Remove(iterator_->Key());
     return false;
   }
 
   return true;
@@ skipping 43 matching lines @@
 bool IndexCursorImpl::LoadCurrentRow() {
   const char* key_position = iterator_->Key().begin();
   const char* key_limit = iterator_->Key().end();
 
   IndexDataKey index_data_key;
   key_position = IndexDataKey::Decode(key_position, key_limit, &index_data_key);
 
   current_key_ = index_data_key.user_key();
   DCHECK(current_key_);
 
-  const char* value_position = iterator_->Value().begin();
-  const char* value_limit = iterator_->Value().end();
-
+  StringPiece slice(iterator_->Value());
   int64 index_data_version;
-  value_position =
-      DecodeVarInt(value_position, value_limit, index_data_version);
-  if (!value_position) {
+  if (!DecodeVarInt(&slice, &index_data_version)) {
     INTERNAL_READ_ERROR(LOAD_CURRENT_ROW);
     return false;
   }
-  value_position = DecodeIDBKey(value_position, value_limit, &primary_key_);
-  if (!value_position) {
+  if (!DecodeIDBKey(&slice, &primary_key_)) {
     INTERNAL_READ_ERROR(LOAD_CURRENT_ROW);
     return false;
   }
 
   primary_leveldb_key_ =
       ObjectStoreDataKey::Encode(index_data_key.DatabaseId(),
                                  index_data_key.ObjectStoreId(),
                                  *primary_key_);
 
-  std::vector<char> result;
+  std::string result;
   bool found = false;
   bool ok =
-      transaction_->Get(LevelDBSlice(primary_leveldb_key_), result, found);
+      transaction_->Get(LevelDBSlice(primary_leveldb_key_), &result, found);
   if (!ok) {
     INTERNAL_READ_ERROR(LOAD_CURRENT_ROW);
     return false;
   }
   if (!found) {
     transaction_->Remove(iterator_->Key());
     return false;
   }
   if (!result.size()) {
     INTERNAL_READ_ERROR(LOAD_CURRENT_ROW);
     return false;
   }
 
   int64 object_store_data_version;
-  value_position = DecodeVarInt(
-      &*result.begin(), &*result.rbegin() + 1, object_store_data_version);
-  if (!value_position) {
+  slice = StringPiece(result);
+  if (!DecodeVarInt(&slice, &object_store_data_version)) {
     INTERNAL_READ_ERROR(LOAD_CURRENT_ROW);
     return false;
   }
 
   if (object_store_data_version != index_data_version) {
     transaction_->Remove(iterator_->Key());
     return false;
   }
 
-  // TODO(jsbell): Make value_position an iterator.
-  std::vector<char> value;
-  value.insert(value.end(),
-               value_position,
-               static_cast<const char*>(&*result.rbegin()) + 1);
-  current_value_.swap(value);
+  current_value_.clear();
+  current_value_.insert(current_value_.end(), slice.begin(), slice.end());
   return true;
 }
 
 bool ObjectStoreCursorOptions(
     LevelDBTransaction* transaction,
     int64 database_id,
     int64 object_store_id,
     const IndexedDBKeyRange& range,
     indexed_db::CursorDirection direction,
     IndexedDBBackingStore::Cursor::CursorOptions& cursor_options) {
@@ skipping 259 matching lines @@
 }
 
 void IndexedDBBackingStore::Transaction::Rollback() {
   IDB_TRACE("IndexedDBBackingStore::Transaction::rollback");
   DCHECK(transaction_);
   transaction_->Rollback();
   transaction_ = NULL;
 }
 
 }  // namespace content