Bug 194844

Summary: Unify WebsiteDataStore::defaultDataStoreConfiguration across ports
Product: WebKit Reporter: Don Olmstead <don.olmstead>
Component: WebKit Misc.Assignee: Don Olmstead <don.olmstead>
Status: RESOLVED FIXED    
Severity: Normal CC: berto, bfulgham, cgarcia, commit-queue, ews-watchlist, gustavo, Hironori.Fujii, mcatanzaro, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
Hironori.Fujii: review-
Patch
none
Patch
youennf: review+
Patch none

Description Don Olmstead 2019-02-19 17:08:33 PST
Currently not all directories are set within the defaultdataStoreConnection. For example ServiceWorker support will crash since setServiceWorkerRegistrationDirectory is not set.
Comment 1 Don Olmstead 2019-02-19 17:13:56 PST
Created attachment 362456 [details]
Patch
Comment 2 Don Olmstead 2019-02-19 17:47:06 PST
Created attachment 362463 [details]
Patch

Not implementing device id hash salt.
Comment 3 Fujii Hironori 2019-02-19 18:00:01 PST
Do you want to add following, too?

configuration->setCacheStorageDirectory(defaultCacheStorageDirectory());
configuration->setMediaCacheDirectory(defaultMediaCacheDirectory());
configuration->setJavaScriptConfigurationDirectory(defaultJavaScriptConfigurationDirectory());

Do you want to implement following, too?

WebsiteDataStore::defaultMediaCacheDirectory()
WebsiteDataStore::defaultJavaScriptConfigurationDirectory()
Comment 4 Don Olmstead 2019-02-19 18:14:35 PST
Created attachment 362465 [details]
Patch
Comment 5 Fujii Hironori 2019-02-19 18:17:45 PST
Then, you should implement following.

WebsiteDataStore::defaultMediaCacheDirectory()
WebsiteDataStore::defaultJavaScriptConfigurationDirectory()
Comment 6 Don Olmstead 2019-03-01 12:40:34 PST
Changing up the focus of this bug. There are very very slight variations in each port's implementation of defaultDataStoreConfiguration. Honestly this seems to be just bit rot and theres no real reason to have things it in a platform specific source file.
Comment 7 Don Olmstead 2019-03-01 12:46:10 PST
Created attachment 363359 [details]
Patch
Comment 8 Don Olmstead 2019-03-01 12:46:59 PST
Created attachment 363360 [details]
Patch
Comment 9 EWS Watchlist 2019-03-01 12:52:09 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 10 youenn fablet 2019-03-01 13:00:06 PST
Comment on attachment 363360 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/APIWebsiteDataStoreGLib.cpp:-193
> -    configuration->setDeviceIdHashSaltsStorageDirectory(defaultDeviceIdHashSaltsStorageDirectory());

This one might be good to add as well above.
Comment 11 Brent Fulgham 2019-03-01 13:00:55 PST
Comment on attachment 363360 [details]
Patch

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

> Source/WebKit/ChangeLog:18
> +        (API::WebsiteDataStore::defaultDataStoreConfiguration): Deleted.

Ooo! Nice.

>> Source/WebKit/UIProcess/API/glib/APIWebsiteDataStoreGLib.cpp:-193
>> -    configuration->setDeviceIdHashSaltsStorageDirectory(defaultDeviceIdHashSaltsStorageDirectory());
> 
> This one might be good to add as well above.

Should we have a "setPlatformConfiguration<Mumble>" that adds any platform-specific things?

Maybe only glib has this need at the moment.
Comment 12 Brent Fulgham 2019-03-01 13:04:23 PST
Comment on attachment 363360 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/glib/APIWebsiteDataStoreGLib.cpp:-193
>>> -    configuration->setDeviceIdHashSaltsStorageDirectory(defaultDeviceIdHashSaltsStorageDirectory());
>> 
>> This one might be good to add as well above.
> 
> Should we have a "setPlatformConfiguration<Mumble>" that adds any platform-specific things?
> 
> Maybe only glib has this need at the moment.

I take it back -- it's in common code, so let's just add it in the unified location.
Comment 13 Don Olmstead 2019-03-01 13:10:27 PST
Created attachment 363363 [details]
Patch

Good catch on missing call to the Glib device id hash bits. Fixed that.
Comment 14 WebKit Commit Bot 2019-03-01 14:18:27 PST
Comment on attachment 363363 [details]
Patch

Clearing flags on attachment: 363363

Committed r242289: <https://trac.webkit.org/changeset/242289>
Comment 15 WebKit Commit Bot 2019-03-01 14:18:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2019-03-01 14:19:24 PST
<rdar://problem/48522763>