IDBDatabase::createObjectStore/removeObjectStore and IDBObjectStore::createIndex/removeIndex should be synchronous.
Created attachment 69321 [details] Patch
Attachment 69321 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4177027
Created attachment 69323 [details] Patch
Comment on attachment 69321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69321&action=review > WebCore/storage/IDBDatabase.cpp:68 > + m_backend->removeObjectStore(name); this needs the transaction > WebCore/storage/IDBDatabaseBackendImpl.cpp:172 > +void IDBDatabaseBackendImpl::removeObjectStore(const String& name) need the transaction passed in > WebCore/storage/IDBObjectStoreBackendImpl.cpp:92 > + , m_id(0), maybe make an invalid id constant?
couple comments
Comment on attachment 69323 [details] Patch besides previous comment, lgtm
Created attachment 69498 [details] Patch
I rebased my patch after http://trac.webkit.org/changeset/68795 and the conflict resolving was non-trivial. Also modified the layout tests and updated the expected tests output. I would therefore be grateful if I could get another review. Thanks!
Comment on attachment 69498 [details] Patch close View in context: https://bugs.webkit.org/attachment.cgi?id=69498&action=review > LayoutTests/storage/indexeddb/objectstore-cursor.html:67 > + if (window.nextToAdd > 0) hu? > LayoutTests/storage/indexeddb/resources/shared.js:82 > + onfinished(); ugh! I'm so stupid...I just made this change...should have realized it's worthless (and not made the change in my last patch). Maybe put in a FIXME to make this function sync and not do a callback like this. > WebCore/storage/IDBDatabaseBackendImpl.cpp:191 > + RefPtr<IDBTransactionBackendInterface> rpTransaction = transaction; transactionPtr...here and elsewhere > WebCore/storage/IDBIndexBackendImpl.cpp:55 > + , m_id(0) use a constant instead of 0 maybe? > WebCore/storage/IDBIndexBackendImpl.h:52 > + int64_t id() const { return m_id; } ditto to all comments below > WebCore/storage/IDBObjectStoreBackendImpl.cpp:335 > + transaction->didCompleteTaskEvents(); Wait...why do we need this? Isn't this implicit whenever a task's events finish? > WebCore/storage/IDBObjectStoreBackendImpl.h:55 > int64_t id() const { return m_id; } Assert the id is valid maybe? ...here and everywhere that id is used? Please put in a comment here describing the subtleties of this..and how stuff using id should only be queued asyncly. We should put a fixme or actual code into the transaction coordinator to make sure that it blocks any transactions on this object store on this current transaction finishing.... > WebCore/storage/IDBTransactionBackendImpl.cpp:146 > +void IDBTransactionBackendImpl::taskEventTimerFired(Timer<IDBTransactionBackendImpl>*) Hm...this still seems a bit fragile...but I guess it's good enough for now.
(In reply to comment #9) > (From update of attachment 69498 [details]) > > close > > > View in context: https://bugs.webkit.org/attachment.cgi?id=69498&action=review > > > LayoutTests/storage/indexeddb/objectstore-cursor.html:67 > > + if (window.nextToAdd > 0) > > hu? > It's the same pattern used elsewhere for loading data into an object store. > > LayoutTests/storage/indexeddb/resources/shared.js:82 > > + onfinished(); > > ugh! I'm so stupid...I just made this change...should have realized it's worthless (and not made the change in my last patch). Maybe put in a FIXME to make this function sync and not do a callback like this. > Added. > > WebCore/storage/IDBDatabaseBackendImpl.cpp:191 > > + RefPtr<IDBTransactionBackendInterface> rpTransaction = transaction; > > transactionPtr...here and elsewhere > > > WebCore/storage/IDBIndexBackendImpl.cpp:55 > > + , m_id(0) > > use a constant instead of 0 maybe? > > > WebCore/storage/IDBIndexBackendImpl.h:52 > > + int64_t id() const { return m_id; } > > ditto to all comments below > You mean above? Done. > > WebCore/storage/IDBObjectStoreBackendImpl.cpp:335 > > + transaction->didCompleteTaskEvents(); > > Wait...why do we need this? Isn't this implicit whenever a task's events finish? > It isn't. These tasks don't have any events. > > WebCore/storage/IDBObjectStoreBackendImpl.h:55 > > int64_t id() const { return m_id; } > > Assert the id is valid maybe? ...here and everywhere that id is used? > Asserting. > Please put in a comment here describing the subtleties of this..and how stuff using id should only be queued asyncly. > > We should put a fixme or actual code into the transaction coordinator to make sure that it blocks any transactions on this object store on this current transaction finishing.... > Yep, that should happen once we have a thread per transaction and we will implement locking. The object store will be locked until the setVersion transaction commits, thereby guaranteeing that no other transaction can use its backend while it has the invalid ID. > > WebCore/storage/IDBTransactionBackendImpl.cpp:146 > > +void IDBTransactionBackendImpl::taskEventTimerFired(Timer<IDBTransactionBackendImpl>*) > > Hm...this still seems a bit fragile...but I guess it's good enough for now.
Created attachment 69715 [details] Patch
Created attachment 69728 [details] Patch
Comment on attachment 69728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=69728&action=review > WebCore/storage/IDBIndexBackendImpl.h:54 > + ASSERT(m_id != InvalidId); ditto > WebCore/storage/IDBObjectStoreBackendImpl.h:60 > + void setId(int64_t id) { m_id = id; } assert it's not valid to begin with? also, i wonder if these should be un-inlined r=me
Comment on attachment 69728 [details] Patch sitll looks fine
Committed r69121.