[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





--- Comment #10 from Mario Sanchez Prada <msanchez at igalia.com>  2012-10-11 01:29:56 PST ---
(In reply to comment #8)
> (From update of attachment 168005 [details])
> 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?

Bad wording. It should be:

 "Further calls for the same instance of #WebKitWebContext after calling won't cause any effect."

> > 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.

Agreed. Removed

> > 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.

Ok.

> > 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 :-)

Actually, it will complain:

Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:361:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]

We can enter the discussion on whether that makes sense or not, but it currently complains, so I would leave the 0 there.

> > 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.

Ok. Changed.

> > 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.

Ok. Will do in a follow up patch

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