[webkit-reviews] review granted: [Bug 125794] DatabaseProcess: Implement openTransaction() : [Attachment 219347] Patch v1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 125794: DatabaseProcess: Implement openTransaction()
https://bugs.webkit.org/show_bug.cgi?id=125794

Attachment 219347: Patch v1
https://bugs.webkit.org/attachment.cgi?id=219347&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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/UniqueIDBDatabaseBackingStoreSQ
Lite.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.