Bug 98153 – [GTK] WebKitWebView doesn't emit notify:favicon when it changes in some cases in WebKit2

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

The main problem is that it relies on icon-ready signal to be emitted by the favicon database, but that signal is only emitted when the icon is loaded from the network or imported from the database, but not when the icon is already in memory.

[Carlos Garcia Campos]

Created attachment 166680 [details]
Patch

[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 166680 [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/WebKitWebView.cpp:1270:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:134:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:142:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 3 in 3 files


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

[Alberto Garcia]

(In reply to comment #0)
> The main problem is that it relies on icon-ready signal to be
> emitted by the favicon database, but that signal is only emitted
> when the icon is loaded from the network or imported from the
> database, but not when the icon is already in memory.

I tested my patch with this and it seems to work fine now.

[Mario Sanchez Prada]

Comment on attachment 166680 [details]
Patch

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

The patch looks good to me. Only concern I have is whether it would be needed to write / extend a unit test for it.

> Source/WebKit2/ChangeLog:23
> +        known by the database and it doesn't have a favicon.

Good catch

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1253
> +static void faviconReadyCallback(GObject* object, GAsyncResult* result, gpointer userData)

Nit. I would probably rename this function to better reflect is the async callback for _get_favicon() and not the handler for the icon-ready signal (as it seems to suggest)

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 166680 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166680&action=review
> 
> The patch looks good to me. Only concern I have is whether it would be needed to write / extend a unit test for it.

Unit tests are currently broken indeed, but I don't know why, I'll look at it.

> > Source/WebKit2/ChangeLog:23
> > +        known by the database and it doesn't have a favicon.
> 
> Good catch
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1253
> > +static void faviconReadyCallback(GObject* object, GAsyncResult* result, gpointer userData)
> 
> Nit. I would probably rename this function to better reflect is the async callback for _get_favicon() and not the handler for the icon-ready signal (as it seems to suggest)

You are right.

[Carlos Garcia Campos]

Comment on attachment 166680 [details]
Patch

The patch seems to break the unit tests, but  still don't see why, I'm working on it.

[Carlos Garcia Campos]

It seems it was not my patch, test is failing in the bots

[Carlos Garcia Campos]

Created attachment 166719 [details]
Updated patch

It fixes some issues with the previous patch, removing race conditions in the cancellation code and cancelling the operation in WebView when needed.

[WebKit Review Bot]

Attachment 166719 [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/WebKitWebView.cpp:1288:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:127:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:135:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 3 in 3 files


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

[Martin Robinson]

Comment on attachment 166719 [details]
Updated patch

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

Okay. This looks reasonable. I'd really like Mario to do an informal review before I r+ this though.

>> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:127
>> +                    _("Unknown favicon for page %s"), pageURL.utf8().data());
> 
> When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]

I know that we have agreed in the past to line up the arguments, but it seems that the style bot now complains about this.  We should follow the herd here. Sticking with what we agreed to before will just cause us pain, because the bot will never stop complaining.

When does this error case happen? I'm a bit confused by the phrase "Unknown favicon for page..."

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:270
> +static void webkitWebViewCancelFavicon(WebKitWebView* webView)

webkitWebViewCancelFaviconRequest?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:288
>  static void iconReadyCallback(WebKitFaviconDatabase *database, const char *uri, WebKitWebView *webView)

Do you mind also fixing the asterisks here?

[Mario Sanchez Prada]

(In reply to comment #11)
> (From update of attachment 166719 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166719&action=review
> 
> Okay. This looks reasonable. I'd really like Mario to do an informal review before I r+ this though.

I have reviewed it and it looks good to me. Only doubt I had was about removing all the code to connect to the cancellable object, but Carlos explained me via jabber that not only it's not needed[*], but that it was also a source of problems due to a race condition that might happen in some cases.

So, I think an r+ is in place :)

[*] We would not be cancelling anything in the middle of the operation anyway, but just checking if it was cancelled in the _finish() method, and report accordingly.

[Carlos Garcia Campos]

(In reply to comment #11)
> (From update of attachment 166719 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166719&action=review
> 
> Okay. This looks reasonable. I'd really like Mario to do an informal review before I r+ this though.
> 
> >> Source/WebKit2/UIProcess/API/gtk/WebKitFaviconDatabase.cpp:127
> >> +                    _("Unknown favicon for page %s"), pageURL.utf8().data());
> > 
> > When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> 
> I know that we have agreed in the past to line up the arguments, but it seems that the style bot now complains about this.  We should follow the herd here. Sticking with what we agreed to before will just cause us pain, because the bot will never stop complaining.
> 
> When does this error case happen? I'm a bit confused by the phrase "Unknown favicon for page..."
> 

There are some cases where we don't know anything about the favicon, because the page hasn't been loaded or is being loaded and the icon hasn't been asked yet. Maybe we could rename it to unknown page, there might be a record in the database for he page, but not for the icon yet.

[Carlos Garcia Campos]

Created attachment 166855 [details]
Updated patch

Addressed review comments, workaround style issues, and fix the unit tests. The problem as a special case that happens when you as for an icon when it's being loaded and the database initial import is ongoing, something that it's unlikely to happen in a real app, but always happens in unit tests because things are done too early and fast. In such case the icon-data-ready callback can be called before the favicon has been loaded. With this patch WebKitWebView now does the right thing, so instead of waiting for the icon-ready signal, we can wait for the notify::favicon signal os WebKitWebView that will be emitted when the favicon data is actually ready.

[Carlos Garcia Campos]

Committed r130279: <http://trac.webkit.org/changeset/130279>