[webkit-reviews] review denied: [Bug 73215] [Qt][WK2] Split QWebPermissionRequest into QWebSecurityOrigin : [Attachment 123489] Previous fixes plus: header file, else clause, QLatin1String

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Adenilson Cavalcanti
Silva <savagobr at yahoo.com>'s request for review:
Bug 73215: [Qt][WK2] Split QWebPermissionRequest into QWebSecurityOrigin
https://bugs.webkit.org/show_bug.cgi?id=73215

Attachment 123489: Previous fixes plus: header file, else clause, QLatin1String
https://bugs.webkit.org/attachment.cgi?id=123489&action=review

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


Some first look! You need to improve the changelogs to better explain what you
are doing and how

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:22
> +#include "config.h"
> +
> +#include "qtwebsecurityorigin_p.h"

should be grouped together

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:35
> +using namespace WebCore;
> +
> +
> +bool QtWebSecurityOrigin::containsScheme(const QString& scheme)

only one newline please

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:43
> +    return (scheme == QLatin1String("qrc")) ? true : (scheme ==
QLatin1String("file"));

That is a pretty weird way of writing that.

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:50
> +{
> +
> +    m_url.setScheme(url.scheme());

unneeded newline

Where is a comment/changelog explaining why it makes sense to use a QUrl
internally?

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:52
> +    if (url.scheme() == QLatin1String("data"))
> +	   return;

a comment would be good here, like "data: urls, inherit the locality of their
owner, thus..."

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:94
> +
> +void QtWebSecurityOrigin::addLocalScheme(const QString& scheme)
> +{

what happens if I add http? https?

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:103
> +    // You cannot remove file:

can I remove qrc?

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:123
> +bool QtWebSecurityOrigin::canAccess(const QUrl& anotherUrl,
OriginPolicyFlags flags)

why anotherUrl and not just url?

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:135
> +    if (scheme() == QLatin1String("data"))
> +	   return false;
> +
> +    if (containsScheme(m_url.scheme())) {
> +	   if (containsScheme(anotherUrl.scheme()) && (flags &
LocalContentHasSystemLocalAccess))
> +	       return true;
> +
> +	   return m_url.path() == QFileInfo(m_url.path()).canonicalFilePath();
> +    }
> +
> +    return m_url.scheme() == anotherUrl.scheme() && m_url.host() ==
anotherUrl.host() && m_url.port() == anotherUrl.port();

This matches the internal webcore implementation? Can you add a comment about
it? or could that code be reused?

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:143
> +{
> +    // FIXME: Might need some versioning.
> +    out << (origin.url().toEncoded().data());
> +    return out;
> +}

Let's add a version tag then... look what Qt uses for other classes

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:154
> +void QtWebSecurityOrigin::setUrl(const QUrl& url)
> +{

What is this needed for?

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin_p.h:40
> +public:
> +    QtWebSecurityOrigin(const QUrl& url, QObject* parent = 0);
> +    virtual ~QtWebSecurityOrigin();

how is the owner ship for this? Does the parent make sense for this class?

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin_p.h:66
> +private:
> +    static bool containsScheme(const QString&);
> +    bool isLocalScheme(const QString&);
> +    QUrl m_url;
> +};

Would it make sense to make this class an implicitly shared class? Will it be
copied around?

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest.cpp:36
>	   , request(permissionRequest)
> +	   , securityInfo(QUrl(), 0)
>	   , allow(false)

look, parent = 0... does it make sense? why is the QUrl not a default ctor
value?

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest.cpp:39
>      {
> +
> +	   QUrl data;

stall newline

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest.cpp:49
> +	  
data.setPort(static_cast<int>(WKSecurityOriginGetPort(origin.get())));
> +	   securityInfo.setUrl(data);
> +
>      }

stall newline

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_origin.qml:16
> +    experimental.onPermissionRequested: {
> +	   if (permission.securityOrigin.port != webView.port) {

Maybe it should just be exported as "origin" ?

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_origin.qml:17
> +	       console.log("Port is wrong")

could be said better? :-) why it is wrong?

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_origin.qml:31
> +	   // Delayed windowShown to workaround problems with Qt5 in debug
mode.
> +	   when: false

bug url?