Bug 65328 - [Qt] QtTestBrowser: add support for saving cookies on disk
Summary: [Qt] QtTestBrowser: add support for saving cookies on disk
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P5 Enhancement
Assignee: Ademar Reis
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2011-07-28 11:23 PDT by Ademar Reis
Modified: 2011-08-05 07:04 PDT (History)
2 users (show)

See Also:


Attachments
patch (13.91 KB, patch)
2011-07-28 11:29 PDT, Ademar Reis
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
patch v2 (13.92 KB, patch)
2011-07-28 13:09 PDT, Ademar Reis
no flags Details | Formatted Diff | Diff
patch v3 (fix misinterpretation of the coding style) (13.90 KB, patch)
2011-07-28 13:25 PDT, Ademar Reis
kling: review+
kling: commit-queue-
Details | Formatted Diff | Diff
patch v4 (fixex from Kling's review + fix for a race in the destructor) (14.06 KB, patch)
2011-08-04 13:14 PDT, Ademar Reis
kling: review+
kling: commit-queue-
Details | Formatted Diff | Diff
patch v5 (fix foreach) (14.06 KB, patch)
2011-08-05 06:05 PDT, Ademar Reis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ademar Reis 2011-07-28 11:23:38 PDT
Patch is on the way.
Comment 1 Ademar Reis 2011-07-28 11:29:32 PDT
Created attachment 102273 [details]
patch
Comment 2 Rafael Brandao 2011-07-28 13:07:27 PDT
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.
Comment 3 Ademar Reis 2011-07-28 13:09:49 PDT
Created attachment 102289 [details]
patch v2
Comment 4 Alexis Menard (darktears) 2011-07-28 13:11:21 PDT
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.
Comment 5 Rafael Brandao 2011-07-28 13:13:11 PDT
(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. :)
Comment 6 Ademar Reis 2011-07-28 13:17:09 PDT
(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
Comment 7 Early Warning System Bot 2011-07-28 13:17:24 PDT
Comment on attachment 102273 [details]
patch

Attachment 102273 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9266264
Comment 8 Ademar Reis 2011-07-28 13:25:06 PDT
Created attachment 102291 [details]
patch v3 (fix misinterpretation of the coding style)
Comment 9 Ademar Reis 2011-07-28 13:27:32 PDT
(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.
Comment 10 WebKit Review Bot 2011-07-28 15:41:27 PDT
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.
Comment 11 WebKit Review Bot 2011-07-28 15:43:31 PDT
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.
Comment 12 Ademar Reis 2011-07-29 06:19:04 PDT
(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.
Comment 13 Andreas Kling 2011-08-04 11:01:00 PDT
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.
Comment 14 Ademar Reis 2011-08-04 13:12:32 PDT
(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.
Comment 15 Ademar Reis 2011-08-04 13:14:16 PDT
Created attachment 102965 [details]
patch v4 (fixex from Kling's review + fix for a race in the destructor)
Comment 16 WebKit Review Bot 2011-08-04 13:17:05 PDT
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.
Comment 17 Ademar Reis 2011-08-04 13:20:10 PDT
(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.
Comment 18 Andreas Kling 2011-08-05 05:16:53 PDT
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.
Comment 19 Ademar Reis 2011-08-05 06:05:27 PDT
Created attachment 103066 [details]
patch v5 (fix foreach)
Comment 20 WebKit Review Bot 2011-08-05 07:03:55 PDT
Comment on attachment 103066 [details]
patch v5 (fix foreach)

Clearing flags on attachment: 103066

Committed r92478: <http://trac.webkit.org/changeset/92478>
Comment 21 WebKit Review Bot 2011-08-05 07:04:01 PDT
All reviewed patches have been landed.  Closing bug.