Bug 116729

Summary: [Windows] Expose database storage and cache locations via preferences
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, bfulgham, commit-queue, dpranke, glenn, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
Bug Depends on:    
Bug Blocks: 75708    
Attachments:
Description Flags
Patch
none
Patch thorton: review+

Brent Fulgham
Reported 2013-05-24 11:04:08 PDT
The Windows port of WebKit stores all local data (e.g., URL cache, Icon Database, DOM Storage) in the OS-defined locations CSIDL_LOCAL_APPDATA or CSIDL_APPDATA (depending on roaming context). There is no way for clients of WebKit to indicate that these items should be stored elsewhere, which could lead to concurrent WebKit clients interacting with each-other's data. We should implement the same mechanism used by the Mac port, which uses an application default/preference setting to specify where this should go. Use the same key labels as the mac port for consistency: NSString *WebDatabaseDirectoryDefaultsKey = @"WebDatabaseDirectory"; NSString *WebKitLocalCacheDefaultsKey = @"WebKitLocalCache"; NSString *WebStorageDirectoryDefaultsKey = @"WebKitLocalStorageDatabasePathPreferenceKey"; This bug needs to: 1. Modify the WebStorageManager to use the WebStorageDirectoryDefaultsKey key. 2. Modify the WebDatabaseManager to use the WebDatabaseDirectoryDefaultsKey key. 3. Modify the URL cache code to use the WebKitLocalCache key.
Attachments
Patch (16.41 KB, patch)
2013-05-24 16:02 PDT, Brent Fulgham 		no flags
DetailsFormatted DiffDiff
Patch (16.64 KB, patch)
2013-05-24 16:56 PDT, Brent Fulgham 		thorton: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2013-05-24 11:04:49 PDT
<rdar://problem/13984436>
Brent Fulgham
Comment 2 2013-05-24 16:02:39 PDT
Created attachment 202862 [details] Patch
Tim Horton
Comment 3 2013-05-24 16:18:44 PDT
Comment on attachment 202862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202862&action=review > Source/WebKit/win/WebDatabaseManager.cpp:413 > + CFStringRef directoryPref = static_cast<CFStringRef>(CFPreferencesCopyAppValue(WebDatabaseDirectoryDefaultsKey, kCFPreferencesCurrentApplication)); You should check that the returned CFPropertyListRef is of the right type before casting it! > Source/WebKit/win/WebView.cpp:485 > + if (!cfurlCacheDirectory || CFStringGetTypeID() != CFGetTypeID(cfurlCacheDirectory.get())) ditto. > Source/WebKit/win/WebView.cpp:2608 > + if (cacheDirectory && CFStringGetTypeID() == CFGetTypeID(cacheDirectory)) etc. > Tools/DumpRenderTree/win/DumpRenderTree.cpp:204 > + if (path[path.length() - 1] != '\\') What if the path is ""? > Tools/DumpRenderTree/win/DumpRenderTree.cpp:1300 > + CFPreferencesSetAppValue(WebDatabaseDirectoryDefaultsKey, WebCore::pathByAppendingComponent(path, "Databases").createCFString().get(), kCFPreferencesCurrentApplication); Are these created strings going to leak? If not, why not?
Brent Fulgham
Comment 4 2013-05-24 16:56:21 PDT
Created attachment 202867 [details] Patch
Tim Horton
Comment 5 2013-05-24 17:07:10 PDT
Comment on attachment 202867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=202867&action=review Seems reasonable. > Tools/DumpRenderTree/win/DumpRenderTree.cpp:209 > + DWORD size = ::GetEnvironmentVariable(dumpRenderTreeTemp, 0, 0); > + Vector<TCHAR> buffer(size); > + if (::GetEnvironmentVariable(dumpRenderTreeTemp, buffer.data(), buffer.size())) { > + wstring path = buffer.data(); > + if (!path.empty() && (path[path.length() - 1] != L'\\')) > + path.append(L"\\"); > + return WTF::String (path.data(), path.length()); > + } > + > + return WebCore::localUserSpecificStorageDirectory(); Do you really need all these explicit namespaces? This looks unusual.
Brent Fulgham
Comment 6 2013-05-24 17:10:13 PDT
(In reply to comment #5) > (From update of attachment 202867 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=202867&action=review > > Seems reasonable. > > > Tools/DumpRenderTree/win/DumpRenderTree.cpp:209 > > + DWORD size = ::GetEnvironmentVariable(dumpRenderTreeTemp, 0, 0); > > + Vector<TCHAR> buffer(size); > > + if (::GetEnvironmentVariable(dumpRenderTreeTemp, buffer.data(), buffer.size())) { > > + wstring path = buffer.data(); > > + if (!path.empty() && (path[path.length() - 1] != L'\\')) > > + path.append(L"\\"); > > + return WTF::String (path.data(), path.length()); > > + } > > + > > + return WebCore::localUserSpecificStorageDirectory(); > > Do you really need all these explicit namespaces? This looks unusual. aroben told me (years ago) that they were using the global namespace decoration on all Windows API calls, so I got in the habit of using it. I think the WTF is explicitly used in DumpRenderTree for some reason. We could add a "using namespace WTF", but I was trying to minimize the changes.
Brent Fulgham
Comment 7 2013-05-24 17:19:44 PDT
Committed r150670: <http://trac.webkit.org/changeset/150670>
Note You need to log in before you can comment on or make changes to this bug.