API review: FavIconManager vs FavIconHostRequestJob

[Kevin Ottens]

Hello,

Here comes my purely API focused comments. I'll snip anything else (even 
comments). If I missed something important due to this approach, feel free to 
point it out.

On Thursday, 28 January 2016 01:33:32 CET David Faure wrote:
> namespace KIO {
> 
> class KIOGUI_EXPORT FavIconsManager : public QObject
> {
>     Q_OBJECT
> 
> public:
>     explicit FavIconsManager(QObject *parent = 0);
> 
>     ~FavIconsManager();
> 
>     void downloadHostIcon(const QUrl &url);
> 
>     void setIconForUrl(const QUrl &url, const QUrl &iconURL);
> 
> Q_SIGNALS:
>     void hostIconChanged(const QString &host, const QString &iconName);
> 
>     void hostIconError(const QString &host, const QString &errorString);
> 
>     void urlIconChanged(const QUrl &url, const QString &iconName);
> 
>     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())
> };
> }

It cries for jobs indeed. :-)

> [...]
> Let's write an alternative API...
> 
> class FavIconHostRequestJob : public KJob
> {
> public:
>     explicit FavIconsHostRequestJob(const QString &host,
>                                     QObject *parent = 0);

Perhaps better to use a setter than a ctor parameter?

>     QString host();
>     QString iconName();
> };
> 
> // explicit start() like KJob says, or automatic start() like KIO does?

If it's in KIO it's auto-start. You don't want inconsistencies within the 
framework itself.

> // 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);

I think I would go for setters in that case for sure. Otherwise one will 
always wonder what the first and the second URL maps too.

> 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.

A bit more remote from API review, but yes I agree with the concern.
 
> That is, I think, a much more common case than two separate processes.

Right, it's not like you'll ever have more than two or three processes asking 
for the same icon at the same time... and it'd be really bad luck.

> 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.

Agreed.

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

Agreed as well.

> 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?

I would shy away from such added complexity early on. Once the job API is in 
place, it could always be changed in the application if it turned out we were 
wrong and quite a few processes ask for the same non-cached favicon at the 
same time. I doubt it'll happen though.

Cheers.
-- 
Kévin Ottens, http://ervin.ipsquad.net

KDAB - proud supporter of KDE, http://www.kdab.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160131/2df75d2e/attachment.sig>