[webkit-reviews] review denied: [Bug 98885] [GTK] It should be possible to disable favicons in WebKit2 GTK+ API : [Attachment 168005] Patch proposal

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 98885: [GTK] It should be possible to disable favicons in WebKit2 GTK+ API
https://bugs.webkit.org/show_bug.cgi?id=98885

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

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=168005&action=review


Please test this new behaviour in the unit tests.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:339
> + * called only once. Further calls for the same instance of
> + * #WebKitWebContext after calling won't cause any effect.

after calling what?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:350
> +    // Nothing to do if the database is already open.
> +    WebIconDatabase* iconDatabase = priv->context->iconDatabase();
> +    if (iconDatabase->isOpen())
> +	   return;

I think the code is explicit enough, it returns early if the database is open,
so I would remove the obvious comment.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:353
> +    if (!priv->faviconDatabase)
> +	   priv->faviconDatabase =
adoptGRef(webkitFaviconDatabaseCreate(iconDatabase));

I prefer a single point where the database is created. Also I would move the
setIconDatabasePath to WebKitFaviconDatabase using a private method. So you can
remove this here and below call something like

webkitFaviconDatabaseSetPath(webkit_web_context_get_favicon_database(context),
faviconDatabasePath.get());

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:355
> +    // Use default if 0 is passed as parameter.

We can use NULL in the comments, the style bot won't complain fortunately :-)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:404
> + * Get the #WebKitFaviconDatabase associated with @context, but you
> + * need to call webkit_web_context_set_favicon_database_directory()
> + * once, either before or after calling this function, to actually
> + * enable the support for favicons.

Instead of but you . . . I would simply explain in another sentence that the
database is not open/enable until a directory has been set.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitFaviconDatabase.cpp:43
> -	   if
(!g_str_equal(webkit_web_context_get_favicon_database_directory(m_webContext),
kTempDirectory))
> +	   if
(g_strcmp0(webkit_web_context_get_favicon_database_directory(m_webContext),
kTempDirectory))
>	       webkit_web_context_set_favicon_database_directory(m_webContext,
kTempDirectory);

I think we don't need to do this here now, it can be done again in the
set-directory test. I would also add a test before setting the directory that
asks for an icon to check that the database is not open error is returned.