[webkit-reviews] review denied: [Bug 65309] [Qt] [WK2] Implement a persistent cookie storage. : [Attachment 106333] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Sep 5 09:47:26 PDT 2011

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Alexis Menard
<alexis.menard at openbossa.org>'s request for review:
Bug 65309: [Qt] [WK2] Implement a persistent cookie storage.

Attachment 106333: Patch

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=106333&action=review

The implementation seems good, though I feel that it should either be renamed
or moved to the WebKit2 directory and initialized as a factory.

> Source/WebCore/WebCore.pri:9
> +QT *= network sql

I'm not a fan of this hard dependency of WebCore on QtSQL. If this feature is
decoupled enough, can we protect its compilation with 
contains(QT_CONFIG, sql) or something like that?

> Source/WebCore/platform/Cookie.h:42
> +	       : expires(0)

Implicit MS-from-epoch :(
But that's been there before your patch.

> Source/WebCore/platform/qt/CookieJarQt.cpp:165
> +    WebKit2SharedCookieJar::shared()->getHostnamesWithCookies(hostnames);
>  }
>  void deleteCookiesForHostname(const String& hostname)
>  {
> -    // FIXME: Not yet implemented
> +    WebKit2SharedCookieJar::shared()->deleteCookiesForHostname(hostname);
>  }
>  void deleteAllCookies()
>  {
> -    // FIXME: Not yet implemented
> +    WebKit2SharedCookieJar::shared()->deleteAllCookies();

WebKit2-specific code inside WebCore? Doesn't look right.
Maybe instead do this factory style, and initialize the factory from WebProcess
main? That way the implementation can live inside WebKit2 if it's WebKit2
btw what's WebKit2-specific about this apart from the name?

> Source/WebCore/platform/qt/CookieJarQt.h:40
> +    void deleteCookiesForHostname(const QString&);

For readability, maybe use String here and not QString, as this is one of the
functions called from WebCore.

> Source/WebKit2/WebProcess/qt/WebProcessQt.cpp:52
> +    jar->setParent(0);

This line is a bit puzzling. Could you add a comment?

More information about the webkit-reviews mailing list