Bug 83016

Summary: [GTK] Add webkit_cookie_manager_set_persistent_storage() to WebKit2 GTK+ API
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: danw, gns, mrobinson, rakuco, svillar, webkit.review.bot, xan.lopez
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 82598    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Updated patch to apply on current git master
gyuyoung.kim: commit-queue-
Updated patch, try to fix efl build
none
Updated patch mrobinson: review+

Description Carlos Garcia Campos 2012-04-03 04:25:16 PDT
To be able to store non-session cookies persistently.
Comment 1 Carlos Garcia Campos 2012-04-03 04:40:46 PDT
Created attachment 135308 [details]
Patch
Comment 2 Carlos Garcia Campos 2012-06-13 08:53:43 PDT
Created attachment 147331 [details]
Updated patch to apply on current git master

The same patch just rebased to apply on current git master.
Comment 3 WebKit Review Bot 2012-06-13 08:56:49 PDT
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 4 Gyuyoung Kim 2012-06-13 11:35:42 PDT
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
Comment 5 Carlos Garcia Campos 2012-06-13 23:14:30 PDT
Created attachment 147494 [details]
Updated patch, try to fix efl build
Comment 6 Xan Lopez 2012-06-14 01:15:24 PDT
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.
Comment 7 Carlos Garcia Campos 2012-06-14 03:32:56 PDT
(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 8 Gustavo Noronha (kov) 2012-07-05 05:31:11 PDT
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
Comment 9 Carlos Garcia Campos 2012-07-05 08:03:38 PDT
(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.
Comment 10 Dan Winship 2012-07-05 14:42:12 PDT
(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.
Comment 11 Carlos Garcia Campos 2012-07-05 23:20:32 PDT
(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.
Comment 12 Carlos Garcia Campos 2012-07-07 02:50:51 PDT
Created attachment 151137 [details]
Updated patch

Patch rebased to apply on current git master
Comment 13 Martin Robinson 2012-07-11 12:09:59 PDT
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()
Comment 14 Carlos Garcia Campos 2012-07-12 00:49:42 PDT
(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.
Comment 15 Carlos Garcia Campos 2012-07-12 01:07:10 PDT
(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.
Comment 16 Carlos Garcia Campos 2012-07-12 01:13:13 PDT
Committed r122425: <http://trac.webkit.org/changeset/122425>