[webkit-reviews] review denied: [Bug 32510] [GTK] provide an API to control the IconDatabase : [Attachment 83333] Implement WebKitIconDatabase API #3

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Christian Dywan
<christian at twotoasts.de>'s request for review:
Bug 32510: [GTK] provide an API to control the IconDatabase
https://bugs.webkit.org/show_bug.cgi?id=32510

Attachment 83333: Implement WebKitIconDatabase API #3
https://bugs.webkit.org/attachment.cgi?id=83333&action=review

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

Great stuff. Watch the style issues. I'd really like to see some GIR
annotations in all the new documentation.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:111
>  void DumpRenderTreeSupportGtk::setIconDatabaseEnabled(bool enabled)
>  {
> -    WebKit::setIconDatabaseEnabled(enabled);
> +    WebKitIconDatabase* database = webkit_get_icon_database();
> +    if (enabled) {
> +	   GOwnPtr<gchar>
iconDatabasePath(g_build_filename(g_get_user_data_dir(), "webkit",
"icondatabase", NULL));
> +	   webkit_icon_database_set_path(database, iconDatabasePath.get());
> +    } else {
> +	   webkit_icon_database_set_path(database, 0);
> +    }
>  }

I would remove this method entirely and just do this in DumpRenderTree itself.
DumpRenderTreeSupport really exists just to expose stuff that isn't in the API
yet. When we remove things from here its a big win. : )

Perhaps DRT should have it's own icon directory too?

> Source/WebKit/gtk/webkit/webkitglobals.cpp:253
> +    if (!database) {
> +	   database =
WEBKIT_ICON_DATABASE(g_object_new(WEBKIT_TYPE_ICON_DATABASE, NULL));
> +	   atexit(closeIconDatabaseOnExit);
> +    }
> +

It makes sense to move this to where you first open the WebCore icon database
actually. I'd move both the atexit call and closeIconDatabaseOnExit to
webkitwebicondatabase.cpp.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:43
> + * SECTION:webkitwebdatabase
> + * @short_description: A WebKit web application database
> + *
> + * #WebKitIconDatabase provides access to website icons, as shown
> + * in tab labels, window captions or bookmarks.
> + *
> + * The icon database is enabled by default and stored in
> + * ~/.local/share/webkit/icondatabase, depending on XDG_DATA_HOME.

It's worth mentioning that all views share the same icon database.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:146
> +	* @page_uri: the URI of the page containing the icon

page_uri -> frame_uri.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:151
> +	* This signal is emitted when a favicon is available for a page,
> +	* including child frames.
> +	* See WebKitWebView::icon-loaded if you simply need the favicon
> +	* for a particular #WebKitWebView.

Might want to clarify this a bit. WebKitWebView::icon-loaded only fires for the
main frame icon. It's probably worth repeating that in the documentation for
WebKitWebView::icon-loaded and pointing to this signal for child frames.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:213
> +    if (database->priv->path.get()) {
> +	   WebCore::iconDatabase()->setEnabled(false);
> +	   WebCore::iconDatabase()->close();
> +    }

Why do you do you disable the database here?

>> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:224
>> +   
//WebCore::iconDatabase()->open(String(WebCore::fileSystemRepresentation(databa
se->priv->path.get())));
> 
> Should have a space between // and comment  [whitespace/comments] [4]

You want
WebCore::iconDatabase()->open(WebCore::filenameToString(database->priv->path.ge
t()));

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:236
> + * Returns: the URI for the favicon, or %NULL

You need to mention whether it's newly allocated or not. It's probably possible
to avoid it using a static CString to just cache it locally in the method.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:246
> +    String pageURL = String::fromUTF8(pageURI);
> +    return
g_strdup(WebCore::iconDatabase()->iconURLForPageURL(pageURL).utf8().data());

You might be able to do something like this:

static CString iconURL;
iconURL =
WebCore::iconDatabase()->iconURLForPageURL(String::fromUTF8(pageURI)).utf8();
return iconURL.data();

If you did that the user does not have to free the data.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:282
> +    return static_cast<GdkPixbuf*>(g_object_ref (pixbuf));

No space after g_object_ref