[webkit-reviews] review granted: [Bug 203690] IndexedDB: perform IDBServer work only on background thread : [Attachment 385206] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 203690: IndexedDB: perform IDBServer work only on background thread
https://bugs.webkit.org/show_bug.cgi?id=203690

Attachment 385206: Patch

https://bugs.webkit.org/attachment.cgi?id=385206&action=review




--- Comment #21 from Brady Eidson <beidson at apple.com> ---
Comment on attachment 385206
  --> https://bugs.webkit.org/attachment.cgi?id=385206
Patch

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

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:147
> +    m_pendingDatabaseRequests.add(ServerOpenDBRequest::create(connection,
requestData));

See other comment about the fact that this set's name should clearly call out
that it is a set of "OpenDBRequest" objects ^^^

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:-1687
> -    ASSERT(isMainThread());

What thread does this run on?
It should be exactly one, and have an assertion to that effect

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h:156
> -    ListHashSet<RefPtr<ServerOpenDBRequest>> m_pendingOpenDBRequests;
> -    RefPtr<ServerOpenDBRequest> m_currentOpenDBRequest;
> +    ListHashSet<RefPtr<ServerOpenDBRequest>> m_pendingDatabaseRequests;
> +    RefPtr<ServerOpenDBRequest> m_currentDatabaseRequest;

I think renaming these makes it harder to understand what these objects are.

They *are* proxies of "IDBOpenDBRequests", so giving them a more generic name
loses that meaning.

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBConnectionToClient.cpp:60
> -    send(Messages::WebIDBConnectionToServer::DidDeleteDatabase(resultData));
> +   
m_connection->send(Messages::WebIDBConnectionToServer::DidDeleteDatabase(result
Data), 0);

Add a send() to WebIDBConnectionToClient that does this repeated boilerplate,
and then many of these don't have to change.

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:370
> +    m_server->getAllDatabaseNames(idbConnection->identifier(), topOrigin,
openingOrigin, callbackID);

No lock?