[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





--- Comment #18 from Deepjyoti Saha <deesaha at cisco.com>  2013-07-30 03:18:13 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
>> +        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.

Ok, I will modify this.

>> 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.

Sorry, I will add an explanation for AllowSubdomains and DisallowSubdomains.

>> 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.

Yes, it sounds more concise :)

>> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:291
>> +        return;
> 
> Not useful.

OK, I will remove this.

>> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:303
>> +        return;
> 
> Ditto

I will remove this as well.

>> Source/WebKit/qt/Api/qwebsecurityorigin.cpp:314
>> +}
> 
> 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?

Yes, this API should not be attached with a particular origin considering the current implementation in SecurityPolicy. But I would suggest to retain this and be implemented as a static API. It will come handy in a case where application wants to flush an entire set in one go instead of iterating though a list of origins. Please let me know your opinion.

>>> Source/WebKit/qt/Api/qwebsecurityorigin.h:53
>>> +    QWebSecurityOrigin(const QUrl& url);
>> 
>> The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
> 
> Please mark the constructor explicit.

Sure, I will do that.

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