[Webkit-unassigned] [Bug 117823] [Qt] Add interface API for origin whitelisting

[bugzilla-daemon at webkit.org]

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


Jocelyn Turcotte <jocelyn.turcotte at digia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #207318|review?                     |review-
               Flag|                            |




--- Comment #17 from Jocelyn Turcotte <jocelyn.turcotte at digia.com>  2013-07-29 09:03:17 PST ---
(From update of attachment 207318)
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.

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