Bug 74931 – [Qt][WK2] Implement custom URL schemes defined in QML.

• Component: WebKit2
• Status: RESOLVED FIXED

[Zeno Albisser]

Implement an experimental feature to allow defining custom URL schemes in QML.
The URL schemes should then be handled in QML/JS as well.

[Zeno Albisser]

Created attachment 120034 [details]
patch for feedback.

[Kenneth Rohde Christiansen]

Comment on attachment 120034 [details]
patch for feedback.

View in context: https://bugs.webkit.org/attachment.cgi?id=120034&action=review

just some preliminary comments, as i am not so much into networking

> Source/WebKit2/Shared/qt/NetworkReplyData.h:37
> +#include "KURL.h"
> +#include "Noncopyable.h"
> +#include "SharedMemory.h"
> +#include "WebCoreArgumentCoders.h"
> +
> +#include <QNetworkAccessManager>
> +#include <QNetworkRequest>
> +#include <wtf/text/WTFString.h>

Why are these separated in two groups?

> Source/WebKit2/Shared/qt/NetworkReplyData.h:41
> +struct NetworkReplyData {

Should this be called NetworkReplyDataQt?

> Source/WebKit2/Shared/qt/NetworkReplyData.h:95
> +    WTF::String m_url;

urlString?

> Source/WebKit2/Shared/qt/NetworkRequestData.h:45
> +    NetworkRequestData(const QNetworkRequest& req, QNetworkReply* rep)

rep? why not spell these out?

> Source/WebKit2/Shared/qt/NetworkRequestData.h:53
> +        // We store a pointer to the reply as an integer value.
> +        // This value can be used as uid in the UIProcess and
> +        // it serves as a pointer to the QNetworkReply when we receive
> +        // the asynchronous NetworkReplyData in the WebProcess.
> +        m_reply = reinterpret_cast<size_t>(rep);

Maybe this could be better reflected in the variable name?

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:29
> +#include "NetworkReplyData.h"
> +#include "NetworkRequestData.h"
> +#include "qquickwebview_p.h"
> +#include "qwebkitglobal.h"
> +
> +#include <QDateTime>

I think these are supposed to be in the same group

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:42
> +}
> +void QQuickNetworkReply::setContentType(const QString& contentType)

Missing newline

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:136
> +    uint64_t smLength = sizeof(UChar)*data.length();

spaces around *

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:142
> +    RefPtr<SharedMemory> sm = SharedMemory::create(smLength);
> +    if (!sm)
> +        return;
> +    memset(sm->data(), 0, sm->size());
> +    memcpy(sm->data(), ucharData, smLength);

Is there a case where smLength != sm->size()?

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:154
> +    QQuickWebViewExperimental* experimentalWebView = qobject_cast<QQuickWebViewExperimental*>(schemeParent->parent());

it is not the webview that is experimental :-) but some of the API of the webview

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:171
> +    // We copy the size_t value that identifies the QQuickNetworkReply
> +    // to the NetworkReplyData structure. So we have it available as
> +    // a pointer in the WebProcess again.
> +    m_networkReplyData->m_reply = data->m_reply;

why isnt that variable called soemthing with *identifier?

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply_p.h:43
> +    QQuickNetworkReply(QObject* parent = 0);

When will this have a parent and when will it not?

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply_p.h:57
> +    void setUserAgent(const QString&);

setUserAgentString?

> Source/WebKit2/UIProcess/API/qt/qquickurlscheme_p.h:31
> +class QWEBKIT_EXPORT QQuickUrlScheme : public QObject {

Why isnt this called somethign with Delegate? If you dont know the api, UrlScheme sounds weird and it is hard to know what it is.

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:659
> +QQuickUrlScheme* QQuickWebViewExperimental::schemeDelegatesAt(QDeclarativeListProperty<QQuickUrlScheme>* prop, int index)

prop*s*

schemeDelegateAt (without s?)

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:683
> +void QQuickWebViewExperimental::schemeDelegatesClear(QDeclarativeListProperty<QQuickUrlScheme>* prop)

Wouldn't removeAllSchemeDelegates be more Qtish?

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:204
> +    static void schemeDelegatesAppend(QDeclarativeListProperty<QQuickUrlScheme>*, QQuickUrlScheme*);

Is it important that the use knows that it is appended? It sounds like an impl detail

> Source/WebKit2/UIProcess/qt/WebPageProxyQt.cpp:55
> +void WebPageProxy::registerApplicationScheme(const String& scheme)

I wonder if it is always an application that handles this, ie. maybe registerSchemeHandler makes more sense?

> Source/WebKit2/WebProcess/WebCoreSupport/qt/PagePointerWrapper.h:37
> +class PagePointerWrapper : public QObject {
> +    Q_OBJECT
> +public:
> +    PagePointerWrapper(WebKit::WebPage*);
> +    WebKit::WebPage* page() const;
> +private:
> +    WebKit::WebPage* m_page;
> +};

What is the purpose of this? Maybe the name could reflect this better

> Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.cpp:46
> +QtNetworkAccessManager::QtNetworkAccessManager(QObject * parent)

style

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

outData?

> Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.h:41
> +    QtNetworkAccessManager(QObject * parent = 0);

style

> Source/WebKit2/WebProcess/qt/QtNetworkReply.cpp:41
> +    : QNetworkReply((QObject*)parent)

static_cast?

> Source/WebKit2/WebProcess/qt/QtNetworkReply.cpp:84
> +qint64 QtNetworkReply::readData(char *data, qint64 maxlen)

style

[Zeno Albisser]

Comment on attachment 120034 [details]
patch for feedback.

View in context: https://bugs.webkit.org/attachment.cgi?id=120034&action=review

Will upload a revised version soon. Thank you very much for the feedback! :-)

>> Source/WebKit2/Shared/qt/NetworkReplyData.h:37
>> +#include <wtf/text/WTFString.h>
> 
> Why are these separated in two groups?

fixed.

>> Source/WebKit2/Shared/qt/NetworkReplyData.h:41
>> +struct NetworkReplyData {
> 
> Should this be called NetworkReplyDataQt?

I'm not sure. Should it? This class is Qt only. Afaik the Qt suffix is usually for a platform specific implementation of a general webkit class. Or am i wrong about that?

>> Source/WebKit2/Shared/qt/NetworkReplyData.h:95
>> +    WTF::String m_url;
> 
> urlString?

done.

>> Source/WebKit2/Shared/qt/NetworkRequestData.h:45
>> +    NetworkRequestData(const QNetworkRequest& req, QNetworkReply* rep)
> 
> rep? why not spell these out?

right - fixed.

>> Source/WebKit2/Shared/qt/NetworkRequestData.h:53
>> +        m_reply = reinterpret_cast<size_t>(rep);
> 
> Maybe this could be better reflected in the variable name?

got rid of that, in favor of a Uuid.

>> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:29
>> +#include <QDateTime>
> 
> I think these are supposed to be in the same group

done.

>> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:42
>> +void QQuickNetworkReply::setContentType(const QString& contentType)
> 
> Missing newline

fixed.

>> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:136
>> +    uint64_t smLength = sizeof(UChar)*data.length();
> 
> spaces around *

done.

>> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:142
>> +    memcpy(sm->data(), ucharData, smLength);
> 
> Is there a case where smLength != sm->size()?

Yes, at least on mac the actual size of the shared memory is rounded to pages. So sm->size() will usually be bigger then smLength.

>> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:154
>> +    QQuickWebViewExperimental* experimentalWebView = qobject_cast<QQuickWebViewExperimental*>(schemeParent->parent());
> 
> it is not the webview that is experimental :-) but some of the API of the webview

good point :-) - i'll rename it to webViewExperimental.

>> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:171
>> +    m_networkReplyData->m_reply = data->m_reply;
> 
> why isnt that variable called soemthing with *identifier?

replaced with m_replyUuid.

>> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply_p.h:43
>> +    QQuickNetworkReply(QObject* parent = 0);
> 
> When will this have a parent and when will it not?

This is instantiated from QML... so it should always have a parent.

>> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply_p.h:57
>> +    void setUserAgent(const QString&);
> 
> setUserAgentString?

Is there a reason for that? We are leaving out parameter names if they do not add information. What information would changing this to setUserAgentString add? I think the function signature clearly shows that a string is expected.
Actually this is only supposed to be called as a property in QML anyway.

>> Source/WebKit2/UIProcess/API/qt/qquickurlscheme_p.h:31
>> +class QWEBKIT_EXPORT QQuickUrlScheme : public QObject {
> 
> Why isnt this called somethign with Delegate? If you dont know the api, UrlScheme sounds weird and it is hard to know what it is.

Hm... yeah i see that from the C++ side of the API this is not really obvious.
Although UrlScheme is the right name i believe, and QQuick prefix is descriptive as well. So i think i will just make it QQuickUrlSchemeDelegate.
I am also wondering how the QML API should look like. Currently it's a list like:
schemeDelegates: [
    UrlScheme {
        scheme: "something" 
        ... },
     ]
It seems to me there is quite a bit of redundancy in the naming. Any ideas how to improve that?

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:659
>> +QQuickUrlScheme* QQuickWebViewExperimental::schemeDelegatesAt(QDeclarativeListProperty<QQuickUrlScheme>* prop, int index)
> 
> prop*s*
> 
> schemeDelegateAt (without s?)

renamed prop to property.
I would prefer to leave schemeDelegatesAt etc. (see also qdeclarativewebview.cpp).
Because this function actually is returning the scheme delegate at a certain position in the "schemeDelegates list". Which in fact not is a real list in this case, but that's not the point.
In Qt we would usually call the function schemeDelegates_At(..). But since webkit style doesn't like underscores in function names, this brings us to schemeDelegatesAt again.

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:683
>> +void QQuickWebViewExperimental::schemeDelegatesClear(QDeclarativeListProperty<QQuickUrlScheme>* prop)
> 
> Wouldn't removeAllSchemeDelegates be more Qtish?

No, i don't think so. This is the way this is usually done for a QDeclarativeListProperty. (see previous comment)

>> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:204
>> +    static void schemeDelegatesAppend(QDeclarativeListProperty<QQuickUrlScheme>*, QQuickUrlScheme*);
> 
> Is it important that the use knows that it is appended? It sounds like an impl detail

I don't follow what you mean here... but i believe that is how it is usually done.

>> Source/WebKit2/UIProcess/qt/WebPageProxyQt.cpp:55
>> +void WebPageProxy::registerApplicationScheme(const String& scheme)
> 
> I wonder if it is always an application that handles this, ie. maybe registerSchemeHandler makes more sense?

I am calling this one "ApplicationScheme" to clearly mention that it is coming from the application side.
There is also the concept of Custom schemes, which is similar but limited to some standardized schemes. (see also: http://www.w3.org/TR/html5/timers.html#custom-handlers)
Afaik this is currently under development and tracked here: https://bugs.webkit.org/show_bug.cgi?id=73176.

>> Source/WebKit2/WebProcess/WebCoreSupport/qt/PagePointerWrapper.h:37
>> +};
> 
> What is the purpose of this? Maybe the name could reflect this better

The purpose of this is to be able to pass a WebKit::WebPage* to WebFrameNetworkingContext::setOriginatingObject(QObject*) which takes a QObject*.
The setter function i defined in WebFrameNetworkingContext. This class nevertheless already had the QObject* m_originatingObject pointer which was unused in WebKit2 so far, but unfortunately is used in WebKit1.
A possible nicer way to solve this would be, to change the pointer to a void* and cast the pointers in WebKit1. - Shall we go with this approach instead? Like that we would not need the PagePointerWrapper class.

>> Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.cpp:46
>> +QtNetworkAccessManager::QtNetworkAccessManager(QObject * parent)
> 
> style

done.

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

ok

>> Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.h:41
>> +    QtNetworkAccessManager(QObject * parent = 0);
> 
> style

fixed.

>> Source/WebKit2/WebProcess/qt/QtNetworkReply.cpp:41
>> +    : QNetworkReply((QObject*)parent)
> 
> static_cast?

oops... that's a leftover. I don't think that actually needs a cast at all.

>> Source/WebKit2/WebProcess/qt/QtNetworkReply.cpp:84
>> +qint64 QtNetworkReply::readData(char *data, qint64 maxlen)
> 
> style

fixed.

[Kenneth Rohde Christiansen]

> Should this be called NetworkReplyDataQt?

>I'm not sure. Should it? This class is Qt only. Afaik the Qt suffix is usually >for a platform specific implementation of a general webkit class. Or am i wrong >about that?

I meant to say QtNetworkReplyData! :-)

[Kenneth Rohde Christiansen]

> Is there a case where smLength != sm->size()?

>Yes, at least on mac the actual size of the shared memory is rounded to pages. So sm->size() will usually be bigger then smLength.

Comment please :-)

>> +    QQuickNetworkReply(QObject* parent = 0);
>
> When will this have a parent and when will it not?

>This is instantiated from QML... so it should always have a parent.

leave out the = 0 then

schemeDelegates: [
   UrlScheme {
       scheme: "something"
       ... },
    ]

seems weird to me. Yes there is dedundency there but it is inconsistent.

The following is also dedundent but at least it is consistent

urlSchemeDelegates: [
   UrlSchemeDelegate {
       scheme: "something"
       ... },
    ]

>A possible nicer way to solve this would be, to change the pointer to a void* and cast the pointers in WebKit1. - Shall we go with this approach instead? >Like that we would not need the PagePointerWrapper class.

I think that would be ok.

[Kenneth Rohde Christiansen]

>In Qt we would usually call the function schemeDelegates_At(..). But since webkit style doesn't like underscores in function names, this brings us to schemeDelegatesAt again.

If this is a Qt only class that is fine to use _At. The style guide is educational (it is a guideline) and not strictly enforced. Exceptions to the style scripts could also be made, like allow _At in classes containing Qt in the name or similar.

[Kenneth Rohde Christiansen]

> +    memset(sm->data(), 0, sm->size());
> +    memcpy(sm->data(), ucharData, smLength);

>Is there a case where smLength != sm->size()?

Why not just null the length difference? ie (data + smLength, size - smLength) ? or is that slower? just wondering

[Zeno Albisser]

(In reply to comment #4)
> > Should this be called NetworkReplyDataQt?
> 
> >I'm not sure. Should it? This class is Qt only. Afaik the Qt suffix is usually >for a platform specific implementation of a general webkit class. Or am i wrong >about that?
> 
> I meant to say QtNetworkReplyData! :-)

okay.


(In reply to comment #5)
> Comment please :-)

done.

> 
> >> +    QQuickNetworkReply(QObject* parent = 0);
> >
> > When will this have a parent and when will it not?
> 
> >This is instantiated from QML... so it should always have a parent.
> 
> leave out the = 0 then

No that does not work, cause QML actually creates the item without a parent, and then sets the parent afterwards. Sorry my first reply was not really precise.

> urlSchemeDelegates: [
>    UrlSchemeDelegate {
>        scheme: "something"
>        ... },
>     ]
>

Sounds good to me. I'll change that accordingly.
 
> >A possible nicer way to solve this would be, to change the pointer to a void* and cast the pointers in WebKit1. - Shall we go with this approach instead? >Like that we would not need the PagePointerWrapper class.
> 
> I think that would be ok.
I took a look at this again. And what i described above does not work. Even if we change the pointer type in the NetworkingContext the QNetworkRequest that receives the originatingObject from the NetworkingContext actually requires a QObject pointer. So unless we want to change that in Qt, we have to wrap the Page pointer with a QObject. That is what the PagePointerWrapper is doing. But we can of course rename this class if we find a better name.


(In reply to comment #6)
> If this is a Qt only class that is fine to use _At. The style guide is educational (it is a guideline) and not strictly enforced. Exceptions to the style scripts could also be made, like allow _At in classes containing Qt in the name or similar.
Let's do it the same way as it is in qdeclarativewebview.cpp.


(In reply to comment #7)
> > +    memset(sm->data(), 0, sm->size());
> > +    memcpy(sm->data(), ucharData, smLength);
> 
> >Is there a case where smLength != sm->size()?
> 
> Why not just null the length difference? ie (data + smLength, size - smLength) ? or is that slower? just wondering
Actually we do not need to null it at all. It always seems to be a good idea to initialize memory properly. But since we always know the exact length of data that we want to transmit we do not rely on any kind of null termination. I'll simply remove the memset(..).

[Zeno Albisser]

Created attachment 120308 [details]
patch for review. - fixed previously issues as commented.

[Kenneth Rohde Christiansen]

Comment on attachment 120308 [details]
patch for review. - fixed previously issues as commented.

View in context: https://bugs.webkit.org/attachment.cgi?id=120308&action=review

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:570
> +    This signal is emitted for every navigation request. The request object contains url, button and modifiers propertyerties

huh?

> Source/WebKit2/WebProcess/WebCoreSupport/qt/PagePointerWrapper.h:30
> +class PagePointerWrapper : public QObject {

It seems more like a QObjectWrapping to me :-)

Alternatively, can't you just create a QObject and set the page as a property? At least it would avoid this class and be very clear in the code.

[Gustavo Noronha (kov)]

Comment on attachment 120308 [details]
patch for review. - fixed previously issues as commented.

Attachment 120308 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10986475

[Zeno Albisser]

(In reply to comment #10)
> (From update of attachment 120308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=120308&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:570
> > +    This signal is emitted for every navigation request. The request object contains url, button and modifiers propertyerties
> 
> huh?
I blame vi for that! ;-) I'll fix it.
> 
> > Source/WebKit2/WebProcess/WebCoreSupport/qt/PagePointerWrapper.h:30
> > +class PagePointerWrapper : public QObject {
> 
> It seems more like a QObjectWrapping to me :-)
Ahm... no. It clearly derives from QObject (what is the point of the whole thing) and it clearly wraps a WebKit::WebPage pointer.
> 
> Alternatively, can't you just create a QObject and set the page as a property? At least it would avoid this class and be very clear in the code.
That could work, and would indeed be nicer if i can get it to work with not too much ugly casting. I'll give it a shot and post a revised patch afterwards.

[Zeno Albisser]

Created attachment 120445 [details]
patch for review. - replaced PagePointerWrapper with QObject property

[Kenneth Rohde Christiansen]

Comment on attachment 120445 [details]
patch for review. - replaced PagePointerWrapper with QObject property

View in context: https://bugs.webkit.org/attachment.cgi?id=120445&action=review

Looks pretty good

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:35
> +    , m_networkReplyData(new WebKit::QtNetworkReplyData) { }

Why is responsible for deleting this? should it be a smart pointer? comment?

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:130
> +
> +void QQuickNetworkReply::setData(const QString& data)
> +{

can this be called multiple times? so that it might already have a shared memory region associated? guard? comment? assert?

> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply_p.h:43
> +    QQuickNetworkReply(QObject* parent = 0);

If this is supposed to always have a parent, I suggest removing the = 0 and adding an assert(parent) in the ctor

> Source/WebKit2/UIProcess/API/qt/qquicknetworkrequest_p.h:35
> +    QQuickNetworkRequest(QObject* parent = 0)
> +        : QObject(parent) { }

same here

> Source/WebKit2/UIProcess/API/qt/qquickurlschemedelegate.cpp:28
> +{ }

ASSERT(parent) ?

> Source/WebKit2/UIProcess/API/qt/qquickurlschemedelegate_p.h:38
> +    QQuickUrlSchemeDelegate(QObject* parent = 0);

same?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:702
> +QQuickUrlSchemeDelegate* QQuickWebViewExperimental::schemeDelegatesAt(QDeclarativeListProperty<QQuickUrlSchemeDelegate>* property, int index)

Didnt you want to add the _At ?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:714
> +    QQuickWebViewExperimental* webView = qobject_cast<QQuickWebViewExperimental*>(property->object->parent());

why not webViewExperimental here?

> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:750
> +        QQuickUrlSchemeDelegate* urlScheme = qobject_cast<QQuickUrlSchemeDelegate*>(children.at(index));
> +        if (!urlScheme)

I think "delegate" would be a better local name than urlScheme

> Source/WebKit2/UIProcess/qt/QtPageClient.cpp:106
> +{
> +    m_webView->experimental()->invokeApplicationSchemeHandler(requestData);
> +}

you check whether the experimental exists elsewhere but not here, why?

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1448
> +    // We encapsulate the WebPage pointer as a property of a QObject,
> +    // because the networking context only accepts a QObject as originating object.
> +    QObject* pagePointerWrapper = new QObject;
> +    pagePointerWrapper->setProperty("PagePointer", QVariant::fromValue(static_cast<void*>(m_frame->page())));
> +    pagePointerWrapper->setProperty("deleteWithContext", QVariant::fromValue(true));
> +    context->setOriginatingObject(pagePointerWrapper);

Very clear!

> Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.cpp:56
> +    if (pagePtrWrapper) {
> +        QVariant pagePtr = pagePtrWrapper->property("PagePointer");
> +        if (pagePtr.isValid() && pagePtr.canConvert<void*>()) {
> +            WebPage* webPage = static_cast<WebPage*>(pagePtr.value<void*>());

ASSERT instead? or as well?

> Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.h:41
> +    QtNetworkAccessManager(QObject* parent = 0);

=0 ?

> Source/WebKit2/WebProcess/qt/QtNetworkReply.cpp:109
> +bool QtNetworkReply::canReadLine () const { return true; }
> +
> +
> +void QtNetworkReply::finalize()

double newline

> Source/WebKit2/WebProcess/qt/QtNetworkReply.h:53
> +    // reimplemented from QNetworkReply / QIODevice

use proper sensentes (Starts with capital, ...)

[Zeno Albisser]

Comment on attachment 120445 [details]
patch for review. - replaced PagePointerWrapper with QObject property

View in context: https://bugs.webkit.org/attachment.cgi?id=120445&action=review

i will upload a revised patch asap.

>> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:35
>> +    , m_networkReplyData(new WebKit::QtNetworkReplyData) { }
> 
> Why is responsible for deleting this? should it be a smart pointer? comment?

I will solve that with a RefPtr.

>> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply.cpp:130
>> +{
> 
> can this be called multiple times? so that it might already have a shared memory region associated? guard? comment? assert?

This can be called multiple times. The SharedMemory dies with the last SharedMemory reference or SharedMemory handle. (last file descriptor to the shared memory on unix... but this is platform dependent.)
 - adding a comment.

>> Source/WebKit2/UIProcess/API/qt/qquicknetworkreply_p.h:43
>> +    QQuickNetworkReply(QObject* parent = 0);
> 
> If this is supposed to always have a parent, I suggest removing the = 0 and adding an assert(parent) in the ctor

You're right, i'll change that.

>> Source/WebKit2/UIProcess/API/qt/qquicknetworkrequest_p.h:35
>> +        : QObject(parent) { }
> 
> same here

done.

>> Source/WebKit2/UIProcess/API/qt/qquickurlschemedelegate.cpp:28
>> +{ }
> 
> ASSERT(parent) ?

Nope. - We can't do that here. The delegate is created from QML and later on reparented by the declarative engine.

>> Source/WebKit2/UIProcess/API/qt/qquickurlschemedelegate_p.h:38
>> +    QQuickUrlSchemeDelegate(QObject* parent = 0);
> 
> same?

No. - necessary.

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:702
>> +QQuickUrlSchemeDelegate* QQuickWebViewExperimental::schemeDelegatesAt(QDeclarativeListProperty<QQuickUrlSchemeDelegate>* property, int index)
> 
> Didnt you want to add the _At ?

No, i wanted to do it the same way as it is in WebKit/qt/declarative/qdeclarativewebview.cpp. But since you insist... yes i will change it.

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:714
>> +    QQuickWebViewExperimental* webView = qobject_cast<QQuickWebViewExperimental*>(property->object->parent());
> 
> why not webViewExperimental here?

done.

>> Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:750
>> +        if (!urlScheme)
> 
> I think "delegate" would be a better local name than urlScheme

done.

>> Source/WebKit2/UIProcess/qt/QtPageClient.cpp:106
>> +}
> 
> you check whether the experimental exists elsewhere but not here, why?

I was only checking after casting. And QQuickWebView always creates a QQuickWebViewExperimental in it's initializer list. But i just realized that m_webView is 0 in case that QtPageClient was not initialized. Even though this should not happen... some extra checking cannot be wrong i think. I'll add that.

>> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1448
>> +    context->setOriginatingObject(pagePointerWrapper);
> 
> Very clear!

yes. - or was that ironic?

>> Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.cpp:56
>> +            WebPage* webPage = static_cast<WebPage*>(pagePtr.value<void*>());
> 
> ASSERT instead? or as well?

done.

>> Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.h:41
>> +    QtNetworkAccessManager(QObject* parent = 0);
> 
> =0 ?

done.

>> Source/WebKit2/WebProcess/qt/QtNetworkReply.cpp:109
>> +void QtNetworkReply::finalize()
> 
> double newline

removed.

>> Source/WebKit2/WebProcess/qt/QtNetworkReply.h:53
>> +    // reimplemented from QNetworkReply / QIODevice
> 
> use proper sensentes (Starts with capital, ...)

removed.

[Zeno Albisser]

Created attachment 121258 [details]
patch for review. - using RefPtr for networkRequestData and networkReplyData.

[WebKit Review Bot]

Attachment 121258 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit/qt/ChangeLog',..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:217:  schemeDelegates_At is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:218:  schemeDelegates_Append is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:219:  schemeDelegates_Count is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:220:  schemeDelegates_Clear is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:703:  QQuickWebViewExperimental::schemeDelegates_At is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:711:  QQuickWebViewExperimental::schemeDelegates_Append is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:722:  QQuickWebViewExperimental::schemeDelegates_Count is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:727:  QQuickWebViewExperimental::schemeDelegates_Clear is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 8 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 121258 [details]
patch for review. - using RefPtr for networkRequestData and networkReplyData.

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

[Zeno Albisser]

Comment on attachment 121258 [details]
patch for review. - using RefPtr for networkRequestData and networkReplyData.

View in context: https://bugs.webkit.org/attachment.cgi?id=121258&action=review

thanks for the reviews. :-)
i'll post an update again.

>> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:217
>> +    static QQuickUrlSchemeDelegate* schemeDelegates_At(QDeclarativeListProperty<QQuickUrlSchemeDelegate>*, int index);
> 
> schemeDelegates_At is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]

do we need to do anything about these???

>> 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 ...

good point. :)

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

done.

>> Source/WebKit2/WebProcess/qt/QtNetworkAccessManager.cpp:53
>> +    if (pagePtrWrapper) {
> 
> I would merge those two lines

replaced with previous change.

>> 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

style fixed.
i don't think you can use nullptr here. nullptr is of type std::nullptr_t, therefore you would have to cast it to QIODevice* afterwards, which would be pretty ugly.
I think you meant to use it for RefPtrs ... cause these actually have a ctor taking an std::nullptr_t argument.

[Zeno Albisser]

Created attachment 121280 [details]
patch for review. - extracted obtainOriginatingWebPage code.

[WebKit Review Bot]

Attachment 121280 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit/qt/ChangeLog',..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:219:  schemeDelegates_At is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:220:  schemeDelegates_Append is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:221:  schemeDelegates_Count is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:222:  schemeDelegates_Clear is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:703:  QQuickWebViewExperimental::schemeDelegates_At is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:711:  QQuickWebViewExperimental::schemeDelegates_Append is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:722:  QQuickWebViewExperimental::schemeDelegates_Count is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:727:  QQuickWebViewExperimental::schemeDelegates_Clear is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 8 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 121280 [details]
patch for review. - extracted obtainOriginatingWebPage code.

Nice, good work

[Kenneth Rohde Christiansen]

> > schemeDelegates_At is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
> 
> do we need to do anything about these???

Ignore :-) the style is educational only.

> style fixed.
> i don't think you can use nullptr here. nullptr is of type std::nullptr_t, therefore you would have to cast it to QIODevice* afterwards, which would be pretty ugly.
> I think you meant to use it for RefPtrs ... cause these actually have a ctor taking an std::nullptr_t argument.

It was more like a general comment to keep in mind in the future

[Zeno Albisser]

Created attachment 121301 [details]
rebased patch after r104168.

I had to change the originatingObject mechanism, because after r104168 the WebFrameNetworkingContext always creates a QObject for the originating object on its own.

[WebKit Review Bot]

Attachment 121301 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit/qt/ChangeLog',..." exit_code: 1

Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:219:  schemeDelegates_At is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:220:  schemeDelegates_Append is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:221:  schemeDelegates_Count is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:222:  schemeDelegates_Clear is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:703:  QQuickWebViewExperimental::schemeDelegates_At is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:711:  QQuickWebViewExperimental::schemeDelegates_Append is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:722:  QQuickWebViewExperimental::schemeDelegates_Count is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp:727:  QQuickWebViewExperimental::schemeDelegates_Clear is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 8 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zeno Albisser]

Comment on attachment 121301 [details]
rebased patch after r104168.

View in context: https://bugs.webkit.org/attachment.cgi?id=121301&action=review

Just highlighting changes after r104168, to make the life of our dear reviewers more easy. :)

> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1443
> +    // We encapsulate the WebPage pointer as a property of the originating QObject.

We are now using the object that is created implicit by WebFrameNetworkingContext, instead of creating one on our own.

> Source/WebKit2/WebProcess/WebCoreSupport/qt/WebFrameNetworkingContext.h:36
> +    virtual QObject* originatingObject() const;

We want to access that from WebFrameLoaderClient::createNetworkingContext().

[Zeno Albisser]

Comment on attachment 121301 [details]
rebased patch after r104168.

Clearing flags on attachment: 121301

Committed r104193: <http://trac.webkit.org/changeset/104193>

[Zeno Albisser]

All reviewed patches have been landed.  Closing bug.