Bug 61865 – [Qt] Implement download feature for QtTestBrowser

• Component: Tools / Tests
• Status: RESOLVED FIXED

[tbarat]

Implement save as feature for QTtestBrowser.

[tbarat]

Created attachment 95752 [details]
proposed patch

[Csaba Osztrogonác]

Comment on attachment 95752 [details]
proposed patch

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

> Tools/QtTestBrowser/launcherwindow.cpp:53
>      , m_zoomAnimation(0)
> +    , m_reply(0)
>  {

Please update this part of the patch, because it conflicts with ToT.

> Tools/QtTestBrowser/launcherwindow.cpp:940
> +    connect(m_reply, SIGNAL(finished()), this, SLOT(slotFinished()));

I can't find slotFinished anywhere, but fileDownloadFinished.

> Tools/QtTestBrowser/launcherwindow.cpp:950
> +    if (fileName.isEmpty() || !m_reply->errorString().isNull())
> +        QMessageBox::warning(this, QString("Download"), QString("Download failed."));

It fails for me, m_reply->errorString() returns always with "Unknown error."

[tbarat]

Created attachment 95867 [details]
proposed patch

[Andras Becsi]

Comment on attachment 95867 [details]
proposed patch

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

> Tools/QtTestBrowser/launcherwindow.cpp:991
> +    if (fileName.isEmpty() && m_reply->error() != QNetworkReply::NoError)

AND does not seem to be correct here, you need OR.

[Andras Becsi]

(In reply to comment #4)
> (From update of attachment 95867 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95867&action=review
> 
> > Tools/QtTestBrowser/launcherwindow.cpp:991
> > +    if (fileName.isEmpty() && m_reply->error() != QNetworkReply::NoError)
> 
> AND does not seem to be correct here, you need OR.

Most probably if you have fileName.isEmpty() then you sould simply return, since the user pressed "Cancel" on the dialog, and only show an error message when m_reply->error() indicates an error.

[Andras Becsi]

Comment on attachment 95867 [details]
proposed patch

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

Later this could be extended with a simple progress bar, but I think this is not the scope of this patch.

Because of the problems previously pointed out, I would say r- for this patch if I were a reviewer.

> Tools/QtTestBrowser/launcherwindow.cpp:992
> +        QMessageBox::warning(this, QString("Download"), QString("Download failed."));

Further more, the message shown here could be more expressive if you would display m_reply->errorString(). And I would use critical here, but that's only a nitpick.

[Csaba Osztrogonác]

Comment on attachment 95867 [details]
proposed patch

I agree with Andras, user shouldn't get "Download failed" error if he or she 
pressed cancel button. And crital message would be better than warning message.
r- now, but I'll set r+ if you fix these things.

> Tools/QtTestBrowser/launcherwindow.cpp:991
> +    if (fileName.isEmpty() && m_reply->error() != QNetworkReply::NoError)

I mean this would be good:

if (fileName.isEmpty())
    return;
if (m_reply->error() != QNetworkReply::NoError)
    QMessageBox::critical(this, QString("Download"), QString("Download failed."));

[tbarat]

Created attachment 96077 [details]
proposed patch

[Csaba Osztrogonác]

Comment on attachment 96077 [details]
proposed patch

LGTM, r=me

[WebKit Commit Bot]

Comment on attachment 96077 [details]
proposed patch

Clearing flags on attachment: 96077

Committed r88161: <http://trac.webkit.org/changeset/88161>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Unreviewed --minimal buildfix landed in http://trac.webkit.org/changeset/88167

[Ademar Reis]

Revision r88161 cherry-picked into qtwebkit-2.2 with commit 1be60bc <http://gitorious.org/webkit/qtwebkit/commit/1be60bc>
Revision r88167 cherry-picked into qtwebkit-2.2 with commit 0558b11 <http://gitorious.org/webkit/qtwebkit/commit/0558b11>