Bug 83302 – IndexedDB: Refactor cursor iteration to remove duplicate code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alec Flett]

IndexedDB: Refactor cursor iteration to remove duplicate code

[Alec Flett]

Created attachment 135871 [details]
Patch

[Alec Flett]

dgrogan@ mind taking a look - this is in preparation for my first actual fix..

[Joshua Bell]

Comment on attachment 135871 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135871&action=review

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1018
> +    bool checkCurrentBoundaries() const;

Boundaries don't change, so is "Current" needed? Maybe rename to "isInBounds"? But it also only checks the end boundary - can that be captured in the name?

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1042
> +        // in the very first seek, we're already in the right

I don't think this comment is necessary; the behavior it's describing is implied by the code.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1060
> +        // check if we've advanced past the range

Unnecessary comment, if the method name can be clarified.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1064
> +        // check if haven't yet *entered* the range

Nit: Capitalization, grammar, and punctuation.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1079
> +        // we'll need more information from the current row to continue

Looking at loadCurrentRow(), it returns false if the row fails to load. So the behavior is to keep iterating if the data is invalid. Can this comment be improved?

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1102
> +// return true if we're still within the upper/lower bounds of this range

Don't need this comment.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1107
> +            // high key not included in range

Nit: capitalize, add period.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1116
> +            // low key not included in range

Nit: capitalize, add period.

[Alec Flett]

Created attachment 135896 [details]
Patch

[Alec Flett]

Comment on attachment 135871 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135871&action=review

>> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1018
>> +    bool checkCurrentBoundaries() const;
> 
> Boundaries don't change, so is "Current" needed? Maybe rename to "isInBounds"? But it also only checks the end boundary - can that be captured in the name?

I reversed it to isPastBounds() and added a clarifying comment (hoping that will make it by the webkit guidelines)

>> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1079
>> +        // we'll need more information from the current row to continue
> 
> Looking at loadCurrentRow(), it returns false if the row fails to load. So the behavior is to keep iterating if the data is invalid. Can this comment be improved?

The semantics of this aren't great, but this is what was there before - It looks like it's not really fatal, because sometimes indexes have stale entries and you just have to keep iterating until you find a non-stale entry.

[David Grogan]

LGTM

[Alec Flett]

tony@ - r? cq?

[Tony Chang]

Comment on attachment 135896 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135896&action=review

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1034
> +    return continueFunction(0, true);

Please declare and use an enum rather than a bool.  There are other examples of this in the code base.

[Alec Flett]

Created attachment 136042 [details]
Patch

[Alec Flett]

Comments addressed, enum added.

tony@ - r? cq?

[Tony Chang]

Comment on attachment 136042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136042&action=review

> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:105
> +

Nit: Extra newline?

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:983
> +    virtual bool continueFunction(const IDBKey*, IteratorState nextState);

Remove the argument name |nextState|.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1048
> +        } else
> +            nextState = Seek;

Why do we set nextState here?  It doesn't look like we use it later on.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1565
> +        print "parameter: ", canonical_parameter_name
> +        print "Acronym: ", acronym
> +        print "In text: ", canonical_text

Did you mean to include this change?

[Alec Flett]

Created attachment 136132 [details]
Patch

[Alec Flett]

Comment on attachment 136042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136042&action=review

>> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:105
>> +
> 
> Nit: Extra newline?

Done.

>> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:983
>> +    virtual bool continueFunction(const IDBKey*, IteratorState nextState);
> 
> Remove the argument name |nextState|.

Done.

>> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1048
>> +            nextState = Seek;
> 
> Why do we set nextState here?  It doesn't look like we use it later on.

Added a comment to explain - it's for the next iteration

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:1565
>> +        print "In text: ", canonical_text
> 
> Did you mean to include this change?

oops! good ol' -a flag :)
Removed.

[Alec Flett]

Comments addressed

tony@ - r? cq?

[Alec Flett]

tony@ - ping?  r? cq?

[Alec Flett]

oops.. tony is on vacation

ojan@ - r? cr?

[Ojan Vafai]

Comment on attachment 136132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136132&action=review

Mostly a bunch of style nits. I know some of this is copy-pasted from the old code, but I think deleting comments is a small enough change from the old code that it's OK. :)

> Source/WebCore/Modules/indexeddb/IDBBackingStore.h:93
> +            Ready = 0, // Already in position, don't advance.
> +            Seek // Seek into position before use.

Nit: Typically we don't include "what" comments in WebKit code. More "why" comments, e.g. an explanation of why we're doing something non-obvious. "what" comments tend to get stale and inaccurate and can usually be easily understood by reading the code.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1060
> +        // Make sure we have *entered* the range.

Instead of adding a comment, you could add a helper function, e.g., haveEnteredRange. Then this code would be:
if (!haveEnteredRange(...))
    continue;

Which is self-describing.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1063
> +                // lowKey not included in the range.

Ditto re "what" comments here and below.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1076
> +        // The row may not load because there's a stale entry in the
> +        // index. This is not fatal.

Good "why" comment. :)

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1100
> +// Returns true if the cursor has gone beyond the bounds, but only in
> +// the direction of the iterator.

"what" comment.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1119
> +            // High key not included in range.
> +            if (compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) >= 0)
> +                return true;
> +        } else {
> +            if (compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) > 0)
> +                return true;
> +        }
> +    } else {
> +        if (m_cursorOptions.lowOpen) {
> +            // Low key not included in range.
> +            if (compareIndexKeys(m_iterator->key(), m_cursorOptions.lowKey) <= 0)
> +                return true;
> +        } else {
> +            if (compareIndexKeys(m_iterator->key(), m_cursorOptions.lowKey) < 0)
> +                return true;

This is a stylistic thing, but I'd prefer if you returned the comparisons instead of true, e.g.,

if (m_cursorOptions.forward) {
    if (m_cursorPoptions.highOpen)
        return compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) >= 0;
    else
        return compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) > 0;
} else {

etc.

Makes the logic a bit more clear (and concise) since you don't have to keep track of what happens after each return.

[Alec Flett]

Created attachment 136934 [details]
Patch

[Alec Flett]

Comment on attachment 136132 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136132&action=review

>> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1060
>> +        // Make sure we have *entered* the range.
> 
> Instead of adding a comment, you could add a helper function, e.g., haveEnteredRange. Then this code would be:
> if (!haveEnteredRange(...))
>     continue;
> 
> Which is self-describing.

that is nicer. done.

>> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1119
>> +                return true;
> 
> This is a stylistic thing, but I'd prefer if you returned the comparisons instead of true, e.g.,
> 
> if (m_cursorOptions.forward) {
>     if (m_cursorPoptions.highOpen)
>         return compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) >= 0;
>     else
>         return compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) > 0;
> } else {
> 
> etc.
> 
> Makes the logic a bit more clear (and concise) since you don't have to keep track of what happens after each return.

Not sure I agree with the clarity, but I've made the change and applied the same style in the new haveEnteredRange()

All other comments addressed... new patch coming up...

[Alec Flett]

ojan@ can you take another shot at this one r? cq?

[Ojan Vafai]

Comment on attachment 136934 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=136934&action=review

Nice cleanup!

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1099
> +
> +        return compareIndexKeys(m_iterator->key(), m_cursorOptions.lowKey) >= 0;
> +    }
> +    if (m_cursorOptions.highOpen)
> +        return compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) < 0;
> +
> +    return compareIndexKeys(m_iterator->key(), m_cursorOptions.highKey) <= 0;

nit: typically, webkit code wouldn't have these extra line-breaks. there's no official style here though.

[WebKit Review Bot]

Comment on attachment 136934 [details]
Patch

Clearing flags on attachment: 136934

Committed r114041: <http://trac.webkit.org/changeset/114041>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.