[webkit-reviews] review denied: [Bug 74931] [Qt][WK2] Implement custom URL schemes defined in QML. : [Attachment 120034] patch for feedback.

[bugzilla-daemon at webkit.org]

Zeno Albisser <zeno at webkit.org> has denied 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 120034: patch for feedback.
https://bugs.webkit.org/attachment.cgi?id=120034&action=review

------- Additional Comments from Zeno Albisser <zeno at webkit.org>
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<QQuickUrl
Scheme>* 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<QQuick
UrlScheme>* 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.