Bug 54823 – [GTK] Implemented missing API for setting and getting application cache directory path database.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Lukasz Slachciak]

There was missing API for setting/getting application cache directory path database.
Instead direct call to WebCore::cacheStorage().setCacheDirectory() was used in webkitInit() (and marked as FIXME)

Attached patch resolves this problem.

[Lukasz Slachciak]

Created attachment 83093 [details]
API for application cache database directory

[Lukasz Slachciak]

Additionally, do you think that we should make webkitapplicationcacheprivate.h API's publlic one day?

[Martin Robinson]

Comment on attachment 83093 [details]
API for application cache database directory

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

As long as the Mac port exposes this, I think it's a good idea for us to expose it. Please consider my suggestions below though.

> Source/WebKit/gtk/webkit/webkitapplicationcache.cpp:28
> +static gchar* webkit_application_cache_directory_path = NULL;

Wouldn't it make more sense to make this a CString? Please use WebKit style when naming your variables.

> Source/WebKit/gtk/webkit/webkitapplicationcache.cpp:47
> + * Returns the current path to the directory WebKit will write Application
> + * Cache databases. By default this path will be in the user data
> + * directory.
> + *

Please explain what's stored in the application cache. Instead of saying user data directory, I recommend actually mentioning the GLib function we use to calculate the default directory.

> Source/WebKit/gtk/webkit/webkitapplicationcache.cpp:62
> +    if (path.isEmpty())
> +        return "";
> +
> +    g_free(webkit_application_cache_directory_path);
> +    webkit_application_cache_directory_path = g_strdup(path.utf8().data());
> +    return webkit_application_cache_directory_path;

No need to special case the empty string. When converting from a String to a raw string you need to use fileSystemRepresentation from FileSystemGtk.cpp in WebCore. I recommend checking if the current path is equal before freeing and updating the cached version.

> Source/WebKit/gtk/webkit/webkitapplicationcache.cpp:73
> + * Sets the current path to the directory WebKit will write Application
> + * Cache databases.

No need to capitalize Application Cache here and above.

> Source/WebKit/gtk/webkit/webkitapplicationcache.cpp:80
> +    WTF::String corePath = WTF::String::fromUTF8(path);

You need to use filenameToString when converting from a path to WebKit string. Filenames are not necessarily in UTF-8.

[Lukasz Slachciak]

Created attachment 83395 [details]
API for application cache database directory

[Lukasz Slachciak]

Hello Martin

My previous patch was based on the webkitwebdatabase.cpp directory path setter and getter. But your arguments sound reasonable so I rewritten this patch following your comments.

[Martin Robinson]

Comment on attachment 83395 [details]
API for application cache database directory

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

Looking good. We should also get a few other GTK+ reviewers to look at this since it adds new public API.

> Source/WebKit/gtk/ChangeLog:7
> +        [GTK] Implemented missing API for setting and getting
> +        web application cache directory path database.
> +        https://bugs.webkit.org/show_bug.cgi?id=54823

I don't think you want a newline after the bug title. I believe that the tools require a specific format.

> Source/WebKit/gtk/ChangeLog:9
> +

Extra newline here?

> Source/WebKit/gtk/ChangeLog:15
> +        * webkit/webkitapplicationcache.cpp: main implementation
> +        (webkit_application_cache_get_database_directory_path): getter
> +        (webkit_application_cache_set_database_directory_path): setter
> +        * webkit/webkitapplicationcacheprivate.h: API declaration
> +        * webkit/webkitglobals.cpp:
> +        (webkitInit): API usage

Please use full sentences for your ChangeLog descriptions.

> Source/WebKit/gtk/webkit/webkitapplicationcache.cpp:48
> + * cache databases. By default this path is set to g_get_user_data_dir()\WebKit
> + * in webkitInit() with webkit_application_cache_set_database_directory_path

We shouldn't mention webkitInit as it isn't public (or shouldn't be if it is!). The default is actually $XDG_DATA_HOME/webkit/databases.

> Source/WebKit/gtk/webkit/webkitapplicationcache.cpp:52
> + * Since: ?.?.?

Please fill these in with reasonable values. Looks like it will be 1.3.13.

> Source/WebKit/gtk/webkit/webkitapplicationcache.cpp:58
> +    WTF::String path =  WebCore::cacheStorage().cacheDirectory();
> +    CString cPath = WebCore::fileSystemRepresentation(path);

If you did this: 

CString path = WebCore::fileSystemRepresentation(WebCore::cacheStorage().cacheDirectory());

you could avoid having to have two path variables, path and cPath.

> Source/WebKit/gtk/webkit/webkitapplicationcache.cpp:84
> +    WTF::CString cPath(path);
> +    if (cPath != cacheDirectoryPath)
> +        cacheDirectoryPath = cPath;
> +

How about pathString instead of cPath?

> Source/WebKit/gtk/webkit/webkitapplicationcache.cpp:86
> +    WTF::String sCacheDirectoryPath = WebCore::filenameToString(cacheDirectoryPath.data());
> +    WebCore::cacheStorage().setCacheDirectory(sCacheDirectoryPath);

Please don't use Hungarian notation. This can just be:
WebCore::cacheStorage().setCacheDirectory(WebCore::filenameToString(cacheDirectoryPath.data()));

> Source/WebKit/gtk/webkit/webkitapplicationcacheprivate.h:38
> +WEBKIT_API G_CONST_RETURN gchar*
> +webkit_application_cache_get_database_directory_path  (void);
> +
> +WEBKIT_API void
> +webkit_application_cache_set_database_directory_path  (const gchar* path);
> +

Don't we want to expose these publically?

[Lukasz Slachciak]

Created attachment 83535 [details]
API for application cache database directory

[Lukasz Slachciak]

I followed Martin's comments and exposed new API as public - new webkitapplicationchache.h file was created.

[WebKit Review Bot]

Attachment 83535 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/gtk/webkit/webkit.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Lukasz Slachciak]

hmm... this "alphabetical sorting problem" looks like false positive for webkit.h

#include <webkit/webkitversion.h
#include <webkit/webkitapplicationcache.h>
#include <webkit/webkitdefines.h>
#include <webkit/webkitdom.h>


I assume that "webkitversion.h" needs always be on the top.

[Martin Robinson]

Comment on attachment 83535 [details]
API for application cache database directory

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

Looks good to me. We need to get another GTK+ reviewer to r+ this since it includes an API addition. If no one steps in, ask around on the IRC channel or find mine. :) Thank you!

>> Source/WebKit/gtk/webkit/webkit.h:25
>> +#include <webkit/webkitapplicationcache.h>
> 
> Alphabetical sorting problem.  [build/include_order] [4]

Yes, this look like a false positive.

[Xan Lopez]

Comment on attachment 83535 [details]
API for application cache database directory

Looks good to me, r+ 2/2

[Lukasz Slachciak]

Thank you Martin, Xan for review.

Looks like we are waiting for commiter now :)

[WebKit Commit Bot]

Comment on attachment 83535 [details]
API for application cache database directory

Clearing flags on attachment: 83535

Committed r79802: <http://trac.webkit.org/changeset/79802>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.