[webkit-reviews] review denied: [Bug 56200] [GTK] WebKitIconDatabase doesn't keep icons cached : [Attachment 92245] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 56200: [GTK] WebKitIconDatabase doesn't keep icons cached
https://bugs.webkit.org/show_bug.cgi?id=56200

Attachment 92245: Patch
https://bugs.webkit.org/attachment.cgi?id=92245&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=92245&action=review

Seems like a very important change, though it's an API break. I'd really like
to get Xan and Gustavo to sign off on this one as well. r- for now for the
nits.

> Source/WebKit/gtk/WebCoreSupport/IconDatabaseClientGtk.cpp:57
> +}
> +void IconDatabaseClientGtk::didChangeIconForPageURL(const String& pageURL)

Need an extra space here for my sanity. ;)

> Source/WebKit/gtk/WebCoreSupport/IconDatabaseClientGtk.h:33
> +namespace WebCore {
> +class IconDatabase;
> +}; // namespace WebCore

I don't think you need this forward declaration.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:64
> + * Every icon in the database has a retain count.  If an icon has a
> + * retain count greater than 0, it will be written to disk for later
> + * use. If an icon's retain count equals zero it will be removed from
> + * disk.  The retain count is not persistent across launches. If the
> + * WebKit client wishes to retain an icon it should retain the icon
> + * once for every launch.  This is best done at initialization time
> + * before the database begins removing icons.  To make sure that the
> + * database does not remove unretained icons prematurely, call
> + * webkit_icon_database_delay_database_cleanup() until all desired
> + * icons are retained.  Once all are retained, call
> + * webkit_icon_database_allow_database_cleanup().

Some of these sentences have two spaces after them and some one. Please use one
everywhere.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:192
> +	       (GSignalFlags)G_SIGNAL_RUN_LAST,

Should use a static_cast here.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:196
> +	       0,
> +	       NULL,
> +	       NULL,
> +	       g_cclosure_marshal_VOID__STRING,

All NULLs should be zero unless that produces a warning.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:218
>	       (GSignalFlags)G_SIGNAL_RUN_LAST,

static_cast

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:236
> +	* After receiving this signal
> +	* webkit_icon_database_get_icon_pixbuf() will successfully return
> +	* the icon data associatted with the provided page URI.

Maybe even this one out a little bit.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:242
> +	       (GSignalFlags)G_SIGNAL_RUN_LAST,

static_cast

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:259
> +	       (GSignalFlags)G_SIGNAL_RUN_LAST,

static_cast

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:275
> +    // Disabled by default

Might want to say here why it's disabled by default.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:301
> +	   // Do not use set_path(), otherwise we could end up calling
> +	   // startupIconDatabase twice.

You can make this one line.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:436
> + * be cached then it should listen to icon-changed signal in order to
> + * know when the icon data is actually available. Otherwise if the
> + * icon need to be retrieved from an external resource the client
> + * should connect to icon-changed signal.

Shouldn't a developer listen for the icon-data-imported-signal? I'm not sure I
fully understand the last sentence. :/

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:499
> + * want to keep and once all are retained call allowDatabaseCleanup to

allowDatabaseCleanup -> webkit_icon_database_allow_cleanup

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:543
> +						 const gchar*	     page_uri)

Please use WebKit coding conventions here. So this declaration should be:
void void webkit_icon_database_retain_icon_for_uri(WebKitIconDatabase*
database, const gchar* pageURI)
{

}