Bug 206601 - Delete the ITP storage file that is not being used (plist or database file) when switching to a new storage type
Summary: Delete the ITP storage file that is not being used (plist or database file) w...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-01-22 11:23 PST by Kate Cheney
Modified: 2020-02-04 13:09 PST (History)
7 users (show)

See Also:

Attachments
Patch (3.20 KB, patch)
2020-01-22 11:32 PST, Kate Cheney 		no flags Details | Formatted Diff | Diff
Patch (9.06 KB, patch)
2020-01-24 17:41 PST, Kate Cheney 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2020-01-22 11:23:55 PST 
Whichever storage mechanism is enabled, the other persistent file should be deleted (either the plist or the db file).
Comment 1 Kate Cheney 2020-01-22 11:24:31 PST 
rdar://problem/58696521
Comment 2 Kate Cheney 2020-01-22 11:32:33 PST 
Created attachment 388450 [details]
Patch
Comment 3 Alex Christensen 2020-01-24 13:28:59 PST 
Comment on attachment 388450 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:167
> +            auto persistentStorageFilePath = FileSystem::pathByAppendingComponent(resourceLoadStatisticsDirectory, "full_browsing_session_resourceLog.plist");

Let's name this variable "legacyPlistFilePath" to indicate that it is something we are migrating from and intent to remove support for in the future.
Comment 4 Kate Cheney 2020-01-24 17:41:27 PST 
Created attachment 388750 [details]
Patch
Comment 5 Kate Cheney 2020-01-24 17:41:56 PST 
This was causing API test failures, I had to make some changes to the tests in this patch. Alex, I thought you might want to take another look before confirming r+.
Comment 6 WebKit Commit Bot 2020-01-27 11:16:28 PST 
Comment on attachment 388750 [details]
Patch

Clearing flags on attachment: 388750

Committed r255156: <https://trac.webkit.org/changeset/255156>
Comment 7 WebKit Commit Bot 2020-01-27 11:16:30 PST 
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2020-01-27 11:17:17 PST 
<rdar://problem/58926444>
Comment 9 Conrad Shultz 2020-02-04 13:09:55 PST 
Comment on attachment 388750 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:168
> +            if (FileSystem::fileExists(legacyPlistFilePath))

It's generally an anti-pattern to check for file-existence. Can we just attempt deletion and handle failure as needed?

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:178
> +            if (FileSystem::fileExists(databaseStorageFilePath)) {

Ditto.