[webkit-reviews] review denied: [Bug 46883] IDBDatabase::createObjectStore/removeObjectStore and IDBObjectStore::createIndex/removeIndex should be synchronous. : [Attachment 69498] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Andrei Popescu
<andreip at google.com>'s request for review:
Bug 46883: IDBDatabase::createObjectStore/removeObjectStore and
IDBObjectStore::createIndex/removeIndex should be synchronous.
https://bugs.webkit.org/show_bug.cgi?id=46883

Attachment 69498: Patch
https://bugs.webkit.org/attachment.cgi?id=69498&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>

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.