[webkit-reviews] review granted: [Bug 123028] Move IDBTransactionBackend operations to the IDBTransactionBackend itself. : [Attachment 214594] Patch v1 - Code shoveling!

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 123028: Move IDBTransactionBackend operations to the IDBTransactionBackend
itself.
https://bugs.webkit.org/show_bug.cgi?id=123028

Attachment 214594: Patch v1 - Code shoveling!
https://bugs.webkit.org/attachment.cgi?id=214594&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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.