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+

Description Brent Fulgham 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.
Comment 1 Radar WebKit Bug Importer 2013-05-24 11:04:49 PDT
<rdar://problem/13984436>
Comment 2 Brent Fulgham 2013-05-24 16:02:39 PDT
Created attachment 202862 [details]
Patch
Comment 3 Tim Horton 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?
Comment 4 Brent Fulgham 2013-05-24 16:56:21 PDT
Created attachment 202867 [details]
Patch
Comment 5 Tim Horton 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.
Comment 6 Brent Fulgham 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.
Comment 7 Brent Fulgham 2013-05-24 17:19:44 PDT
Committed r150670: <http://trac.webkit.org/changeset/150670>