Return number of values deleted by IDBObjectStore.delete(range)

content/browser/indexed_db/leveldb/leveldb_transaction.cc

Index: content/browser/indexed_db/leveldb/leveldb_transaction.cc
diff --git a/content/browser/indexed_db/leveldb/leveldb_transaction.cc b/content/browser/indexed_db/leveldb/leveldb_transaction.cc
index 2abd7e6033d7cd457d591bb9bc1b8f623e308648..675547ef9c5dfd931304a24539c6dd2fb44d40d5 100644
--- a/content/browser/indexed_db/leveldb/leveldb_transaction.cc
+++ b/content/browser/indexed_db/leveldb/leveldb_transaction.cc
@@ -36,33 +36,34 @@ void LevelDBTransaction::Clear() {
 
 LevelDBTransaction::~LevelDBTransaction() { Clear(); }
 
-void LevelDBTransaction::Set(const StringPiece& key,
-                             std::string* value,
-                             bool deleted) {
+void LevelDBTransaction::Put(const StringPiece& key, std::string* value) {
   DCHECK(!finished_);
   DataType::iterator it = data_.find(key);
-
   if (it == data_.end()) {
     Record* record = new Record();
     record->key.assign(key.begin(), key.end() - key.begin());
     record->value.swap(*value);
-    record->deleted = deleted;
+    record->deleted = false;

[cmumford, 2016/05/19 17:07:16]

Redundant: the constructor initializes deleted to false already.

[palakj1, 2016/05/19 23:47:52]

code segment removed

     data_[record->key] = record;
     NotifyIterators();
     return;
   }
-
+  it->second->deleted = false;
   it->second->value.swap(*value);
-  it->second->deleted = deleted;
-}
-
-void LevelDBTransaction::Put(const StringPiece& key, std::string* value) {
-  Set(key, value, false);
 }
 
-void LevelDBTransaction::Remove(const StringPiece& key) {
+// if key non existent or already recorded as deleted : false

[cmumford, 2016/05/19 17:07:16]

API comment is better in the header only.

[palakj1, 2016/05/19 23:47:52]

Done

+bool LevelDBTransaction::Remove(const StringPiece& key) {
   std::string empty;
-  Set(key, &empty, true);
+  DCHECK(!finished_);

[cmumford, 2016/05/19 17:07:16]

This seems to be an overall more complicated approach, Remove() before was a
one-liner. How about this - because Set() is a private function why don't we
have it return a bool value which is when true means a prior data value was
deleted. So it would be

bool LevelDBTransaction::Set(...)
{
  ...
  bool replaced_deleted_value = it->second->deleted;
  it->second->deleted = deleted;
  return replaced_deleted_value;
}

And then Remove would become:

void LevelDBTransaction::Remove(const StringPiece& key) {
  std::string empty;
  return !Set(key, &empty, true);
}

To me that seems a little easier to follow. What do you all think?

[palakj1, 2016/05/19 23:47:52]

changes made as suggested by cmumford: 
call to Set by Remvoe and Put. Remove returns false iff record already deleted.

+  DataType::iterator it = data_.find(key);
+  if (it == data_.end())
+    return false;

[dmurph, 2016/05/19 01:01:39]

We still want to create our deleted record here. Copy the code from above, in
the it == data_.end(), but have deleted = true and return true.

[palakj1, 2016/05/19 23:47:52]

done.

+  if (it->second->deleted)
+    return false;
+  it->second->deleted = true;
+  it->second->value.swap(empty);
+  return true;
 }
 
 leveldb::Status LevelDBTransaction::Get(const StringPiece& key,