API review: FavIconManager vs FavIconHostRequestJob

[David Faure]

I'm working on a replacement for the favicons kded module (which lives in libkonq),
which will be a class in kio instead (proper C++ API certainly beats just a DBus interface
as public API).

I started from the DBus API of the kded module, cleaned up a number
of things (in particular the awful "(bool isHost, QString hostOrUrl)").

And this led me to the API I'm pasting below.

I think it kind of works and would be easy enough to port the apps from the current
code to that, but the more I think about it, the less I like it .... I'm not 100% sure
though so I'd like to discuss it with you guys before I rewrite it all and then think
some more and have regrets :-)

So, first, to give some context, here's what I currently have:

namespace KIO {

/**
 * FavIconsManager is the class for handling favicons.
 *
 * For instance, the icon for http://www.google.com exists at http://www.google.com/favicon.ico
 * This class will download the favicon, and make it available as a local PNG for fast lookups afterwards.
 *
 * Usage:
 * For a given HTTP URL, you can find out if a favicon is available by calling KIO::favIconForUrl() in KIOCore.
 * If it returns empty, you can then trigger a download of the favicon by calling downloadHostIcon(),
 * and wait for the signal hostIconChanged() to be emitted.
 *
 * @code
 * // Assuming a member variable: FavIconsManager m_faviconsManager;
 * connect(&m_faviconsManager, &KIO::FavIconsManager::hostIconChanged, this, &MyClass::slotHostIconChanged);
 *
 * // Now let's say we want to show the icon for QUrl m_url
 * QString icon = KIO::favIconForUrl(m_url);
 * if (!icon.isEmpty()) {
 *     // show icon right away
 * } else {
 *     m_favIconsManager.downloadHostIcon(m_url); // asynchronous
 * }
 *
 * void MyClass::slotHostIconChanged(const QString &host, const QString &iconName)
 * {
 *     if (m_url.host() == host) {
 *         // show iconName
 *     }
 * }
 * @endcode
 *
 * In Web Browsers, additional information exists: the HTML for a given page can
 * specify something like
 *  <link rel="shortcut icon" href="another_favicon.ico" />
 * To handle this, call setIconForUrl(pageUrl, iconUrl).
 * (KParts-based web engines use the signal BrowserExtension::setIconUrl to call back
 * into the web browser application, which should then call this).
 * The signal urlIconChanged will be emitted once the icon has been downloaded.
 *
 * The on-disk cache is shared between processes.
 *
 * @since 5.19
 */
class KIOGUI_EXPORT FavIconsManager : public QObject
{
    Q_OBJECT

public:
    /**
     * Creates a FavIconsManager.
     * You can create multiple instance, they will share the same cache.
     */
    explicit FavIconsManager(QObject *parent = 0);

    /**
     * Deletes the FavIconsManager.
     */
    ~FavIconsManager();

    /**
     * Downloads the icon for a given host if it was not downloaded before
     * or the download was too long ago. If the download finishes
     * successfully, the hostIconChanged() signal is emitted, otherwise
     * hostIconError() is emitted.
     *
     * @param url any URL on the host for which the icon is to be downloaded
     */
    void downloadHostIcon(const QUrl &url);

    /**
     * Associates an icon with the given URL. If the icon is already
     * available (in cache) the signal urlIconChanged() is emitted immediately.
     * If the icon was not downloaded before or the downloaded was too long ago,
     * a download attempt will be started and the urlIconChanged()
     * signal is emitted after the download finished successfully.
     * In case of an error during the download, urlIconError() is emitted.
     *
     * @param url the URL which will be associated with the icon
     * @param iconURL the URL of the icon to be downloaded
     */
    void setIconForUrl(const QUrl &url, const QUrl &iconURL);

Q_SIGNALS:
    /**
     * Emitted once a new icon is available, for a host
     */
    void hostIconChanged(const QString &host, const QString &iconName);

    /**
     * Emitted if an error occurred while downloading the icon for the given host.
     * You can usually ignore this (e.g. web browsers don't need to do anything if
     * no favicon was found), but this signal can be useful in some cases, e.g.
     * to let keditbookmarks know that it should move on to the next bookmark.
     */
    void hostIconError(const QString &host, const QString &errorString);

    /**
     * Emitted once a new icon is available, for a url
     */
    void urlIconChanged(const QUrl &url, const QString &iconName);

    /**
     * Emitted if an error occurred while downloading the icon for the given host.
     * You can usually ignore this (e.g. web browsers don't need to do anything if
     * no favicon was found), but this signal can be useful in some cases, e.g.
     * to let keditbookmarks know that it should move on to the next bookmark.
     */
    void urlIconError(const QUrl &url, const QString &errorString);

private:
    FavIconsManagerPrivate *const d;

    Q_PRIVATE_SLOT(d, void slotData(KIO::Job *, QByteArray))
    Q_PRIVATE_SLOT(d, void slotResult(KJob *))
    Q_PRIVATE_SLOT(d, void slotKillJobs())
};
}

(full code at http://www.davidfaure.fr/2016/faviconsmanager.diff for the curious)

Notice how the sample code shows
1) that you need a manager object (could also be a singleton, but that's not very Qt like)
2) that you need to store m_url to be able to find out if the slot is being called for that
url or for something unrelated.

The reason for that API was that it was all centralized between processes, so if
process1 would trigger the download of the favicon for host1, all processes would
then be informed that this favicon is now available.

My idea with FavIconManager was: shared on disk cache, but no DBus needed.
So if process2 starts caring about host1 later, it can find it in the cache, but
if for some reason it starts caring at the exact same time as process1 then
they will both download it, and both save it (I'll use a lock file); the last one wins.
Minor downside, but not expected to happen often.

What this means though, is that it's always one signal for one request.
Which leads to a Job API rather than this global-manager-with-signals.

Let's write an alternative API...

class FavIconHostRequestJob : public KJob
{
public:
    explicit FavIconsHostRequestJob(const QString &host, QObject *parent = 0);

    QString host(); // as passed to the ctor
    QString iconName(); // available once result() was emitted (and error() is 0)
};

// explicit start() like KJob says, or automatic start() like KIO does?
// I'm always afraid people will forget to call start(), given the current inconsistency...

and another job with a ctor
  FavIconsIconUrlRequestJob(const QUrl &url, const QUrl &favIconUrl, QObject *parent = 0);
These classnames are a bit long though ;)

Typical slot:
void slotFavIconResult(KJob *job) {
    FavIconHostRequestJob *iconJob = static_cast<FavIconHostRequestJob *>(job);
    if (iconJob->error()) {
       // do something, or not
    } else {
        // show iconJob->iconName() as the icon for iconJob->host()
    }  
}

One downside of these nice jobs: if two classes in one process need a favicon, they will
both trigger a download. Unless..... a hidden singleton behind the scenes takes care
of the downloads and associates jobs with the downloads... smells a bit like the
KDirListerCache hell, but simpler so I hope it is doable somewhat cleanly.

That is, I think, a much more common case than two separate processes.
Open one URL in a webbrowser -> favicon for the tab, favicon for the mainwindow,
favicon for the combobox icon, for the history tree (and later on for adding as bookmark,
but by then it's cached, hopefully).
In Akregator I see both BrowserFrame and Feed registering for the favicon.

Migration strategy: the kded module will stay around until KF6, so we can take
our time porting the apps to the new favicon class.

Well, that was much thinking aloud. Any comments?

Anyone thinks the backend should be a kiod module instead, to avoid
concurrent downloads of the same icon and notify all processes when the
favicon is available?

-- 
David Faure, faure at kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5