RESOLVED FIXED 89379
IndexedDB: Implement IDBTransaction internal active flag
https://bugs.webkit.org/show_bug.cgi?id=89379
Summary IndexedDB: Implement IDBTransaction internal active flag
Joshua Bell
Reported 2012-06-18 14:09:43 PDT
The IDB spec http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html says: Transaction lifecycle: "Transactions have an active flag, which determines if new requests can be made against the transaction." "When a transaction is created its active flag is initially set to true." - i.e. right after the db.transaction() call "The implementation must allow requests to be placed against the transaction whenever the active flag is true." "Requests may be placed against a transaction only while that transaction is active. If an attempt is made to place a request against a transaction when that transaction is not active, the implementation must reject the attempt by throwing a DOMException of type TransactionInactiveError." Version changes: "This transaction will be active inside the onupgradeneeded event handler, allowing the creation of new object stores and indexes." Abort: "Otherwise this method sets the transaction's active flag to false..." Transaction creation: "When control is returned to the event loop, the implementation must set the active flag to false." Firing events: ... "Set the active flag of transaction to true." ... "Set the active flag of transaction to false." ... Right now we don't have a notion of non-active transaction - until it is |finished| we allow requests to be placed against it.
Attachments
Patch (86.26 KB, patch)
2012-06-26 17:40 PDT, Joshua Bell
no flags
Patch (84.93 KB, patch)
2012-06-27 14:29 PDT, Joshua Bell
no flags
Patch for landing (84.88 KB, patch)
2012-06-28 14:22 PDT, Joshua Bell
no flags
Joshua Bell
Comment 1 2012-06-18 14:30:15 PDT
A plausible test for this looks like: trans = db.transaction('store'); trans.oncomplete = finishJSTest; var done = false; function doRequest() { if (done) return; trans.objectStore('store').get('key').onsuccess = doRequest; }; setTimeout(function () { evalAndExpectException("trans.objectStore('store').get('key')", "IDBDatabaseException.TRANSACTION_INACTIVE_ERR", "'TransactionInactiveError'"); done = true; }, 0); In other words, show
Joshua Bell
Comment 2 2012-06-18 15:27:14 PDT
Looks like I'll have to do this as part of webkit.org/b/89377
Joshua Bell
Comment 3 2012-06-19 09:38:40 PDT
Also, don't abort empty transactions - commit them instead. (May require plumbing for Chromium port.)
Joshua Bell
Comment 4 2012-06-26 17:40:55 PDT
Joshua Bell
Comment 5 2012-06-26 17:43:57 PDT
alecflett@ - please take a look Landing sequence will be: (1) Trivial patch with WebKit::WebIDBTransaction::commit() (2) Chromium patch for plumbing through commit() and disabling some layout-tests-as-browser-tests that are getting renamed (3) This patch (4) Re-enable the layout-tests-as-browser-tests Forgot to mention in the ChangeLog, but the transaction-complete-XXX tests are renames of the transaction-abort-XXX tests, now that empty transactions commit rather than abort.
Alec Flett
Comment 6 2012-06-27 11:56:02 PDT
Comment on attachment 149646 [details] Patch This all looks quite reasonable, I wish I had more to add but it seems fine.
Joshua Bell
Comment 7 2012-06-27 11:57:49 PDT
Chromium WebKit public API addition is at: http://webkit.org/b/90089
Joshua Bell
Comment 8 2012-06-27 13:09:10 PDT
tony@ - can you take a look at your leisure? Various other things need to land before this can be submitted.
Tony Chang
Comment 9 2012-06-27 14:05:35 PDT
Comment on attachment 149646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149646&action=review I don't understand all these changes, but it seems OK. > Source/WebCore/Modules/indexeddb/IDBPendingTransactionMonitor.cpp:37 > +static ThreadSpecific<Vector<RefPtr<IDBTransaction> > >& transactions() Nit: Maybe use a typedef for Vector<RefPtr<IDBTransaction> > ?
Joshua Bell
Comment 10 2012-06-27 14:29:39 PDT
Joshua Bell
Comment 11 2012-06-27 14:31:44 PDT
(In reply to comment #9) > > Nit: Maybe use a typedef for Vector<RefPtr<IDBTransaction> > ? Good idea - done. New patch just adds this and rebases on http://trac.webkit.org/changeset/121366 Waiting on that to roll into Chromium and landing http://codereview.chromium.org/10692017/ before this can land.
Joshua Bell
Comment 12 2012-06-28 14:22:11 PDT
Created attachment 150014 [details] Patch for landing
WebKit Review Bot
Comment 13 2012-06-28 17:38:25 PDT
Comment on attachment 150014 [details] Patch for landing Clearing flags on attachment: 150014 Committed r121492: <http://trac.webkit.org/changeset/121492>
WebKit Review Bot
Comment 14 2012-06-28 17:38:30 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.