Bug 123028 - Move IDBTransactionBackend operations to the IDBTransactionBackend itself.
Summary: Move IDBTransactionBackend operations to the IDBTransactionBackend itself.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 123027
  Show dependency tree / graph
 
Reported: 2013-10-18 10:29 PDT by Brady Eidson
Modified: 2013-10-18 13:19 PDT (History)
5 users (show)

See Also:

Attachments
Patch v1 - Code shoveling! (114.60 KB, patch)
2013-10-18 12:22 PDT, Brady Eidson 		ap: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2013-10-18 10:29:53 PDT 
Move IDBTransactionBackend operations to the IDBTransactionBackend itself.
Comment 1 Brady Eidson 2013-10-18 12:22:35 PDT 
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)
Comment 2 WebKit Commit Bot 2013-10-18 12:25:09 PDT 
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.
Comment 3 Alexey Proskuryakov 2013-10-18 12:40:22 PDT 
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.
Comment 4 Brady Eidson 2013-10-18 12:51:14 PDT 
(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.
Comment 5 Brady Eidson 2013-10-18 13:11:07 PDT 
(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.
Comment 6 Brady Eidson 2013-10-18 13:13:46 PDT 
(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.
Comment 7 Brady Eidson 2013-10-18 13:19:11 PDT 
Landed in http://trac.webkit.org/changeset/157644

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