101272 2012-11-05 15:58:44 -0800 [Qt] Implement deleteCookie() for persistent storage 2012-11-07 09:39:49 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 svillar svillar hausmann jturcotte menard rwlbuis svillar oldest_to_newest 758979 0 svillar 2012-11-05 15:58:44 -0800 [Qt] Implement deleteCookie() for persistent storage 758983 1 172426 svillar 2012-11-05 16:06:47 -0800 Created attachment 172426 Patch 759429 2 172426 jturcotte 2012-11-06 02:07:40 -0800 Comment on attachment 172426 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? 759896 3 svillar 2012-11-06 11:12:22 -0800 (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. 760125 4 svillar 2012-11-06 15:44:12 -0800 (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. 760510 5 jturcotte 2012-11-07 03:47:47 -0800 (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. 760781 6 172426 svillar 2012-11-07 09:39:46 -0800 Comment on attachment 172426 Patch Clearing flags on attachment: 172426 Committed r133770: <http://trac.webkit.org/changeset/133770> 760782 7 svillar 2012-11-07 09:39:49 -0800 All reviewed patches have been landed. Closing bug. 172426 2012-11-05 16:06:47 -0800 2012-11-07 09:39:46 -0800 Patch bug-101272-20121105160501.patch text/plain 2734 svillar U3VidmVyc2lvbiBSZXZpc2lvbjogMTMzNDk3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNmZkMTE5NjhjM2U0OTky ZDRlN2FmNjRhNTYwNTFlN2U0NTk0NGJmYy4uYjMxZThiYjE1NTE2OTAxOWQxYTVhY2QxODY4MTQ4 OTk1OGJmZGZhYiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDEyLTExLTA1ICBTZXJn aW8gVmlsbGFyIFNlbmluICA8c3ZpbGxhckBpZ2FsaWEuY29tPgorCisgICAgICAgIFtRdF0gSW1w bGVtZW50IGRlbGV0ZUNvb2tpZSgpIGZvciBwZXJzaXN0ZW50IHN0b3JhZ2UKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwMTI3MgorCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEluZGl2aWR1YWwgY29va2llcyB3 ZXJlIG5ldmVyIHBlcnNpc3RlbnRseSBjbGVhcmVkIGFzIHRoZQorICAgICAgICBkZWxldGVDb29r aWUoKSB2aXJ0dWFsIG1ldGhvZCB3YXMgbm90IGltcGxlbWVudGVkIGZvciBwZXJzaXN0ZW50Cisg ICAgICAgIHN0b3JhZ2UuIFRoYXQncyB3aHkgY29va2llcyB3ZXJlIG9ubHkgZGVsZXRlZCBmb3Ig dGhlIGN1cnJlbnQKKyAgICAgICAgc2Vzc2lvbi4KKworICAgICAgICAqIHBsYXRmb3JtL3F0L0Nv b2tpZUphclF0LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlNoYXJlZENvb2tpZUphclF0OjpkZWxl dGVDb29raWUpOgorICAgICAgICAoV2ViQ29yZSk6IGFkZGVkIGRlbGV0ZUNvb2tpZSgpIGltcGxl bWVudGF0aW9uIGZvciBwZXJzaXN0ZW50CisgICAgICAgIHN0b3JhZ2UuCisgICAgICAgICogcGxh dGZvcm0vcXQvQ29va2llSmFyUXQuaDoKKyAgICAgICAgKFNoYXJlZENvb2tpZUphclF0KToKKwog MjAxMi0xMS0wNSAgU3RlcGhlbiBXaGl0ZSAgPHNlbm9yYmxhbmNvQGNocm9taXVtLm9yZz4KIAog ICAgICAgICBbY2hyb21pdW1dIEJ1aWxkIGZpeCBhZnRlciBodHRwOi8vdHJhYy53ZWJraXQub3Jn L2NoYW5nZXNldC8xMzM0ODguCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9x dC9Db29raWVKYXJRdC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9xdC9Db29raWVKYXJR dC5jcHAKaW5kZXggNjAyMTc0YTE3OGM5MTMzYzZhZWRjY2YzZWU0Zjg2MTNjOGQzNjQ0Ni4uNjg2 NjViMzI4NmVlOTk4ZjQyY2RjZDI1NDVkMTA5MGMyNWY3Nzk1OCAxMDA2NDQKLS0tIGEvU291cmNl L1dlYkNvcmUvcGxhdGZvcm0vcXQvQ29va2llSmFyUXQuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3Jl L3BsYXRmb3JtL3F0L0Nvb2tpZUphclF0LmNwcApAQCAtMTk5LDYgKzE5OSwyMiBAQCB2b2lkIFNo YXJlZENvb2tpZUphclF0OjpnZXRIb3N0bmFtZXNXaXRoQ29va2llcyhIYXNoU2V0PFN0cmluZz4m IGhvc3RuYW1lcykKICAgICAgICAgaG9zdG5hbWVzLmFkZChuZXR3b3JrQ29va2llLmRvbWFpbigp KTsKIH0KIAorYm9vbCBTaGFyZWRDb29raWVKYXJRdDo6ZGVsZXRlQ29va2llKGNvbnN0IFFOZXR3 b3JrQ29va2llJiBjb29raWUpCit7CisgICAgaWYgKCFRTmV0d29ya0Nvb2tpZUphcjo6ZGVsZXRl Q29va2llKGNvb2tpZSkpCisgICAgICAgIHJldHVybiBmYWxzZTsKKworICAgIGlmICghbV9kYXRh YmFzZS5pc09wZW4oKSkKKyAgICAgICAgcmV0dXJuIGZhbHNlOworCisgICAgUVNxbFF1ZXJ5IHNx bFF1ZXJ5KG1fZGF0YWJhc2UpOworICAgIHNxbFF1ZXJ5LnByZXBhcmUoUUxhdGluMVN0cmluZygi REVMRVRFIEZST00gY29va2llcyBXSEVSRSBjb29raWVJZD06Y29va2llSWR2YWx1ZSIpKTsKKyAg ICBzcWxRdWVyeS5iaW5kVmFsdWUoUUxhdGluMVN0cmluZygiOmNvb2tpZUlkdmFsdWUiKSwgY29v a2llLmRvbWFpbigpLmFwcGVuZChRTGF0aW4xU3RyaW5nKGNvb2tpZS5uYW1lKCkpKSk7CisgICAg c3FsUXVlcnkuZXhlYygpOworCisgICAgcmV0dXJuIHRydWU7Cit9CisKIHZvaWQgU2hhcmVkQ29v a2llSmFyUXQ6OmRlbGV0ZUNvb2tpZXNGb3JIb3N0bmFtZShjb25zdCBTdHJpbmcmIGhvc3RuYW1l KQogewogICAgIGlmICghbV9kYXRhYmFzZS5pc09wZW4oKSkKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL3BsYXRmb3JtL3F0L0Nvb2tpZUphclF0LmggYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9y bS9xdC9Db29raWVKYXJRdC5oCmluZGV4IDk2YWM3MWEyMzE5YjE1MGY2OWY5YTY4YjE5MGMwZGQ1 Y2E0NWFjZDcuLjQ2OWZmYTdjODliNTI4ODM5M2ViMDhhMDE1MWRiMDkzYmQ4OGQ1YmMgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3F0L0Nvb2tpZUphclF0LmgKKysrIGIvU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vcXQvQ29va2llSmFyUXQuaApAQCAtMzgsNiArMzgsNyBAQCBw dWJsaWM6CiAgICAgdm9pZCBkZXN0cm95KCk7CiAKICAgICB2b2lkIGdldEhvc3RuYW1lc1dpdGhD b29raWVzKEhhc2hTZXQ8U3RyaW5nPiYpOworICAgIGJvb2wgZGVsZXRlQ29va2llKGNvbnN0IFFO ZXR3b3JrQ29va2llJik7CiAgICAgdm9pZCBkZWxldGVDb29raWVzRm9ySG9zdG5hbWUoY29uc3Qg U3RyaW5nJik7CiAgICAgdm9pZCBkZWxldGVBbGxDb29raWVzKCk7CiAgICAgYm9vbCBzZXRDb29r aWVzRnJvbVVybChjb25zdCBRTGlzdDxRTmV0d29ya0Nvb2tpZT4mLCBjb25zdCBRVXJsJik7Cg==