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

[bugzilla-daemon at webkit.org]

Jocelyn Turcotte <jocelyn.turcotte at nokia.com> 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 107646: Patch
https://bugs.webkit.org/attachment.cgi?id=107646&action=review

------- Additional Comments from Jocelyn Turcotte <jocelyn.turcotte at nokia.com>
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.