Bug 84174 – IndexedDB: implement cursor.advance()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Alec Flett]

As per the spec.

[Alec Flett]

Created attachment 138470 [details]
Patch

[Alec Flett]

Created attachment 138610 [details]
Patch

[Alec Flett]

jsbell@ - here's the webkit side of things - with a patch for prefetching

the one open issue I had was how to throw a JS type error. Spec sez:

If the value for count is 0 (zero) or a negative number, this method must throw a JavaScript TypeError exception.

I couldn't for the life of me figure out how to throw a non-DOM TypeError - I suspect it's in the ScriptExecutionContext somewhere...?

[Joshua Bell]

(In reply to comment #3)
> the one open issue I had was how to throw a JS type error.

Yeah, we're not wired up to do that yet. "IDB Exception Reform" is tracked on the Chromium side as crbug.com/112664 - we need to set up DOM4-style exceptions and use JS errors for certain things. Use TYPE_MISMATCH_ERR

[Alec Flett]

Created attachment 138618 [details]
Patch

[Alec Flett]

added TYPE_MISMATCH_ERR and a test with relevant  FIXMEs (can't add bug#'s yet because we don't have webkit bugs for the exception stuff)

[Joshua Bell]

My comments (against the second patch):

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

> Source/WebCore/ChangeLog:10
> +        Implement IDBCursor.advance() to spec.

This should go above the Test: line

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:132
> +    // FIXME: spec says if count == 0, we should throw a JavaScript TypeError

As noted: if (!count) { ec = TYPE_MISMATCH_ERR; return; }

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:135
>  // IMPORTANT: If this ever 1) fires an 'error' event and 2) it's possible to fire another event afterwards,

This comment should probably either stick with continueFunctionInternal or be duplicated.

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:147
> +    // do I need something special here?

Remove comment

> Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:984
> +    virtual bool continueFunction(const IDBKey* = 0, IteratorState = Seek);

Are these default values necessary? Won't they be provided by the declaration in IDBBackingStore::Cursor?

> LayoutTests/storage/indexeddb/resources/cursor-advance.js:8
> +// test advance within range

Remove these comments.

> LayoutTests/storage/indexeddb/resources/cursor-advance.js:31
> +var indexData = [

This is unused, remove it.

> LayoutTests/storage/indexeddb/resources/cursor-advance.js:41
> +    description = "My Test Database";

Remove description (obsolete API; dgrogan is working on a patch to drop this from all tests)

> LayoutTests/storage/indexeddb/resources/cursor-advance.js:74
> +// make a test that uses continue() to get to startPos, then passes

Capitalization, punctuation.

> LayoutTests/storage/indexeddb/resources/cursor-advance.js:76
> +function makeAdvanceTest(startPos, bounds, direction, count, expectedValue)

It looks like all callers pass null for bounds and direction - are further tests forthcoming?

> LayoutTests/storage/indexeddb/resources/cursor-advance.js:119
> +

Use blank lines consistently.

> LayoutTests/storage/indexeddb/resources/shared.js:120
> +function waitFor()

Unused - don't think you want this in this patch.

[Alec Flett]

(In reply to comment #7)
> > Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:135
> >  // IMPORTANT: If this ever 1) fires an 'error' event and 2) it's possible to fire another event afterwards,
> 
> This comment should probably either stick with continueFunctionInternal or be duplicated.

Duplicated.

> > Source/WebCore/Modules/indexeddb/IDBLevelDBBackingStore.cpp:984
> > +    virtual bool continueFunction(const IDBKey* = 0, IteratorState = Seek);
> 
> Are these default values necessary? Won't they be provided by the declaration in IDBBackingStore::Cursor?

You'd think, but no. That's why I had to add them here...


> 
> It looks like all callers pass null for bounds and direction - are further tests forthcoming?
> 

Ah yes that was the original intention, got lost...adding now.

[Alec Flett]

Created attachment 138666 [details]
Patch

[Alec Flett]

jsbell@ latest patch has issues addressed, and adds more tests. The tests are not totally exhaustive but they address the most painful / error-prone stuff like PREV_NO_DUPLICATE. (They don't, for example, test PREV/PREV_NO_DUPLICATE for IDBObjectStore)

[Joshua Bell]

Comment on attachment 138666 [details]
Patch

Overall lgtm, with nits. It would be nice to simplify the test data, though.

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

> Source/WebCore/Modules/indexeddb/IDBCursor.idl:49
> +

Delete these blank lines.

> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:135
> +// IMPORTANT: If this ever 1) fires an 'error' event and 2) it's possible to fire another event afterwards,

Not relevant for this patch, but on further consideration we can probably delete these comments. There is a ton of logic in IDBRequest that asserts this, so the comment(s) don't add value.

> LayoutTests/ChangeLog:26
> +        * storage/indexeddb/resources/shared.js:

This can be removed.

> LayoutTests/storage/indexeddb/resources/cursor-advance.js:192
> +                    // Sue (weight 130 - skipping weight 100, 110, 120)

These tests might be simpler to read if the data was less real-world, and instead had e.g. primary keys "p0" ... "p9" and index keys "i0" ... "i9"

[Alec Flett]

Created attachment 138831 [details]
Patch

[Alec Flett]

Comment on attachment 138666 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBCursor.idl:49
>> +
> 
> Delete these blank lines.

Done.

>> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:135
>> +// IMPORTANT: If this ever 1) fires an 'error' event and 2) it's possible to fire another event afterwards,
> 
> Not relevant for this patch, but on further consideration we can probably delete these comments. There is a ton of logic in IDBRequest that asserts this, so the comment(s) don't add value.

Removed.

>> LayoutTests/ChangeLog:26
>> +        * storage/indexeddb/resources/shared.js:
> 
> This can be removed.

Done.

>> LayoutTests/storage/indexeddb/resources/cursor-advance.js:192
>> +                    // Sue (weight 130 - skipping weight 100, 110, 120)
> 
> These tests might be simpler to read if the data was less real-world, and instead had e.g. primary keys "p0" ... "p9" and index keys "i0" ... "i9"

As discussed, I personally find the real-world data more readable, easier to keep in your head, so I'm leaving it.

[Alec Flett]

Comment on attachment 138831 [details]
Patch

ojan@ - one more - cq? r?

[Ojan Vafai]

Comment on attachment 138831 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:143
> +    if (m_request->resetReadyState(m_transaction.get())) {
> +        m_request->setCursor(this);
> +        m_gotValue = false;
> +        m_backend->advance(count, m_request, ec);
> +    } else
> +        ec = IDBDatabaseException::NOT_ALLOWED_ERR;

Nit: I'd reverse the order of this if-statement to early return in the exceptional case. Typical webkit style is to do all the exceptional cases first with early returns and then the common-case.

[Alec Flett]

Created attachment 138892 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 138892 [details]
Patch for landing

Rejecting attachment 138892 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 138892 [details]
Patch for landing

Clearing flags on attachment: 138892

Committed r115282: <http://trac.webkit.org/changeset/115282>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.