Bug 125794

Summary: DatabaseProcess: Implement openTransaction()
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Severity: Normal CC: commit-queue
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 125798    
Bug Blocks: 124521    
Description Flags
Patch v1 darin: review+

Description Brady Eidson 2013-12-16 11:56:48 PST
DatabaseProcess: Implement openTransaction()
Comment 1 Brady Eidson 2013-12-16 13:27:22 PST
Created attachment 219347 [details]
Patch v1
Comment 2 WebKit Commit Bot 2013-12-16 13:29:11 PST
Attachment 219347 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.messages.in', u'Source/WebKit2/DatabaseProcess/IndexedDB/IDBTransactionIdentifier.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabaseBackingStore.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/SQLiteIDBTransaction.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/SQLiteIDBTransaction.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.h', u'Source/WebKit2/Shared/WebCrossThreadCopier.cpp', u'Source/WebKit2/Shared/WebCrossThreadCopier.h', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.cpp', u'Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.h', u'Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.messages.in', '--commit-queue']" exit_code: 1
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:212:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 18 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Brady Eidson 2013-12-16 13:33:19 PST
(In reply to comment #2)
> Attachment 219347 [details] did not pass style-queue:
> ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:212:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
> Total errors found: 1 in 18 files
> If any of these errors are false positives, please file a bug against check-webkit-style.

Yup, https://bugs.webkit.org/show_bug.cgi?id=125616 still exists.
Comment 4 Darin Adler 2013-12-16 14:40:28 PST
Comment on attachment 219347 [details]
Patch v1

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

> Source/WebKit2/DatabaseProcess/IndexedDB/IDBTransactionIdentifier.h:41
> +        : m_connection(0)


> Source/WebKit2/DatabaseProcess/IndexedDB/IDBTransactionIdentifier.h:54
> +        return IDBTransactionIdentifier(*this);

Why doesn’t this just say:

     return *this;

I don’t understand the reason for repeating the class name.

> Source/WebKit2/DatabaseProcess/IndexedDB/IDBTransactionIdentifier.h:68
> +    static IDBTransactionIdentifier constructDeletedValue()

Not sure I understand the meaning of the word “construct” here. This function should just be named deletedValue.

> Source/WebKit2/DatabaseProcess/IndexedDB/IDBTransactionIdentifier.h:72
> +        return std::move(value);

Not sure there is any benefit to using std::move here. Just return value should work.

> Source/WebKit2/DatabaseProcess/IndexedDB/IDBTransactionIdentifier.h:101
> +template<> struct HashTraits<WebKit::IDBTransactionIdentifier> : GenericHashTraits<WebKit::IDBTransactionIdentifier> {

This should derive from SimpleClassHashTraits and use the idiom where we construct with HashTableDeletedValue and provide a constructor that takes a HashTableDeletedValue argument. Then we would not need any members defined here at all.

> Source/WebKit2/DatabaseProcess/IndexedDB/IDBTransactionIdentifier.h:102
> +    static const bool emptyValueIsZero = 0;

0 seems wrong here since it’s a boolean. Also, why is this false? It seems to me that the empty value is zero! I think this should be true, and perhaps just inherited from the other.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:216
> +    m_pendingOpenTransactionRequests.set(identifier, request.release());

Why is this a set rather than an add? Should this have an assertion that there is no already a request for this identifier?

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:234
> +    RefPtr<AsyncRequest> request = m_pendingOpenTransactionRequests.take(identifier);
> +    ASSERT(request);

Is this a solid guarantee? Is there any way the request might have been canceled, for example?

> Source/WebKit2/DatabaseProcess/IndexedDB/sqlite/UniqueIDBDatabaseBackingStoreSQLite.cpp:201
> +    m_transactions.set(identifier, SQLiteIDBTransaction::create(identifier));

Why set rather than add?

> Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.cpp:121
> +    m_serverRequests.set(requestID, serverRequest.release());

Why set rather than add? Should we assert there is no request already with this ID?

> Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.cpp:129
> +    RefPtr<AsyncRequest> serverRequest = m_serverRequests.take(requestID);
> +    ASSERT(serverRequest);

Is this assertion guaranteed? Should there be an early return? Generally I don’t know why we are trusting IDs that have traveled across the process boundary.
Comment 5 Brady Eidson 2013-12-16 15:04:50 PST
Took Darin's review feedback across the board.  Testing new patch, then will land locally.
Comment 6 Brady Eidson 2013-12-16 15:20:33 PST