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

[bugzilla-daemon at webkit.org]

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





--- Comment #14 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-07-12 00:49:42 PST ---
(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.

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