Bug 238269 – Introduce an SPI to get website data directory for origin and use it in API tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Website Storage

[Sihui Liu]

...

[Sihui Liu]

Created attachment 455514 [details]
Patch

[Sihui Liu]

Created attachment 455523 [details]
Patch

[Alex Christensen]

Comment on attachment 455523 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        storage), we need to update these tests with new paths, otherwise the tests will check the wrong paths. Let's 
> +        just add an SPI so these tests can get the paths in use dynamically, which would avoid test breakage when
> +        we do website data migration.

As we do this, we should also add tests that write data to the old location and verify that the migration happens successfully.

> Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:653
> +        removeOriginStorageManagerIfPossible(origin);

Why is this done in a function that just gets the directory?

[Sihui Liu]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 455523 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455523&action=review
> 
> > Source/WebKit/ChangeLog:12
> > +        storage), we need to update these tests with new paths, otherwise the tests will check the wrong paths. Let's 
> > +        just add an SPI so these tests can get the paths in use dynamically, which would avoid test breakage when
> > +        we do website data migration.
> 
> As we do this, we should also add tests that write data to the old location
> and verify that the migration happens successfully.

We have test for migration, e.g. WebKit.MigrateLocalStorageDataToGeneralStorageDirectory, where we still use hardcoded paths (since we want to ensure files are moved correctly from old locations to new locations.)

> 
> > Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp:653
> > +        removeOriginStorageManagerIfPossible(origin);
> 
> Why is this done in a function that just gets the directory?

Because localOriginStorageManager(origin) may create a new OriginStorageManager to get directory, and we don't want to hold the object if it is not in use after getting directory.

[Chris Dumez]

Comment on attachment 455523 [details]
Patch

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

> Source/WebKit/Scripts/webkit/messages.py:965
> +        'WebKit::WebsiteDataType': ['"WebsiteDataType.h"'],

Why is this needed? The header name matches the class name so it shouldn't be needed.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:854
> +    _websiteDataStore->originDirectoryForTesting(origin, topOrigin, *websiteDataType, [completionHandlerCopy](auto result) {

completionHandlerCopy = WTFMove(completionHandlerCopy)

[Sihui Liu]

Created attachment 455727 [details]
Patch for landing

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 455727 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Sihui Liu]

Created attachment 455730 [details]
Patch for landing

[EWS]

Committed r291851 (248863@main): <https://commits.webkit.org/248863@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455730 [details].

[Radar WebKit Bug Importer]

<rdar://problem/90825693>