[Webkit-unassigned] [Bug 73215] [Qt][WK2] Split QWebPermissionRequest into QWebSecurityOrigin

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73215


Kenneth Rohde Christiansen <kenneth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #124447|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #45 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2012-01-29 04:15:48 PST ---
(From update of attachment 124447)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.