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, fujii, gustavo, 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
fujii: review-
Patch
none
Patch
youennf: review+
Patch none

Don Olmstead
Reported 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.
Attachments
Patch (2.92 KB, patch)
2019-02-19 17:13 PST, Don Olmstead 		no flags
DetailsFormatted DiffDiff
Patch (1.94 KB, patch)
2019-02-19 17:47 PST, Don Olmstead 		no flags
DetailsFormatted DiffDiff
Patch (2.14 KB, patch)
2019-02-19 18:14 PST, Don Olmstead 		fujii: review-
DetailsFormatted DiffDiff
Patch (7.16 KB, patch)
2019-03-01 12:46 PST, Don Olmstead 		no flags
DetailsFormatted DiffDiff
Patch (7.32 KB, patch)
2019-03-01 12:46 PST, Don Olmstead 		youennf: review+
DetailsFormatted DiffDiff
Patch (8.80 KB, patch)
2019-03-01 13:10 PST, Don Olmstead 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Don Olmstead
Comment 1 2019-02-19 17:13:56 PST
Created attachment 362456 [details] Patch
Don Olmstead
Comment 2 2019-02-19 17:47:06 PST
Created attachment 362463 [details] Patch Not implementing device id hash salt.
Fujii Hironori
Comment 3 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()
Don Olmstead
Comment 4 2019-02-19 18:14:35 PST
Created attachment 362465 [details] Patch
Fujii Hironori
Comment 5 2019-02-19 18:17:45 PST
Then, you should implement following. WebsiteDataStore::defaultMediaCacheDirectory() WebsiteDataStore::defaultJavaScriptConfigurationDirectory()
Don Olmstead
Comment 6 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.
Don Olmstead
Comment 7 2019-03-01 12:46:10 PST
Created attachment 363359 [details] Patch
Don Olmstead
Comment 8 2019-03-01 12:46:59 PST
Created attachment 363360 [details] Patch
EWS Watchlist
Comment 9 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
youenn fablet
Comment 10 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.
Brent Fulgham
Comment 11 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.
Brent Fulgham
Comment 12 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.
Don Olmstead
Comment 13 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.
WebKit Commit Bot
Comment 14 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>
WebKit Commit Bot
Comment 15 2019-03-01 14:18:29 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 16 2019-03-01 14:19:24 PST
<rdar://problem/48522763>
Note You need to log in before you can comment on or make changes to this bug.