[webkit-reviews] review denied: [Bug 204458] ITP Database crashes if table schema is not correct : [Attachment 384108] 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 384108: Patch

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




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

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

I think this is close, but not quite correct.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:316
> +	       || statement.bindText(1, table) != SQLITE_OK) {

I think this would be more efficient if you created the SQLiteStatement outside
the loop, did the prepare outside the loop, and did the 'bindText' inside the
loop for each table name.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:321
> +	       return false;

I'm not sure we should return if we don't receive a result here. Don't we want
to log the missing table name, and then keep going to see what other tables are
missing? This would be helpful when trying to debug a problem.

It would also give us the ability to perform fix-up (in the future) if we want
to add tables to the Schema.

So maybe have a boolean that you set to 'true' if you find any missing tables,
then return that value after completing the loop?

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:323
> +	   resetStatement(statement);

If you leave the object construction inside the loop, you don't need this reset
because the statement is fully reconstructed at each loop iteration.

However, I think you should move construction outside the loop (as I suggest
above).