IndexedDB: Attributes for a cursor "run past the end" should be undefined.
Created attachment 136281 [details] Patch
Created attachment 136282 [details] Patch
jsbell@ - mind taking a look? in particular I want to make sure I have the RefPtr stuff right for m_currentValue, since I'm storing a SerializedScriptValue() and converting it to an IDBAny on demand in IDBCursor::value()
Comment on attachment 136282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136282&action=review > Source/WebCore/ChangeLog:18 > + update and delete can be called before continue, but not afterwards. Nit: For consistency, should be update() and delete() and reword to start the sentence with a capital. > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:87 > + return IDBAny::create(m_currentValue.get()); As an aside, the spec includes "if this property returns an object, it returns the same object instance every time it is inspected, until the cursor's value is changed. This means that if the object is modified, those modifications will be seen by anyone inspecting the value of the cursor." - do we implement that? Would returning the same IDBAny instance on each call affect that behavior? My guess is no and no. Implementing that part of the spec may be problematic. (This change doesn't affect it either way.) > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:100 > + ec = IDBDatabaseException::TRANSACTION_INACTIVE_ERR; Spec says this should be InvalidStateError (DOM4-style) - can you make it INVALID_STATE_ERR (not part of the IDBDatabaseException enum) > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:123 > + ec = IDBDatabaseException::TRANSACTION_INACTIVE_ERR; ditto > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:150 > + ec = IDBDatabaseException::TRANSACTION_INACTIVE_ERR; ditto > Source/WebCore/Modules/indexeddb/IDBCursor.h:87 > + RefPtr<IDBKey> m_currentKey; Does "current" add anything here? Can it just be m_key etc? > Source/WebCore/Modules/indexeddb/IDBCursor.h:89 > + RefPtr<SerializedScriptValue> m_currentValue; Although I believe this code is correct, I would have this be a RefPtr<IDBAny> and re-use the IDBAny across calls so that we minimize the number of places IDB objects hold SSV pointers. It shouldn't make any practical difference, though. > LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:52 > + result = evalAndLog("objectStore.add({'x': " + nextToAdd + " }, " + nextToAdd + ")"); Multiple add calls can be done w/o waiting for the success callback of the previous one. Just use a for a loop here. > LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:148 > + // behavior against pre-caching. Make sure to use prime Nit: "pre-fetching" (so readers know what to search on) > LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:173 > + onTransactionComplete(); The transaction is not actually complete here, the cursor has just run to the end... > LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:179 > + evalAndExpectException("cursor.continue()", ""); I'd move this inline since it's part of the consistency checks... > LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:180 > + finishJSTest(); And either call this directly, or hang it off he actual transaction oncomplete callback.
Comment on attachment 136282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136282&action=review > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:88 > } Since m_currentValue is a RefPtr<SerializedScriptValue> this should just be: return IDBAny::create(m_currentValue)
Created attachment 136319 [details] Patch
Created attachment 136325 [details] Patch
Comment on attachment 136282 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136282&action=review >> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:87 >> + return IDBAny::create(m_currentValue.get()); > > As an aside, the spec includes "if this property returns an object, it returns the same object instance every time it is inspected, until the cursor's value is changed. This means that if the object is modified, those modifications will be seen by anyone inspecting the value of the cursor." - do we implement that? Would returning the same IDBAny instance on each call affect that behavior? My guess is no and no. Implementing that part of the spec may be problematic. (This change doesn't affect it either way.) changed to be an IDBAny, and sadly you're correct: the obj does change each time. >> Source/WebCore/Modules/indexeddb/IDBCursor.h:87 >> + RefPtr<IDBKey> m_currentKey; > > Does "current" add anything here? Can it just be m_key etc? I personally think it does because m_backend can get out of sync with m_current* >> Source/WebCore/Modules/indexeddb/IDBCursor.h:89 >> + RefPtr<SerializedScriptValue> m_currentValue; > > Although I believe this code is correct, I would have this be a RefPtr<IDBAny> and re-use the IDBAny across calls so that we minimize the number of places IDB objects hold SSV pointers. It shouldn't make any practical difference, though. Done. >> LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:52 >> + result = evalAndLog("objectStore.add({'x': " + nextToAdd + " }, " + nextToAdd + ")"); > > Multiple add calls can be done w/o waiting for the success callback of the previous one. Just use a for a loop here. this was copied from another test, and I thought the same until I realized that you don't have a final onsuccess to fire if you do it like that - you could attach to the last-added value, but there's no guarantee that the other values have been written by then. (at least that I could see...)
jsbell@ - comments addressed...
Comment on attachment 136325 [details] Patch Attachment 136325 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12371585 New failing tests: storage/indexeddb/cursor-continue.html storage/indexeddb/mozilla/cursors.html
Created attachment 136379 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 136498 [details] Patch
jsbell@ - ok, one more try, the other tests pass this time! (one mozilla test had to be altered because the new behavior is correct)
(In reply to comment #8) > changed to be an IDBAny, and sadly you're correct: the obj does change each time. I filed https://bugs.webkit.org/show_bug.cgi?id=83526 to track this. > this was copied from another test, and I thought the same until I realized that you don't have a final onsuccess to fire if you do it like that - you could attach to the last-added value, but there's no guarantee that the other values have been written by then. (at least that I could see...) Within a transaction, tasks a guaranteed to execute in the order they are requested, so if you run: req1 = store.put(1, "key"); req2 = store.get("key"); then req2.result will eventually be 1, even though you're not waiting for req1 to succeed before you issue the get() call.
Comment on attachment 136498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136498&action=review The cursor-continue.html test failed (exception code changed) - did its expected file need to be updated as well? > LayoutTests/ChangeLog:11 > + * storage/indexeddb/resources/shared.js: Should add storage/indexeddb/mozilla/cursors-expected.txt to the list (Sadly, the list is not automagically updated if you revise a patch)
Created attachment 136538 [details] Patch
Comment on attachment 136538 [details] Patch lgtm
ojan@ - one more for today - r? cq?
Comment on attachment 136538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136538&action=review > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:175 > +void IDBCursor::setGotValue() This does more than just set m_gotValue. Maybe this should be called something like syncToBackendValues? > LayoutTests/storage/indexeddb/cursor-continue-validity.html:1 > +<html> New tests should all be in standards mode unless they're explicitly testing quirks behavior. So, this should start with: <!DOCTYPE html> > LayoutTests/storage/indexeddb/cursor-continue-validity.html:7 > +<script src="resources/cursor-continue-validity.js"></script> We've been moving away from using a separate script file for tests that are not pure JS tests (i.e. test that can be run without a browser context). Instead, just inline cursor-continue-validity.js. It makes maintaining the tests much easier when the whole test is contained in one file (with the test harness code still in external files of course). > LayoutTests/storage/indexeddb/resources/cursor-continue-validity.js:132 > +function testModifyContinueOrder() { Here and below, opening curly brace should be on the next line.
(In reply to comment #19) > > > LayoutTests/storage/indexeddb/cursor-continue-validity.html:7 > > +<script src="resources/cursor-continue-validity.js"></script> > > We've been moving away from using a separate script file for tests that are not pure JS tests (i.e. test that can be run without a browser context). Instead, just inline cursor-continue-validity.js. It makes maintaining the tests much easier when the whole test is contained in one file (with the test harness code still in external files of course). dgrogan@ just moved most of the IDB test logic from .html into .js files so we can run them both in a Window context and in a Worker (or SharedWorker) context. The only exceptions are tests than span Window+Worker or places where the APIs differ.
(In reply to comment #20) > (In reply to comment #19) > > > > > LayoutTests/storage/indexeddb/cursor-continue-validity.html:7 > > > +<script src="resources/cursor-continue-validity.js"></script> > > > > We've been moving away from using a separate script file for tests that are not pure JS tests (i.e. test that can be run without a browser context). Instead, just inline cursor-continue-validity.js. It makes maintaining the tests much easier when the whole test is contained in one file (with the test harness code still in external files of course). > > dgrogan@ just moved most of the IDB test logic from .html into .js files so we can run them both in a Window context and in a Worker (or SharedWorker) context. The only exceptions are tests than span Window+Worker or places where the APIs differ. oic. That makes sense then.
Created attachment 136941 [details] Patch
Comment on attachment 136538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=136538&action=review >> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:175 >> +void IDBCursor::setGotValue() > > This does more than just set m_gotValue. Maybe this should be called something like syncToBackendValues? renamed to setValueReady All other comments addressed.
Comment on attachment 136941 [details] Patch FYI, I believe if you get an r+ then the next patch you upload you can do with "webkit-patch land-safely" and it should add it to the commit queue and use the previous r+. Not 100% sure if that works for people without commit access though.
Comment on attachment 136941 [details] Patch Clearing flags on attachment: 136941 Committed r114022: <http://trac.webkit.org/changeset/114022>
All reviewed patches have been landed. Closing bug.