WebsiteDataStore::excludeDirectoryFromBackup should set attribute for existing directories

[Sihui Liu]

...

[Sihui Liu]

Created attachment 447390 [details]
Patch

[Sihui Liu]

Created attachment 447392 [details]
Patch

[Geoffrey Garen]

What's the use case where the directory already exists but is not excluded? Can we add a test for that case?

[Sihui Liu]

(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.

[Sihui Liu]

Created attachment 447459 [details]
Patch

[Sihui Liu]

Created attachment 447618 [details]
Patch

[youenn fablet]

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?

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 447825 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/86829012>