Bug 65328 – [Qt] QtTestBrowser: add support for saving cookies on disk

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Ademar Reis]

Patch is on the way.

[Ademar Reis]

Created attachment 102273 [details]
patch

[Rafael Brandao]

Comment on attachment 102273 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102273&action=review

> Tools/QtTestBrowser/cookiejar.cpp:41
> +       syscalls in sequence (when loading pages which sets multiple cookies) */

Comment style.

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

Shame on QDir, but you could use a better name for it. :)

> Tools/QtTestBrowser/cookiejar.cpp:73
> +    /* when disabling, save current cookies */

Comment style.

> Tools/QtTestBrowser/cookiejar.cpp:78
> +void TestBrowserCookieJar::saveToDisk()

saveToDisk() doesn't really save anything, you should rename it.

> Tools/QtTestBrowser/cookiejar.cpp:83
> +    /* non optimal, we could use some database and incremental save */

Comment style.

> Tools/QtTestBrowser/launcherwindow.cpp:36
> +#include "cookiejar.h"

This include should precede "launcherwindow.h", to keep it sorted.

> Tools/QtTestBrowser/launcherwindow.cpp:56
> +static TestBrowserCookieJar* testBrowserCookieJarinstance()

You should use testBrowserCookieJarInstance instead.

> Tools/QtTestBrowser/launcherwindow.cpp:135
> +    /* we reuse the same cookieJar on multiple QNAMs, which is OK */

Comment style.

[Ademar Reis]

Created attachment 102289 [details]
patch v2

[Alexis Menard (darktears)]

Comment on attachment 102273 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102273&action=review

>> Tools/QtTestBrowser/launcherwindow.cpp:36
>> +#include "cookiejar.h"
> 
> This include should precede "launcherwindow.h", to keep it sorted.

No as the includes rules say that the h file which define the current cpp file as to be before all.

[Rafael Brandao]

(In reply to comment #4)
> (From update of attachment 102273 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102273&action=review
> 
> >> Tools/QtTestBrowser/launcherwindow.cpp:36
> >> +#include "cookiejar.h"
> > 
> > This include should precede "launcherwindow.h", to keep it sorted.
> 
> No as the includes rules say that the h file which define the current cpp file as to be before all.

Sure, nevermind then. :)

[Ademar Reis]

(In reply to comment #2)
> (From update of attachment 102273 [details])
> > Tools/QtTestBrowser/cookiejar.cpp:47
> > +    QDir foo; /* QDir::mkpath() is not static :( */
> 
> Shame on QDir, but you could use a better name for it. :)

What's wrong with foo? :P

[Early Warning System Bot]

Comment on attachment 102273 [details]
patch

Attachment 102273 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9266264

[Ademar Reis]

Created attachment 102291 [details]
patch v3 (fix misinterpretation of the coding style)

[Ademar Reis]

(In reply to comment #6)
> (In reply to comment #2)
> > (From update of attachment 102273 [details] [details])
> > > Tools/QtTestBrowser/cookiejar.cpp:47
> > > +    QDir foo; /* QDir::mkpath() is not static :( */
> > 
> > Shame on QDir, but you could use a better name for it. :)
> 
> What's wrong with foo? :P

I was just kidding. I don't think the name makes any difference in that context, but I changed it from QDir foo to QDir d in patch-v3.

[WebKit Review Bot]

Attachment 102289 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/QtTestBrowser/Qt..." exit_code: 1

Tools/QtTestBrowser/cookiejar.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 102291 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/QtTestBrowser/Qt..." exit_code: 1

Tools/QtTestBrowser/cookiejar.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ademar Reis]

(In reply to comment #11)
> Tools/QtTestBrowser/cookiejar.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
> Total errors found: 1 in 7 files
>
> If any of these errors are false positives, please file a bug against check-webkit-style.

False positive, opened bug 65372 for that.

[Andreas Kling]

Comment on attachment 102291 [details]
patch v3 (fix misinterpretation of the coding style)

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.

[Ademar Reis]

(In reply to comment #13)
> (From update of attachment 102291 [details])

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

It's a singleShot timer, it would be cumbersome to do that with timerEvent().

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

Actually it should be toUtf8(), fixed.

(everything else was fixed, thanks!)

I also fixed a race in the destructor: it should not call saveCookies() directly, as this (private) method expects m_rawCookies to be filled. Fixed it by introducing extractRawCookies(), which is called from scheduleSaveToDisk() and the destructor.

(I can't call scheduleSaveToDisk() from the destructor because by the time the cookiejar is destroyed there's no mainloop anymore and I get a warning because of m_timer.start()).

Also added a comment to clarify why we don't extract the rawcookies directly at saveToDisk():

    // We extract the raw cookies here because the user may
    // enable/disable/clear cookies while the timer is running.

Will upload the patch again with r? because of these changes.

[Ademar Reis]

Created attachment 102965 [details]
patch v4 (fixex from Kling's review + fix for a race in the destructor)

[WebKit Review Bot]

Attachment 102965 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/QtTestBrowser/Qt..." exit_code: 1

Tools/QtTestBrowser/cookiejar.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ademar Reis]

(In reply to comment #16)
> Attachment 102965 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/QtTestBrowser/Qt..." exit_code: 1
> 
> Tools/QtTestBrowser/cookiejar.cpp:28:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
> Total errors found: 1 in 7 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

False positive, opened bug 65372 for that.

[Andreas Kling]

Comment on attachment 102965 [details]
patch v4 (fixex from Kling's review + fix for a race in the destructor)

View in context: https://bugs.webkit.org/attachment.cgi?id=102965&action=review

Kool!

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

Iterator should be a const-reference :)

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

Ditto.

[Ademar Reis]

Created attachment 103066 [details]
patch v5 (fix foreach)

[WebKit Review Bot]

Comment on attachment 103066 [details]
patch v5 (fix foreach)

Clearing flags on attachment: 103066

Committed r92478: <http://trac.webkit.org/changeset/92478>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.