[webkit-reviews] review granted: [Bug 65328] [Qt] QtTestBrowser: add support for saving cookies on disk : [Attachment 102291] patch v3 (fix misinterpretation of the coding style)

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has granted Ademar Reis
<ademar.reis at openbossa.org>'s request for review:
Bug 65328: [Qt] QtTestBrowser: add support for saving cookies on disk
https://bugs.webkit.org/show_bug.cgi?id=65328

Attachment 102291: patch v3 (fix misinterpretation of the coding style)
https://bugs.webkit.org/attachment.cgi?id=102291&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=102291&action=review


r=me with some nitpicks.

> Tools/QtTestBrowser/cookiejar.cpp:33
> +#include <QTimer>

Already included by the cookiejar.h

> Tools/QtTestBrowser/cookiejar.cpp:40
> +    /* We use a timer for the real disk write to avoid multiple IO
> +	  syscalls in sequence (when loading pages which set multiple cookies).
*/

C++ style comments are preferred.

> Tools/QtTestBrowser/cookiejar.cpp:43
> +    m_timer.setInterval(10000);
> +    m_timer.setSingleShot(true);
> +    connect(&m_timer, SIGNAL(timeout()), this, SLOT(saveToDisk()));

We typically use QObject::timerEvent() instead of mucking with QTimer, but I'm
not super fussed.

> Tools/QtTestBrowser/cookiejar.cpp:47
> +    QDir d; // QDir::mkpath() is not static. :(
> +    d.mkpath(path);

Wow, really? Bleh.
Would look slightly better as QDir().mkpath(path); I guess.

> Tools/QtTestBrowser/cookiejar.cpp:83
> +    foreach (QNetworkCookie cookie, cookies) {

foreach (const QNetworkCookie& cookie, cookies) {

> Tools/QtTestBrowser/cookiejar.cpp:97
> +	   foreach (QByteArray cookie, m_rawCookies)

foreach (const QByteArray& cookie, cookies) {

> Tools/QtTestBrowser/cookiejar.cpp:114
> +	      
cookies.append(QNetworkCookie::parseCookies(in.readLine().toAscii()));

Why toAscii() and not toLatin1()?

> Tools/QtTestBrowser/cookiejar.h:42
> +    virtual bool setCookiesFromUrl(const QList<QNetworkCookie>& cookieList,
const QUrl&);

The 'cookieList' argument name is superfluous.

> Tools/QtTestBrowser/cookiejar.h:44
> +    void enableDiskStorage(bool enabled);

I'd prefer setDiskStorageEnabled(bool);

> Tools/QtTestBrowser/launcherwindow.cpp:39
> +#include <QApplication>

QCoreApplication would be enough, I think.