[webkit-reviews] review denied: [Bug 54823] [GTK] Implemented missing API for setting and getting application cache directory path database. : [Attachment 83395] API for application cache database directory

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Lukasz Slachciak
<l.slachciak at samsung.com>'s request for review:
Bug 54823: [GTK] Implemented missing API for setting and getting application
cache directory path database.
https://bugs.webkit.org/show_bug.cgi?id=54823

Attachment 83395: API for application cache database directory
https://bugs.webkit.org/attachment.cgi?id=83395&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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(cacheDirect
oryPath.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?