As per the spec.
Created attachment 138470 [details] Patch
Created attachment 138610 [details] Patch
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...?
(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
Created attachment 138618 [details] Patch
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)
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.
(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.
Created attachment 138666 [details] Patch
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 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"
Created attachment 138831 [details] Patch
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 on attachment 138831 [details] Patch ojan@ - one more - cq? r?
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.
Created attachment 138892 [details] Patch for landing
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 on attachment 138892 [details] Patch for landing Clearing flags on attachment: 138892 Committed r115282: <http://trac.webkit.org/changeset/115282>
All reviewed patches have been landed. Closing bug.