[webkit-reviews] review granted: [Bug 135218] IDB transactions never reset if the Web Process ends before cleaning up : [Attachment 235557] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jeffrey Pfau <jpfau at apple.com>'s
request for review:
Bug 135218: IDB transactions never reset if the Web Process ends before
cleaning up
https://bugs.webkit.org/show_bug.cgi?id=135218

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=235557&action=review


> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:1084
> +    auto transactions =
m_establishedTransactions.add(&transactionIdentifier.connection(),
HashSet<IDBIdentifier>());

It’s a little strange to call this result of the add function “transactions”. I
normally call such things “addResult”.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:1095
> +    auto transactions =
m_establishedTransactions.add(&transactionIdentifier.connection(),
HashSet<IDBIdentifier>());
> +    transactions.iterator->value.remove(transactionIdentifier);

Here we could use find instead of add, since we don’t need to add anything if
it’s empty:

    auto slot =
m_establishedTransactions.find(&transactionIdentifier.connection());
    if (slot == m_establishedTransactions.end())
	return;
    slot->value.remove(transactionIdentifier);

It will be a little lower overhead than doing the add, avoiding the creation of
the temporary HashSet.

We could even assert that it is not equal to end if we are sure that won’t
happen.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:1101
> +    auto transactionIdentifiers =
m_establishedTransactions.find(&connection);

It’s a little peculiar to call the iterator “identifiers”. I normally call such
things “slot” or sometimes “iterator”, or maybe “map entry”.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:1105
> +    for (auto transactionIdentifier : transactionIdentifiers.get()->value) {


I am surprised the get() is needed here.

I also think we’d get something slightly more efficient if we used auto&
instead of auto.