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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54823


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #83395|review?                     |review-
               Flag|                            |




--- Comment #6 from Martin Robinson <mrobinson at webkit.org>  2011-02-22 14:59:05 PST ---
(From update of attachment 83395)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.