[Webkit-unassigned] [Bug 98885] [GTK] It should be possible to disable favicons in WebKit2 GTK+ API
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 11 01:29:17 PDT 2012
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.
More information about the webkit-unassigned
mailing list