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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Nov 21 11:26:27 PST 2019

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

Attachment 384068: Patch


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

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)

> +    };

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.

> +	   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?

> +	   FileSystem::deleteFile(m_storageDirectoryPath);

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

More information about the webkit-reviews mailing list