[Webkit-unassigned] [Bug 83016] [GTK] Add webkit_cookie_manager_set_persistent_storage() to WebKit2 GTK+ API

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jul 11 12:10:00 PDT 2012


https://bugs.webkit.org/show_bug.cgi?id=83016


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #151137|review?                     |review+
               Flag|                            |




--- Comment #13 from Martin Robinson <mrobinson at webkit.org>  2012-07-11 12:09:59 PST ---
(From update of attachment 151137)
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()

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list