[webkit-reviews] review denied: [Bug 73215] [Qt][WK2] Split QWebPermissionRequest into QWebSecurityOrigin : [Attachment 124447] Removed methods from changelog

[bugzilla-daemon at webkit.org]

Kenneth Rohde Christiansen <kenneth at webkit.org> has denied Adenilson Cavalcanti
Silva <savagobr at yahoo.com>'s request for review:
Bug 73215: [Qt][WK2] Split QWebPermissionRequest into QWebSecurityOrigin
https://bugs.webkit.org/show_bug.cgi?id=73215

Attachment 124447: Removed methods from changelog
https://bugs.webkit.org/attachment.cgi?id=124447&action=review

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