Bug 101272 – [Qt] Implement deleteCookie() for persistent storage

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[Qt] Implement deleteCookie() for persistent storage

[Sergio Villar Senin]

Created attachment 172426 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 172426 [details]
Patch

Oh, nice catch!
There are some issues I could see related to this:
- Before we had one prepared statement in setCookiesFromUrl and would iterate on each added cookie with it. Now since setCookiesFromUrl calls deleteCookie for each cookie, we'll end up preparing and executing an additional query for every cookie add/update.
- Another thing is if we have both a valid and an expired cookie passed to setCookiesFromUrl. deleteCookie will be called for both, but since QNetworkCookieJar::setCookiesFromUrl will return true we'll end up re-adding the expired cookie to the DB in SharedCookieJarQt::setCookiesFromUrl.
- If we do it that way, we should use INSERT instead of INSERT OR REPLACE in SharedCookieJarQt::setCookiesFromUrl

Would it be possible to do the deletion after the insert/replace in SharedCookieJarQt::setCookiesFromUrl instead?

[Sergio Villar Senin]

(In reply to comment #2)
> (From update of attachment 172426 [details])
> Oh, nice catch!
> There are some issues I could see related to this:
> - Before we had one prepared statement in setCookiesFromUrl and would iterate on each added cookie with it. Now since setCookiesFromUrl calls deleteCookie for each cookie, we'll end up preparing and executing an additional query for every cookie add/update.

That's true, but I think that's unavoidable due to the implementation of the QNetworkCookieJar. Anyway I don't see any performance penalty on that as cookies should be a negligible part of the total load time.

> - Another thing is if we have both a valid and an expired cookie passed to setCookiesFromUrl. deleteCookie will be called for both, but since QNetworkCookieJar::setCookiesFromUrl will return true we'll end up re-adding the expired cookie to the DB in SharedCookieJarQt::setCookiesFromUrl.

Not really, note that in the loop that builds the cookie ids in SharedCookieJarQt::setCookiesFromUrl(), we check for isSessionCookie() which means that only the cookies added to the QtNetwokCookieJar will be stored in the database.

[Sergio Villar Senin]

(In reply to comment #3)
> > - Another thing is if we have both a valid and an expired cookie passed to setCookiesFromUrl. deleteCookie will be called for both, but since QNetworkCookieJar::setCookiesFromUrl will return true we'll end up re-adding the expired cookie to the DB in SharedCookieJarQt::setCookiesFromUrl.
> 
> Not really, note that in the loop that builds the cookie ids in SharedCookieJarQt::setCookiesFromUrl(), we check for isSessionCookie() which means that only the cookies added to the QtNetwokCookieJar will be stored in the database.

Beh, forget about this last comment which is totally wrong (I got misled by the session thing). The case you mention (cookies rejected by the QNetworkCookieJar but added to the DB) cannot happen because the loop does not iterate over the original list but over the accepted cookies:

    foreach (const QNetworkCookie &cookie, cookiesForUrl(url)) {

so we'll only store the ones accepted by the QNetworkCookieJar anyway.

[Jocelyn Turcotte]

(In reply to comment #3)
> That's true, but I think that's unavoidable due to the implementation of the QNetworkCookieJar. Anyway I don't see any performance penalty on that as cookies should be a negligible part of the total load time.

I'm being a bit paranoiac but this is disk IO and can easily become a bottleneck when the data grows bigger. I did some tests with a cookies.db containing 20000 cookies and our INSERT OR REPLACE statement already take 10ms on my desktop for every call to setCookiesFromUrl. It's going to be even slower on cheaper embedded devices so it could add half a second to the load time in some circumstances.

I tried and the delete seems to be pretty quick even if the database is full so it's fine.

(In reply to comment #4)
> Beh, forget about this last comment which is totally wrong (I got misled by the session thing). The case you mention (cookies rejected by the QNetworkCookieJar but added to the DB) cannot happen because the loop does not iterate over the original list but over the accepted cookies:
> 
>     foreach (const QNetworkCookie &cookie, cookiesForUrl(url)) {
> 
> so we'll only store the ones accepted by the QNetworkCookieJar anyway.

Ahh yep you're right, I didn't see that cookiesForUrl(url), sorry for spreading my wrong assumptions.

[Sergio Villar Senin]

Comment on attachment 172426 [details]
Patch

Clearing flags on attachment: 172426

Committed r133770: <http://trac.webkit.org/changeset/133770>

[Sergio Villar Senin]

All reviewed patches have been landed.  Closing bug.