Bug 156295 - [SOUP] Implement PlatformCookieJar::addCookie
Summary: [SOUP] Implement PlatformCookieJar::addCookie
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on: 156091
Blocks:
  Show dependency treegraph
 
Reported: 2016-04-06 11:11 PDT by Brian Burg
Modified: 2016-04-27 16:31 PDT (History)
9 users (show)

See Also:


Attachments
[SOUP] Implement PlatformCookieJar::addCookie (2.82 KB, patch)
2016-04-07 11:39 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.76 KB, patch)
2016-04-27 15:09 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2016-04-06 11:11:13 PDT
Stub for implementing new CookieJar API.
Comment 1 Michael Catanzaro 2016-04-06 16:54:17 PDT
Thanks for the heads-up on this!
Comment 2 Michael Catanzaro 2016-04-06 19:56:51 PDT
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!
Comment 3 Brian Burg 2016-04-06 20:07:06 PDT
(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 :)
Comment 4 Carlos Garcia Campos 2016-04-06 23:09:25 PDT
(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.
Comment 5 Dan Winship 2016-04-07 05:35:56 PDT
(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()
Comment 6 Michael Catanzaro 2016-04-07 05:39:45 PDT
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.
Comment 7 Brian Burg 2016-04-07 10:23:35 PDT
(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.
Comment 8 Michael Catanzaro 2016-04-07 11:21:06 PDT
(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.
Comment 9 Michael Catanzaro 2016-04-07 11:39:10 PDT
Created attachment 275904 [details]
[SOUP] Implement PlatformCookieJar::addCookie
Comment 10 Carlos Garcia Campos 2016-04-07 22:18:38 PDT
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?
Comment 11 Michael Catanzaro 2016-04-08 07:59:55 PDT
(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. :(
Comment 12 Michael Catanzaro 2016-04-23 18:53:50 PDT
Carlos?
Comment 13 Carlos Garcia Campos 2016-04-24 23:57:42 PDT
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);
Comment 14 Michael Catanzaro 2016-04-27 14:15:21 PDT
(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.
Comment 15 Michael Catanzaro 2016-04-27 15:09:57 PDT
Created attachment 277540 [details]
Patch
Comment 16 WebKit Commit Bot 2016-04-27 16:31:45 PDT
Comment on attachment 277540 [details]
Patch

Clearing flags on attachment: 277540

Committed r200159: <http://trac.webkit.org/changeset/200159>
Comment 17 WebKit Commit Bot 2016-04-27 16:31:51 PDT
All reviewed patches have been landed.  Closing bug.