WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
68153
[Qt][WK2] Implement Download support in WebProcess
https://bugs.webkit.org/show_bug.cgi?id=68153
Summary
[Qt][WK2] Implement Download support in WebProcess
Jesus Sanchez-Palencia
Reported
2011-09-15 05:27:06 PDT
[Qt][WK2] Implement Download support in WebProcess
Attachments
Patch
(24.28 KB, text/plain)
2011-09-15 05:28 PDT
,
Jesus Sanchez-Palencia
no flags
Details
Patch
(28.48 KB, patch)
2011-09-15 05:58 PDT
,
Jesus Sanchez-Palencia
no flags
Details
Formatted Diff
Diff
Patch
(24.10 KB, patch)
2011-09-16 07:15 PDT
,
Jesus Sanchez-Palencia
no flags
Details
Formatted Diff
Diff
Patch
(24.22 KB, patch)
2011-09-16 15:34 PDT
,
Jesus Sanchez-Palencia
no flags
Details
Formatted Diff
Diff
Patch
(24.21 KB, patch)
2011-09-19 08:57 PDT
,
Jesus Sanchez-Palencia
no flags
Details
Formatted Diff
Diff
Patch
(23.57 KB, patch)
2011-09-20 05:52 PDT
,
Jesus Sanchez-Palencia
no flags
Details
Formatted Diff
Diff
Patch
(23.35 KB, patch)
2011-09-20 06:48 PDT
,
Jesus Sanchez-Palencia
no flags
Details
Formatted Diff
Diff
Patch
(24.60 KB, patch)
2011-09-21 07:49 PDT
,
Jesus Sanchez-Palencia
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Jesus Sanchez-Palencia
Comment 1
2011-09-15 05:28:43 PDT
Created
attachment 107479
[details]
Patch
Jesus Sanchez-Palencia
Comment 2
2011-09-15 05:30:30 PDT
Comment on
attachment 107479
[details]
Patch webkit-patch upload didn't run prepare-changelog... =/ Doing it manually and uploading a new patch in a few minutes.
Jesus Sanchez-Palencia
Comment 3
2011-09-15 05:58:18 PDT
Created
attachment 107482
[details]
Patch
Jesus Sanchez-Palencia
Comment 4
2011-09-16 07:15:43 PDT
Created
attachment 107646
[details]
Patch
Jocelyn Turcotte
Comment 5
2011-09-16 08:02:14 PDT
Comment on
attachment 107646
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=107646&action=review
> Source/WebKit2/WebProcess/Downloads/qt/DownloadQt.cpp:48 > + m_qtDownloader = new QtFileDownloader(this, manager, m_request.toNetworkRequest(0));
The (0) is not talking much by itself. If a null originating object is correctly allowed by toNetworkRequest, I think it would be better to set null as originatingObject's default parameter value in it's declaration.
> Source/WebKit2/WebProcess/Downloads/qt/DownloadQt.cpp:68 > + m_qtDownloader->deleteLater();
+ m_qtDownloader = 0;
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:39 > +QtFileDownloader::QtFileDownloader(Download* download, QNetworkAccessManager* manager, const QNetworkRequest& request)
m_manager and m_request aren't needed with the single threading in trunk, and this constructor could be merged with the QNetworkReply constructor.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:72 > +void QtFileDownloader::start()
start() was needed because of the asynchronous calls between threads. The logic could be moved to the constructor and the reply could be created outside by the Download object.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:101 > + m_download->didFail(downloadError, CoreIPC::DataReference(reinterpret_cast<const uint8_t*>(m_reply->readAll().data()), m_reply->size() + 1));
Why are those "+ 1" necessary? I don't see them in the Mac port.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:142 > + QFile* downloadFile = new QFile(decidedFilePath);
It would be safer to use an OwnPtr or a QScopedPointer here, same thing for m_destinationFile. I think m_destinationFile isn't deleted in the destructor, for example.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.h:57 > + void onDecidedDestination(const QString& decidedFilePath, bool allowOverwrite); > + void onCancel();
Those don't need to be slots anymore and could be renamed as actions instead of reactions.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.h:73 > + friend class Download;
You shouldn't need this.
Jesus Sanchez-Palencia
Comment 6
2011-09-16 15:34:02 PDT
Created
attachment 107731
[details]
Patch
Andreas Kling
Comment 7
2011-09-16 16:10:38 PDT
Comment on
attachment 107731
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=107731&action=review
> Source/WebKit2/WebProcess/Downloads/qt/DownloadQt.cpp:60 > + m_qtDownloader->cancel();
Let's put an ASSERT(m_qtDownloader); up in here for good measure.
> Source/WebKit2/WebProcess/Downloads/qt/DownloadQt.cpp:65 > + Q_ASSERT(m_qtDownloader);
We strive to use ASSERT rather than Q_ASSERT in WebKit code.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:58 > + if (m_destinationFile) {
Early-return style would look better here. if (!m_destinationFile) return;
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:65 > + m_destinationFile->close(); > + m_destinationFile.clear();
Both of these lines are redundant since ~QFile will close() and ~OwnPtr will delete the QFile.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:71 > + Q_ASSERT(!m_destinationFile);
Q_ASSERT -> ASSERT
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:73 > + QString fileName;
This declaration should be moved below where 'fileName' is first used.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:74 > + QString fileNameCandidate = filenameFromHTTPContentDisposition(QString::fromAscii(m_reply->rawHeader("Content-Disposition")));
QString::fromAscii() is almost always wrong as its behavior depends on the current system locale. You probably want QString::fromUtf8() or QString::fromLatin1(). Check other implementations.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:86 > + // Don't use size + 1 so the ending '\0' byte from QByteArray::data() is discarded
Period at end of sentences, bro.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:105 > + bool allowOverwrite; > + m_download->decideDestinationWithSuggestedFilename(fileName, allowOverwrite);
We don't need the value returned in 'allowOverwrite' for anything?
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:110 > + Q_ASSERT(!m_destinationFile);
Q_ASSERT -> ASSERT
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:140 > + // FIXME: here's a race condition due to qt api not allowing > + // files to be created only if they don't exist
This is unclear, could you clean up the English a bit and be more specific?
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:185 > + if (bytesWritten == -1 || bytesWritten != content.size()) {
Are you sure that (bytesWritten != content.size()) is always an error? In other words, should we retry at an offset in that case?
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:199 > + WTF::String contentType = m_reply->header(QNetworkRequest::ContentTypeHeader).toString(); > + WTF::String encoding = extractCharsetFromMediaType(contentType); > + WTF::String mimeType = extractMIMETypeFromMediaType(contentType);
I'd prefer that we were using namespace WTF;
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:216 > + if (m_destinationFile) {
Prefer early-return style here.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:218 > + m_destinationFile->close(); > + m_destinationFile.clear();
Just clear() as noted earlier.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.h:31 > +class QFile; > +class QNetworkAccessManager; > +class QNetworkRequest;
These need to be wrapped in QT_BEGIN_NAMESPACE and QT_END_NAMESPACE.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.h:54 > + DownloadErrorFilename = -1
What does DownloadErrorFilename mean?
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.h:60 > + void onError(QNetworkReply::NetworkError code);
The argument name 'code' adds nothing here.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.h:63 > + void abortDownloadWritingAndEmitError(const WebCore::ResourceError& errorCode);
The argument name 'errorCode' adds nothing here.
Jesus Sanchez-Palencia
Comment 8
2011-09-19 08:57:17 PDT
Created
attachment 107865
[details]
Patch
Jesus Sanchez-Palencia
Comment 9
2011-09-20 05:52:14 PDT
Created
attachment 107989
[details]
Patch
Jocelyn Turcotte
Comment 10
2011-09-20 06:18:50 PDT
Comment on
attachment 107989
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=107989&action=review
LGTM beside one thing:
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:145 > + m_download->didFail(QNetworkReplyHandler::errorForReply(m_reply.get()), CoreIPC::DataReference(reinterpret_cast<const uint8_t*>(m_resumeData.data()), dataSize));
Looked a bit further and resumeData is a mac-specific thing, it should be an URL+offest+partialFileName kind of thing and not actual downloaded data. DownloadCFNet.cpp set it to DataReference(0,0) and we can do the same until we properly support and test download resuming, if ever.
Jesus Sanchez-Palencia
Comment 11
2011-09-20 06:37:28 PDT
(In reply to
comment #10
)
> Looked a bit further and resumeData is a mac-specific thing, it should be an URL+offest+partialFileName kind of thing and not actual downloaded data. DownloadCFNet.cpp set it to DataReference(0,0) and we can do the same until we properly support and test download resuming, if ever.
Ok, thanks, I will do the same and take our m_resumeData out of this patch. New patch coming after the break...
Jesus Sanchez-Palencia
Comment 12
2011-09-20 06:48:34 PDT
Created
attachment 107996
[details]
Patch
Andreas Kling
Comment 13
2011-09-21 00:37:27 PDT
Comment on
attachment 107996
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=107996&action=review
Looks good to me as well, though I'm gonna say r- due to the missing QWebError code.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:141 > + // Don't use size + 1 so the ending '\0' byte from QByteArray::data() is discarded.
This comment no longer applies.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:170 > + ResourceError downloadError("Download", errorCode, m_reply->url().toString(), translatedErrorMessage);
Since you're adding the "Download" class of errors, you also need to add a corresponding error type to QWebError and hook that up.
> Source/WebKit2/WebProcess/Downloads/qt/QtFileDownloader.cpp:172 > + // Don't use size + 1 so the ending '\0' byte from QByteArray::data() is discarded.
This comment no longer applies.
Jesus Sanchez-Palencia
Comment 14
2011-09-21 07:49:47 PDT
Created
attachment 108157
[details]
Patch
WebKit Review Bot
Comment 15
2011-09-21 08:09:49 PDT
Comment on
attachment 108157
[details]
Patch Clearing flags on attachment: 108157 Committed
r95631
: <
http://trac.webkit.org/changeset/95631
>
WebKit Review Bot
Comment 16
2011-09-21 08:09:55 PDT
All reviewed patches have been landed. Closing bug.
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