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

[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 83169: Implement WebKitIconDatabase API  #2
https://bugs.webkit.org/attachment.cgi?id=83169&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
(sorry the view in context link won't work here since, I made this against your
old patch. :()

Great stuff! I'd really like to see this update the old DumpRenderTree code and
the removal of the old DumpRenderTreeSupport code in this patch. Please fix all
the style issues as well as the stuff I mentioned below.

> Source/WebKit/gtk/webkit/webkitglobals.cpp:248
>  static void closeIconDatabaseOnExit()
>  {
>      iconDatabase()->close();

This code should probably go in webkiticondatabase.c now along with the code
that actuall does the initialization.

> Source/WebKit/gtk/webkit/webkitglobals.cpp:-301
>	   atexit(closeIconDatabaseOnExit);
>      }
>  
> +    WebKitIconDatabase* database = webkit_get_icon_database();
>      if (enabled) {
> -	   iconDatabase()->setEnabled(true);
>	   GOwnPtr<gchar>
iconDatabasePath(g_build_filename(g_get_user_data_dir(), "webkit",
"icondatabase", NULL));
> -	   iconDatabase()->open(iconDatabasePath.get());
> -	   return;
> +	   webkit_icon_database_set_path(database, iconDatabasePath.get());
> +    }
> +    else {
> +	   webkit_icon_database_set_path(database, 0);
>      }
> -
> -    iconDatabase()->setEnabled(false);
> -    iconDatabase()->close();

I'd rather you removed this entirely along with where we call it in
DumpRenderTreeSupport. If it's possible for DumpRenderTree to enable and diable
the icon database with the API it should use that.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:40
> + * @short_description: A WebKit web application database
> + *
> + * #WebKitIconDatabase provides access to website icons, as shown
> + * in tab labels, window captions or bookmarks.
> + */

If it's possible to annotate these with a @since you should do that. You should
expand this greatly. Explain that when the icon database is enabled WebKit will
request so and so with every page. Mention that you can disable it by setting
an null path.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:69
> +    priv->path.set(0);

Instead of explicitly clearing this, you should explicitly "new" and destruct
the private area. Take a look at what happens in the webkit_web_view_finalize
and webkit_web_view_init for one way to do that.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:120
> +	 * Since: 1.3.4

I'm pretty sure all of the 1.3.4's should be 1.3.11's.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:182
> + * Passing %NULL disables the icon database.

Both null and "" should probably disable the database.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:187
> +void webkit_icon_database_set_path(WebKitIconDatabase* database,
> +				      const gchar*	  path)

Please use WebKit style here and put this all on one line.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:205
> +    WebCore::iconDatabase()->open(database->priv->path.get());

Casting a gchar* directly to a string here is incorrect. You need to use
fileSystemRepresentation from FileSystemGtk.cpp.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:222
> +gchar* webkit_icon_database_get_icon_uri(WebKitIconDatabase* database,
> +					    const gchar*	pageURI)
> +{

Ditto.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:226
> +    String pageURL(pageURI);

Use String::fromUTF8 here.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:238
> + * The pixbuf can have a variable size and should be resized
> + * before it is displayed.

Does this return the icon in the largest size possible? If so, I'd mention
that. If not, that's probably the behavior we want to get somehow. The
documentation for this webkit_web_view_get_icon_pixbuf and actually very
different. They should be in sync.

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:241
> + * Returns: the a new reference to a #GdkPixbuf

This should also mention that null is returned when there is a problem. What if
the page has no favicon?

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:252
> +    // The size we pass is irrelevant as long as it is > 0,0

I think we should expand this comment a big. Why is the size irrelevant?

> Source/WebKit/gtk/webkit/webkiticondatabase.cpp:256
> +    GdkPixbuf* pixbuf = icon->getGdkPixbuf ();

Extra space here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5076
> + * Obtains a #GdkPixbuf for the favicon for the given #WebKitWebView, or

Probably should be Obtains a #GdkPixbuf for the favicon of the given
#WebKitWebView

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5077
> + * a default icon. Use webkit_web_view_get_icon_uri() if you need to

Should say when a default icon is returned.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:5080
> + * Usually you want to connect to WebKitWebView::icon-loaded and load the
> + * pixbuf in the callback.

Please fix the flow here, so that this abuts the end of the last sentence. You
might want to mention that this is a just a shortcut for getting the icon
database and then getting the pixbuf from there.