Bug 156295 – [SOUP] Implement PlatformCookieJar::addCookie

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Blaze Burg]

Stub for implementing new CookieJar API.

[Michael Catanzaro]

Thanks for the heads-up on this!

[Michael Catanzaro]

Hey Brian, since you haven't landed your patch in bug #156091 yet, here's an implementation you can add to it:

void addCookie(const NetworkStorageSession& session, const URL&, const Cookie& cookie)
{
    // Ref is consumed by soup_cookie_jar_add_cookie.
    SoupCookie* soupCookie = soup_cookie_new(cookie.name.utf8().data(), cookie.value.utf8().data(),
        cookie.domain.utf8().data(), cookie.path.utf8().data(), static_cast<int>(cookie.expires));
    soup_cookie_set_http_only(soupCookie, cookie.httpOnly);
    soup_cookie_set_secure(soupCookie, cookie.secure);
    if (cookie.session)
        soup_cookie_set_max_age(soupCookie, -1);

    SoupCookieJar* cookieJar = cookieJarForSession(session);
    soup_cookie_jar_add_cookie(cookieJar, soupCookie);
}

It compiles, and it might even work!

[Blaze Burg]

(In reply to comment #2)
> Hey Brian, since you haven't landed your patch in bug #156091 yet, here's an
> implementation you can add to it:
> 
> void addCookie(const NetworkStorageSession& session, const URL&, const
> Cookie& cookie)
> {
>     // Ref is consumed by soup_cookie_jar_add_cookie.
>     SoupCookie* soupCookie = soup_cookie_new(cookie.name.utf8().data(),
> cookie.value.utf8().data(),
>         cookie.domain.utf8().data(), cookie.path.utf8().data(),
> static_cast<int>(cookie.expires));
>     soup_cookie_set_http_only(soupCookie, cookie.httpOnly);
>     soup_cookie_set_secure(soupCookie, cookie.secure);
>     if (cookie.session)
>         soup_cookie_set_max_age(soupCookie, -1);
> 
>     SoupCookieJar* cookieJar = cookieJarForSession(session);
>     soup_cookie_jar_add_cookie(cookieJar, soupCookie);
> }
> 
> It compiles, and it might even work!

Awesome! Let's land it separately though, so that it can be reviewed by someone familiar with Soup :)

[Carlos Garcia Campos]

(In reply to comment #2)
> Hey Brian, since you haven't landed your patch in bug #156091 yet, here's an
> implementation you can add to it:
> 
> void addCookie(const NetworkStorageSession& session, const URL&, const
> Cookie& cookie)
> {
>     // Ref is consumed by soup_cookie_jar_add_cookie.

This comment is very confusing. I normally use consume for floating references, meaning that the weak ref is converted to a normal ref, but SoupCookieJar is not even ref counted. So, I would not use Ref, and would say adopted instead of consumed.

>     SoupCookie* soupCookie = soup_cookie_new(cookie.name.utf8().data(),
> cookie.value.utf8().data(),
>         cookie.domain.utf8().data(), cookie.path.utf8().data(),
> static_cast<int>(cookie.expires));

I'm not sure this is correct, max_age is the number of seconds since now, but expires is an exact date (stored as an interval in milliseconds, it seems). so, I think we should pass -1 here always, and then use soup_cookie_set_expires() converting the expires interval to a SoupDate.

>     soup_cookie_set_http_only(soupCookie, cookie.httpOnly);
>     soup_cookie_set_secure(soupCookie, cookie.secure);
>     if (cookie.session)
>         soup_cookie_set_max_age(soupCookie, -1);
> 
>     SoupCookieJar* cookieJar = cookieJarForSession(session);
>     soup_cookie_jar_add_cookie(cookieJar, soupCookie);

This could be a single line, no need to keep the local variable. Actually, I think even the cookie transfer could be more obvious if we use a helper function to create a SoupCookie out of a Cookie and then simply do:

soup_cookie_jar_add_cookie(cookieJarForSession(session), toSoupCookie(cookie));

> }
> 
> It compiles, and it might even work!

I'm not sure it works . . . Please, check what expires actually means in both soup and WebCore.

[Dan Winship]

(In reply to comment #4)
> I'm not sure this is correct, max_age is the number of seconds since now,
> but expires is an exact date (stored as an interval in milliseconds, it
> seems). so, I think we should pass -1 here always, and then use
> soup_cookie_set_expires() converting the expires interval to a SoupDate.

...unless...

> >     if (cookie.session)
> >         soup_cookie_set_max_age(soupCookie, -1);

...it's a session cookie. You only want to call one of set_expires() and set_max_age()

[Michael Catanzaro]

Thanks Dan.

(In reply to comment #4)
> This comment is very confusing. I normally use consume for floating
> references, meaning that the weak ref is converted to a normal ref, but
> SoupCookieJar is not even ref counted. So, I would not use Ref, and would
> say adopted instead of consumed.

OK.

> expires is an exact date (stored as an interval in milliseconds, it
> seems).

No way to tell from the struct. :(

> so, I think we should pass -1 here always, and then use
> soup_cookie_set_expires() converting the expires interval to a SoupDate.

Good idea.

> This could be a single line, no need to keep the local variable.

OK.

> Actually, I
> think even the cookie transfer could be more obvious if we use a helper
> function to create a SoupCookie out of a Cookie and then simply do:
> 
> soup_cookie_jar_add_cookie(cookieJarForSession(session),
> toSoupCookie(cookie));

OK.

[Blaze Burg]

(In reply to comment #6)
> Thanks Dan.
> 
> (In reply to comment #4)
> > This comment is very confusing. I normally use consume for floating
> > references, meaning that the weak ref is converted to a normal ref, but
> > SoupCookieJar is not even ref counted. So, I would not use Ref, and would
> > say adopted instead of consumed.
> 
> OK.
> 
> > expires is an exact date (stored as an interval in milliseconds, it
> > seems).
> 
> No way to tell from the struct. :(

It is milliseconds since the UNIX epoch. I added a comment in another patch to be landed, since I had this question as well.

> 
> > so, I think we should pass -1 here always, and then use
> > soup_cookie_set_expires() converting the expires interval to a SoupDate.
> 
> Good idea.
> 
> > This could be a single line, no need to keep the local variable.
> 
> OK.
> 
> > Actually, I
> > think even the cookie transfer could be more obvious if we use a helper
> > function to create a SoupCookie out of a Cookie and then simply do:
> > 
> > soup_cookie_jar_add_cookie(cookieJarForSession(session),
> > toSoupCookie(cookie));
> 
> OK.

[Michael Catanzaro]

(In reply to comment #7)
> It is milliseconds since the UNIX epoch. I added a comment in another patch
> to be landed, since I had this question as well.

Great. Another reminder not to assume....

Anyway, this was a bit tricky to handle properly; I was particularly disappointed to see it's impossible to portably convert from a double to a time_t, but fortunately WTF has better time functions than the standard library, so it wasn't needed in the end.

[Michael Catanzaro]

Created attachment 275904 [details]
[SOUP] Implement PlatformCookieJar::addCookie

[Carlos Garcia Campos]

Comment on attachment 275904 [details]
[SOUP] Implement PlatformCookieJar::addCookie

View in context: https://bugs.webkit.org/attachment.cgi?id=275904&action=review

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:205
> -void addCookie(const NetworkStorageSession&, const URL&, const Cookie&)
> +static SoupDate* msToSoupDate(double ms)
>  {
> -    // FIXME: implement this command. <https://webkit.org/b/156295>
> -    notImplemented();
> +    int year = msToYear(ms);
> +    int dayOfYear = dayInYear(ms, year);
> +    bool leapYear = isLeapYear(year);
> +    int day = dayInMonthFromDayInYear(dayOfYear, leapYear);
> +    int month = monthFromDayInYear(dayOfYear, leapYear);
> +    int hour = msToHours(ms);
> +    int minute = msToMinutes(ms);
> +    int second = static_cast<int>(ms / 1000) % 60;
> +    return soup_date_new(year, month, day, hour, minute, second);
> +}

I don't understand this. If the WebCore cookie expires is the number of milliseconds elapsed since the epoch, can't we just convert from milliseconds to seconds and use soup_date_new_from_time_t?

[Michael Catanzaro]

(In reply to comment #10)
> I don't understand this. If the WebCore cookie expires is the number of
> milliseconds elapsed since the epoch, can't we just convert from
> milliseconds to seconds and use soup_date_new_from_time_t?

Kinda, but I don't know what systems that might break on. Unfortunately there seems to be no portable way to convert from a double (or int) to a time_t. I wanted to assume that the time_t stores time in seconds since the epoch and do static_cast<std::time_t>(cookie.expires / 1000), but turns out C++ does not guarantee that; very frustrating. :(

[Michael Catanzaro]

Carlos?

[Carlos Garcia Campos]

Comment on attachment 275904 [details]
[SOUP] Implement PlatformCookieJar::addCookie

View in context: https://bugs.webkit.org/attachment.cgi?id=275904&action=review

EWS is red, please submit a new patch for EWS before landing this

> Source/WebCore/platform/network/soup/CookieJarSoup.cpp:204
> +    int year = msToYear(ms);
> +    int dayOfYear = dayInYear(ms, year);
> +    bool leapYear = isLeapYear(year);
> +    int day = dayInMonthFromDayInYear(dayOfYear, leapYear);
> +    int month = monthFromDayInYear(dayOfYear, leapYear);
> +    int hour = msToHours(ms);
> +    int minute = msToMinutes(ms);
> +    int second = static_cast<int>(ms / 1000) % 60;
> +    return soup_date_new(year, month, day, hour, minute, second);

I still don't understand this, but if you say there's no better way, it's fine with me. I wonder if it would look simpler without using all the local variables, something like:

return soup_date_new(year, monthFromDayInYear(dayOfYear, leapYear), dayInMonthFromDayInYear(dayOfYear, leapYear), msToHours(ms), msToMinutes(ms), static_cast<int>(ms / 1000) % 60);

[Michael Catanzaro]

(In reply to comment #13)
> I still don't understand this

C++ doesn't guarantee how time_t is used to store time. I think maybe POSIX guarantees it's seconds since the epoch, but I'm not sure, and I think this code should be portable.

[Michael Catanzaro]

Created attachment 277540 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 277540 [details]
Patch

Clearing flags on attachment: 277540

Committed r200159: <http://trac.webkit.org/changeset/200159>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.