[Webkit-unassigned] [Bug 73215] [Qt][WK2] Split QWebPermissionRequest into QWebSecurityOrigin

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73215


Kenneth Rohde Christiansen <kenneth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #123489|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #34 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2012-01-22 15:13:20 PST ---
(From update of attachment 123489)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.