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

Modify leveldb::DeleteRangeBasic function to return number of values deleted by IDBObjectStore.delete(range) with modifications to other functions as needed.

BUG=611201
Committed: https://crrev.com/dbb2725b92683d709296e19fb107ceb483180729
Cr-Commit-Position: refs/heads/master@{#398974}

[palakj1, 2016-05-19 00:13:56]

palakj@google.com changed reviewers:
+ cmumford@chromium.org, dmurph@chromium.org, jsbell@chromium.org

[palakj1, 2016-05-19 00:13:56]

Modified DeleteRange to return #elements deleted.

Status: Patch is working completely. But I have a few queries about the working
of the code. 

With regards to repeated deletes of same key in a transaction, 
I modified the LevelDBTransaction::Remove to return false if the key has already
been recorded as deleted and introduced a check in DeleteRangeBasic
(indexed_db_backing_store.cc: 1377).

However, even without the modifications introduced, I am obtaining correct
results. I fail to spot any checks for records already marked as deleted. Is the
case 'deleting already deleted records' being handled somewhere?

Would there be any corner cases where the my modifications are helpful?

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

Thanks Palak!

A few nits, and I think I found a couple bugs. Josh & Chris, Palak was having
trouble replicating what we expect in the level db transaction. We were
expecting the following:

In the unittest, when we called delete twice on a range, the second time we
called 'Remove' on the LevelDBTransaction, it would find the record in data_
with 'deleted' already true. However it seemed to always not find the record. Do
you know what's supposed to be happening here? We might have also coded it
wrong.

Thanks!
Daniel

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:1370: if (delete_count) {
Remove brackets here and keep on same
line.(https://google.github.io/styleguide/cppguide.html#Conditionals)

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:2045: s =
DeleteRangeBasic(transaction->transaction(), start_key, stop_key, false,
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).

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:2855:
index_meta_data_end, true, NULL);
we use 'nullptr'
https://google.github.io/styleguide/cppguide.html#0_and_nullptr/NULL

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_backing_store_unittest.cc (right):

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store_unittest.cc:462:
EXPECT_TRUE(
Do you want to put a check in here somewhere that our deleted count is correct?

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/leveldb/leveldb_transaction.cc (right):

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:61: return false;
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.

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/leveldb/leveldb_transaction.h (right):

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/leveldb/leveldb_transaction.h:29: bool Remove(const
base::StringPiece& key);
Put a comment here:

"Returns true if the record was deleted, and false if it was already deleted."

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html
(right):

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:24: var
test = tests[0], test1 = tests[1];
This is confusing, don't you just want to use the 'tests' variable above (and
rename it 'test')?

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:30:
deleteRequestRepeat = store.delete(IDBKeyRange.bound(
So repeat the same request again, same arguments, right?

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:41:
assert_equals(delete_count, test1.expected, 'Records already been deleted');
This should be 0, correct?

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

I think description might read better if "Return number of values deleted by
IDBObjectStore.delete(range)"

dmurph@ Not sure I'm completely following your question, as data_ is a map of
records and you mention finding it. Maybe the bug you found in
LevelDBTransaction::Remove() was the cause?

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:1370: if (delete_count) {
On 2016/05/19 01:01:38, dmurph wrote:
> Remove brackets here and keep on same
> line.(https://google.github.io/styleguide/cppguide.html#Conditionals)

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

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:1805: NULL);
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.

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:2008: (*delete_count) =
0;  // set delete_count to 0 for empty ranges
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;

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:2045: s =
DeleteRangeBasic(transaction->transaction(), start_key, stop_key, false,
On 2016/05/19 01:01:38, dmurph wrote:
> 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).

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;

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_backing_store_unittest.cc (right):

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store_unittest.cc:462:
EXPECT_TRUE(
On 2016/05/19 01:01:38, dmurph wrote:
> Do you want to put a check in here somewhere that our deleted count is
correct?

+1 dmurph's suggestion - also for the other calls in this file. It's "low
hanging test fruit"

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:1430:
callbacks->OnSuccess(delete_count);
You now need to move the "IndexedDBDatabase::Delete" comment in:

./content/browser/indexed_db/indexed_db_callbacks.h

because the prior OnSuccess call returned no params, but it now returns one.

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/leveldb/leveldb_transaction.cc (right):

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:46: record->deleted =
false;
Redundant: the constructor initializes deleted to false already.

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:55: // if key non
existent or already recorded as deleted : false
API comment is better in the header only.

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:58:
DCHECK(!finished_);
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?

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html
(right):

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:11:
deleteRequest.onsuccess = t.step_func(function(){
space before {

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:13:
openRequest.onupgradeneeded= t.step_func(function(e) {
space before =

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:19:
trans = db.transaction('store', 'readwrite');
var before |trans| and |store| and |i| to prevent them from being scoped to
window.

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:26:
test.lower ,
no space before ,

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:59:
delete_count(tests[0], 'Delete Range : Overlapping Intervals');
no space before :

[jsbell, 2016-05-19 18:23:43]

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:2045: s =
DeleteRangeBasic(transaction->transaction(), start_key, stop_key, false,
On 2016/05/19 17:07:15, cmumford wrote:
> On 2016/05/19 01:01:38, dmurph wrote:
> > 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).
> 
> 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;

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:53]

Please look into the modified design.

A few queries:
1. Layout Tests (delete-range-count.html: 32): Using trans2.oncomplete handler
instead of deleteRequest1.onsuccess yields delete_count as undefined. could you
please explain why that's so?

2. indexed_db_backing_store.cc: 2003
Instead of the current implementation, should I create a new variable (size_t
exists_delete_count) and assign its final value to the passed parameter in
DeleteRange? (better readability)

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:1370: if (delete_count) {
On 2016/05/19 at 17:07:15, cmumford wrote:
> On 2016/05/19 01:01:38, dmurph wrote:
> > Remove brackets here and keep on same
> > line.(https://google.github.io/styleguide/cppguide.html#Conditionals)
> 
> My preference would be to make delete_count a required parameter - don't allow
it to be null.

Done. nullptr not permitted.

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:1805: NULL);
On 2016/05/19 at 17:07:15, cmumford wrote:
> 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.

Done

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:2008: (*delete_count) =
0;  // set delete_count to 0 for empty ranges
On 2016/05/19 at 17:07:15, cmumford wrote:
> 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;

Done

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:2045: s =
DeleteRangeBasic(transaction->transaction(), start_key, stop_key, false,
On 2016/05/19 at 18:23:42, jsbell wrote:
> On 2016/05/19 17:07:15, cmumford wrote:
> > On 2016/05/19 01:01:38, dmurph wrote:
> > > 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).
> > 
> > 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;
> 
> 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.)

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)

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:2855:
index_meta_data_end, true, NULL);
On 2016/05/19 at 01:01:38, dmurph wrote:
> we use 'nullptr'
> https://google.github.io/styleguide/cppguide.html#0_and_nullptr/NULL

value not null anymore

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_backing_store_unittest.cc (right):

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store_unittest.cc:462:
EXPECT_TRUE(
On 2016/05/19 at 17:07:15, cmumford wrote:
> On 2016/05/19 01:01:38, dmurph wrote:
> > Do you want to put a check in here somewhere that our deleted count is
correct?
> 
> +1 dmurph's suggestion - also for the other calls in this file. It's "low
hanging test fruit"

Added.

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/leveldb/leveldb_transaction.cc (right):

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:46: record->deleted =
false;
On 2016/05/19 at 17:07:16, cmumford wrote:
> Redundant: the constructor initializes deleted to false already.

code segment removed

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:55: // if key non
existent or already recorded as deleted : false
On 2016/05/19 at 17:07:16, cmumford wrote:
> API comment is better in the header only.

Done

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:58:
DCHECK(!finished_);
On 2016/05/19 at 17:07:16, cmumford wrote:
> 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?

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

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/leveldb/leveldb_transaction.cc:61: return false;
On 2016/05/19 at 01:01:39, dmurph wrote:
> 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.

done.

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/leveldb/leveldb_transaction.h (right):

https://codereview.chromium.org/1996443003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/leveldb/leveldb_transaction.h:29: bool Remove(const
base::StringPiece& key);
On 2016/05/19 at 01:01:39, dmurph wrote:
> Put a comment here:
> 
> "Returns true if the record was deleted, and false if it was already deleted."

Done in header file

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html
(right):

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:11:
deleteRequest.onsuccess = t.step_func(function(){
On 2016/05/19 at 17:07:16, cmumford wrote:
> space before {

Done

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:13:
openRequest.onupgradeneeded= t.step_func(function(e) {
On 2016/05/19 at 17:07:16, cmumford wrote:
> space before =

Done

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:19:
trans = db.transaction('store', 'readwrite');
On 2016/05/19 at 17:07:16, cmumford wrote:
> var before |trans| and |store| and |i| to prevent them from being scoped to
window.

Done

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:24: var
test = tests[0], test1 = tests[1];
On 2016/05/19 at 01:01:39, dmurph wrote:
> This is confusing, don't you just want to use the 'tests' variable above (and
rename it 'test')?

tests[0] and tests[1] used here might be different. Elaborated below.

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:26:
test.lower ,
On 2016/05/19 at 17:07:16, cmumford wrote:
> no space before ,

Done

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:30:
deleteRequestRepeat = store.delete(IDBKeyRange.bound(
On 2016/05/19 at 01:01:39, dmurph wrote:
> So repeat the same request again, same arguments, right?

Resolved below

https://codereview.chromium.org/1996443003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:41:
assert_equals(delete_count, test1.expected, 'Records already been deleted');
On 2016/05/19 at 01:01:39, dmurph wrote:
> This should be 0, correct?

One of my tests have overlapping intervals test = [3, 4] and test1 = [1, 2, 3,
4, 5]. I'll rename the variables for clarity.

[jsbell, 2016-05-20 00:36:12]

https://codereview.chromium.org/1996443003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html
(right):

https://codereview.chromium.org/1996443003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:20: var
store = trans1.objectStore('store');
Just to simplify this a bit, I'd do the put()s in onupgradeneeded so one of the
transactions is no longer needed here in onsuccess.

https://codereview.chromium.org/1996443003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:31:
test.upperOpen));
test.first.upperOpen?

https://codereview.chromium.org/1996443003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:57: {
first: { lower: 3, upper: 4, lowerOpen: false, upperOpen: false, expected: 2},
You can leave out falsy values like lowerOpen: false here, and just let
undefined turn into false.

[dmurph, 2016-05-20 18:41:15]

To answer your question #2, the transaction oncomplete handler's event doesn't
have the same target, and only the delete's 'request' will have the deletion
result for us.

https://codereview.chromium.org/1996443003/diff/40001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/1996443003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:1370:
DCHECK(delete_count);
Can you move this up 2 lines? We usually have our input sanitation dchecks at
the very top of the method.

https://codereview.chromium.org/1996443003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:2003: size_t*
exists_delete_count) {
Regarding your comment, I'm fine with either way. (using this pointer below, or
creating a new size_t and assigning at the end).

https://codereview.chromium.org/1996443003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:2005:
DCHECK(exists_delete_count);
Can you move this up one line?

https://codereview.chromium.org/1996443003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:2007: size_t
data_delete_count = 0;
Can you put this right next to where we use it? (line 2043)

https://codereview.chromium.org/1996443003/diff/40001/content/browser/indexed...
File content/browser/indexed_db/leveldb/leveldb_transaction.h (right):

https://codereview.chromium.org/1996443003/diff/40001/content/browser/indexed...
content/browser/indexed_db/leveldb/leveldb_transaction.h:135: 
Can you document what this returns?

https://codereview.chromium.org/1996443003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html
(right):

https://codereview.chromium.org/1996443003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:1:
<!DOCTYPE html>
We could probably just remove the 'lowerOpen' and 'upperOpen' values, they just
add text and we can test this without specifying them.

[palakj1, 2016-05-20 18:46:44]

Description was changed from

==========
Reutrn number of elements deleted for DeleteRange


Remove signature changed


Modified delete range


delete range


unit test modified

BUG=611201
==========

to

==========
Modify leveldb::DeleteRangeBasic function to return number of values deleted by
IDBObjectStore.delete(range) with modifications to other functions as needed.

BUG=611201
==========

[palakj1, 2016-05-20 19:13:46]

Changes made as per comments.

https://codereview.chromium.org/1996443003/diff/40001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/1996443003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:1370:
DCHECK(delete_count);
On 2016/05/20 at 18:41:15, dmurph wrote:
> Can you move this up 2 lines? We usually have our input sanitation dchecks at
the very top of the method.

Done

https://codereview.chromium.org/1996443003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:2003: size_t*
exists_delete_count) {
On 2016/05/20 at 18:41:15, dmurph wrote:
> Regarding your comment, I'm fine with either way. (using this pointer below,
or creating a new size_t and assigning at the end).

Thanks. Will keep as it is then.

https://codereview.chromium.org/1996443003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:2005:
DCHECK(exists_delete_count);
On 2016/05/20 at 18:41:14, dmurph wrote:
> Can you move this up one line?

Done

https://codereview.chromium.org/1996443003/diff/40001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:2007: size_t
data_delete_count = 0;
On 2016/05/20 at 18:41:15, dmurph wrote:
> Can you put this right next to where we use it? (line 2043)

Done

https://codereview.chromium.org/1996443003/diff/40001/content/browser/indexed...
File content/browser/indexed_db/leveldb/leveldb_transaction.h (right):

https://codereview.chromium.org/1996443003/diff/40001/content/browser/indexed...
content/browser/indexed_db/leveldb/leveldb_transaction.h:135: 
On 2016/05/20 at 18:41:15, dmurph wrote:
> Can you document what this returns?

Done

https://codereview.chromium.org/1996443003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html
(right):

https://codereview.chromium.org/1996443003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:1:
<!DOCTYPE html>
On 2016/05/20 at 18:41:15, dmurph wrote:
> We could probably just remove the 'lowerOpen' and 'upperOpen' values, they
just add text and we can test this without specifying them.

Done

https://codereview.chromium.org/1996443003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:20: var
store = trans1.objectStore('store');
On 2016/05/20 at 00:36:12, jsbell wrote:
> Just to simplify this a bit, I'd do the put()s in onupgradeneeded so one of
the transactions is no longer needed here in onsuccess.

Done

https://codereview.chromium.org/1996443003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:31:
test.upperOpen));
On 2016/05/20 at 00:36:12, jsbell wrote:
> test.first.upperOpen?

code segment removed

https://codereview.chromium.org/1996443003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:57: {
first: { lower: 3, upper: 4, lowerOpen: false, upperOpen: false, expected: 2},
On 2016/05/20 at 00:36:12, jsbell wrote:
> You can leave out falsy values like lowerOpen: false here, and just let
undefined turn into false.

Done

[jsbell, 2016-05-20 19:34:51]

Since this is a behavior change, we should gate the on
RuntimeEnabled=IndexedDBExperimental (controlled by
--enable-experimental-web-platform-features) - we'll have to discuss exactly how
we want to do that before landing.

https://codereview.chromium.org/1996443003/diff/60001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_callbacks.h (right):

https://codereview.chromium.org/1996443003/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.h:104: //
IndexedDBDatabase::Delete
nit: DeleteRange

(the previous comment was incorrect)

https://codereview.chromium.org/1996443003/diff/60001/content/browser/indexed...
File content/browser/indexed_db/leveldb/leveldb_transaction.h (right):

https://codereview.chromium.org/1996443003/diff/60001/content/browser/indexed...
content/browser/indexed_db/leveldb/leveldb_transaction.h:31: // deleted.
already deleted or not present?

https://codereview.chromium.org/1996443003/diff/60001/content/browser/indexed...
content/browser/indexed_db/leveldb/leveldb_transaction.h:135: // Returns true if
the key was originally marked deleted, false otherwise
Nit: period at end of comment.

https://codereview.chromium.org/1996443003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html
(right):

https://codereview.chromium.org/1996443003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:2:
<title>IndexedDB: UpgradeNeeded Tests</title>
Correct the title

https://codereview.chromium.org/1996443003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:17:
store.put(i,i);
nit: space after comma

https://codereview.chromium.org/1996443003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:47:
(function() {
This closure doesn't really provide any benefit here.

https://codereview.chromium.org/1996443003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:49: {
first: { lower: 3, upper: 4, expected: 2},
These cases could just be inline, since we're never iterating over `tests`.

https://codereview.chromium.org/1996443003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:56:
delete_count(tests[0], 'Delete Range for Overlapping Intervals');
i.e. delete_count({first: {lower 3, ....

https://codereview.chromium.org/1996443003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:59: 
Nit: remove some of his extra whitespace

[palakj1, 2016-05-23 17:17:40]

Changes post jsbell comments. Please have a look.

https://codereview.chromium.org/1996443003/diff/60001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_callbacks.h (right):

https://codereview.chromium.org/1996443003/diff/60001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.h:104: //
IndexedDBDatabase::Delete
On 2016/05/20 at 19:34:51, jsbell wrote:
> nit: DeleteRange
> 
> (the previous comment was incorrect)

Done

https://codereview.chromium.org/1996443003/diff/60001/content/browser/indexed...
File content/browser/indexed_db/leveldb/leveldb_transaction.h (right):

https://codereview.chromium.org/1996443003/diff/60001/content/browser/indexed...
content/browser/indexed_db/leveldb/leveldb_transaction.h:31: // deleted.
On 2016/05/20 at 19:34:51, jsbell wrote:
> already deleted or not present?

Remove returns true if a new delete record is inserted or an existing record is
deleted.
If the record has already been marked as deleted, false is returned

https://codereview.chromium.org/1996443003/diff/60001/content/browser/indexed...
content/browser/indexed_db/leveldb/leveldb_transaction.h:135: // Returns true if
the key was originally marked deleted, false otherwise
On 2016/05/20 at 19:34:51, jsbell wrote:
> Nit: period at end of comment.

Done

https://codereview.chromium.org/1996443003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html
(right):

https://codereview.chromium.org/1996443003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:2:
<title>IndexedDB: UpgradeNeeded Tests</title>
On 2016/05/20 at 19:34:51, jsbell wrote:
> Correct the title

Sorry for this. Corrected

https://codereview.chromium.org/1996443003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:56:
delete_count(tests[0], 'Delete Range for Overlapping Intervals');
On 2016/05/20 at 19:34:51, jsbell wrote:
> i.e. delete_count({first: {lower 3, ....

Done

[dmurph, 2016-05-23 18:13:45]

This is looking good! Last step is to enable the feature behind a flag like so:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

Can you:
1. Add a member variable to the indexed_db_database.h class called
experimental_web_platform_features_enabled_
2. Use the call above in the constructor of IndexedDBDatabase
3. Check if that boolean is true to decide how to call the IndexedDbCallbacks
object in DeleteOperation.

Does that make sense?

https://codereview.chromium.org/1996443003/diff/80001/content/browser/indexed...
File content/browser/indexed_db/leveldb/leveldb_transaction.h (right):

https://codereview.chromium.org/1996443003/diff/80001/content/browser/indexed...
content/browser/indexed_db/leveldb/leveldb_transaction.h:30: // Returns false if
a record was originally marked deleted,
nitpick:
"Returns true if this operation performs a change, where the value wasn't
already deleted."

[palakj1, 2016-05-24 18:23:09]

On 2016/05/23 at 18:13:45, dmurph wrote:
> This is looking good! Last step is to enable the feature behind a flag like
so:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> 
> Can you:
> 1. Add a member variable to the indexed_db_database.h class called
experimental_web_platform_features_enabled_
> 2. Use the call above in the constructor of IndexedDBDatabase
> 3. Check if that boolean is true to decide how to call the IndexedDbCallbacks
object in DeleteOperation.
> 
> Does that make sense?
> 
>
https://codereview.chromium.org/1996443003/diff/80001/content/browser/indexed...
> File content/browser/indexed_db/leveldb/leveldb_transaction.h (right):
> 
>
https://codereview.chromium.org/1996443003/diff/80001/content/browser/indexed...
> content/browser/indexed_db/leveldb/leveldb_transaction.h:30: // Returns false
if a record was originally marked deleted,
> nitpick:
> "Returns true if this operation performs a change, where the value wasn't
already deleted."

Not really sure about this. Do you want me to have 2 functions for
DeleteRangeOperation : the original implementation and modifed one and call as
per the flag
                               Or
Use the same function (modified by me) and pass null value if flag disabled

[dmurph, 2016-05-24 18:49:19]

https://codereview.chromium.org/1996443003/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/1996443003/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:1431:
callbacks->OnSuccess(base::checked_cast<int64_t>(delete_count));
Put the check right here.

if (experiment_web_platform_features_enabled_) {
  callbacks->OnSuccess(base::checked_cast<int64_t>(delete_count));
} else {
  callbacks->OnSuccess();
}

[palakj1, 2016-05-26 18:04:28]

Experimental flag enabled

https://codereview.chromium.org/1996443003/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/1996443003/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_database.cc:1431:
callbacks->OnSuccess(base::checked_cast<int64_t>(delete_count));
On 2016/05/24 at 18:49:18, dmurph wrote:
> Put the check right here.
> 
> if (experiment_web_platform_features_enabled_) {
>   callbacks->OnSuccess(base::checked_cast<int64_t>(delete_count));
> } else {
>   callbacks->OnSuccess();
> }

Thanks. Had done the same thing.

[palakj1, 2016-05-26 18:20:11]

The CQ bit was checked by palakj@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-26 18:20:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996443003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996443003/100001

[dmurph, 2016-05-26 18:35:27]

lgtm with style nit

https://codereview.chromium.org/1996443003/diff/100001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/1996443003/diff/100001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database.cc:1436: if
(experimental_web_platform_features_enabled_)
Please add brackets to both here.

Our style guide is a little weird. Basically, removing braces is ok IFF
* the next statement fits on one line, and
* there is no 'else'

https://google.github.io/styleguide/cppguide.html#Conditionals

[palakj1, 2016-05-26 20:07:58]

On 2016/05/26 at 18:35:27, dmurph wrote:
> lgtm with style nit
> 
>
https://codereview.chromium.org/1996443003/diff/100001/content/browser/indexe...
> File content/browser/indexed_db/indexed_db_database.cc (right):
> 
>
https://codereview.chromium.org/1996443003/diff/100001/content/browser/indexe...
> content/browser/indexed_db/indexed_db_database.cc:1436: if
(experimental_web_platform_features_enabled_)
> Please add brackets to both here.
> 
> Our style guide is a little weird. Basically, removing braces is ok IFF
> * the next statement fits on one line, and
> * there is no 'else'
> 
> https://google.github.io/styleguide/cppguide.html#Conditionals

Done.

[commit-bot: I haz the power, 2016-05-26 20:08:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-26 20:08:28]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[palakj1, 2016-05-26 20:12:51]

Changes post dmurph's style comments.

[cmumford, 2016-05-26 21:01:58]

> * the next statement fits on one line, and
> * there is no 'else'
> 
> https://google.github.io/styleguide/cppguide.html#Conditionals

Are you sure? I read that as if one of the clauses (if or else) uses braces then
they all must. But I still think that:

if (foo)
  bar();
else
  baz();

is OK. Please correct me if I'm wrong.

[cmumford, 2016-05-26 21:02:11]

lgtm % nit.

https://codereview.chromium.org/1996443003/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html
(right):

https://codereview.chromium.org/1996443003/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:24:
deleteRequest1 = store.delete(IDBKeyRange.bound(
Nit: "var" before deleteRequest1.

[palakj1, 2016-05-26 21:13:14]

On 2016/05/26 at 21:02:11, cmumford wrote:
> lgtm % nit.
> 
>
https://codereview.chromium.org/1996443003/diff/120001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html
(right):
> 
>
https://codereview.chromium.org/1996443003/diff/120001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/storage/indexeddb/delete-range-count.html:24:
deleteRequest1 = store.delete(IDBKeyRange.bound(
> Nit: "var" before deleteRequest1.

done. Thanks for the observation.

[palakj1, 2016-05-26 21:13:52]

changes post cmumford's style suggestions.

[dmurph, 2016-05-26 21:24:42]

Ah, I guess the guide is more lenient here, true. Do what you like then.

On Thu, May 26, 2016 at 2:13 PM <palakj@google.com> wrote:

> changes post cmumford's style suggestions.
>
> https://codereview.chromium.org/1996443003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmurph, 2016-05-26 21:24:43]

Ah, I guess the guide is more lenient here, true. Do what you like then.

On Thu, May 26, 2016 at 2:13 PM <palakj@google.com> wrote:

> changes post cmumford's style suggestions.
>
> https://codereview.chromium.org/1996443003/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[palakj1, 2016-05-27 22:39:08]

The CQ bit was checked by palakj@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-05-27 22:39:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996443003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996443003/160001

[palakj1, 2016-05-27 22:39:33]

The CQ bit was unchecked by palakj@google.com

[palakj1, 2016-05-31 20:41:24]

The CQ bit was checked by palakj@google.com

[palakj1, 2016-05-31 20:41:24]

The patchset sent to the CQ was uploaded after l-g-t-m from dmurph@chromium.org,
cmumford@chromium.org
Link to the patchset: https://codereview.chromium.org/1996443003/#ps180001
(title: "Delete Range experimental flag enabled")

[commit-bot: I haz the power, 2016-05-31 20:42:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996443003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1996443003/180001

[commit-bot: I haz the power, 2016-05-31 22:14:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-31 22:14:52]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dmurph, 2016-06-02 01:29:25]

On 2016/05/31 at 22:14:52, commit-bot wrote:
> Try jobs failed on following builders:
>   linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

Just copy the 'actual' column here
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...

into the expected txt. I don't know if you need the png (you can delete those I
think).

[palakj1, 2016-06-02 18:43:24]

Expected files changed for FAIL cases which disagree with the spec change.

[palakj1, 2016-06-06 22:20:18]

The CQ bit was checked by palakj@google.com

[palakj1, 2016-06-06 22:20:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
cmumford@chromium.org, dmurph@chromium.org
Link to the patchset: https://codereview.chromium.org/1996443003/#ps200001
(title: "Expected files changed")

[commit-bot: I haz the power, 2016-06-06 22:20:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996443003/200001

[commit-bot: I haz the power, 2016-06-06 23:22:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-06 23:22:53]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[palakj1, 2016-06-07 23:56:42]

The CQ bit was checked by palakj@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-07 23:57:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996443003/240001

[commit-bot: I haz the power, 2016-06-08 01:55:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-08 01:55:49]

Dry run: This issue passed the CQ dry run.

[palakj1, 2016-06-09 17:26:30]

The CQ bit was checked by palakj@google.com

[palakj1, 2016-06-09 17:26:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
cmumford@chromium.org, dmurph@chromium.org
Link to the patchset: https://codereview.chromium.org/1996443003/#ps240001
(title: "Delete Range")

[commit-bot: I haz the power, 2016-06-09 17:27:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1996443003/240001

[commit-bot: I haz the power, 2016-06-09 19:01:44]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-06-09 19:02:05]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-09 19:03:41]

Description was changed from

==========
Modify leveldb::DeleteRangeBasic function to return number of values deleted by
IDBObjectStore.delete(range) with modifications to other functions as needed.

BUG=611201
==========

to

==========
Modify leveldb::DeleteRangeBasic function to return number of values deleted by
IDBObjectStore.delete(range) with modifications to other functions as needed.

BUG=611201

Committed: https://crrev.com/dbb2725b92683d709296e19fb107ceb483180729
Cr-Commit-Position: refs/heads/master@{#398974}
==========

[commit-bot: I haz the power, 2016-06-09 19:03:43]

Patchset 13 (id:??) landed as
https://crrev.com/dbb2725b92683d709296e19fb107ceb483180729
Cr-Commit-Position: refs/heads/master@{#398974}