Indexed DB: Simplify cursor iteration logic

Indexed DB: Simplify cursor iteration logic

The internal logic for cursor iteration in Continue()
needs to handle four cases: "next", "nextunique", "prev",
"prevunique". This was handled in a single function with a
single loop and hairy `if (forwards) {} else {}` and `if
(unique) {}` clauses, which made understanding and
extending it difficult.

This CL splits the function into two cases: ContinueNext()
for "next"/"nextunique" cursors and ContinuePrevious() for
"prev"/"prevunique" cursors, which greatly simplifies the
logic for each.

The "prevunique" case is particularly complex, since the
spec requires that the first duplicate (in index order) is
returned rather than the last (the one encountered first
when iterating backwards). This was previously done by
iterating further then reversing, all within the same
loop. Now, this case is handled by a subsequent loop.

BUG=497454
R=cmumford@chromium.org

Committed: https://crrev.com/1f54336fed5113fc440658d2a8465ae7a4d9a02c
Cr-Commit-Position: refs/heads/master@{#333780}

[jsbell, 2015-06-09 15:47:33]

cmumford@ - please take a look?

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

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3281: bool
find_first_duplicate = false;
Renamed from "flipped"

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3381: 
I removed two clauses here from when we were pairing on the code:

* "If seeking to a key (or key and primary key), continue until found."
* ""Unique" cursors should continue seeking until a new key value is seen."

To get to the point where we changed direction, both of those constraints must
have been satisfied already when doing the initial iteration.

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

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.h:348: // being symmetric
with PrevNoDuplicate.
I meant this to say "NextNoDuplicate"

[jsbell, 2015-06-09 15:53:13]

jsbell@chromium.org changed reviewers:
+ cmumford@chromium.org

[jsbell, 2015-06-09 15:53:36]

cmumford@ - please take a look? (now actually added you as a reviewer)

[cmumford, 2015-06-09 17:06:24]

lgtm

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

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3207: // Cursor may be at
the next value already; don't advance it again below.
Is it a maybe, or is it for sure at the next value?

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3363: if (!s->ok())
What about:

if (!s->ok() || !iterator_->IsValid() || IsPastBounds())

[jsbell, 2015-06-09 18:52:19]

A few "notes to self" reviewing the code.

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

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3198: if (next_state ==
SEEK && key) {
It should never be the case that key is specified if next_state is READY -
DCHECK?

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3200: if (primary_key)
Clearer as ternary (?:) operation?

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3241: // If seeking to a
key (or key and primary key), continue until found.
Due to the Seek() optimization above, this block should never be used.

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3249: 
Add a comment here:

// Cursor is now positioned at a non-stale record in range.

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3328: 
Add comment here:

// Cursor is now positioned at a non-stale record in range

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3331: // We should never
be able to walk forward all the way
This comment/DCHECK no longer make sense since direction is never reversed in
this loop.

Instead, add a comment above:

// If entry is a duplicate, keep going. Although the cursor should be
// positioned at the first duplicate, new duplicates may have been
// inserted since the cursor was last iterated, and should be skipped
// to maintain "unique" iteration.

[jsbell, 2015-06-10 17:51:58]

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

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3198: if (next_state ==
SEEK && key) {
On 2015/06/09 18:52:19, jsbell wrote:
> It should never be the case that key is specified if next_state is READY -
> DCHECK?

Done.

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3200: if (primary_key)
On 2015/06/09 18:52:19, jsbell wrote:
> Clearer as ternary (?:) operation?

Done.

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3207: // Cursor may be at
the next value already; don't advance it again below.
On 2015/06/09 17:06:24, cmumford wrote:
> Is it a maybe, or is it for sure at the next value?

Done.

(It may be positioned at a stale entry and still need to be further iterated,
but I simplified the comment)

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3241: // If seeking to a
key (or key and primary key), continue until found.
On 2015/06/09 18:52:19, jsbell wrote:
> Due to the Seek() optimization above, this block should never be used.

Added comment. Will run tests w/ DCHECK and remove if possible.

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3249: 
On 2015/06/09 18:52:19, jsbell wrote:
> Add a comment here:
> 
> // Cursor is now positioned at a non-stale record in range.

Done.

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3328: 
On 2015/06/09 18:52:19, jsbell wrote:
> Add comment here:
> 
> // Cursor is now positioned at a non-stale record in range

Done.

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3331: // We should never
be able to walk forward all the way
On 2015/06/09 18:52:19, jsbell wrote:
> This comment/DCHECK no longer make sense since direction is never reversed in
> this loop.
> 
> Instead, add a comment above:
> 
> // If entry is a duplicate, keep going. Although the cursor should be
> // positioned at the first duplicate, new duplicates may have been
> // inserted since the cursor was last iterated, and should be skipped
> // to maintain "unique" iteration.

Done.

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.cc:3363: if (!s->ok())
On 2015/06/09 17:06:24, cmumford wrote:
> What about:
> 
> if (!s->ok() || !iterator_->IsValid() || IsPastBounds())

Done.

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

https://codereview.chromium.org/1170833004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_backing_store.h:348: // being symmetric
with PrevNoDuplicate.
On 2015/06/09 15:47:32, jsbell wrote:
> I meant this to say "NextNoDuplicate"

Done.

[jsbell, 2015-06-10 18:00:38]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2015-06-10 18:00:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
cmumford@chromium.org
Link to the patchset: https://codereview.chromium.org/1170833004/#ps20001
(title: "Review feedback and comments")

[commit-bot: I haz the power, 2015-06-10 18:01:31]

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

[commit-bot: I haz the power, 2015-06-10 20:09:18]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-10 20:10:10]

Patchset 2 (id:??) landed as
https://crrev.com/1f54336fed5113fc440658d2a8465ae7a4d9a02c
Cr-Commit-Position: refs/heads/master@{#333780}