Bug 84174 - IndexedDB: implement cursor.advance()
Summary: IndexedDB: implement cursor.advance()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alec Flett
URL:
Keywords:
Depends on: 84280
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-17 11:28 PDT by Alec Flett
Modified: 2012-04-25 19:22 PDT (History)
5 users (show)

See Also:


Attachments
Patch (27.11 KB, patch)
2012-04-23 17:30 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (37.30 KB, patch)
2012-04-24 11:35 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (37.99 KB, patch)
2012-04-24 12:16 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (39.98 KB, patch)
2012-04-24 15:25 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch (39.66 KB, patch)
2012-04-25 10:01 PDT, Alec Flett
no flags Details | Formatted Diff | Diff
Patch for landing (39.61 KB, patch)
2012-04-25 16:21 PDT, Alec Flett
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alec Flett 2012-04-17 11:28:25 PDT
As per the spec.
Comment 1 Alec Flett 2012-04-23 17:30:42 PDT
Created attachment 138470 [details]
Patch
Comment 2 Alec Flett 2012-04-24 11:35:52 PDT
Created attachment 138610 [details]
Patch
Comment 3 Alec Flett 2012-04-24 11:37:51 PDT
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...?
Comment 4 Joshua Bell 2012-04-24 11:46:38 PDT
(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
Comment 5 Alec Flett 2012-04-24 12:16:32 PDT
Created attachment 138618 [details]
Patch
Comment 6 Alec Flett 2012-04-24 12:17:52 PDT
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)
Comment 7 Joshua Bell 2012-04-24 13:06:21 PDT
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.
Comment 8 Alec Flett 2012-04-24 14:23:58 PDT
(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.
Comment 9 Alec Flett 2012-04-24 15:25:29 PDT
Created attachment 138666 [details]
Patch
Comment 10 Alec Flett 2012-04-24 15:27:50 PDT
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)
Comment 11 Joshua Bell 2012-04-24 15:45:48 PDT
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"
Comment 12 Alec Flett 2012-04-25 10:01:46 PDT
Created attachment 138831 [details]
Patch
Comment 13 Alec Flett 2012-04-25 10:02:34 PDT
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.
Comment 14 Alec Flett 2012-04-25 14:44:37 PDT
Comment on attachment 138831 [details]
Patch

ojan@ - one more - cq? r?
Comment 15 Ojan Vafai 2012-04-25 15:43:59 PDT
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.
Comment 16 Alec Flett 2012-04-25 16:21:58 PDT
Created attachment 138892 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-04-25 16:23:22 PDT
Comment on attachment 138892 [details]
Patch for landing

Rejecting attachment 138892 [details] from commit-queue.

alecflett@chromium.org 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.
Comment 18 WebKit Review Bot 2012-04-25 19:22:50 PDT
Comment on attachment 138892 [details]
Patch for landing

Clearing flags on attachment: 138892

Committed r115282: <http://trac.webkit.org/changeset/115282>
Comment 19 WebKit Review Bot 2012-04-25 19:22:56 PDT
All reviewed patches have been landed.  Closing bug.