Created attachment 237972 [details] [review]
Replace old headers

Comment on attachment 237972 [details] [review]
Replace old headers

> static void
>-copy_headers (const char *name, const char *value, SoupMessageHeaders *headers)
>+replace_headers (const char *name, const char *value, SoupMessageHeaders *headers)
> {
>-	soup_message_headers_append (headers, name, value);
>+	soup_message_headers_replace (headers, name, value);
> }

>-	soup_message_headers_foreach (source, (SoupMessageHeadersForeachFunc) copy_headers, destination);
>+	soup_message_headers_foreach (source, (SoupMessageHeadersForeachFunc) replace_headers, destination);

This won't work, because if a header appears multiple times in source (eg, Set-Cookie), only the last appearance of it will end up in destination.

You need to do two passes; first remove all the headers in destination that are also in source, then copy all the headers from source to destination like before.

(In reply to comment #2)
> (From update of attachment 237972 [details] [review])
> > static void
> >-copy_headers (const char *name, const char *value, SoupMessageHeaders *headers)
> >+replace_headers (const char *name, const char *value, SoupMessageHeaders *headers)
> > {
> >-	soup_message_headers_append (headers, name, value);
> >+	soup_message_headers_replace (headers, name, value);
> > }
> 
> >-	soup_message_headers_foreach (source, (SoupMessageHeadersForeachFunc) copy_headers, destination);
> >+	soup_message_headers_foreach (source, (SoupMessageHeadersForeachFunc) replace_headers, destination);
> 
> This won't work, because if a header appears multiple times in source (eg,
> Set-Cookie), only the last appearance of it will end up in destination.

Good point.

Created attachment 238021 [details] [review]
Update cached headers on revalidations

So this allowed me to review a bit how headers were updated and I realized that we weren't updating them after conditional requests, this was only done by revalidations started by libsoup clients. That's why there was a test in cache-test.c that was not working (the test was ok Dan the problem was the cache).

This patch fixes that problem, properly updates the values for the cached headers and provides a new test for it.

I also noticed that two computed values, freshness_lifetime and must_revalidate, were not reset in the case that the server stopped returning them to us. They are now initialized at the beginning of soup_cache_entry_set_freshness()(they'll get the proper value if the server specifies the corresponding headers).

Comment on attachment 238021 [details] [review]
Update cached headers on revalidations

>-	if (msg->status_code == SOUP_STATUS_NOT_MODIFIED) {
>-		SoupCache *cache = (SoupCache *)soup_session_get_feature (session, SOUP_TYPE_CACHE);
>+	cache = (SoupCache *)soup_session_get_feature (session, SOUP_TYPE_CACHE);
>+	soup_cache_update_from_conditional_request (cache, msg);
> 
>+	if (msg->status_code == SOUP_STATUS_NOT_MODIFIED) {

The update_from_conditional_request should happen inside the if body shouldn't it? In particular, if it's an error status code, or a redirection, we shouldn't update the cache with its headers.


everything else looks good, and yay for more tests

(In reply to comment #5)
> (From update of attachment 238021 [details] [review])
> >-	if (msg->status_code == SOUP_STATUS_NOT_MODIFIED) {
> >-		SoupCache *cache = (SoupCache *)soup_session_get_feature (session, SOUP_TYPE_CACHE);
> >+	cache = (SoupCache *)soup_session_get_feature (session, SOUP_TYPE_CACHE);
> >+	soup_cache_update_from_conditional_request (cache, msg);
> > 
> >+	if (msg->status_code == SOUP_STATUS_NOT_MODIFIED) {
> 
> The update_from_conditional_request should happen inside the if body shouldn't
> it? In particular, if it's an error status code, or a redirection, we shouldn't
> update the cache with its headers.

Yep you're right
 
 
> everything else looks good, and yay for more tests

Created attachment 238100 [details] [review]
Update cached headers on revalidations

I left the code before the "== 304" check because we need to update the being_validated flag anyway. The difference is that soup_cache_update_from_conditional_request() won't update the headers for non 304 responses.

Comment on attachment 238100 [details] [review]
Update cached headers on revalidations

great

Committed in master 74f914e2