Bug 73215 – [Qt][WK2] Split QWebPermissionRequest into QWebSecurityOrigin

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Adenilson Cavalcanti Silva]

Have a second class to handle security origin information, based on this: http://pastebin.ubuntu.com/748139/

[Adenilson Cavalcanti Silva]

Created attachment 122049 [details]
Imported code, implemented missing methods/operators, integrated with QWebPermissionRequest

I still don't known if a scheme is local or not. Ideas?

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest_p.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/qt/declarative/plugin.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:151:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 3 in 7 files


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

[Kenneth Rohde Christiansen]

(In reply to comment #1)
> Created an attachment (id=122049) [details]
> Imported code, implemented missing methods/operators, integrated with QWebPermissionRequest
> 
> I still don't known if a scheme is local or not. Ideas?

file: is always local. data: has the same locality as its parent.

ie if I use data: in a file:// url, then it is local as well as it is embedded.

[Kenneth Rohde Christiansen]

Comment on attachment 122049 [details]
Imported code, implemented missing methods/operators, integrated with QWebPermissionRequest

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

No qml example?

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:37
> +// Look at webkit1 on how to do this properly.
> +// These needs to be in sync at web process start up and send over when changed.
> +static QStringList s_localSchemes;

This seems to need fixing

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:45
> +        s_localSchemes.clear();
> +        const URLSchemesMap& map = SchemeRegistry::localSchemes();

If SchemeRegistry::localSchemes already exists any reason for s_localSchemes?

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:48
> +        URLSchemesMap::const_iterator end = map.end();
> +        for (URLSchemesMap::const_iterator i = map.begin(); i != end; ++i) {
> +            const QString scheme = *i;

we use 'it' for real iterators, not i

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:56
> +    QtWebSecurityOriginPrivate()
> +    {
> +        localSchemes();
> +    }

Would need a comment

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:63
> +
> +    QUrl url;
> +};

why is this public? why not a real property?

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:77
> +    // FIXME: how to say if a scheme is local or not?
> +    Q_UNUSED(scheme)
> +    return true;
> +}
> +
> +QtWebSecurityOrigin::QtWebSecurityOrigin(const QUrl& url, QObject *parent)
> +    : QObject(parent)
> +    , d(new QtWebSecurityOriginPrivate)
> +{
> +    if (s_localSchemes.empty())
> +        s_localSchemes << QLatin1String("file") << QLatin1String("qrc");

you have local schemes here... I dont understand the commetn

coding style issue (* placement)

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

Some comment needs to explain why the url is enough

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

* placement

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest.cpp:35
> +        , sameOrigin(QUrl(), 0)

sameOrigin? weird name

[Rafael Brandao]

Comment on attachment 122049 [details]
Imported code, implemented missing methods/operators, integrated with QWebPermissionRequest

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

Do you have any use case in mind for this API? Maybe adding a test to show how it could be used would be very nice. :-)

> Source/WebKit2/ChangeLog:16
> +

I fail to see how this change log is related to the rest of the patch. You've added QtWebSecurityOrigin, but did not mention it here... neither the new files are listed there.

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:65
> +bool QtWebSecurityOrigin::isLocalScheme(const QString &scheme)

Style (const QString&)

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:67
> +    // FIXME: how to say if a scheme is local or not?

Maybe you could use SchemeRegistry::shouldTreatURLSchemeAsLocal, not sure if this alone is enough.

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:77
> +        s_localSchemes << QLatin1String("file") << QLatin1String("qrc");

I believe you don't need to do this... we already have code inside WebCore to deal with local schemes.

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin_p.h:32
> +class QWEBKIT_EXPORT QtWebSecurityOrigin : public QObject {

I believe the naming is incorrect here, the others start with Q prefix instead of Qt, but I see that we already have QWebSecurityOrigin. Maybe someone else has a suggestion how to fix this, I don't know.

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin_p.h:67
> +    bool isLocalScheme(const QString &);

Style (use const QString&, no whitespace)

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest_p.h:53
> +    QtWebSecurityOrigin &securityOrigin();

Style (use QWebSecurityOrigin& instead)

[Kenneth Rohde Christiansen]

> Maybe you could use SchemeRegistry::shouldTreatURLSchemeAsLocal, not sure if this alone is enough.

This might need some fixes for qrc: ?

> 
> > Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:77
> > +        s_localSchemes << QLatin1String("file") << QLatin1String("qrc");
> 
> I believe you don't need to do this... we already have code inside WebCore to deal with local schemes.

Same here

> 
> > Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin_p.h:32
> > +class QWEBKIT_EXPORT QtWebSecurityOrigin : public QObject {
> 
> I believe the naming is incorrect here, the others start with Q prefix instead of Qt, but I see that we already have QWebSecurityOrigin. Maybe someone else has a suggestion how to fix this, I don't know.

Basically everything publically exposed via C++ starts with Q, everything else starts with Qt. This is unfortunately not consistent in our code.

[Adenilson Cavalcanti Silva]

Created attachment 122298 [details]
Still missing the test

Removed the static QStringList object, better usage of HashSet & SchemeRegistry methods.

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest_p.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:148:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 8 files


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

[Jesus Sanchez-Palencia]

Comment on attachment 122298 [details]
Still missing the test

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

> Source/WebKit/qt/declarative/plugin.cpp:69
> +        qmlRegisterUncreatableType<QtWebSecurityOrigin>(uri, 3, 0, "SecurityOrigin", QObject::tr("Cannot create separate instance of SecurityOrigin"));

Hey, just a nitpicking comment from my part. :)

I believe that if this is gonna sit in API/qt/ then you should follow the new naming convention and it should be called QWebSecurityOrigin. The same for the private class, cpp and header files.

[Jesus Sanchez-Palencia]

(In reply to comment #9)
> I believe that if this is gonna sit in API/qt/ then you should follow the new naming convention and it should be called QWebSecurityOrigin. The same for the private class, cpp and header files.

Oh, just realized we have a QWebSecurityOrigin already... =/

[Jesus Sanchez-Palencia]

Comment on attachment 122298 [details]
Still missing the test

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

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin_p.h:38
> +    Q_PROPERTY(QString scheme READ scheme)
> +    Q_PROPERTY(QString host READ host)
> +    Q_PROPERTY(int port READ port)
> +    Q_PROPERTY(QString path READ path)
> +    Q_PROPERTY(QUrl url READ url)

Shouldn't these properties have CONSTANT (or NOTIFY, in case it makes sense to have signals for any of them)? Just noticed they were already missing on QWebPermissionRequest.

[Adenilson Cavalcanti Silva]

Good point about the CONSTANT (I will add it).

[Kenneth Rohde Christiansen]

(In reply to comment #9)
> (From update of attachment 122298 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122298&action=review
> 
> > Source/WebKit/qt/declarative/plugin.cpp:69
> > +        qmlRegisterUncreatableType<QtWebSecurityOrigin>(uri, 3, 0, "SecurityOrigin", QObject::tr("Cannot create separate instance of SecurityOrigin"));
> 
> Hey, just a nitpicking comment from my part. :)
> 
> I believe that if this is gonna sit in API/qt/ then you should follow the new naming convention and it should be called QWebSecurityOrigin. The same for the private class, cpp and header files.

As those classes are not exported in C++ sense, but only to QML, I think that Qt* actually makes more sense than using Q[A-Z]*

[Adenilson Cavalcanti Silva]

Created attachment 123123 [details]
QML test, linker error fixed, access to security property in QML, constant properties

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest_p.h:25:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:148:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 11 files


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

[Adenilson Cavalcanti Silva]

Created attachment 123174 [details]
Changelog typo/date plus previous fixes.

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:148:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 11 files


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

[Simon Hausmann]

Comment on attachment 123174 [details]
Changelog typo/date plus previous fixes.

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

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin_p.h:57
> +    QtWebSecurityOrigin& operator=(const QtWebSecurityOrigin&);
> +    QtWebSecurityOrigin& operator=(const QUrl&);
> +    bool operator!=(const QtWebSecurityOrigin&);
> +    bool operator==(const QtWebSecurityOrigin&);

I don't think these operators are a good idea for a QObject sub-class. We generally avoid them with polymorphic types. I'd prefer explicit functions to change the state, i.e. a setUrl.

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin_p.h:69
> +    QSharedDataPointer<QtWebSecurityOriginPrivate> d;

A shared data pointer for a QObject that cannot be copied and where the shared data pointer contains a QUrl, which itself has a shared private? I think this can be simplified to a simple QUrl member in here ;-)

[Adenilson Cavalcanti Silva]

Simon

Thanks for the comments. I have one question: do you feel is better to remove all the operators? Or just the attribution one (=)?

[Adenilson Cavalcanti Silva]

Created attachment 123467 [details]
Patch

[WebKit Review Bot]

Attachment 123467 [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/qtwebsecurityorigin.cpp:130:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 11 files


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

[Adenilson Cavalcanti Silva]

Created attachment 123468 [details]
No more operators or shared pointer/pimple

[WebKit Review Bot]

Attachment 123468 [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/qtwebsecurityorigin.cpp:130:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 11 files


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

[Adenilson Cavalcanti Silva]

Removed the pimple/shared pointer plus all the operators.

[Adenilson Cavalcanti Silva]

Created attachment 123469 [details]
Including the qml test.

Including the qml utest file.

[WebKit Review Bot]

Attachment 123469 [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/qtwebsecurityorigin.cpp:130:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 12 files


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

[Adenilson Cavalcanti Silva]

Created attachment 123470 [details]
ChangeLog was missing the new qml test file

[WebKit Review Bot]

Attachment 123470 [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/qtwebsecurityorigin.cpp:130:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 12 files


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

[Rafael Brandao]

Comment on attachment 123470 [details]
ChangeLog was missing the new qml test file

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

I couldn't look into the whole patch, but just giving an informal review anyway... Besides the test is there, they don't really cover what should be tested there.

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:133
> +            return m_url.path() == QFileInfo(m_url.path()).canonicalFilePath();

You could eliminate the "else" because getting into the condition of the previous if would have early returned anyway. :)

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_origin.qml:41
> +            compare(webView.success, true)

So how exactly are we testing the case when webView.success is false (you set it in this test but you don't use it)?

[Alexander Færøy]

Comment on attachment 123470 [details]
ChangeLog was missing the new qml test file

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

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:24
> +#include <KURL.h>

Is this really needed?

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

Couldn't you do something like: return scheme == QLatin1String("qrc") || scheme == QLatin1String("file"); ?

[Adenilson Cavalcanti Silva]

Rafael

Thanks for the comments. I will answer inline.

(In reply to comment #29)
> (From update of attachment 123470 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123470&action=review
> 
> I couldn't look into the whole patch, but just giving an informal review anyway... Besides the test is there, they don't really cover what should be tested there.
> 

What is not covered? Do you mean all the remaining properties? Or something else?

> > Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:133
> > +            return m_url.path() == QFileInfo(m_url.path()).canonicalFilePath();
> 
> You could eliminate the "else" because getting into the condition of the previous if would have early returned anyway. :)

Indeed.

> 
> > Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_origin.qml:41
> > +            compare(webView.success, true)
> 
> So how exactly are we testing the case when webView.success is false (you set it in this test but you don't use it)?

The idea is that the test will fail if the reported url port/scheme is different from the expected value (then webView.success is set to false inside the if clause in onPermissionRequested).

The default behavior for the test is to succeed, this is why compare checks for true. If different, the test fails, since it will eval to: compare(false, true).

[Adenilson Cavalcanti Silva]

Alexander

Thanks for the comments, answers inline.

> 
> > Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:24
> > +#include <KURL.h>
> 
> Is this really needed?

Good point! I will remove it.

> 
> > Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:44
> > +    return (scheme == QString::fromUtf8("qrc")) ? true : (scheme == QString::fromUtf8("file"));
> 
> Couldn't you do something like: return scheme == QLatin1String("qrc") || scheme == QLatin1String("file"); ?

Checking in Qt Assistant it seems that using QLatin1String should be faster than creating a new QString object (which is what the code was doing).

I will change it too.

[Adenilson Cavalcanti Silva]

Created attachment 123489 [details]
Previous fixes plus: header file, else clause, QLatin1String

[Kenneth Rohde Christiansen]

Comment on attachment 123489 [details]
Previous fixes plus: header file, else clause, QLatin1String

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?

[Simon Hausmann]

Comment on attachment 123489 [details]
Previous fixes plus: header file, else clause, QLatin1String

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

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

And shouldn't this function use the WebCore Scheme registry instead of hard-coding qrc and file?

>> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:52
>> +        return;
> 
> a comment would be good here, like "data: urls, inherit the locality of their owner, thus..."

Isn't there a central function in WebCore to call for that instead of hard-coding the scheme here?

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:99
> +    if (containsScheme(scheme))
> +        return;
> +
> +    SchemeRegistry::registerURLSchemeAsLocal(scheme);
> +}

Why the containsScheme check? AFAICS the scheme registry uses a hashset, so the "contains" check appears redundant to me.

Why is this method exported, who is the caller?

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:107
> +    if (scheme == QLatin1String("file"))
> +        return;
> +
> +    SchemeRegistry::removeURLSchemeRegisteredAsLocal(scheme);

This is again a redundant check, because the implementation in SchemeRegistry does the same checking.

[Simon Hausmann]

I think the general "theme" of this feature should be about exporting existing functionality. I don't like that we have all these "qrc:", "data:" and "file:" checks in the code, because this is security sensitive code that I don't think we should duplicate but instead share with WebCore as much as possible. The goal should be make existing functionality available, not to duplicate it before the export.

[Adenilson Cavalcanti Silva]

Created attachment 123733 [details]
Addressing reviewers comments (still missing the versioning in stream operator)

[Kenneth Rohde Christiansen]

Comment on attachment 123733 [details]
Addressing reviewers comments (still missing the versioning in stream operator)

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

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:109
> +bool QtWebSecurityOrigin::canAccess(const QUrl& url, OriginPolicyFlags flags)
> +{
> +    if (scheme() == QLatin1String("data"))
> +        return false;
> +
> +    if (containsScheme(m_url.scheme())) {
> +        if (containsScheme(url.scheme()) && (flags & LocalContentHasSystemLocalAccess))
> +            return true;
> +
> +        return m_url.path() == QFileInfo(m_url.path()).canonicalFilePath();
> +    }
> +
> +    return m_url.scheme() == url.scheme() && m_url.host() == url.host() && m_url.port() == url.port();
> +}

I think we should leave this out for now, or at least make it use webcore internals.

[Simon Hausmann]

Comment on attachment 123733 [details]
Addressing reviewers comments (still missing the versioning in stream operator)

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

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:33
> +bool QtWebSecurityOrigin::containsScheme(const QString& scheme)

I think this function should be called isLocalScheme or something. The current name suggests that it checks if the given scheme is "contained" in the _instance_ of the security origin, which doesn't make much sense to me and it's also not what the function does :)

[Adenilson Cavalcanti Silva]

Created attachment 123943 [details]
Removed canAccess() method, added versioning in stream object

[Adenilson Cavalcanti Silva]

And containsScheme() now is called isLocalScheme().

[Kenneth Rohde Christiansen]

Comment on attachment 123943 [details]
Removed canAccess() method, added versioning in stream object

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

Getting there, maybe Simon should have a look more.

I think the changelogs could be a bit more elaborate... ie. explain what it really adds, and maybe how it is implemented (ie why use a QUrl inside)

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:101
> +QDataStream& operator<<(QDataStream& out, const QtWebSecurityOrigin &origin)
> +{
> +    out.setVersion(QDataStream::Qt_4_0);
> +    out << (origin.url().toEncoded().data());
> +    return out;
> +}

hmm, is this correct simon?

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin_p.h:58
> +
> +    enum OriginPolicyFlags {
> +        ContentHasSameOriginAccess = 0x0,
> +        LocalContentHasRemoteAccess = 0x1,
> +        LocalContentHasSystemLocalAccess = 0x2
> +    };

This is dead code for now, remove.

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin_p.h:61
> +private:
> +    bool isLocalScheme(const QString&);

Is it ok to not have this in a private class?

[Adenilson Cavalcanti Silva]

Created attachment 124446 [details]
Removed dead code, more descriptive changelog

[Adenilson Cavalcanti Silva]

Created attachment 124447 [details]
Removed methods from changelog

[Kenneth Rohde Christiansen]

Comment on attachment 124447 [details]
Removed methods from changelog

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

> ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Introducing a new class to expose security origin information (port/scheme/etc), useful for inspecting the origin of permission requests.
> +
> +        This class uses methods from SchemeRegistry, thus allowing to add/remove/list local schemes too.

could you please wrap these lines in 80-100 chars

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin.cpp:107
> +
> +// Used to set security information in a permission request event (e.g.
> +// geolocation permission)
> +void QtWebSecurityOrigin::setUrl(const QUrl& url)

Seems like info for the header instead

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin_p.h:36
> +    Q_PROPERTY(QUrl url READ url CONSTANT)

Huh? Why is that exposed? What is the url for the origin? it is pretty ambigious.

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest.cpp:45
> +        QUrl data;
> +        WKRetainPtr<WKStringRef> url = adoptWK(WKSecurityOriginCopyProtocol(origin.get()));
> +        data.setScheme(WKStringCopyQString(url.get()));
> +
> +        WKRetainPtr<WKStringRef> host = adoptWK(WKSecurityOriginCopyHost(origin.get()));
> +        data.setHost(WKStringCopyQString(host.get()));
> +
> +        data.setPort(static_cast<int>(WKSecurityOriginGetPort(origin.get())));
> +        securityInfo.setUrl(data);

That you are storing it internally as an url is probably fine, but it shouldn't be exposed that much in the api.

I really dislike the setUrl API (you are making an implementation detail shine through the api, bad).

Why not have a securityOrigin = QtWebSecurityOrigin::create(scheme, host, port); or make it the ctor.

[Adenilson Cavalcanti Silva]

Kenneth

Thanks for the review, I will comment inline.

> 
> could you please wrap these lines in 80-100 chars
> 

Cool, I will do that.

> 
> Seems like info for the header instead
> 

Easy to fix, np.


> Huh? Why is that exposed? What is the url for the origin? it is pretty ambigious.
> 

You suggest to (exclusive):
a) Not have this as a property

or

b) Not have any way to access the QUrl?


> 
> I really dislike the setUrl API (you are making an implementation detail shine through the api, bad).
> 
> Why not have a securityOrigin = QtWebSecurityOrigin::create(scheme, host, port); or make it the ctor.

At time the object will be constructed, I lack the information (port/scheme/etc) to fill in the object, at least in the case of QWebPermissionRequest.

The 'setUrl' was suggested by Simon (but I can change if you wish) and allows to fill in the information after the object is constructed. Maybe you will recall that a previous version of this patch was passing an empty QUrl() object in the constructor (and you suggested to remove it)?

Going with a factory like approach (i.e. a static ::create() method) would imply that the user of this class (e.g. QWebPermissionRequest) is going to always use a pointer to handle it? This adds some complexity in its usage, since it will need to have a shared pointer of some sort to ensure that the pointer will be freed (instead of an object living in the stack) or even worst to explicitly delete it.

The problems that I'm trying to solve are:
a) Allow to fill in the security origin information into the object after the object was constructed

b) Allow the user of this new class to both create/use the object in both the stack and the heap.


Best regards


Adenilson

[Kenneth Rohde Christiansen]

> 
> > Huh? Why is that exposed? What is the url for the origin? it is pretty ambigious.
> > 
> 
> You suggest to (exclusive):
> a) Not have this as a property
> 
> or
> 
> b) Not have any way to access the QUrl?

a) is a must, b) would be nice.

> At time the object will be constructed, I lack the information (port/scheme/etc) to fill in the object, at least in the case of QWebPermissionRequest.
> 
> The 'setUrl' was suggested by Simon (but I can change if you wish) and allows to fill in the information after the object is constructed. Maybe you will recall that a previous version of this patch was passing an empty QUrl() object in the constructor (and you suggested to remove it)?

I think that an inline setScheme, setPort etc would be better.

[Adenilson Cavalcanti Silva]

Created attachment 124941 [details]
Adding setters for security info (instead of a setUrl()), removed the stream operators.

[Adenilson Cavalcanti Silva]

Created attachment 124943 [details]
Previous fixes, changelog

[Simon Hausmann]

Comment on attachment 124943 [details]
Previous fixes, changelog

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

This is much much better :-). A few comments/questions.

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin_p.h:55
> +    static void addLocalScheme(const QString& scheme);
> +    static void removeLocalScheme(const QString& scheme);
> +    static QStringList localSchemes();

I don't see any code in this patch calling these newly added functions, so it looks like adding dead code. Should they perhaps be part of another patch that adds them when they're needed? (as-is they are not even callable in QML)

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest_p.h:37
> +    Q_PROPERTY(QtWebSecurityOrigin* origin READ securityOrigin)

Any particular reason to call the property "origin" but the getter function (and class) is called _security_Origin?

> Source/qtwebkit-export.map:161
> +        *QtWebSecurityOrigin;
> +        non-virtual?thunk?to?QtWebSecurityOrigin*;
> +        QtWebSecurityOrigin::*;

Thankfully this file is gone and this hunk won't be needed anymore.

[Kenneth Rohde Christiansen]

> I don't see any code in this patch calling these newly added functions, so it looks like adding dead code. Should they perhaps be part of another patch that adds them when they're needed? (as-is they are not even callable in QML)

Yeah, adding this with a test (lets say using geolocation from a local qrc file?) would rock.


> > Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest_p.h:37
> > +    Q_PROPERTY(QtWebSecurityOrigin* origin READ securityOrigin)
> 
> Any particular reason to call the property "origin" but the getter function (and class) is called _security_Origin?

origin is a better name... it is the origin of the premission request and origin also fits better with the CORS spec (cross-origin...)

[Simon Hausmann]

Comment on attachment 124943 [details]
Previous fixes, changelog

Taking this out of the review queue. There are some open questions in earlier comments and the patch needs a rebase due to the export map file removal.

[Adenilson Cavalcanti Silva]

Created attachment 126979 [details]
Adapt to recent trunk changes (export map/qml imports), removed the static methods

Less is more.
:-)

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets
Using index info to reconstruct a base tree...
<stdin>:1578: trailing whitespace.
        
<stdin>:1647: trailing whitespace.
    
<stdin>:1657: trailing whitespace.
    
<stdin>:1672: trailing whitespace.
        return 0;        
<stdin>:1674: trailing whitespace.
    
warning: squelched 7 whitespace errors
warning: 12 lines add whitespace errors.
Falling back to patching base and 3-way merge...
warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebKit2/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog
Auto-merging Tools/ChangeLog
CONFLICT (content): Merge conflict in Tools/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


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

[Kenneth Rohde Christiansen]

Comment on attachment 126979 [details]
Adapt to recent trunk changes (export map/qml imports), removed the static methods

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

> Source/WebKit2/UIProcess/API/qt/qtwebsecurityorigin_p.h:51
> +    // Used to set security information in a permission request event (e.g.
> +    // geolocation permission)
> +    void setScheme(const QString& scheme) { m_url.setScheme(scheme); }
> +    void setHost(const QString& host) { m_url.setHost(host); }
> +    void setPath(const QString& path) { m_url.setPath(path); }
> +    void setPort(int port) { m_url.setPort(port); }

Maybe we should make those classes friends instead?

[Adenilson Cavalcanti Silva]

Kenneth

Thanks for the comment.

I thought about friend-ing both classes (since this would help to remove some boilerplate code i.e. the setters/getters), but this poses a question: what happens if other class got to use QtWebSecurityOrigin? 

This would force any user to also become a friend (thus breaking data encapsulation and inspiring layer violations). 

That would not be *that* different about how the current trunk code is (i.e. QWebPermissionRequest has basically the same code that was moved to QtWebSecurityOrigin).


Adenilson

[WebKit Review Bot]

Comment on attachment 126979 [details]
Adapt to recent trunk changes (export map/qml imports), removed the static methods

Rejecting attachment 126979 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
s-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
46>At revision 10635.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...
Updating OpenSource
fatal: The remote end hung up unexpectedly
Died at Tools/Scripts/update-webkit line 162.

Full output: http://queues.webkit.org/results/11515808

[Adenilson Cavalcanti Silva]

Created attachment 127295 [details]
Just rebased the patch, style bot probably is broken

[WebKit Review Bot]

Comment on attachment 127295 [details]
Just rebased the patch, style bot probably is broken

Rejecting attachment 127295 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 10703 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
46>At revision 10703.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/11534697

[Simon Hausmann]

Committed r107914: <http://trac.webkit.org/changeset/107914>