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

content/browser/indexed_db/indexed_db_backing_store.cc

Index: content/browser/indexed_db/indexed_db_backing_store.cc
diff --git a/content/browser/indexed_db/indexed_db_backing_store.cc b/content/browser/indexed_db/indexed_db_backing_store.cc
index b33afc020c1bdfe08a258e8271d33bf2ac065ee5..47a802d55985e8567a97b3555143feb396b9f8bd 100644
--- a/content/browser/indexed_db/indexed_db_backing_store.cc
+++ b/content/browser/indexed_db/indexed_db_backing_store.cc
@@ -1363,14 +1363,23 @@ bool IndexedDBBackingStore::UpdateIDBDatabaseIntVersion(
 static leveldb::Status DeleteRangeBasic(LevelDBTransaction* transaction,
                                         const std::string& begin,
                                         const std::string& end,
-                                        bool upper_open) {
+                                        bool upper_open,
+                                        size_t* delete_count) {
   std::unique_ptr<LevelDBIterator> it = transaction->CreateIterator();
   leveldb::Status s;
+  if (delete_count) {

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

Remove brackets here and keep on same
line.(https://google.github.io/styleguide/cppguide.html#Conditionals)

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

My preference would be to make delete_count a required parameter - don't allow
it to be null.

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

Done. nullptr not permitted.

+    *delete_count = 0;
+  }
   for (s = it->Seek(begin); s.ok() && it->IsValid() &&
-                                (upper_open ? CompareKeys(it->Key(), end) < 0
-                                            : CompareKeys(it->Key(), end) <= 0);
-       s = it->Next())
-    transaction->Remove(it->Key());
+                            (upper_open ? CompareKeys(it->Key(), end) < 0
+                                        : CompareKeys(it->Key(), end) <= 0);
+       s = it->Next()) {
+    if (transaction->Remove(it->Key())) {
+      if (delete_count) {
+        (*delete_count)++;
+      }
+    }
+  }
   return s;
 }
 
@@ -1792,8 +1801,8 @@ leveldb::Status IndexedDBBackingStore::DeleteObjectStore(
   s = DeleteRangeBasic(
       leveldb_transaction,
       ObjectStoreMetaDataKey::Encode(database_id, object_store_id, 0),
-      ObjectStoreMetaDataKey::EncodeMaxKey(database_id, object_store_id),
-      true);
+      ObjectStoreMetaDataKey::EncodeMaxKey(database_id, object_store_id), true,
+      NULL);

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

At a minimum s/NULL/nullptr/g.

I still prefer to pass in a local size_t variable (which would be unused in this
function). I'm just not a big fan of optional parameters as they increase
failure paths.

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

Done

 
   if (s.ok()) {
     leveldb_transaction->Remove(
@@ -1802,16 +1811,16 @@ leveldb::Status IndexedDBBackingStore::DeleteObjectStore(
     s = DeleteRangeBasic(
         leveldb_transaction,
         IndexFreeListKey::Encode(database_id, object_store_id, 0),
-        IndexFreeListKey::EncodeMaxKey(database_id, object_store_id),
-        true);
+        IndexFreeListKey::EncodeMaxKey(database_id, object_store_id), true,
+        NULL);
   }
 
   if (s.ok()) {
     s = DeleteRangeBasic(
         leveldb_transaction,
         IndexMetaDataKey::Encode(database_id, object_store_id, 0, 0),
-        IndexMetaDataKey::EncodeMaxKey(database_id, object_store_id),
-        true);
+        IndexMetaDataKey::EncodeMaxKey(database_id, object_store_id), true,
+        NULL);
   }
 
   if (!s.ok()) {
@@ -1955,9 +1964,8 @@ leveldb::Status IndexedDBBackingStore::ClearObjectStore(
       KeyPrefix(database_id, object_store_id).Encode();
   const std::string stop_key =
       KeyPrefix(database_id, object_store_id + 1).Encode();
-
-  leveldb::Status s =
-      DeleteRangeBasic(transaction->transaction(), start_key, stop_key, true);
+  leveldb::Status s = DeleteRangeBasic(transaction->transaction(), start_key,
+                                       stop_key, true, NULL);
   if (!s.ok()) {
     INTERNAL_WRITE_ERROR(CLEAR_OBJECT_STORE);
     return s;
@@ -1993,8 +2001,11 @@ leveldb::Status IndexedDBBackingStore::DeleteRange(
     IndexedDBBackingStore::Transaction* transaction,
     int64_t database_id,
     int64_t object_store_id,
-    const IndexedDBKeyRange& key_range) {
+    const IndexedDBKeyRange& key_range,
+    size_t* delete_count) {
   leveldb::Status s;
+  if (delete_count)
+    (*delete_count) = 0;  // set delete_count to 0 for empty ranges

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

I think the reason to initialize delete_count is obvious so no need for comment.
Can also ditch the parenthesis do just have:

*delete_count = 0;

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

Done

   std::unique_ptr<IndexedDBBackingStore::Cursor> start_cursor =
       OpenObjectStoreCursor(transaction, database_id, object_store_id,
                             key_range, blink::WebIDBCursorDirectionNext, &s);
@@ -2002,7 +2013,6 @@ leveldb::Status IndexedDBBackingStore::DeleteRange(
     return s;
   if (!start_cursor)
     return leveldb::Status::OK();  // Empty range == delete success.
-
   std::unique_ptr<IndexedDBBackingStore::Cursor> end_cursor =
       OpenObjectStoreCursor(transaction, database_id, object_store_id,
                             key_range, blink::WebIDBCursorDirectionPrev, &s);
@@ -2013,7 +2023,6 @@ leveldb::Status IndexedDBBackingStore::DeleteRange(
     return leveldb::Status::OK();  // Empty range == delete success.
 
   BlobEntryKey start_blob_key, end_blob_key;
-
   std::string start_key = ObjectStoreDataKey::Encode(
       database_id, object_store_id, start_cursor->key());
   base::StringPiece start_key_piece(start_key);
@@ -2033,15 +2042,17 @@ leveldb::Status IndexedDBBackingStore::DeleteRange(
                          false);
   if (!s.ok())
     return s;
-  s = DeleteRangeBasic(transaction->transaction(), start_key, stop_key, false);
+  s = DeleteRangeBasic(transaction->transaction(), start_key, stop_key, false,

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

Since both of these calls are going to make delete_range=0, do you want to
create a new variable for the second one maybe? I don't think we want to add
them together, as that would double your number (is that right, josh/chris)?

IF that's so, let's just count the second one (existance table).

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

jsbell please confirm, but I believe they will both return the same count, and
should *not* be summed. I'd create a variable for the second call, something
like:

size_t ignore_delete_count;

[jsbell, 2016/05/19 18:23:42]

Alternately, call the first one data_count and the second one exists_entry_count
and DCHECK_EQ(data_count, exists_entry_count)

(https://chromium.googlesource.com/chromium/src/+/master/content/browser/index...
may be useful in understanding why we have two.)

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

Thanks for this bit of explanation. Changed as per jsbell's suggestion:
call the first one data_count and the second one exists_entry_count and
DCHECK_EQ(data_count, exists_entry_count)

+                       delete_count);
   if (!s.ok())
     return s;
   start_key =
       ExistsEntryKey::Encode(database_id, object_store_id, start_cursor->key());
   stop_key =
       ExistsEntryKey::Encode(database_id, object_store_id, end_cursor->key());
-  return DeleteRangeBasic(
-      transaction->transaction(), start_key, stop_key, false);
+
+  return DeleteRangeBasic(transaction->transaction(), start_key, stop_key,
+                          false, delete_count);
 }
 
 leveldb::Status IndexedDBBackingStore::GetKeyGeneratorCurrentNumber(
@@ -2839,16 +2850,17 @@ leveldb::Status IndexedDBBackingStore::DeleteIndex(
       IndexMetaDataKey::Encode(database_id, object_store_id, index_id, 0);
   const std::string index_meta_data_end =
       IndexMetaDataKey::EncodeMaxKey(database_id, object_store_id, index_id);
-  leveldb::Status s = DeleteRangeBasic(
-      leveldb_transaction, index_meta_data_start, index_meta_data_end, true);
+  leveldb::Status s =
+      DeleteRangeBasic(leveldb_transaction, index_meta_data_start,
+                       index_meta_data_end, true, NULL);

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

we use 'nullptr'
https://google.github.io/styleguide/cppguide.html#0_and_nullptr/NULL

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

value not null anymore

 
   if (s.ok()) {
     const std::string index_data_start =
         IndexDataKey::EncodeMinKey(database_id, object_store_id, index_id);
     const std::string index_data_end =
         IndexDataKey::EncodeMaxKey(database_id, object_store_id, index_id);
-    s = DeleteRangeBasic(
-        leveldb_transaction, index_data_start, index_data_end, true);
+    s = DeleteRangeBasic(leveldb_transaction, index_data_start, index_data_end,
+                         true, NULL);
   }
 
   if (!s.ok())