D5638: Use a single QNAM (and a disk cache) for HTTP jobs

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> httpworker.cpp:41
> +        QStorageInfo storageInfo(cacheLocation);
> +        cache.setMaximumCacheSize(storageInfo.bytesTotal() / 1000);
> +        nam.setCache(&cache);

0.1% of the partition size is a rather arbitrary value, no? It could go from something very tiny to something really big...

On my 470GB partition this would lead to a 470MB cache for knewstuff, that's maybe a bit much, given the average size for knewstuff stuff? Maybe a qMin() call with a maximum value would be useful?

> httpworker.cpp:47
> +
> +    QNetworkReply* get(const QNetworkRequest& request )
> +    {

remove space before ')'

> httpworker.cpp:144
> +    // Check if the data was obtained from cache or not
> +    QString fromCache = d->reply->attribute( QNetworkRequest::SourceIsFromCacheAttribute ).toBool() ? "(cached)" : "(NOT cached)";
> +

remove spaces inside ( ... )

> httpworker.cpp:151
>          if (d->redirectUrl.scheme().startsWith("http")) {
> -            qCInfo(KNEWSTUFFCORE) << "Redirected to " << d->redirectUrl.toDisplayString() << "...";
> -            reply->deleteLater();
> -            d->reply = d->qnam->get(QNetworkRequest(d->redirectUrl));
> +            qCInfo(KNEWSTUFFCORE) << d->reply->url().toDisplayString() << "was redirected to" << d->redirectUrl.toDisplayString() << fromCache << d->reply->attribute( QNetworkRequest::HttpStatusCodeAttribute ).toInt();
> +            d->reply->deleteLater();

remove spaces inside ( ... )

Also note that you added debugging values to a qCInfo message which is user-visible even in non debug builds. I think it might be better to revert this or split it out into a qCDebug call.

> httpworker.cpp:164
> +    else {
> +        qCInfo(KNEWSTUFFCORE) << "Data for" << d->reply->url().toDisplayString() << "was fetched" << fromCache;
> +    }

should probably be qCDebug()

REPOSITORY
  R304 KNewStuff

REVISION DETAIL
  https://phabricator.kde.org/D5638

To: leinir, whiting, apol, dfaure
Cc: dfaure, #frameworks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170506/896a07b9/attachment.html>