[webkit-reviews] review denied: [Bug 68153] [Qt][WK2] Implement Download support in WebProcess : [Attachment 107731] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Jesus Sanchez-Palencia
<jesus at webkit.org>'s request for review:
Bug 68153: [Qt][WK2] Implement Download support in WebProcess
https://bugs.webkit.org/show_bug.cgi?id=68153

Attachment 107731: Patch
https://bugs.webkit.org/attachment.cgi?id=107731&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
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("Conte
nt-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.