Bug 59200 – [Qt][WK2] Implement permission API for Qt port

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Mahesh Kulkarni]

Implement common permission between UIProcess-WebProcess. Also implement Geolocation permission.

[Mahesh Kulkarni]

Created attachment 92226 [details]
patch

Original Author: [e-mail]

Implements permission API for QtWebKit WK2 and hooked up with MiniBrowser.

[Eric Seidel (no email)]

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

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

Source/WebKit2/UIProcess/API/qt/qwkpermissionrequest.cpp:21:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 18 files


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

[Early Warning System Bot]

Attachment 92226 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8553498

[Mahesh Kulkarni]

Created attachment 92232 [details]
patch

fixed trivial style issue

[Mahesh Kulkarni]

Comment on attachment 92232 [details]
patch

My bad. Some unwanted stuff in this patch. Clearing flag for review.

[Early Warning System Bot]

Attachment 92232 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8556531

[Mahesh Kulkarni]

Created attachment 92235 [details]
patch

Cleaned up the mess from previous patch. Please review.

[Kenneth Rohde Christiansen]

Comment on attachment 92235 [details]
patch

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

> Tools/MiniBrowser/qt/BrowserWindow.cpp:399
> +void BrowserWindow::permissionRequested(QWKPermissionRequest& request)
> +{
> +    request.setAccepted(true);
> +}

We probably want some UI for this later

> Source/WebKit2/ChangeLog:8
> +        Original Author: [e-mail]

Please use [e-mail] that is what I most commonly use.

> Source/WebKit2/UIProcess/API/qt/qwkpermissionrequest_p.h:43
> +    }
> +    ~QWKPermissionRequestPrivate()

newline needed here

[Siddharth Mathur]

Comment on attachment 92235 [details]
patch

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

Disclaimer: I am not a reviewer

> Tools/MiniBrowser/qt/MiniBrowser.pro:51
> +    TARGET.CAPABILITY = ReadUserData WriteUserData NetworkServices Location

Please remove this change as it's being modified via Bug 59462

> Source/WebKit2/UIProcess/API/qt/qwksecurityorigin.cpp:3
> +

Please change copyright year to 2011 in all files

> Source/WebKit2/UIProcess/API/qt/qwksecurityorigin.cpp:31
> +

: d(priv)

> Source/WebKit2/UIProcess/API/qt/qwksecurityorigin.h:43
> +};

Why use a QExplicitlySharedDataPointer ? Would a QScopedPointer not be sufficient here, assuming of course that a QWKSecurityOriginPrivate is owned by a QWKSecurityOrigin?

> Source/WebKit2/UIProcess/API/qt/qwkpermissionrequest_p.h:50
> +    WKRetainPtr<WKGeolocationPermissionRequestRef> m_request;

I need to read up on my smart pointers :)

> Source/WebKit2/UIProcess/API/qt/qwkpermissionrequest.cpp:29
> +    d = priv;

: d(priv)

[Kenneth Rohde Christiansen]

> > Source/WebKit2/UIProcess/API/qt/qwkpermissionrequest_p.h:50
> > +    WKRetainPtr<WKGeolocationPermissionRequestRef> m_request;
> 
> I need to read up on my smart pointers :)

Nothing special about that one. The WK2 C structs needs a call to "retain" and when not needed anymore a call to "release". The WKRetainPtr takes care of that. When it goes out of scope it calls release. In that way it is quite similar to an auto pointer or a scoped pointer.

[Yael]

Comment on attachment 92235 [details]
patch

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

This is an unofficial review as I am not a reviewer.

> Source/WebKit2/UIProcess/API/qt/ClientImpl.cpp:189
> +void qt_wk_decidePolicyForGeolocationPermissionRequest(WKPageRef page, WKFrameRef frame, WKSecurityOriginRef origin, WKGeolocationPermissionRequestRef request, const void* clientInfo)
> +{
> +    QWKPermissionRequest req = QWKPermissionRequestPrivate::create(origin, request);
> +    emit toQWKPage(clientInfo)->permissionRequested(req);

This is going to be pretty ugly if the client application wants to display asynchronous UI.

> Source/WebKit2/UIProcess/API/qt/qwkpermissionrequest.h:11
> +class QWEBKIT_EXPORT QWKPermissionRequest {

The only purpose of this class is to wrap the origin and type of the request.
You are adding a lot of code just for that, and it makes the client side more complicated if they wish to have an asynchronous UI.
Do we really want that?

[Andreas Kling]

Comment on attachment 92235 [details]
patch

r- as this won't merge after view refactoring. Also Yael had some good questions.

[Mahesh Kulkarni]

(In reply to comment #11)
> (From update of attachment 92235 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92235&action=review
> 
> This is an unofficial review as I am not a reviewer.
> 
> > Source/WebKit2/UIProcess/API/qt/ClientImpl.cpp:189
> > +void qt_wk_decidePolicyForGeolocationPermissionRequest(WKPageRef page, WKFrameRef frame, WKSecurityOriginRef origin, WKGeolocationPermissionRequestRef request, const void* clientInfo)
> > +{
> > +    QWKPermissionRequest req = QWKPermissionRequestPrivate::create(origin, request);
> > +    emit toQWKPage(clientInfo)->permissionRequested(req);
> 
> This is going to be pretty ugly if the client application wants to display asynchronous UI.
> 

Actually it is  already async. Client app has to just store QWKPermissionRequest instance and call setAccepted() when it obtains the permission. WebCore::GeolocationController waits for the permission and queues all the requests. 

> > Source/WebKit2/UIProcess/API/qt/qwkpermissionrequest.h:11
> > +class QWEBKIT_EXPORT QWKPermissionRequest {
> 
> The only purpose of this class is to wrap the origin and type of the request.
> You are adding a lot of code just for that, and it makes the client side more complicated if they wish to have an asynchronous UI.
> Do we really want that?

[Adenilson Cavalcanti Silva]

An update in this bug. I got most of the original patch to apply in current code, ATM studying the differences between the QWKPage X QTouchWebPage to finish it.

[Mahesh Kulkarni]

(In reply to comment #14)
> An update in this bug. I got most of the original patch to apply in current code, ATM studying the differences between the QWKPage X QTouchWebPage to finish it.

Again this piece of implementation is is already in http://gitorious.org/+mob-team/webkit/mob-team-qtwebkit2 branch with new qtouchwebview code which I was planning to upstream eventually, if you are want to upstream the code asap you may wanna refer the code from branch.

[Adenilson Cavalcanti Silva]

Mahesh

That is pretty cool, I will have a look on it.

Cheers


Adenilson

[Adenilson Cavalcanti Silva]

Created attachment 113073 [details]
Implements geopermission api in Qt webkit2

[Kenneth Rohde Christiansen]

Comment on attachment 113073 [details]
Implements geopermission api in Qt webkit2

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

I think Simon had some API ideas, so cc'ing him

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:508
> +    // For while set it to true by default.

not good English

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview_p.h:97
>  
> +    virtual void permissionRequested(QWKPermissionRequest&);
> +

Where is the public API? Also an API test would be great, and should be possible to do. It would also show how this API is supposed to be used, which is important to see if this kind of API makes sense.

> Source/WebKit2/UIProcess/qt/QtViewInterface.h:99
> +    virtual void permissionRequested(QWKPermissionRequest&) = 0;

We don't use WK as naming for our classes anymore

> Source/WebKit2/UIProcess/qt/qwkpermissionrequest.cpp:30
> +{
> +    d = priv;
> +}

Why not use initializer lists?

> Source/WebKit2/UIProcess/qt/qwkpermissionrequest.cpp:43
> +{
> +    d = other.d;

Here as well

> Source/WebKit2/UIProcess/qt/qwkpermissionrequest.cpp:57
> +void QWKPermissionRequest::setAccepted(bool accepted)

So what happens if the page is reloaded before this method is called?

> Source/WebKit2/UIProcess/qt/qwksecurityorigin.cpp:31
> +QWKSecurityOrigin::QWKSecurityOrigin(const QWKSecurityOrigin& other) : d(other.d)

Weird coding style

> Source/WebKit2/UIProcess/qt/qwksecurityorigin.h:33
> +    unsigned short port() const;

Maybe int makes more sense for a Qt API? Simon?

[Simon Hausmann]

Comment on attachment 113073 [details]
Implements geopermission api in Qt webkit2

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

So.... shouldn't this be private API only at this point? (i.e. only be accessible using QT += webkit-private?

>> Source/WebKit2/UIProcess/qt/qwksecurityorigin.h:33
>> +    unsigned short port() const;
> 
> Maybe int makes more sense for a Qt API? Simon?

Absolutely, the port should be an integer.

[Adenilson Cavalcanti Silva]

Created attachment 113282 [details]
Fixed: initialization list, class/file names.

QAbstractSocket uses quint16 for port and WKSecurityOriginGetPort is a unsigned short.

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/qt/qsecurityorigin_p.h:20:  #ifndef header guard has wrong style, please use: qsecurityorigin_p_h  [build/header_guard] [5]
Source/WebKit2/UIProcess/qt/qsecurityorigin_p.h:40:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/qt/qsecurityorigin_p.cpp:22:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Source/WebKit2/UIProcess/qt/qsecurityorigin_p.cpp:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/qt/qpermissionrequest_p.h:46:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit2/UIProcess/qt/qpermissionrequest_p.h:46:  The parameter name "request" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 6 in 11 files


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

[Adenilson Cavalcanti Silva]

About what happens if the page is reloaded before setAccepted() is called: my guess is that the parent object (Frame?) will be destroyed to avoid clients to popup dialogs unnecessarily  (IIRC, this scenario was thought when the new client based implementation design was devised).

[Adenilson Cavalcanti Silva]

Created attachment 113285 [details]
Fixed: initialization list, class/file names, coding style

[Simon Hausmann]

Comment on attachment 113285 [details]
Fixed: initialization list, class/file names, coding style

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

Ok, so if this is an intermediate step, what do you envision as the way of dealing with this on the application side?

I'd rather r+ when we have an idea where this is going.

What I'd like to see is an API that can be dealt with on the QML side. Something along the lines of:

WebView {

    onPermissionRequested: {
        if (show a dialog)
            request.accepted = true;
        else
            request.accepted = false;

}

> Source/WebKit2/ChangeLog:-612
> -

Accidental removal of whitespace/newline?

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:509
> +    // For while, set it to true.
> +    request.setAccepted(true);

(it's "For a while" btw :)

> Source/WebKit2/UIProcess/qt/qsecurityorigin_p.cpp:32
> +class QtSecurityOriginPrivate : public QSharedData {
> +public:
> +    WKRetainPtr<WKSecurityOriginRef> m_origin;
> +    QtSecurityOriginPrivate() { };
> +};

So this seems a bit overkill, doesn't it? We have a shared pointer to a data structure that contains another shared pointer :)

[Adenilson Cavalcanti Silva]

Created attachment 114282 [details]
Fixed shared pointer, changelog, comment

[Adenilson Cavalcanti Silva]

Concerning the API for QML side, you are right, the idea is to let the user the option to display an authorization dialog and enable (or not) the retrieval of geolocation info.

Should I expose the QtPermissionRequest class now? Or maybe create a new issue for discussion of the QML API?

[Simon Hausmann]

Comment on attachment 114282 [details]
Fixed shared pointer, changelog, comment

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

This is an improvement, but I think before we can land this the refcounting needs to be fixed as well as the setAccept(true) being replaced by an actual private C++ API.

Idea: Even if you add a signal to the class that is exposed in QML, then the signal cannot be used unless all of the argument types are available in QML. If we choose not to export them, then the signal
is only available to "users" who use QT += webkit-private and access the C++ types.

> Source/WebKit2/UIProcess/API/qt/qdesktopwebview.cpp:503
> +    // For a while, set it to true.
> +    request.setAccepted(true);

I don't think we should land it like that. I'd prefer to land a patch that provides a private C++ API (or alternatively a private QML one) in one shot.

> Source/WebKit2/UIProcess/qt/qsecurityorigin_p.cpp:32
> +class QtSecurityOriginPrivate : public QSharedData {
> +public:
> +    WKSecurityOriginRef m_origin;
> +    QtSecurityOriginPrivate() { };
> +};

My criticism from earlier remains: QtSecurityOrigin keeps a shared pointer to a WKSecurityOriginRef, which itself is a shared pointer.

On top of that WK*Ref types are not value based. They are reference counted and either need to be "refcounted" using WKRetain and WKRelease
or you need to use a WKRetainPtr.

I suggest to replace the QtSecurityOriginPrivate with a WKSecurityOriginRef and call Retain/Release accordingly from the constructore, copy constructor, assignment operator and destructor. This means that you can use the opaque forward declaration of WKSecurityOrigin in the _p.h "semit public" header file.

[Adenilson Cavalcanti Silva]

Created attachment 114527 [details]
Solved shared pointer issue (hopefully!), file naming, no longer setting permission to true

[WebKit Review Bot]

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

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

Source/WebKit2/UIProcess/qt/qsecurityorigin.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


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

[Kenneth Rohde Christiansen]

Comment on attachment 114527 [details]
Solved shared pointer issue (hopefully!), file naming, no longer setting permission to true

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

> Source/WebKit2/UIProcess/qt/QtViewInterface.cpp:248
> +void QtViewInterface::permissionRequested(QtPermissionRequest& request)
> +{
> +    // Here it should call QtPermissionRequest::setAccepted().
> +}

That comment doesnt make sense to me, the use can accept it at anytime (while still being valid). It is an explicitly shared data pointer

> Source/WebKit2/UIProcess/qt/qpermissionrequest.cpp:44
> +    QtSecurityOrigin m_origin;
> +    QtPermissionRequest::Type m_type;
> +
> +    WKRetainPtr<WKGeolocationPermissionRequestRef> m_request;

we don't use m_ in privates are everything is publically accessible

> Source/WebKit2/UIProcess/qt/qpermissionrequest.cpp:55
> +QtPermissionRequest::QtPermissionRequest(QtPermissionRequestPrivate* priv)
> +    : d(priv)
> +{
> +}

This would not be ok if this is to become public api class later

> Source/WebKit2/UIProcess/qt/qpermissionrequest_p.h:46
> +    static QtPermissionRequest create(WKSecurityOriginRef, WKGeolocationPermissionRequestRef);

Why isnt this in the private (which should be friend)? Any why is this exported class name starting with Qt*?

> Source/WebKit2/UIProcess/qt/qsecurityorigin_p.h:46
> +class QWEBKIT_EXPORT QtSecurityOrigin {
> +public:
> +    ~QtSecurityOrigin();
> +
> +    QString scheme() const;
> +    QString host() const;
> +    quint16 port() const;
> +
> +    QtSecurityOrigin(WKSecurityOriginRef);
> +    QtSecurityOrigin(const QtSecurityOrigin& other);
> +    QtSecurityOrigin &operator=(const QtSecurityOrigin& other);
> +    static QtSecurityOrigin create(WKSecurityOriginRef);
> +
> +private:
> +    QtSecurityOrigin() { };
> +    WKRetainPtr<WKSecurityOriginRef> m_origin;
> +};

This is written like a public class, even exported, but the naming suggest it is private and so does the implementation (ctor, private section etc). The filename also suggests it is public, though not in the api directory... im confused

[Adenilson Cavalcanti Silva]

Created attachment 115809 [details]
Simplified the patch, exposes it to QML side

ATM, it will fail to link (would love some ideas about what changed in buildsystem). Already added entries in qtwebkit-export.map to no avail.

+        *QWebPermissionRequest;
+        non-virtual?thunk?to?QWebPermissionRequest*;
+        QWebPermissionRequest::*;
+

[WebKit Review Bot]

Attachment 115809 [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/qt/QtWebPageProxy.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 10 files


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

[Adenilson Cavalcanti Silva]

Created attachment 115855 [details]
Permission api hook for geolocation

[Simon Hausmann]

Comment on attachment 115855 [details]
Permission api hook for geolocation

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

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest.cpp:93
> +    return WKStringCopyQString(WKSecurityOriginCopyProtocol(d->origin.get()));

This will leak the WKStringRef returned by WKSecurityOriginCopyProtocol. You should use adoptWK on the return value.

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest.cpp:98
> +    return WKStringCopyQString(WKSecurityOriginCopyHost(d->origin.get()));

Same here :)

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest_p.h:35
> +    Q_PROPERTY(qint32 type READ type)

Why is this an qint32 instead of the real type?

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest_p.h:52
> +    quint16 port() const;

In QUrl the port is just a plain "int". It may be not "mathematically strict enough", but it's perfectly consistent with the rest of the Qt API. I suggest to use int here, too :)

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.cpp:928
> +
> +    if (m_allow)
> +        delete m_allow;
> +
> +    m_allow = request;
> +    emit m_qmlWebView->permissionRequested(m_allow);

Is this correct? Won't this cancel any previous permission request? What if a page asks for gelocation _and_ notification permission? Won't the second permission request cancel the previous?

If you're worried about leaking "unanswered" permission requests, how about making them (here) children of the QtWebPageProxy?

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:293
> +    QWebPermissionRequest *m_allow;

I don't think this is needed :)

[Adenilson Cavalcanti Silva]

Created attachment 116139 [details]
Experimental api, changed port type, removed m_allow, etc

[Adenilson Cavalcanti Silva]

Created attachment 116161 [details]
Previous fixes, plus Changelog

[Adenilson Cavalcanti Silva]

The QML API is looking like: 

WebView {

        experimental.onPermissionRequested: {
            console.log("######## Status: " + permission.allow +
            "\tscheme: " + permission.scheme +
            "\thost: " + permission.host +
            "\tport: " + permission.port +
            "\ttype: " + permission.type)
            //I will fix this
            if (permission.type == 0) {
                console.log("########## geolocation: granting permission")
                permission.setAllow(true)
            }

        }

}

[Adenilson Cavalcanti Silva]

Ah, this works fine too: permission.allow = true

[Simon Hausmann]

Comment on attachment 116161 [details]
Previous fixes, plus Changelog

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

This is getting there :). I'd like to see the int32 type() thing to get fixed as well as the request object being scheduled for deletion _before_ the parent dies. (the parent thing is a last resort, in the common case we should clean up properly before that)

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest.cpp:70
> +void QWebPermissionRequest::setAllow(bool accepted)

After calling setAllow, shouldn't the request object get deleted at some point? (and I mean before the parent QQuickWebView)

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest_p.h:48
> +    qint32 type() const;

I think the return type should be "Type" and it should be exposed in QML.

> Source/WebKit2/UIProcess/qt/QtWebPageProxy.h:293
> +

Stray newline?

[Kenneth Rohde Christiansen]

I would like us to separate our the Security Origin. It is an integrated part of XHR, DOM and could even be used from QML etc.

Maybe somethign like this: http://pastebin.ubuntu.com/748139/

[Kenneth Rohde Christiansen]

Comment on attachment 116161 [details]
Previous fixes, plus Changelog

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

> Source/WebKit2/UIProcess/API/qt/qquickwebview_p.h:185
> +    void permissionRequested(QWebPermissionRequest* permission);

I would leave out the permission... I wonder why the style bit didnt complain...

[Adenilson Cavalcanti Silva]

Created attachment 116525 [details]
Rebased with master, QENUM, etc.

[Kenneth Rohde Christiansen]

Comment on attachment 116525 [details]
Rebased with master, QENUM, etc.

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

Will you hook this up this in the mini browser?

> Source/WebKit/qt/ChangeLog:6
> +        Permission API hookup for Geolocation, it allows in WebView to receive

in? the?

> Source/WebKit2/ChangeLog:6
> +        Permission API hookup for Geolocation, it allows in WebView to receive

same thing

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest.cpp:32
> +public:
> +
> +    QWebPermissionRequestPrivate(WKSecurityOriginRef securityOrigin, WKGeolocationPermissionRequestRef permissionRequest)

I would remove that newline, nitpick

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest.cpp:55
> +QWebPermissionRequest::QWebPermissionRequest(WKSecurityOriginRef securityOrigin,
> +                                             WKGeolocationPermissionRequestRef permissionRequest, QObject* parent)

We always keep the method declaration on one line

> Source/WebKit2/UIProcess/API/qt/qwebpermissionrequest.cpp:103
> +    return WKStringCopyQString(origin.get());
> +

remove newline here

[Simon Hausmann]

Comment on attachment 116525 [details]
Rebased with master, QENUM, etc.

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

r=me, with Kenneth's comments and my nitpick.

> Source/WebKit2/UIProcess/qt/QtWebPageUIClient.cpp:85
> +void QtWebPageUIClient::permissionRequest(QWebPermissionRequest *request)

Coding style nitpick: The * placement is wrong.

[Adenilson Cavalcanti Silva]

Created attachment 116747 [details]
Coding style fixes, plus qml test

[Adenilson Cavalcanti Silva]

Created attachment 116750 [details]
Coding style fixes, plus qml test, html cleanup

[Adenilson Cavalcanti Silva]

Created attachment 116751 [details]
Coding style fixes, plus qml test, html cleanup, changelog.

[Adenilson Cavalcanti Silva]

Additional work to be done here:
https://bugs.webkit.org/show_bug.cgi?id=73215

[Simon Hausmann]

Comment on attachment 116751 [details]
Coding style fixes, plus qml test, html cleanup, changelog.

r=me but land with care (test might fail on the bot unless qtlocation is installed)

[Csaba Osztrogonác]

(In reply to comment #49)
> (From update of attachment 116751 [details])
> r=me but land with care (test might fail on the bot unless qtlocation is installed)

Only one API test will fail on the bot:
FAIL!  : qmltests::WebViewGeopermission::test_permissionRequest() 'wait for signal permissionRequested' returned FALSE. ()
   Loc: [/usr/local/Trolltech/Qt5/Qt-5.0.0-r13/imports/QtTest/TestCase.qml(473)]

I think it isn't a big problem.

[Jesus Sanchez-Palencia]

Comment on attachment 116751 [details]
Coding style fixes, plus qml test, html cleanup, changelog.

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

> Source/WebKit2/ChangeLog:32
> +

I believe you forgot to 'git add' some files before generating the Changelog, as I don't see either qmltests.pro or the new qml test listed here.

[Adenilson Cavalcanti Silva]

Created attachment 116776 [details]
Previous fixes, plus Changelog.

For while, skip the test until the bot has qtlocation module.

[Rafael Brandao]

Comment on attachment 116776 [details]
Previous fixes, plus Changelog.

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

I think it's really nice, just found some little style problems, not sure if you really need to fix them (do we follow the same webkit guidelines for qml files?).

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_geopermission.qml:20
> +        //Must be false by default

style nitpick: space after // and finishing with . in the end of sentence.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_geopermission.qml:21
> +        if (!permission.allow) {

style: as you only have one line inside the if block, you don't need to use brackets.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_geopermission.qml:26
> +        if (permission.type == PermissionRequest.Geolocation) {

style: same as before (brackets)

[Adenilson Cavalcanti Silva]

Rafael

Thanks for the comments.

I'm not sure if there is a convention concerning QML for webkit. But already exists a Qt convention for QML: http://doc.qt.nokia.com/latest/qml-coding-conventions.html

This one suggests brackets around conditionals.

[Adenilson Cavalcanti Silva]

Created attachment 116951 [details]
Previous fixes, plus qml class register version

[Simon Hausmann]

Comment on attachment 116951 [details]
Previous fixes, plus qml class register version

r=me

[WebKit Review Bot]

Comment on attachment 116951 [details]
Previous fixes, plus qml class register version

Clearing flags on attachment: 116951

Committed r101456: <http://trac.webkit.org/changeset/101456>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.