Bug 68153 – [Qt][WK2] Implement Download support in WebProcess

• Component: New Bugs
• Status: RESOLVED FIXED

[Jesus Sanchez-Palencia]

[Qt][WK2] Implement Download support in WebProcess

[Jesus Sanchez-Palencia]

Created attachment 107479 [details]
Patch

[Jesus Sanchez-Palencia]

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]

Created attachment 107482 [details]
Patch

[Jesus Sanchez-Palencia]

Created attachment 107646 [details]
Patch

[Jocelyn Turcotte]

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]

Created attachment 107731 [details]
Patch

[Andreas Kling]

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]

Created attachment 107865 [details]
Patch

[Jesus Sanchez-Palencia]

Created attachment 107989 [details]
Patch

[Jocelyn Turcotte]

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]

(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]

Created attachment 107996 [details]
Patch

[Andreas Kling]

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]

Created attachment 108157 [details]
Patch

[WebKit Review Bot]

Comment on attachment 108157 [details]
Patch

Clearing flags on attachment: 108157

Committed r95631: <http://trac.webkit.org/changeset/95631>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.