Bug 150148

Created attachment 263696 [details] Patch v1

Created attachment 263699 [details] Patch v1

Comment on attachment 263696 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=263696&action=review Looks mostly good. A few comments. Merge conflict is probably in project.pbxproj Also, with this new MemoryIDBBackingStore stuff, where does it actually store the information? I see calls to commit, but I'm not sure where they go. > Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:204 > + ASSERT(m_activeTransactions.contains(transaction.info().identifier()) > + || m_committingTransactions.contains(transaction.info().identifier()) > + || m_abortingTransactions.contains(transaction.info().identifier())); It should be in only one of these sets, right? Could you assert that the sum is one? > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:55 > , m_operationTimer(*this, &IDBTransaction::operationTimerFired) > > { > + m_activationTimer = std::make_unique<Timer>(*this, &IDBTransaction::activationTimerFired); Could you initialize m_activationTimer like you do m_operationTimer? > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:105 > + if (isFinishedOrFinishing()) { > + ec = INVALID_STATE_ERR; How would this be reached? Can it be tested? > Source/WebCore/Modules/indexeddb/shared/IDBError.cpp:69 > + static NeverDestroyed<String> entry = ASCIILiteral("Operation was called on an object on which it is not allowed or at a time when it is not allowed."); Should these errors be localized? Where do we see these errors? There aren't any tests that run into errors.

Comment on attachment 263699 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=263699&action=review > Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.h:40 > +class MemoryIDBBackingStore; > + > +class MemoryBackingStoreTransaction { > + friend std::unique_ptr<MemoryBackingStoreTransaction> std::make_unique<MemoryBackingStoreTransaction>(MemoryIDBBackingStore&, const IDBTransactionInfo&); It looks like the EFL EWS wants WebCore::IDBTransactionInfo. I'm not sure why it doesn't like MemoryIDBBackingStore. It's declared right there.

The EFL build failure appears to be yet-another-disappointing compiler limitation that clang handles fine. :(

(In reply to comment #5) > The EFL build failure appears to be yet-another-disappointing compiler > limitation that clang handles fine. :( (Which will likely be fixable by fully qualifying WebCore::IDBTransactionInfo)

I didn't even see that Alex had taken a look, including noting the WebCore:: qualification. *sigh*

(In reply to comment #3) > Comment on attachment 263696 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=263696&action=review > > Looks mostly good. A few comments. Merge conflict is probably in > project.pbxproj > Also, with this new MemoryIDBBackingStore stuff, where does it actually > store the information? I see calls to commit, but I'm not sure where they > go. No information is stored yet. Iterative process. Important new addition here is the backing store transaction, which *does* do something interesting on abort. > > > Source/WebCore/Modules/indexeddb/client/IDBDatabaseImpl.cpp:204 > > + ASSERT(m_activeTransactions.contains(transaction.info().identifier()) > > + || m_committingTransactions.contains(transaction.info().identifier()) > > + || m_abortingTransactions.contains(transaction.info().identifier())); > > It should be in only one of these sets, right? Could you assert that the > sum is one? You just requested an #ifndef NDEBUG block of actual code for a very simple assertion. Remember that you did this to yourself. > > > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:55 > > , m_operationTimer(*this, &IDBTransaction::operationTimerFired) > > > > { > > + m_activationTimer = std::make_unique<Timer>(*this, &IDBTransaction::activationTimerFired); > > Could you initialize m_activationTimer like you do m_operationTimer? m_operationTimer exists for the lifetime of the object. m_activationTimer is a one-off (note it deletes itself once it's fired) > > > Source/WebCore/Modules/indexeddb/client/IDBTransactionImpl.cpp:105 > > + if (isFinishedOrFinishing()) { > > + ec = INVALID_STATE_ERR; > > How would this be reached? A transaction that has already being aborted being asked to abort again. > Can it be tested? Yes, by doing the above. > > > Source/WebCore/Modules/indexeddb/shared/IDBError.cpp:69 > > + static NeverDestroyed<String> entry = ASCIILiteral("Operation was called on an object on which it is not allowed or at a time when it is not allowed."); > > Should these errors be localized? No, we don't localize error strings that are only meant for developers in the console that are explicitly called for in the spec. > Where do we see these errors? There aren't any tests that run into errors. Any time a test runs into errors they can get the description. We do have tests that run into errors, though they're testing other things and not the contents of the errors. Now's not quite the right time to try testing all the descriptions because not all of them can be hit yet. Good idea to test them all - https://bugs.webkit.org/show_bug.cgi?id=150402

Created attachment 263710 [details] Patch v2

Comment on attachment 263710 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=263710&action=review > Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.h:40 > + friend std::unique_ptr<MemoryBackingStoreTransaction> std::make_unique<MemoryBackingStoreTransaction>(MemoryIDBBackingStore&, const WebCore::IDBTransactionInfo&); EFL bot build failure: ../../Source/WebCore/Modules/indexeddb/server/MemoryBackingStoreTransaction.h:40:107: error: 'MemoryIDBBackingStore' has not been declared friend std::unique_ptr<MemoryBackingStoreTransaction> std::make_unique<MemoryBackingStoreTransaction>(MemoryIDBBackingStore&, const WebCore::IDBTransactionInfo&); Strange because I see a forward declaration of this class two lines above!

Created attachment 263712 [details] Patch v3 - Try to fix EFL again.

Created attachment 263713 [details] Patch v4 - Try to fix EFL again

Created attachment 263716 [details] Patch v5 - `git add` fail