To be able to store non-session cookies persistently.
Created attachment 135308 [details] Patch
Created attachment 147331 [details] Updated patch to apply on current git master The same patch just rebased to apply on current git master.
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment on attachment 147331 [details] Updated patch to apply on current git master Attachment 147331 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12956062
Created attachment 147494 [details] Updated patch, try to fix efl build
Comment on attachment 147494 [details] Updated patch, try to fix efl build All this looks sensible to me :) It's unfortunate to copy&paste the sqlite jar with changes, if I could choose I'd try to take the code as-is so that it's trivial to cherry-pick changes from upstream, but if you guys (Martin and Carlos I guess) think it's best to do it like this I won't complain.
(In reply to comment #6) > (From update of attachment 147494 [details]) > All this looks sensible to me :) > > It's unfortunate to copy&paste the sqlite jar with changes, if I could choose I'd try to take the code as-is so that it's trivial to cherry-pick changes from upstream, but if you guys (Martin and Carlos I guess) think it's best to do it like this I won't complain. I agree, but that code hardly ever changes and it's not a lot of code. Also adapting the code to wk allows us to use the SQLite classes from WebCore.
Comment on attachment 147494 [details] Updated patch, try to fix efl build View in context: https://bugs.webkit.org/attachment.cgi?id=147494&action=review It looks mostly fine to me, but I'm missing the rationale for having our own sqlite implementation, especially since we're using the text one from soup. Could you add an explanation to the ChangeLog? > Source/WebKit2/ChangeLog:35 > + new cookie jar for the given filename and sotrage type and add it sotrage -> storage
(In reply to comment #8) > (From update of attachment 147494 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147494&action=review > > It looks mostly fine to me, but I'm missing the rationale for having our own sqlite implementation, especially since we're using the text one from soup. Could you add an explanation to the ChangeLog? Sure. It's because the libsoup implementation is part of libsoup-gnome, for whatever reason. Dan told me that moving it to libsoup would be an abi break and he suggested to just copy the implementation in WebKit, since it's not a lot of code and it hardly ever changes. > > Source/WebKit2/ChangeLog:35 > > + new cookie jar for the given filename and sotrage type and add it > > sotrage -> storage Oops.
(In reply to comment #9) > Sure. It's because the libsoup implementation is part of libsoup-gnome, > for whatever reason. Dan told me that moving it to libsoup would be an abi > break and he suggested to just copy the implementation in WebKit, since > it's not a lot of code and it hardly ever changes. *Moving* it would be an ABI break, but we could copy-and-rename it, and maybe I will at some point. It's currently in libsoup-gnome because I didn't want to add more library dependencies to libsoup-2.4.so.
(In reply to comment #10) > (In reply to comment #9) > > Sure. It's because the libsoup implementation is part of libsoup-gnome, > > for whatever reason. Dan told me that moving it to libsoup would be an abi > > break and he suggested to just copy the implementation in WebKit, since > > it's not a lot of code and it hardly ever changes. > > *Moving* it would be an ABI break, but we could copy-and-rename it, and maybe I will at some point. It's currently in libsoup-gnome because I didn't want to add more library dependencies to libsoup-2.4.so. If we eventually can use the soup implementation we can simply remove the webkit code and use the soup one.
Created attachment 151137 [details] Updated patch Patch rebased to apply on current git master
Comment on attachment 151137 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=151137&action=review Looks good, but I have a few minor comments. > Source/WebKit2/Shared/soup/SoupCookiePersistentStorageType.h:35 > +enum { > + SoupCookiePersistentStorageText = 0, > + SoupCookiePersistentStorageSQLite = 1 > +}; > +typedef unsigned SoupCookiePersistentStorageType; Since these will never be combined it makes sense to do this: enum SoupCookiePersistentStorageType { SoupCookiePersistentStorageText, SoupCookiePersistentStorageSQLite, }; Note that you don't need to specify the enum values since the defaults are correct. > Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.h:49 > + * file in the new Mozilla format. Perhaps it's better to say "current" rather than "new." I'm not sure how new the format is nowadays. > Source/WebKit2/WebProcess/Cookies/soup/WebKitSoupCookieJarSqlite.cpp:46 > + COLUMN_ID, > + COLUMN_NAME, > + COLUMN_VALUE, > + COLUMN_HOST, > + COLUMN_PATH, > + COLUMN_EXPIRY, > + COLUMN_LAST_ACCESS, > + COLUMN_SECURE, > + COLUMN_HTTP_ONLY The enum names here don't follow WebKit coding style. Do you mind fixing before landing? > Source/WebKit2/WebProcess/Cookies/soup/WebKitSoupCookieJarSqlite.cpp:100 > + time_t now = time(0); Perhaps it would be better to use currentTime() here to avoid a dependency on POSIX? You could do time_t now = floorf(currentTime()); > Source/WebKit2/WebProcess/Cookies/soup/WebKitSoupCookieJarSqlite.cpp:141 > + query.bindInt(5, (gulong)soup_date_to_time_t(cookie->expires)); It looks like you might be losing some precision because you are binding a long as an integer. Perhaps do this: query.bindInt64(5, static_cast<int64_t>(soup_date_to_time_t(cookie->expires))); > Source/WebKit2/WebProcess/Cookies/soup/WebKitSoupCookieJarSqlite.cpp:177 > + webkitSoupCookieJarSqliteDeleteCookie(sqliteJar, oldCookie); > + webkitSoupCookieJarSqliteInsertCookie(sqliteJar, newCookie); Perhaps it would be good to use a transaction for this operation so that if one of these steps fails, then nothing happens. It would looks something like this: SQLiteTransaction transaction(sqliteJar->priv->database, false); if (!webkitSoupCookieJarSqliteDeleteCookie(sqliteJar, oldCookie)) return; if (!webkitSoupCookieJarSqliteInsertCookie(sqliteJar, newCookie)) return; transaction.commit()
(In reply to comment #13) > (From update of attachment 151137 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151137&action=review > > Looks good, but I have a few minor comments. Thanks for reviewing. > > Source/WebKit2/Shared/soup/SoupCookiePersistentStorageType.h:35 > > +enum { > > + SoupCookiePersistentStorageText = 0, > > + SoupCookiePersistentStorageSQLite = 1 > > +}; > > +typedef unsigned SoupCookiePersistentStorageType; > > Since these will never be combined it makes sense to do this: > > enum SoupCookiePersistentStorageType { > SoupCookiePersistentStorageText, > SoupCookiePersistentStorageSQLite, > }; > > Note that you don't need to specify the enum values since the defaults are correct. Enums that are going to be serialized in IPC messages are declared that way because generated code uses unsigned. Using enum SoupCookiePersistentStorageType would fail to build the message receiver generated code. > > Source/WebKit2/UIProcess/API/gtk/WebKitCookieManager.h:49 > > + * file in the new Mozilla format. > > Perhaps it's better to say "current" rather than "new." I'm not sure how new the format is nowadays. Ok. > > Source/WebKit2/WebProcess/Cookies/soup/WebKitSoupCookieJarSqlite.cpp:46 > > + COLUMN_ID, > > + COLUMN_NAME, > > + COLUMN_VALUE, > > + COLUMN_HOST, > > + COLUMN_PATH, > > + COLUMN_EXPIRY, > > + COLUMN_LAST_ACCESS, > > + COLUMN_SECURE, > > + COLUMN_HTTP_ONLY > > The enum names here don't follow WebKit coding style. Do you mind fixing before landing? Sure. > > Source/WebKit2/WebProcess/Cookies/soup/WebKitSoupCookieJarSqlite.cpp:100 > > + time_t now = time(0); > > Perhaps it would be better to use currentTime() here to avoid a dependency on POSIX? > > You could do time_t now = floorf(currentTime()); Ok. > > Source/WebKit2/WebProcess/Cookies/soup/WebKitSoupCookieJarSqlite.cpp:141 > > + query.bindInt(5, (gulong)soup_date_to_time_t(cookie->expires)); > > It looks like you might be losing some precision because you are binding a long as an integer. Perhaps do this: > > query.bindInt64(5, static_cast<int64_t>(soup_date_to_time_t(cookie->expires))); Ok. > > Source/WebKit2/WebProcess/Cookies/soup/WebKitSoupCookieJarSqlite.cpp:177 > > + webkitSoupCookieJarSqliteDeleteCookie(sqliteJar, oldCookie); > > + webkitSoupCookieJarSqliteInsertCookie(sqliteJar, newCookie); > > Perhaps it would be good to use a transaction for this operation so that if one of these steps fails, then nothing happens. It would looks something like this: > > SQLiteTransaction transaction(sqliteJar->priv->database, false); > if (!webkitSoupCookieJarSqliteDeleteCookie(sqliteJar, oldCookie)) > return; > if (!webkitSoupCookieJarSqliteInsertCookie(sqliteJar, newCookie)) > return; > transaction.commit() Sure, I'll use a transaction.
(In reply to comment #14) > Enums that are going to be serialized in IPC messages are declared that way because generated code uses unsigned. Using enum SoupCookiePersistentStorageType would fail to build the message receiver generated code. Well, we can also fix it by using uint32_t instead of the enum name in the web process receiver handler.
Committed r122425: <http://trac.webkit.org/changeset/122425>