[webkit-reviews] review denied: [Bug 117823] [Qt] Add interface API for origin whitelisting : [Attachment 207318] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 29 09:03:29 PDT 2013


Jocelyn Turcotte <jocelyn.turcotte at digia.com> has denied Deepjyoti Saha
<deesaha at cisco.com>'s request for review:
Bug 117823: [Qt] Add interface API for origin whitelisting
https://bugs.webkit.org/show_bug.cgi?id=117823

Attachment 207318: Patch
https://bugs.webkit.org/attachment.cgi?id=207318&action=review

------- Additional Comments from Jocelyn Turcotte <jocelyn.turcotte at digia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=207318&action=review


> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:278
> +    if (origin)
> +	   d = new QWebSecurityOriginPrivate(origin);

It doesn't seem like SecurityOrigin::create can ever return null.
Plus most classes are always expecting to have a d-pointer.

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:284
> +    The subdomain access is based on the \a subdomainSetting.

This isn't telling much to the behavior a different value would trigger, could
you instead explain here in one sentence what is the differentce between
AllowSubdomains and DisallowSubdomains? I can't tell precisely myself just with
this information.

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:288
> +void QWebSecurityOrigin::addOriginAccessWhitelistEntry(const QString&
scheme, const QString& host, SubdomainSetting subdomainSetting)

Origin is already in the name of the class, addAccessWhitelistEntry would be
more concise.

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:291
> +    if (!d)
> +	   return;

Not useful.

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:303
> +    if (!d)
> +	   return;

Ditto

> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:314
> +void QWebSecurityOrigin::resetOriginAccessWhitelists()
> +{
> +    SecurityPolicy::resetOriginAccessWhitelists();
> +}

This one isn't consistent with the two others, it should either:
- Be static and be named "All" something (e.g. resetAllAccessWhitelists).
- A proper per-origin resetAccessWhitelist should be implemented in
SecurityPolicy
- Be removed

Since the API should last as long as possible, removing it would be the easiest
and safest way. Most people will probably only add entries and can remove them
through the remove method. Do you need it specifically in your case?

> Source/WebKit/qt/Api/qwebsecurityorigin.h:53
> +    QWebSecurityOrigin(const QUrl& url);

Please mark the constructor explicit.


More information about the webkit-reviews mailing list