[webkit-reviews] review granted: [Bug 115680] More work on LocalStorageDatabaseTracker : [Attachment 200851] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <akling at apple.com> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 115680: More work on LocalStorageDatabaseTracker
https://bugs.webkit.org/show_bug.cgi?id=115680

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

------- Additional Comments from Andreas Kling <akling at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=200851&action=review


r=me

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp:96
> +    if (!fileExists(databasePath) && openingStrategy == SkipIfNonExistent)
> +	   return;
> +
> +    if (!m_database.open(databasePath)) {
> +	   LOG_ERROR("Failed to open databasePath %s.",
databasePath.ascii().data());
> +	   return;
> +    }

There's a race between checking if the file exists and opening it. Looks a bit
shoddy.

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp:98
> +    m_database.disableThreadingChecks();

This needs a comment.

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp:111
> +    openTrackerDatabase(SkipIfNonExistent);
> +
> +    if (m_database.isOpen()) {

I'd make openTrackerDatabase() return a bool for success and branch on that
instead.

> Source/WebKit2/UIProcess/Storage/LocalStorageDatabaseTracker.cpp:129
> +    updateTrackerDatabaseFromLocalStorageDatabaseFiles();

Should we still be doing this if opening the database failed?