[Webkit-unassigned] [Bug 98885] [GTK] It should be possible to disable favicons in WebKit2 GTK+ API

[bugzilla-daemon at webkit.org]

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


Carlos Garcia Campos <cgarcia at igalia.com> changed:

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




--- Comment #8 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-10-11 00:14:51 PST ---
(From update of attachment 168005)
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.

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