[webkit-reviews] review granted: [Bug 45455] [Qt] Support third-party cookie policy for Qt clients : [Attachment 97701] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Robert Hogan
<robert at webkit.org>'s request for review:
Bug 45455: [Qt] Support third-party cookie policy for Qt clients
https://bugs.webkit.org/show_bug.cgi?id=45455

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=97701&action=review

Should we change the default to AllowThirdPartyWithExistingCookies and document
it? Do you know if there is a security reason justifying the behavior of
Safari?

> Source/WebCore/platform/qt/ThirdPartyCookiesQt.cpp:61
> +    firstPartyDomain.remove(firstPartyDomain.length() -
firstPartyTLD.length(), firstPartyTLD.length());
> +    firstPartyDomain.prepend(QLatin1Char('.'));

It would be nice to have an inline function to do this. That would make the
code naturally self documented.

> Source/WebCore/platform/qt/ThirdPartyCookiesQt.h:24
> +#include <QNetworkCookieJar>
> +#include <QUrl>

You don't need those includes.

> Source/WebCore/platform/qt/ThirdPartyCookiesQt.h:29
> +bool thirdPartyCookiePolicyPermits(QNetworkCookieJar* , const QUrl&, const
QUrl& firstPartyUrl);
> +bool thirdPartyCookiePolicyPermits(QObject* originatingFrame, const QUrl&,
const QUrl& firstPartyUrl);

QNetworkCookieJar is also a QObject. This is ok but I think that is slightly
more confusing than it should be.
I think
-thirdPartyCookiePolicyPermits should take a QWebFrame, and the cast could be
done before
-alternatively, thirdPartyCookiePolicyPermits could be renamed to
thirdPartyCookiePolicyPermitsForFrame()
Just to make sure they are never mixed by accident.

> Source/WebKit/qt/Api/qwebsettings.cpp:395
> +    \since 4.8

Probably not.
Do we do since QtWebKit 2.3?
I guess we can add \since 4.8 if the patch is integrated.

> Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:1069
> +bool DumpRenderTreeSupportQt::thirdPartyCookiePolicy(QNetworkCookieJar* jar,
const QUrl& url, const QUrl& firstPartyUrl)

I don't like the name of the function thirdPartyCookiePolicy(), it returns a
bool, the name suggest it return an enum with the policy.

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:92
> +#if QT_VERSION >= 0x040800

Why not >= QT_VERSION_CHECK(4, 8, 0)?

> Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:2936
> +#if QT_VERSION >= 0x040800

Why not >= QT_VERSION_CHECK(4, 8, 0)?