[webkit-reviews] review denied: [Bug 77007] [Qt][WK2] Use QVariant for payload data in application URL schemes. : [Attachment 123935] patch for review.

Thu Jan 26 00:15:54 PST 2012

Simon Hausmann <hausmann at webkit.org> has denied Zeno Albisser
<zeno at webkit.org>'s request for review:
Bug 77007: [Qt][WK2] Use QVariant for payload data in application URL schemes.
https://bugs.webkit.org/show_bug.cgi?id=77007

Attachment 123935: patch for review.
https://bugs.webkit.org/attachment.cgi?id=123935&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=123935&action=review

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:54
> +    return QVariant(QByteArray::fromRawData(static_cast<const
char*>(m_sharedMemory->data()), m_dataLength));

This is unfortunately also not a safe operation as it requires the caller to
keep the network reply alive while keeping a reference to the bytearray.

> Source/WebKit2/UIProcess/API/qt/tests/bytearraytestdata.cpp:25
> +    , m_iso8859_1(QByteArray("<html><head><title>this is iso8859-1
content</title></head><body>content</body></html>"))
> +    , m_utf8(QByteArray(QString("<html><head><title>this is utf-8
content</title></head><body>content</body></html>").toUtf8()))

How about including a character in the test strings that is not part of ASCII
but ISO-8859-1 such as

    251   169	A9     ©     COPYRIGHT SIGN

Then you could do:

QString text = QStringLiteral("<html><head><title>title with copyright %1");
text = text.arg(QChar::fromLatin1(169));

and then

latin1Data = text.toLatin1();
utf8Data = text.toUtf8();

Q_ASSERT(latin1Data != utf8Data);

> Source/WebKit2/UIProcess/API/qt/tests/bytearraytestdata.h:32
> +Q_OBJECT
> +Q_PROPERTY(QVariant iso88591data READ iso88591data)
> +Q_PROPERTY(QVariant utf8data READ utf8data)

Missing indentation.

> Source/WebKit2/UIProcess/API/qt/tests/bytearraytestdata.h:37
> +    QVariant iso88591data();
> +    QVariant utf8data();

Missing const.