Bug 58937 – [Qt][WK2] Initial support for favicons.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Andreas Kling]

In WebKit1, we have this API for favicons:

- static void QWebSettings::setIconDatabasePath(QString)
- void QWebFrame::iconChanged() [signal]
- QIcon QWebFrame::icon() const

In WebKit2, the WebContext has a WebIconDatabase, so the most straightforward thing approach to expose the favicons through QWKContext. This patch adds the following API:

- void QWKContext::setIconDatabasePath(QString)
- void QWKContext::iconChangedForPageURL(QUrl) [signal]
- QPixmap QWKContext::iconForPageURL(QUrl)

This could be hooked into internally by QWKPage to provide e.g QWKPage::iconChanged() [signal] and QPixmap QWKPage::icon(), but I'd prefer to do that later as "convenience API".

[Andreas Kling]

Created attachment 90273 [details]
Proposed patch

Proposed patch.

This does not cover the case where an icon is in already in the cache, though this could be added in a follow-up patch. QWebFrame::iconChanged() didn't work like that until very recently either.. :)

@Brady: In Qt's WebKit1 API, we always emit the iconChanged() signal, also after an icon is retrieved from cache. Do you have any suggestions how to accomplish this in WebKit2?

[WebKit Review Bot]

Attachment 90273 [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/qt/qwkcontext.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qwkcontext.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qwkcontext.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qwkcontext.cpp:57:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit2/UIProcess/API/qt/qwkcontext.cpp:58:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit2/UIProcess/API/qt/ClientImpl.h:56:  The parameter name "iconDatabase" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/API/qt/ClientImpl.h:57:  The parameter name "iconDatabase" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 7 in 7 files


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

[Simon Hausmann]

Comment on attachment 90273 [details]
Proposed patch

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

Looks good to me, as discussed. r- because of the nitpicks :)

> Source/WebKit2/UIProcess/API/qt/qwkcontext.cpp:86
> +    // FIXME: Should we disable the icon database if path.isEmpty() (like the WebKit1 API does?)

Yes :)

> Source/WebKit2/UIProcess/API/qt/qwkcontext.h:41
> +    QPixmap iconForPageURL(const QUrl&) const;

This should be a QIcon, as discussed at the Review-a-thon.

[Andreas Kling]

Created attachment 93799 [details]
Proposed patch v2

Same patch with QIcon instead of QPixmap as discussed.
We're still missing a way to disable the icon database once enabled, so I've left a FIXME about that.

[WebKit Review Bot]

Attachment 93799 [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/qt/qwkcontext.cpp:25:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qwkcontext.cpp:30:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qwkcontext.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qwkcontext.cpp:57:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit2/UIProcess/API/qt/qwkcontext.cpp:58:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 5 in 7 files


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

[Kenneth Rohde Christiansen]

Comment on attachment 93799 [details]
Proposed patch v2

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

> Source/WebKit2/UIProcess/API/qt/ClientImpl.cpp:208
> +void qt_wk_didRemoveAllIcons(WKIconDatabaseRef iconDatabase, const void* clientInfo)
> +{
> +}

notImplemented() ?

[Andreas Kling]

Comment on attachment 93799 [details]
Proposed patch v2

Clearing flags on attachment: 93799

Committed r86694: <http://trac.webkit.org/changeset/86694>

[Andreas Kling]

All reviewed patches have been landed.  Closing bug.