Bug 125794 – DatabaseProcess: Implement openTransaction()

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

DatabaseProcess: Implement openTransaction()

[Brady Eidson]

Created attachment 219347 [details]
Patch v1

[WebKit Commit Bot]

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.

[Brady Eidson]

(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.

[Darin Adler]

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)

nullptr

> 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.

[Brady Eidson]

Took Darin's review feedback across the board.  Testing new patch, then will land locally.

[Brady Eidson]

http://trac.webkit.org/changeset/160667