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

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

The current API always uses favicons unconditionally. The thing is that in WebKit2 it's not possible to close the icon database and open it again with a different patch, so we designed the API with that approach. However, I don't why is that limitation but it might change. So the idea was that when you call webkit_web_context_get_favicon_databse(), it's created automatically and, if the favicon database path hasn't been called the default one will be used. WebKitWebView always calls webkit_web_context_get_favicon_databse() to implement the favicon property, so we always end up with a favicon database. The idea was that if apps are not interested in favicons, thay can simply ignore the favicon property of the web view, but the thing is that favicons cause an overload and other side effects (like having one subresource more loaded) that could be avoided if the app is not interested in the favicons. 

So, I think we should switch to an API were the user has to explicitly enable the favicon database, but that would allow us to implement reopne if it's eventually supported. So I think we could do something like this:

We keep the methods to set/get the favicons directory path:

void webkit_web_context_set_favicon_database_directory()

When it receives NULL the default database path will be used. This creates the favicon database if needed and sets the path. If the database is open it does nothing.

gchar *webkit_web_context_get_favicon_database_directory()

Returns NULL unless webkit_web_context_set_favicon_database_directory has been called.

WebKitFaviconDatabase *webkit_web_context_get_favicon_database()

Creates the favicon database if it doesn't exist, but it doesn't set the path.

So, to enable the favicons, the user needs to call webkit_web_context_set_favicon_database_directory(). If it hans't been called, web view will use the database, that will return WEBKIT_FAVICON_DATABASE_ERROR_NOT_INITIALIZED every time it's asked for the favicon. This way the favicon database will never emit the favicon-changed signal and web view won't emit notify::favicon. In the worst case we have a WebKitFaviconDatabase object that does nothing but we don't need special case when the database is not open, and the web view code will be the same.

[Carlos Garcia Campos]

Btw, with this we wouldn't need a fix for https://bugs.webkit.org/show_bug.cgi?id=98185 because favicons will be disabled for all unit tests except the favicons one.

[Mario Sanchez Prada]

I agree with the idea, yet I have some comments about the specific implementation for it. See them below...

(In reply to comment #0)
> [...]
> So, to enable the favicons, the user needs to call 
> webkit_web_context_set_favicon_database_directory(). If it hans't been called,
> web view will use the database, that will return
> WEBKIT_FAVICON_DATABASE_ERROR_NOT_INITIALIZED every time it's asked for the
> favicon. 

If we are going to force that calling to this method is necessary to enable the favicon database (will be disabled by default) and this method will only do stuff make sense if called once, then I think it would make sense to rename it to something like this:

  void webkit_web_context_initialize_favicon_database(const char* path);

This would be also more consistent with the NOT_INITIALIZED error that would be issuing when trying to use the icon database before calling this method.

In case we wanted to support re-setting the path of the database later on (once it's allowed in WK2), it would be as easy as adding the method webkit_web_context_set_database_directory() again, which would do something like this:

  1. Close the current IconDatabase
  2. Set the path as the new directory to be used
  3. Open the IconDatabase again.

> This way the favicon database will never emit the favicon-changed
> signal and web view won't emit notify::favicon. In the worst case we have a
> WebKitFaviconDatabase object that does nothing but we don't need special
> case when the database is not open, and the web view code will be the same.

Yes. Makes perfect sense to me.

[Mario Sanchez Prada]

(In reply to comment #1)
> Btw, with this we wouldn't need a fix for https://bugs.webkit.org/show_bug.cgi?id=98185 because favicons will be disabled for all unit tests except the favicons one.

Agreed, but let's keep it open in the meanwhile. You know... just in case :)

[Mario Sanchez Prada]

(In reply to comment #2)
> I agree with the idea, yet I have some comments about the specific 
> implementation for it. See them below...

Forget about those comments. I realized that what you're suggesting is slightly different from what I've understood: you're proposing to decouple the process of creating the WebKitFaviconDatabase (which would happen both in get_favicon_database() and set_favicon_database_directory(), when needed) from the process of opening the IconDatabase, which would only happen in set_favicon_database_directory().

This way, one could call get_favicon_database() as many times as needed but it would not cause any effect until a call (and only) to set_favicon_database_directory() was invoked.

I'm already working on a patch for this.

[Mario Sanchez Prada]

Created attachment 168005 [details]
Patch proposal

Here comes the patch, implementing Carlos's original idea (not my rants)

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Review Bot]

Attachment 168005 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:363:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:364:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 168005 [details]
Patch proposal

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.

[Carlos Garcia Campos]

Comment on attachment 168005 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=168005&action=review

>> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:353
>> +        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());

Oh, I see, the setIconDatabasePath is in web context, so it's better to do it here, for get this comment, sorry.

[Mario Sanchez Prada]

(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

[Mario Sanchez Prada]

Created attachment 168171 [details]
Patch proposal

New patch addressing the issues pointed out by Carlos

[WebKit Review Bot]

Attachment 168171 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:369:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:370:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 168171 [details]
Patch proposal

View in context: https://bugs.webkit.org/attachment.cgi?id=168171&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:407
> + * Get the #WebKitFaviconDatabase associated with @context

@context.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:412
> + * Notice that this method simply returns the #WebKitFaviconDatabase,
> + * but doesn't enable the database. To enable it you will still need to
> + * call webkit_web_context_set_favicon_database_directory() at least
> + * once, either before or after calling this method.

I think I would avoid using enable/disable, because the error you get from the favicon database is NOT_INITIALIZED. I would simplify this by something like. 

"To initialize the database you need to call webkit_web_context_set_favicon_database_directory()."

[Mario Sanchez Prada]

Committed r131033: <http://trac.webkit.org/changeset/131033>