Bug 89377

Created attachment 148887 [details] Patch

Patch depends on the 88467, but here's some of the back->front migration work.

Comment on attachment 148887 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148887&action=review > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:136 > + if (!m_transaction->isActive()) { The transaction internal |active| flag used here is implemented in a non-spec-compliant way. http://webkit.org/b/89379 will land a real implementation. Just enough logic exists to move these checks from the back-end to the front-end. As seen below, a transaction will be born with active=true and will have active=false when either abort is called explicitly or when an complete or abort event is fired at it. > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:150 > + RefPtr<IDBObjectStore> objectStore = effectiveObjectStore(); Logic moved from IDBObjectStoreBackendImpl::put() to IDBCursor::update(). > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:163 > + ASSERT(!ec); This pattern (that the back end calls are guaranteed to succeed, so requests don't need "early death" states) is used throughout now. > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:175 > + if (!m_transaction->isActive()) { Previously, m_request was cleared out when the transaction is finished. This narrows the scope further (and http://webkit.org/b/89379 will take it even farther) > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:198 > + if (key && !key->isValid()) { As seen in such patches as http://webkit.org/b/86123 > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:213 > + if (!m_request->resetReadyState(m_transaction.get())) { WebKit style - return early in error case. > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:266 > +PassRefPtr<IDBObjectStore> IDBCursor::effectiveObjectStore() The "effective object store" terminology is per spec. > Source/WebCore/Modules/indexeddb/IDBCursor.h:94 > + virtual bool isKeyCursor() const { return true; } "Key" cursors and "value" cursors are distinguished in the back end by type flags, but on the front end by different classes. This seemed like the easiest way to expose this distinction, but alternatives welcome. > Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:123 > + ASSERT_NOT_REACHED(); ... and this pattern is now common on the back end - the only reason task scheduling can fail is if the back end is inactive, but the front end now tracks this state. > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:132 > + if (!usesInLineKeys && !hasKeyGenerator && !key) { Logic moved from IDBObjectStoreBackendImpl::add() to IDBObjectStore::add() - it should also match the ordering in the spec for these clauses. > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:161 > + // TODO: Pass through keyPathKey as key Oops, need to change to a FIXME. Changing what's passed through will be a follow-up patch. > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:189 > + const IDBKeyPath& keyPath = m_metadata.keyPath; Right now, the logic is duplicated between add() and put() - I'll refactor to a common method that takes the enum flag. > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:327 > + m_readyState = EarlyDeath; The public method for setting this state goes away, but we still need it for this case (context stopping while request is pending).

Created attachment 149139 [details] Patch

alecflett@, dgrogan@ - how's this look? tony@ - r?

Comment on attachment 149139 [details] Patch David or Alec, can one or both of you review this?

Comment on attachment 149139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149139&action=review > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:151 > + const IDBKeyPath& keyPath = objectStore->metadata().keyPath; Minor, but in general I'd sooner see calling objectStore->keyPath() so most callers aren't dependent on the actual use of metadata objects. > Source/WebCore/Modules/indexeddb/IDBCursor.cpp:213 > + if (!m_request->resetReadyState(m_transaction.get())) { Sanity check just because it wasn't mentioned: the FIXME is ok to remove? > Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:123 > + ASSERT_NOT_REACHED(); Why wouldn't this be reached? Is there something else we can also assert here, like m_transaction->isActive()? > Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:186 > + ASSERT_NOT_REACHED(); Same here as my previous comment > Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:166 > + ASSERT_NOT_REACHED(); another.. you get the idea... :) > Source/WebCore/Modules/indexeddb/IDBIndex.cpp:125 > + ASSERT(!ec); Assert does seem dangerous here - maybe worth a comment (I see the m_transaction->isActive() (looks like you got a lot of them, but I wonder how many of the ones that are left could be removed? Mostly in the interest of getting rid of an extra state for IDBRequest) > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:-160 > - if (putMode != CursorUpdate) { this is great, and vastly simplifies some of my put() work coming up! > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:434 > + ASSERT(transaction->mode() == IDBTransaction::VERSION_CHANGE); this is the sort of ASSERT I was wondering if we could throw above with all the ASSERT_NOT_REACHED stuff - i.e. making sure we're in a specific state > Source/WebCore/Modules/indexeddb/IDBObjectStoreBackendImpl.cpp:445 > + ASSERT_NOT_REACHED(); Can we remove this entirely then?

Comment on attachment 149139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149139&action=review >> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:151 >> + const IDBKeyPath& keyPath = objectStore->metadata().keyPath; > > Minor, but in general I'd sooner see calling objectStore->keyPath() so most callers aren't dependent on the actual use of metadata objects. Unfortunately, objectStore->keyPath() implements the IDL which produces an IDBAny which is a one-way transform. Otherwise I'd agree. >> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:213 >> + if (!m_request->resetReadyState(m_transaction.get())) { > > Sanity check just because it wasn't mentioned: the FIXME is ok to remove? Hrm, good point. I thought it was a dead comment, but I now understand the question. I'll put it back, and undo this unrelated cleanup. >> Source/WebCore/Modules/indexeddb/IDBCursorBackendImpl.cpp:123 >> + ASSERT_NOT_REACHED(); > > Why wouldn't this be reached? Is there something else we can also assert here, like m_transaction->isActive()? Re: the logic: IDBTransactionBackendImpl::scheduleTask only fails if the transaction is finished. The front end should never call into this while the transaction is inactive (via the isActive tests). Once the transaction is finished it can never be active again. However (as we discussed offline), there's a scenario where this isn't true in a multiprocess port. If the transaction aborts while the front-end->back-end call is "in flight" the transaction could be in a "finished" state before this is reached, and we need to signal the front-end as the code previously did. So I'm going to remove these assertions and restore the error handling path on the front-end. >> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:125 >> + ASSERT(!ec); > > Assert does seem dangerous here - maybe worth a comment (I see the m_transaction->isActive() > > (looks like you got a lot of them, but I wonder how many of the ones that are left could be removed? Mostly in the interest of getting rid of an extra state for IDBRequest) This is now reverted. The extra state needs to stay, alas.

Created attachment 149394 [details] Patch

Comment on attachment 149394 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149394&action=review > Source/WebCore/Modules/indexeddb/IDBTransaction.h:71 > + // FIXME: Implement internal |active| flag per spec. http://webkit.org/b/89379 Last thing: Is this fixme just cruft from development? if not, how are we divergent from the spec at this point - put at least a simple explanation here..

(In reply to comment #10) > > Source/WebCore/Modules/indexeddb/IDBTransaction.h:71 > > + // FIXME: Implement internal |active| flag per spec. http://webkit.org/b/89379 > > Last thing: Is this fixme just cruft from development? if not, how are we divergent from the spec at this point - put at least a simple explanation here.. The details on how we diverge from the spec are captured in the linked bug but I can clarify the comment somewhat.

Created attachment 149549 [details] Patch

> The details on how we diverge from the spec are captured in the linked bug but I can clarify the comment somewhat. Actually, I just removed the comment; the subtle details are more than belongs in a FIXME when there's already a bug filed.

ok, sorry I wasn't sure if the details in the other bug were a superset of what is left after this patch... LGTM

Comment on attachment 149549 [details] Patch Clearing flags on attachment: 149549 Committed r121291: <http://trac.webkit.org/changeset/121291>

All reviewed patches have been landed. Closing bug.