Bug 35671

Summary: [Qt] Even if QNetworkRequest::AlwaysCache is set on a request, a page will be loaded from the network the first time
Product: WebKit Reporter: Tor Arne Vestbø <vestbo>
Component: New BugsAssignee: QtWebKit Unassigned <webkit-qt-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: jhanssen, kent.hansen, webkit.review.bot
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Tor Arne Vestbø 2010-03-03 04:01:00 PST
This bug report originated from issue QTBUG-4839
http://bugreports.qt.nokia.com/browse/QTBUG-4839

--- Description ---

The following example (whole example in the attachment) will load the page from the network even if QNetworkRequest::AlwaysCache is set. the documentation says "only load from cache, indicating error if the item was not cached (i.e., off-line mode)":

<div class="code panel" style="border-width: 1px;"><div class="codeContent panelContent">
<pre class="code-java">QWebPage *webPage = ui-&gt;webView-&gt;page();
QNetworkAccessManager *netAccessMgr = <span class="code-keyword">new</span> QNetworkAccessManager(<span class="code-keyword">this</span>);
QNetworkDiskCache *dc = <span class="code-keyword">new</span> QNetworkDiskCache(<span class="code-keyword">this</span>);
dc-&gt;setCacheDirectory(<span class="code-quote">"cache"</span>);
netAccessMgr-&gt;setCache(dc);
webPage-&gt;setNetworkAccessManager(netAccessMgr);

QNetworkRequest *request = <span class="code-keyword">new</span> QNetworkRequest(QUrl(ui-&gt;lineEdit-&gt;text()));
request-&gt;setAttribute(QNetworkRequest::CacheLoadControlAttribute, QNetworkRequest::AlwaysCache);
ui-&gt;webView-&gt;load(*request);</pre>
</div></div>

--- Comments ---

Do we have an AlwaysCache autotest?

Yes, we do have a test. This is not a QNetworkAccessManager issue. It's a QWebFrame issue, that doesn't honour the attributes set in the request.

PS: your sample code leaks the QNetworkRequest. Don't use "new".
Comment 1 Kent Hansen 2010-03-12 05:59:43 PST
Reproduced with r55658.
Comment 2 Jan Erik Hanssen 2010-12-05 23:26:37 PST
Created attachment 75649 [details]
Patch
Comment 3 Antonio Gomes 2010-12-06 07:25:24 PST
Comment on attachment 75649 [details]
Patch

lgtm
Comment 4 Eric Seidel (no email) 2010-12-09 23:55:40 PST
Comment on attachment 75649 [details]
Patch

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

> WebKit/qt/Api/qwebframe.cpp:867
> +            switch (cacheLoadValue) {
> +            case QNetworkRequest::AlwaysNetwork:
> +                request.setCachePolicy(WebCore::ReloadIgnoringCacheData);
> +                break;
> +            case QNetworkRequest::PreferCache:
> +                request.setCachePolicy(WebCore::ReturnCacheDataElseLoad);
> +                break;
> +            case QNetworkRequest::AlwaysCache:
> +                request.setCachePolicy(WebCore::ReturnCacheDataDontLoad);
> +                break;
> +            case QNetworkRequest::PreferNetwork:
> +                request.setCachePolicy(WebCore::UseProtocolCachePolicy);
> +                break;

This would be much better as a helper function to convert the enum.  Then you only need one request.setCachePolicy call and this function gets much cleaner.
Comment 5 Jan Erik Hanssen 2010-12-10 14:25:35 PST
Created attachment 76261 [details]
Patch
Comment 6 Andreas Kling 2010-12-11 03:31:28 PST
Comment on attachment 76261 [details]
Patch

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

LGTM, two small things:

> WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3075
> +        : QNetworkAccessManager(parent), m_lastCacheLoad(QNetworkRequest::PreferNetwork)

Style, initializers should be on separate lines.

> WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3083
> +            m_lastCacheLoad = static_cast<QNetworkRequest::CacheLoadControl>(cacheLoad.toInt());

You're using toInt() here and toUInt() in QWebFrame::load().
Comment 7 Jan Erik Hanssen 2010-12-11 05:46:00 PST
Created attachment 76305 [details]
Patch
Comment 8 Jan Erik Hanssen 2010-12-11 05:47:03 PST
(In reply to comment #6)
> (From update of attachment 76261 [details])
> > WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:3083
> > +            m_lastCacheLoad = static_cast<QNetworkRequest::CacheLoadControl>(cacheLoad.toInt());
> 
> You're using toInt() here and toUInt() in QWebFrame::load().

Indeed, nice catch. I should have spotted that :)
Comment 9 WebKit Review Bot 2010-12-11 19:22:06 PST
Comment on attachment 76305 [details]
Patch

Clearing flags on attachment: 76305

Committed r73865: <http://trac.webkit.org/changeset/73865>
Comment 10 WebKit Review Bot 2010-12-11 19:22:12 PST
All reviewed patches have been landed.  Closing bug.