[webkit-reviews] review denied: [Bug 54823] [GTK] Implemented missing API for setting and getting application cache directory path database. : [Attachment 83093] 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 83093: API for application cache database directory
https://bugs.webkit.org/attachment.cgi?id=83093&action=review

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