[webkit-reviews] review granted: [Bug 211004] Create a mechanism to add missing ITP Database tables when the schema is updated : [Attachment 397733] Patch

[bugzilla-daemon at webkit.org]

John Wilander <wilander at apple.com> has granted katherine_cheney at apple.com's
request for review:
Bug 211004: Create a mechanism to add missing ITP Database tables when the
schema is updated
https://bugs.webkit.org/show_bug.cgi?id=211004

Attachment 397733: Patch

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




--- Comment #5 from John Wilander <wilander at apple.com> ---
Comment on attachment 397733
  --> https://bugs.webkit.org/attachment.cgi?id=397733
Patch

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

Please address my comments. Other than that, looks good!

> Source/WebKit/ChangeLog:62
> +	   the new table wasn't added upon opening the database.

Your description of the test sounds like the code is still broken. Is it a
description of the bug? If so, please rephrase so that the test is described to
test fixed functionality. Something like "This tests that the database is not
dropped when a new table is added upon opening the database."

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:368
> +void
ResourceLoadStatisticsDatabaseStore::isCorrectTableSchema(Vector<String>&
missingTables)

We should renamed this function now that it no longer returns a boolean. Would
checkForMissingTablesInSchema() work? See my subsequent comment on this.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:478
> +	       RELEASE_LOG_ERROR(Network, "%p -
ResourceLoadStatisticsDatabaseStore::addMissingTables failed to execute, error
message: %" PUBLIC_LOG_STRING, this, m_database.lastErrorMsg());

I assume you want to carry on trying here and not return early on an error.

>
Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp
:493
> +    isCorrectTableSchema(missingTables);

Seeing how this is used, I think it would be better to have a function like
this Optional<Vector<String>>
ResourceLoadStatisticsDatabaseStore::getMissingTablesInSchema().

>
Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:329
> +	       completionHandler(false);

We should call ASSERT_NOT_REACHED() here since such a call should never happen.
It's risky since a call to completionHandler(false) will be interpreted as a
passing test.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1593
> +	       return;

This assumes there is only one network process. Is that what you want?

> Tools/ChangeLog:10
> +	   the ITP database file, then ensures the pre-seeded data has been

ensures the pre-seeded data is …

> Tools/ChangeLog:11
> +	   migrated over and that the schema has all tables (including the

the schema now has all tables …

> Tools/ChangeLog:12
> +	   missing one).

previously missing one