Bug 68153

Summary: [Qt][WK2] Implement Download support in WebProcess
Product: WebKit Reporter: Jesus Sanchez-Palencia <jesus>
Component: New BugsAssignee: Jesus Sanchez-Palencia <jesus>
Severity: Normal CC: jturcotte, kling, menard, webkit.review.bot
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch none

Description Jesus Sanchez-Palencia 2011-09-15 05:27:06 PDT
[Qt][WK2] Implement Download support in WebProcess
Comment 1 Jesus Sanchez-Palencia 2011-09-15 05:28:43 PDT
Created attachment 107479 [details]
Comment 2 Jesus Sanchez-Palencia 2011-09-15 05:30:30 PDT
Comment on attachment 107479 [details]

webkit-patch upload didn't run prepare-changelog... =/

Doing it manually and uploading a new patch in a few minutes.
Comment 3 Jesus Sanchez-Palencia 2011-09-15 05:58:18 PDT
Created attachment 107482 [details]
Comment 4 Jesus Sanchez-Palencia 2011-09-16 07:15:43 PDT
Created attachment 107646 [details]
Comment 5 Jocelyn Turcotte 2011-09-16 08:02:14 PDT
Comment on 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.
Comment 6 Jesus Sanchez-Palencia 2011-09-16 15:34:02 PDT
Created attachment 107731 [details]
Comment 7 Andreas Kling 2011-09-16 16:10:38 PDT
Comment on 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)

> 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);


> 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);


> 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.
Comment 8 Jesus Sanchez-Palencia 2011-09-19 08:57:17 PDT
Created attachment 107865 [details]
Comment 9 Jesus Sanchez-Palencia 2011-09-20 05:52:14 PDT
Created attachment 107989 [details]
Comment 10 Jocelyn Turcotte 2011-09-20 06:18:50 PDT
Comment on 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.
Comment 11 Jesus Sanchez-Palencia 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...
Comment 12 Jesus Sanchez-Palencia 2011-09-20 06:48:34 PDT
Created attachment 107996 [details]
Comment 13 Andreas Kling 2011-09-21 00:37:27 PDT
Comment on 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.
Comment 14 Jesus Sanchez-Palencia 2011-09-21 07:49:47 PDT
Created attachment 108157 [details]
Comment 15 WebKit Review Bot 2011-09-21 08:09:49 PDT
Comment on attachment 108157 [details]

Clearing flags on attachment: 108157

Committed r95631: <http://trac.webkit.org/changeset/95631>
Comment 16 WebKit Review Bot 2011-09-21 08:09:55 PDT
All reviewed patches have been landed.  Closing bug.