[webkit-reviews] review denied: [Bug 50082] [Qt] Use QNetworkAccessManager zerocopy feature from QNetworkReplyHandler : [Attachment 74922] patch attempt 2

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Markus Goetz
<markus.goetz at nokia.com>'s request for review:
Bug 50082: [Qt] Use QNetworkAccessManager zerocopy feature from
QNetworkReplyHandler
https://bugs.webkit.org/show_bug.cgi?id=50082

Attachment 74922: patch attempt 2
https://bugs.webkit.org/attachment.cgi?id=74922&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=74922&action=review

> WebCore/platform/SharedBuffer.cpp:242
> -#if !PLATFORM(CF) || PLATFORM(QT)
> +#if !PLATFORM(CF) && !PLATFORM(QT)

this wont break normal build?

> WebCore/platform/SharedBuffer.h:45
>  #else
>  class NSData;
>  #endif
> +#endif

This added endif here seems wrong

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:238
> +    m_request.setAttribute(gMaximumDownloadBufferSizeAttribute, 1024*256);
// 256 kB

needs spaces around *. Need dot at end of comment

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:370
> +    // Check if there is a zerocopy buffer

Needs dot at end of comment

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:378
> +	   // Old school way. Use QNetworkReply as a QIODevice

Strange comment, rewrite also add dot at the end.

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:380
> +	   m_usingZeroCopy = false;

maybe call this m_usingZeroCopyFeature

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:485
> +    // don't emit the "Document has moved here" type of HTML

End with dot, starts with capital

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:503
> +			      oldSize, bytesReceived - oldSize, bytesReceived -
oldSize /*FixMe*/);

call it FIXME: plus add a description on WHAT to fix!

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:638
> +    // For zero copy

Misses dot.

> WebCore/platform/network/qt/QNetworkReplyHandler.h:94
> +    // for zerocopy. Holds the download data:

fix comment

> WebCore/platform/network/qt/QNetworkReplyHandler.h:96
> +    // for zerocopy it wraps m_byteBlock, otherwise it holds data normally:

also

> WebCore/platform/qt/QByteBlock.cpp:29
> +    QByteBlock* bb = new QByteBlock;

write out bb

> WebCore/platform/qt/QByteBlock.cpp:46
> +    return (const char*)(m_data.data());

I do not think the latter () are needed

> WebCore/platform/qt/QByteBlock.cpp:51
> +    m_size = s;

just write out s

> WebCore/platform/qt/QByteBlock.cpp:56
> +    m_capacity = c;

and c!