Summary: | WebsiteDataStore::excludeDirectoryFromBackup should set attribute for existing directories | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sihui Liu <sihui_liu> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Sihui Liu <sihui_liu> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | achristensen, beidson, benjamin, cdumez, cmarcelo, ews-watchlist, ggaren, webkit-bug-importer, youennf | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Sihui Liu
2021-12-16 13:48:34 PST
Created attachment 447390 [details]
Patch
Created attachment 447392 [details]
Patch
What's the use case where the directory already exists but is not excluded? Can we add a test for that case? (In reply to Geoffrey Garen from comment #3) > What's the use case where the directory already exists but is not excluded? > Can we add a test for that case? We currently create storage directories when we decide the location, for example see WebsiteDataStore::defaultLocalStorageDirectory() and WebsiteDataStore::websiteDataDirectoryFileSystemRepresentation in WebsiteDataStoreCocoa.mm. That means when we call excludeDirectoryFromBackup, the directory should already exist (WebsiteDataStore parameters should be decided before that). I can try creating an API test. Created attachment 447459 [details]
Patch
Created attachment 447618 [details]
Patch
Comment on attachment 447618 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447618&action=review > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:649 > + if (![url setResourceValue:@YES forKey:NSURLIsExcludedFromBackupKey error:nil]) It would be good to log error. Maybe write something like [url setResourceValue:@YES forKey:NSURLIsExcludedFromBackupKey error:error]; RELEASE_LOG_ERROR_IF(error, Storage, ... +log error). > Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:463 > + [[NSFileManager defaultManager] removeItemAtPath:url.absoluteString error:nil]; Should we create the directory without the key to make sure the new code path makes it working? Comment on attachment 447618 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=447618&action=review >> Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:649 >> + if (![url setResourceValue:@YES forKey:NSURLIsExcludedFromBackupKey error:nil]) > > It would be good to log error. > Maybe write something like > [url setResourceValue:@YES forKey:NSURLIsExcludedFromBackupKey error:error]; > RELEASE_LOG_ERROR_IF(error, Storage, ... +log error). Sure, will add. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:463 >> + [[NSFileManager defaultManager] removeItemAtPath:url.absoluteString error:nil]; > > Should we create the directory without the key to make sure the new code path makes it working? Sure. Will add the test. Created attachment 447825 [details]
Patch
Committed r287377 (245517@main): <https://commits.webkit.org/245517@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 447825 [details]. |