WebKit Bugzilla
You need to log in before you can comment on or make changes to this bug.
[Qt][WK2] Implement Download support in WebProcess
Created an attachment (id=107479) [details] Patch
(From update of attachment 107479 [details]) webkit-patch upload didn't run prepare-changelog... =/ Doing it manually and uploading a new patch in a few minutes.
Created an attachment (id=107482) [details] Patch
Created an attachment (id=107646) [details] Patch
(From update of attachment 107646 [details]) 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.
Created an attachment (id=107731) [details] Patch
(From update of attachment 107731 [details]) 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.
Created an attachment (id=107865) [details] Patch
Created an attachment (id=107989) [details] Patch
(From update of attachment 107989 [details]) 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.
(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...
Created an attachment (id=107996) [details] Patch
(From update of attachment 107996 [details]) 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.
Created an attachment (id=108157) [details] Patch
(From update of attachment 108157 [details]) Clearing flags on attachment: 108157 Committed r95631: <http://trac.webkit.org/changeset/95631>
All reviewed patches have been landed. Closing bug.