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

content/browser/indexed_db/leveldb/leveldb_transaction.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/leveldb/leveldb_transaction.h"
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram.h"
 #include "base/time/time.h"
@@ skipping 18 matching lines @@
 LevelDBTransaction::Record::~Record() {}
 
 void LevelDBTransaction::Clear() {
   for (const auto& it : data_)
     delete it.second;
   data_.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,
                                         std::string* value,
                                         bool* found) {
   *found = false;
   DCHECK(!finished_);
   std::string string_key(key.begin(), key.end() - key.begin());
   DataType::const_iterator it = data_.find(string_key);
 
@@ skipping 418 matching lines @@
 
   leveldb::Status s = db_->Write(*write_batch_);
   if (s.ok()) {
     finished_ = true;
     write_batch_->Clear();
   }
   return s;
 }
 
 }  // namespace content