[webkit-reviews] review denied: [Bug 83302] IndexedDB: Refactor cursor iteration to remove duplicate code : [Attachment 136132] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Alec Flett <alecflett at chromium.org>'s
request for review:
Bug 83302: IndexedDB: Refactor cursor iteration to remove duplicate code
https://bugs.webkit.org/show_bug.cgi?id=83302

Attachment 136132: Patch
https://bugs.webkit.org/attachment.cgi?id=136132&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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.