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

[bugzilla-daemon at webkit.org]

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.
https://bugs.webkit.org/show_bug.cgi?id=65309

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

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