Bug 83016 – [GTK] Add webkit_cookie_manager_set_persistent_storage() to WebKit2 GTK+ API

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

To be able to store non-session cookies persistently.

[Carlos Garcia Campos]

Created attachment 135308 [details]
Patch

[Carlos Garcia Campos]

Created attachment 147331 [details]
Updated patch to apply on current git master

The same patch just rebased to apply on current git master.

[WebKit Review Bot]

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

[Gyuyoung Kim]

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

[Carlos Garcia Campos]

Created attachment 147494 [details]
Updated patch, try to fix efl build

[Xan Lopez]

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.

[Carlos Garcia Campos]

(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.

[Gustavo Noronha (kov)]

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

[Carlos Garcia Campos]

(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.

[Dan Winship]

(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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 151137 [details]
Updated patch

Patch rebased to apply on current git master

[Martin Robinson]

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()

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Committed r122425: <http://trac.webkit.org/changeset/122425>