[webkit-reviews] review denied: [Bug 59200] [Qt][WK2] Implement permission API for Qt port : [Attachment 114282] Fixed shared pointer, changelog, comment

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann 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 114282: Fixed shared pointer, changelog, comment
https://bugs.webkit.org/attachment.cgi?id=114282&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
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.