[webkit-reviews] review granted: [Bug 83016] [GTK] Add webkit_cookie_manager_set_persistent_storage() to WebKit2 GTK+ API : [Attachment 151137] Updated patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 83016: [GTK] Add webkit_cookie_manager_set_persistent_storage() to WebKit2
GTK+ API
https://bugs.webkit.org/show_bug.cgi?id=83016

Attachment 151137: Updated patch
https://bugs.webkit.org/attachment.cgi?id=151137&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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()