[webkit-reviews] review denied: [Bug 60746] IndexedDB chooses wrong record on PREV_NO_DUPLICATE index cursor : [Attachment 137106] Patch

[bugzilla-daemon at webkit.org]

Joshua Bell <jsbell at chromium.org> has denied Alec Flett
<alecflett at chromium.org>'s request for review:
Bug 60746: IndexedDB chooses wrong record on PREV_NO_DUPLICATE index cursor
https://bugs.webkit.org/show_bug.cgi?id=60746

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

------- Additional Comments from Joshua Bell <jsbell at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=137106&action=review


> Source/WebCore/ChangeLog:4
> +	   https://bugs.webkit.org/show_bug.cgi?id=60746

This also fixes 60763 so mention that in the comment block and/or mark that bug
as a duplicate if there's no distinct code/test.

> Source/WebCore/ChangeLog:10
> +	   When iterating backwards with PREV_NO_DUPLICATE, keep walking past

These comments should appear before the Test: line(s)

> Source/WebCore/ChangeLog:12
> +	   covers the index and non-index (objectStore) case.

Nit: just write "object store and index cases"

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1045
> +    RefPtr<IDBKey> previousDupKey;

Avoid abbreviations.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1058
> +		   // We need to turn around because we hit the end of

"turn around" or simply "back up"? This doesn't appear to repeat or change the
iteration direction.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1065
> +

Remove at least one of these blank lines

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1069
> +

Remove blank line.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1072
>	   if (!m_transaction->get(m_iterator->key(), trash))

This implies that there are rows that should be skipped over during iteration.
What happens if one of these is the last seen before the iterator ends (case
above and case below) or a non-dupe is seen? Would the if (prevDupKey.get()) {
m_iterator->next(); } cases back up to the row that can't be loaded? (In that
case, the notion of "turning around" makes more sense as multiple next() calls
may be necessary.

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1112
> +		   // We need to turn around because we hit the boundary

This comment seems to apply to the next if() block

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:1120
> +		       //

Remove empty comment.

> LayoutTests/ChangeLog:4
> +	   https://bugs.webkit.org/show_bug.cgi?id=60746

If not resolving 60763 as a duplicate, mention in the comment block.

> LayoutTests/ChangeLog:10
> +	   * storage/indexeddb/cursor-reverse-bug-expected.txt:

Is there a cursor-reverse-bug.html that is missing from the patch?

> LayoutTests/storage/indexeddb/cursor-prev-no-duplicate-expected.txt:27
> +runTest(store.openCursor(IDBKeyRange.upperBound(7, false), 2)...

The setting up the test (starting the transaction, opening the cursor) and
checking the results (cursor.key etc) are separated, which makes this logging
not very useful in understanding the test. Either drop this logging or try and
move the setup and results closer together (one transaction per test?)

> LayoutTests/storage/indexeddb/mozilla/index-prev-no-duplicate.html:20
> +<script>

Should the JS be factored out of this test to match the others?

> LayoutTests/storage/indexeddb/resources/cursor-prev-no-duplicate.js:6
> +description("Test IndexedDB keys ordering and readback from cursors.");

Description could be more specific.

> LayoutTests/storage/indexeddb/resources/cursor-prev-no-duplicate.js:59
> +    // help us distinguish the queuing of the tests from the execution

Restructure the tests so this isn't necessary, otherwise the logging isn't
valuable.

> LayoutTests/storage/indexeddb/resources/cursor-prev-no-duplicate.js:63
> +

Too many blank lines.

> LayoutTests/storage/indexeddb/resources/cursor-prev-no-duplicate.js:67
> +    // upper bound is well out of range, results always the same,

Prefer debug() statements to script comments, so the intent of the test is
captured in the log if it isn't inherently clear.

> LayoutTests/storage/indexeddb/resources/cursor-prev-no-duplicate.js:136
> +function makeOpenCursor(obj, upperBound, open, direction)

I think use of these functions make the test harder to read vs. just putting
the string inline.

> LayoutTests/storage/indexeddb/resources/cursor-prev-no-duplicate.js:179
> +	   debug("     cursor.key = " + cursor.key);

To make this take less space and improve the readability in the log, I've been
using this style:
shouldBe("cursor.key", JSON.stringify(expectation.expectedKey));

> LayoutTests/storage/indexeddb/resources/shared.js:116
> +// take an unspecified number of callback-like objects, wait for each

Capitalize.
Also, I'd refer to these as "request-like" or "promise-like" or "future-like"
rather than "callback-like". Probably settle on "request" since this is an
IDB-specific utility file (even though the function is generic, it's a good
name)

> LayoutTests/storage/indexeddb/resources/shared.js:119
> +// appropriate onerror/onabort immediately.

Will onerror/onabort be called more than once? (Looks like yes.) Do we care?
(Probably not.)

> LayoutTests/storage/indexeddb/resources/shared.js:122
> +

Remove blank line.

> LayoutTests/storage/indexeddb/resources/shared.js:137
> +	   var event = arguments[i];

I'd name this something other than event - request/future/promise.

> LayoutTests/storage/indexeddb/resources/shared.js:146
> +function waitFor_onsuccess(index, successes, eventResult)

Move this function inside the definition of waitFor() since it won't be called
elsewhere.
If that is done, the successes argument can just become the progress var via
the closure, and ditto for eventResult.