[webkit-reviews] review granted: [Bug 74931] [Qt][WK2] Implement custom URL schemes defined in QML. : [Attachment 121258] patch for review. - using RefPtr for networkRequestData and networkReplyData.

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has granted Zeno Albisser
<zeno at webkit.org>'s request for review:
Bug 74931: [Qt][WK2] Implement custom URL schemes defined in QML.
https://bugs.webkit.org/show_bug.cgi?id=74931

Attachment 121258: patch for review. - using RefPtr for networkRequestData and
networkReplyData.
https://bugs.webkit.org/attachment.cgi?id=121258&action=review

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


> Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.cpp:49
> +

Why not make a 

static WebPage* obtainOriginatingWebPage(const QNetworkRequest& request)
{
    QObject* originatingObject = request.originatingObject();
    if (!originatingObject)
	return 0;

    QVariant var = originatingObject->property("PagePointer");
    if (!var.isValid() || !var.canConvert<void*>())
	return 0;

    WebPage* webPage = static_cast<WebPage*>(var.value<void*>());
    ASSERT(webPage);
    return webPage;
}


then ...

WebPage* origin = obtainOriginatingWebPage(request);
if (m_applicationSchemes.contains(origin, request.url().scheme().toLower())) {
    ...
}
return ...

> Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.cpp:50
> +QNetworkReply* QtNetworkAccessManager::createRequest(Operation operation,
const QNetworkRequest & request, QIODevice* outData)

style

> Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.cpp:53
> +    QObject* pagePtrWrapper = request.originatingObject();
> +    if (pagePtrWrapper) {

I would merge those two lines

> Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.h:46
> +    virtual QNetworkReply* createRequest(Operation, const QNetworkRequest &,
QIODevice* outgoingData = 0);

style. Also remember that we use OVERRIDE macro and nullptr in webkit today