[webkit-reviews] review denied: [Bug 204458] ITP Database crashes if table schema is not correct : [Attachment 384068] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied katherine_cheney at apple.com's
request for review:
Bug 204458: ITP Database crashes if table schema is not correct
https://bugs.webkit.org/show_bug.cgi?id=204458

Attachment 384068: Patch

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




--- Comment #3 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 384068
  --> https://bugs.webkit.org/attachment.cgi?id=384068
Patch

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

> Source/WebKit/ChangeLog:12
> +	   any manual changes to the database file.

How does this relate to the "migration" task (see Bug 195294)

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:305
> +    };

It's a shame we need this array of strings in addition to the schema creation
string (createObservedDomain) and the enum of column ID's for each of these. I
wonder if there is a way to consolidate this information somehow.

At the very least, I feel like this array of strings should live very close to
the table creation string to make it harder to add something in one place and
not the other.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:308
> +	   SQLiteStatement statement(m_database, "SELECT 1 from sqlite_master
WHERE type='table' and tbl_name=?");

Can this be done more efficiently?

For example, could we do something like "SELECT COUNT(*) FROM sqlite_master
WHERE type='table' and tbl_name IN (... the list of tables)" or something?

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:326
> +	   FileSystem::deleteFile(m_storageDirectoryPath);

We should add a FIXME(195294): Perform migration here.