WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
61865
[Qt] Implement download feature for QtTestBrowser
https://bugs.webkit.org/show_bug.cgi?id=61865
Summary
[Qt] Implement download feature for QtTestBrowser
tbarat
Reported
2011-06-01 08:51:07 PDT
Implement save as feature for QTtestBrowser.
Attachments
proposed patch
(3.53 KB, patch)
2011-06-02 05:16 PDT
,
tbarat
ossy
: review-
ossy
: commit-queue-
Details
Formatted Diff
Diff
proposed patch
(3.54 KB, patch)
2011-06-03 00:57 PDT
,
tbarat
ossy
: review-
ossy
: commit-queue-
Details
Formatted Diff
Diff
proposed patch
(3.56 KB, patch)
2011-06-06 05:22 PDT
,
tbarat
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
tbarat
Comment 1
2011-06-02 05:16:37 PDT
Created
attachment 95752
[details]
proposed patch
Csaba Osztrogonác
Comment 2
2011-06-02 06:32:27 PDT
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
Comment 3
2011-06-03 00:57:51 PDT
Created
attachment 95867
[details]
proposed patch
Andras Becsi
Comment 4
2011-06-03 06:23:50 PDT
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
Comment 5
2011-06-03 06:30:28 PDT
(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 6
2011-06-03 06:46:14 PDT
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 7
2011-06-03 07:43:05 PDT
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
Comment 8
2011-06-06 05:22:27 PDT
Created
attachment 96077
[details]
proposed patch
Csaba Osztrogonác
Comment 9
2011-06-06 05:57:38 PDT
Comment on
attachment 96077
[details]
proposed patch LGTM, r=me
WebKit Commit Bot
Comment 10
2011-06-06 06:19:35 PDT
Comment on
attachment 96077
[details]
proposed patch Clearing flags on attachment: 96077 Committed
r88161
: <
http://trac.webkit.org/changeset/88161
>
WebKit Commit Bot
Comment 11
2011-06-06 06:19:41 PDT
All reviewed patches have been landed. Closing bug.
Csaba Osztrogonác
Comment 12
2011-06-06 07:57:59 PDT
Unreviewed --minimal buildfix landed in
http://trac.webkit.org/changeset/88167
Ademar Reis
Comment 13
2011-06-06 08:21:10 PDT
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
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug