[webkit-reviews] review denied: [Bug 59200] [Qt][WK2] Implement permission API for Qt port : [Attachment 113073] Implements geopermission api in Qt webkit2

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Adenilson Cavalcanti
Silva <adenilson.silva at openbossa.org>'s request for review:
Bug 59200: [Qt][WK2] Implement permission API for Qt port
https://bugs.webkit.org/show_bug.cgi?id=59200

Attachment 113073: Implements geopermission api in Qt webkit2
https://bugs.webkit.org/attachment.cgi?id=113073&action=review

------- Additional Comments from Kenneth Rohde Christiansen
<kenneth at webkit.org>
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?