Bug 123028 – Move IDBTransactionBackend operations to the IDBTransactionBackend itself.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Move IDBTransactionBackend operations to the IDBTransactionBackend itself.

[Brady Eidson]

Created attachment 214594 [details]
Patch v1 - Code shoveling!

Tested the build on Mac and it worked great.

Let EWS churn on this.  Unfortunately non-Mac EWS doesn't run tests...  (But there should be no change in behavior)

[WebKit Commit Bot]

Attachment 214594 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Modules/indexeddb/IDBTransactionBackendInterface.h', u'Source/WebCore/Modules/indexeddb/leveldb/IDBCursorBackendLevelDB.cpp', u'Source/WebCore/Modules/indexeddb/leveldb/IDBDatabaseBackendLevelDB.cpp', u'Source/WebCore/Modules/indexeddb/leveldb/IDBDatabaseBackendLevelDB.h', u'Source/WebCore/Modules/indexeddb/leveldb/IDBObjectStoreBackendLevelDB.cpp', u'Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDB.cpp', u'Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDB.h', u'Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDBOperations.cpp', u'Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDBOperations.h', u'Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionCoordinatorLevelDB.cpp']" exit_code: 1
Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDB.h:82:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/Modules/indexeddb/IDBTransactionBackendInterface.h:57:  The parameter name "key" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/Modules/indexeddb/IDBTransactionBackendInterface.h:68:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 3 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 214594 [details]
Patch v1 - Code shoveling!

View in context: https://bugs.webkit.org/attachment.cgi?id=214594&action=review

> Source/WebCore/Modules/indexeddb/leveldb/IDBCursorBackendLevelDB.cpp:50
> +    virtual void perform();

Please go over the patch and add OVERRIDE and FINAL everywhere.

> Source/WebCore/Modules/indexeddb/leveldb/IDBDatabaseBackendLevelDB.h:126
> +        PassRefPtr<IDBCallbacks> callbacks() { return m_callbacks; }
> +        PassRefPtr<IDBDatabaseCallbacks> databaseCallbacks() { return m_databaseCallbacks; }

PassRefPtr seems very inappropriate here, should be a raw pointer. There is no passing of ownership.

> Source/WebCore/Modules/indexeddb/leveldb/IDBDatabaseBackendLevelDB.h:144
>      Deque<OwnPtr<PendingOpenCall> > m_pendingOpenCalls;

Please change "> >" to ">>" everywhere.

> Source/WebCore/Modules/indexeddb/leveldb/IDBDatabaseBackendLevelDB.h:153
> +        PassRefPtr<IDBCallbacks> callbacks() { return m_callbacks; }

Same comment about PassRefPtr.

> Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDB.h:82
> +    virtual void scheduleGetOperation(const IDBDatabaseMetadata&, int64_t objectStoreId, int64_t indexId, PassRefPtr<IDBKeyRange>, IndexedDB::CursorType, PassRefPtr<IDBCallbacks>);
> +    virtual void schedulePutOperation(const IDBObjectStoreMetadata&, PassRefPtr<SharedBuffer> value, PassRefPtr<IDBKey> key, IDBDatabaseBackendInterface::PutMode, PassRefPtr<IDBCallbacks>, const Vector<int64_t>& indexIds, const Vector<IDBDatabaseBackendInterface::IndexKeys>&);

All these PassRefPtr make me a little nervous too.

> Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDBOperations.cpp:98
> +                // Index Value Retrieval Operation
> +                backingStoreCursor = m_backingStore->openIndexKeyCursor(m_transaction->backingStoreTransaction(), m_databaseId, m_objectStoreId, m_indexId, m_keyRange.get(), IndexedDB::CursorNext);

There should be braces here.

> Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDBOperations.cpp:101
> +            else
> +                // Index Referenced Value Retrieval Operation
> +                backingStoreCursor = m_backingStore->openIndexCursor(m_transaction->backingStoreTransaction(), m_databaseId, m_objectStoreId, m_indexId, m_keyRange.get(), IndexedDB::CursorNext);

And here.

> Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDBOperations.cpp:135
> +

Blank line.

> Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDBOperations.cpp:192
> +    ASSERT(key && key->isValid());

Please split into two ASSERTs.

> Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDBOperations.cpp:208
> +    Vector<OwnPtr<IDBObjectStoreBackendLevelDB::IndexWriter> > indexWriters;

Don't we use unique_ptr everywhere now?

Please try to replace all OwnPtrs and PassOwnPtrs to see what happens.

[Brady Eidson]

(In reply to comment #3)
> (From update of attachment 214594 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=214594&action=review
> 
> > Source/WebCore/Modules/indexeddb/leveldb/IDBCursorBackendLevelDB.cpp:50
> > +    virtual void perform();
> 
> Please go over the patch and add OVERRIDE and FINAL everywhere.
> 
> > Source/WebCore/Modules/indexeddb/leveldb/IDBDatabaseBackendLevelDB.h:126
> > +        PassRefPtr<IDBCallbacks> callbacks() { return m_callbacks; }
> > +        PassRefPtr<IDBDatabaseCallbacks> databaseCallbacks() { return m_databaseCallbacks; }
> 
> PassRefPtr seems very inappropriate here, should be a raw pointer. There is no passing of ownership.

Longstanding from the original Google implementation. I fixed it in some places, but will clean up a few more obvious ones like this.

> > Source/WebCore/Modules/indexeddb/leveldb/IDBDatabaseBackendLevelDB.h:144
> >      Deque<OwnPtr<PendingOpenCall> > m_pendingOpenCalls;
> 
> Please change "> >" to ">>" everywhere.

YAY!

> > Source/WebCore/Modules/indexeddb/leveldb/IDBDatabaseBackendLevelDB.h:153
> > +        PassRefPtr<IDBCallbacks> callbacks() { return m_callbacks; }
> 
> Same comment about PassRefPtr.
> 
> > Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDB.h:82
> > +    virtual void scheduleGetOperation(const IDBDatabaseMetadata&, int64_t objectStoreId, int64_t indexId, PassRefPtr<IDBKeyRange>, IndexedDB::CursorType, PassRefPtr<IDBCallbacks>);
> > +    virtual void schedulePutOperation(const IDBObjectStoreMetadata&, PassRefPtr<SharedBuffer> value, PassRefPtr<IDBKey> key, IDBDatabaseBackendInterface::PutMode, PassRefPtr<IDBCallbacks>, const Vector<int64_t>& indexIds, const Vector<IDBDatabaseBackendInterface::IndexKeys>&);
> 
> All these PassRefPtr make me a little nervous too.

Longstanding, so not changing behavior here...  but I'll nuke them.

> > Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDBOperations.cpp:98
> > +                // Index Value Retrieval Operation
> > +                backingStoreCursor = m_backingStore->openIndexKeyCursor(m_transaction->backingStoreTransaction(), m_databaseId, m_objectStoreId, m_indexId, m_keyRange.get(), IndexedDB::CursorNext);
> 
> There should be braces here.
> > Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDBOperations.cpp:101
> > +            else
> > +                // Index Referenced Value Retrieval Operation
> > +                backingStoreCursor = m_backingStore->openIndexCursor(m_transaction->backingStoreTransaction(), m_databaseId, m_objectStoreId, m_indexId, m_keyRange.get(), IndexedDB::CursorNext);
> 
> And here.

Will do.


> > Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDBOperations.cpp:192
> > +    ASSERT(key && key->isValid());
> 
> Please split into two ASSERTs.

Okay.

> 
> > Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDBOperations.cpp:208
> > +    Vector<OwnPtr<IDBObjectStoreBackendLevelDB::IndexWriter> > indexWriters;
> 
> Don't we use unique_ptr everywhere now?
> 
> Please try to replace all OwnPtrs and PassOwnPtrs to see what happens.

Ruh roh...  I'll consider this.

[Brady Eidson]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 214594 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=214594&action=review
> > > Source/WebCore/Modules/indexeddb/leveldb/IDBDatabaseBackendLevelDB.h:126
> > > +        PassRefPtr<IDBCallbacks> callbacks() { return m_callbacks; }
> > > +        PassRefPtr<IDBDatabaseCallbacks> databaseCallbacks() { return m_databaseCallbacks; }
> > 
> > PassRefPtr seems very inappropriate here, should be a raw pointer. There is no passing of ownership.
> 
> > > Source/WebCore/Modules/indexeddb/leveldb/IDBDatabaseBackendLevelDB.h:153
> > > +        PassRefPtr<IDBCallbacks> callbacks() { return m_callbacks; }
> > 
> > Same comment about PassRefPtr.

Changed these, but...

> > 
> > > Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDB.h:82
> > > +    virtual void scheduleGetOperation(const IDBDatabaseMetadata&, int64_t objectStoreId, int64_t indexId, PassRefPtr<IDBKeyRange>, IndexedDB::CursorType, PassRefPtr<IDBCallbacks>);
> > > +    virtual void schedulePutOperation(const IDBObjectStoreMetadata&, PassRefPtr<SharedBuffer> value, PassRefPtr<IDBKey> key, IDBDatabaseBackendInterface::PutMode, PassRefPtr<IDBCallbacks>, const Vector<int64_t>& indexIds, const Vector<IDBDatabaseBackendInterface::IndexKeys>&);
> > 
> > All these PassRefPtr make me a little nervous too.
> 
> Longstanding, so not changing behavior here...  but I'll nuke them.

Most of these actually do pass ownership, so I'm going to leave all of them.

[Brady Eidson]

(In reply to comment #3)
> (From update of attachment 214594 [details])
> > Source/WebCore/Modules/indexeddb/leveldb/IDBTransactionBackendLevelDBOperations.cpp:208
> > +    Vector<OwnPtr<IDBObjectStoreBackendLevelDB::IndexWriter> > indexWriters;
> 
> Don't we use unique_ptr everywhere now?
> 
> Please try to replace all OwnPtrs and PassOwnPtrs to see what happens.

This explodes the scope of the patch a bit more than it already has.  I'll pass for now, but keep it in mind for a followup and future refactors in this area.

[Brady Eidson]

Landed in http://trac.webkit.org/changeset/157644

With the wrong log message.  :(  God dammit...