Bug 234404

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 Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch none

Description Sihui Liu 2021-12-16 13:48:34 PST
...
Comment 1 Sihui Liu 2021-12-16 13:54:12 PST
Created attachment 447390 [details]
Patch
Comment 2 Sihui Liu 2021-12-16 13:59:42 PST
Created attachment 447392 [details]
Patch
Comment 3 Geoffrey Garen 2021-12-16 15:31:43 PST
What's the use case where the directory already exists but is not excluded? Can we add a test for that case?
Comment 4 Sihui Liu 2021-12-16 16:11:27 PST
(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.
Comment 5 Sihui Liu 2021-12-17 10:20:32 PST
Created attachment 447459 [details]
Patch
Comment 6 Sihui Liu 2021-12-20 12:35:37 PST
Created attachment 447618 [details]
Patch
Comment 7 youenn fablet 2021-12-21 01:43:45 PST
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 8 Sihui Liu 2021-12-21 11:00:53 PST
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.
Comment 9 Sihui Liu 2021-12-22 12:13:34 PST
Created attachment 447825 [details]
Patch
Comment 10 EWS 2021-12-22 15:56:25 PST
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].
Comment 11 Radar WebKit Bug Importer 2021-12-22 15:57:19 PST
<rdar://problem/86829012>