[webkit-reviews] review denied: [Bug 76700] [Qt][WK2] Application URL schemes cause asserts when using debug. : [Attachment 123545] patch for review. - updated as commented previously.

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Zeno Albisser
<zeno at webkit.org>'s request for review:
Bug 76700: [Qt][WK2] Application URL schemes cause asserts when using debug.
https://bugs.webkit.org/show_bug.cgi?id=76700

Attachment 123545: patch for review. - updated as commented previously.
https://bugs.webkit.org/attachment.cgi?id=123545&action=review

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


Sorry, not all RefPtrs are used correctly yet :)

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:125
> +    uint64_t stringLength = m_dataLength / sizeof(UChar);
> +    return QString(reinterpret_cast<const QChar*>(m_sharedMemory->data()),
stringLength);

I see a wild mix of UChar and QChar here, assuming that they are of the same
size. While that is indeed given, I think the code would be cleaner if it was
using one type only.

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:137
>      const UChar* ucharData = reinterpret_cast<const
UChar*>(data.constData());
>      uint64_t smLength = sizeof(UChar) * data.length();

Why bother with UChar at all? If the goal is to serialize the string, then why
not simply copy the QChar data straight into it and also use sizeof(QChar)?

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply_p.h:63
> +    WTF::RefPtr<WebKit::QtRefCountedNetworkRequestData> networkRequestData()
const;
> +    void
setNetworkRequestData(WTF::RefPtr<WebKit::QtRefCountedNetworkRequestData>
data);
> +    WTF::RefPtr<WebKit::QtRefCountedNetworkReplyData> networkReplyData()
const;

See my earlier comment, these functions should follow the refptr guidelines.

> Source/WebKit2/UIProcess/API/qt/qquicknetworkrequest_p.h:33
>      Q_PROPERTY(QString url READ url)

Any particular reason the url is a string instead of a QUrl?