WebKit::Database::needsUpdatedSchema() has a comment saying it should be removed late 2021. WebKit::DatabaseUtilities::migrateDataToNewTablesIfNecessary() returns directly if WebKit::Database::needsUpdatedSchema() returns false so WebKit::DatabaseUtilities::migrateDataToNewTablesIfNecessary() should be removed at the same time. Likewise the static WebKit::insertDistinctValuesInTableStatement() in DatabaseUtilities.cpp will no longer be needed. We should also add a unit test for the new columns for attributed measurements with destination tokens.
<rdar://problem/86443135>
Created attachment 447089 [details] Patch
Thanks, Alex! I'll wait for Kate to comment before landing since this patch removes a bunch of code she wrote.
Comment on attachment 447089 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447089&action=review I'm still a bit fuzzy on what was causing the crash and how this will fix it. > Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:-274 > -void DatabaseUtilities::migrateDataToNewTablesIfNecessary() Please keep this for ITP. It has shipped with schema changes and therefore needs migration code. PCM schema changes did not ship which is why it is OK to remove them.
(In reply to Kate Cheney from comment #4) > Comment on attachment 447089 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=447089&action=review > > I'm still a bit fuzzy on what was causing the crash and how this will fix it. Agreed. Error message at the failed assertion was "DatabaseUtilities::migrateDataToNewTablesIfNecessary failed to rename table, error message: There is already another table or index with this name: _UnattributedPrivateClickMeasurement." > > Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:-274 > > -void DatabaseUtilities::migrateDataToNewTablesIfNecessary() > > Please keep this for ITP. It has shipped with schema changes and therefore > needs migration code. PCM schema changes did not ship which is why it is OK > to remove them. Aha! Will do.
Created attachment 447099 [details] Patch
Comment on attachment 447099 [details] Patch Waiting for green EWS before landing.
mac-AS-debug-wk2 layout test failure is unrelated. ios-wk2 far behind and hasn't even started. api-ios is >50 minutes into running tests.
Committed r287013 (245218@main): <https://commits.webkit.org/245218@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447099 [details].
(In reply to John Wilander from comment #5) > (In reply to Kate Cheney from comment #4) > > Comment on attachment 447089 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=447089&action=review > > > > I'm still a bit fuzzy on what was causing the crash and how this will fix it. > > Agreed. Error message at the failed assertion was > "DatabaseUtilities::migrateDataToNewTablesIfNecessary failed to rename > table, error message: There is already another table or index with this > name: _UnattributedPrivateClickMeasurement." > This doesn't really address the root cause of the problem, which is that data migration failed to delete the altered tables at some point and then when trying again it crashes when it finds the existing tables. Instead this just leaves those altered tables around and stops migration. But I guess since existing tests are passing it's probably not a bug with ITP and this will be sufficient. One thing we could do to make this more robust in the future is check if any tables exist that we don't expect and delete them. > > > Source/WebKit/NetworkProcess/DatabaseUtilities.cpp:-274 > > > -void DatabaseUtilities::migrateDataToNewTablesIfNecessary() > > > > Please keep this for ITP. It has shipped with schema changes and therefore > > needs migration code. PCM schema changes did not ship which is why it is OK > > to remove them. > > Aha! Will do.
Rebaselined tests for iOS in https://trac.webkit.org/changeset/287102/webkit