Bug 44261 – [GTK] Add HTTP caching support

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Sergio Villar Senin]

This bug comes from https://bugzilla.gnome.org/show_bug.cgi?id=523100.

Summarizing, a cache for libsoup was implemented using the SoupURILoader stuff that is under development. Dan Winship considered that the SoupURILoader stuff is not ready for trunk https://bugzilla.gnome.org/show_bug.cgi?id=523100#c27. That's why he suggested to move all the cache stuff to webkitgtk while the libsoup stuff does not land upstream. Since now it's purely a webkitgtk+ issue it's better to manage it here.

[Sergio Villar Senin]

Created attachment 64849 [details]
HTTP cache for WebKitGtk+

Basically this patch is made of:

   * changes to ResourceHandleSoup already reviewed in bug 40222
   * libsoup's URI loader code discussed here https://bugzilla.gnome.org/show_bug.cgi?id=557777
   * caching support https://bugzilla.gnome.org/show_bug.cgi?id=523100

Once applied to current trunk all the tests work fine except these two:
   * http/test/history/back-to-post.php: recently changed by http://trac.webkit.org/changeset/65340. Need to review what happens
   * fast/encoding/percent-escaping.html: as stated here https://bugzilla.gnome.org/show_bug.cgi?id=523100#c28 it needs some changes in libsoup to work properly, as libsoup incorrectly escapes file URL's with '%'

[Martin Robinson]

Comment on attachment 64849 [details]
HTTP cache for WebKitGtk+

This patch is huge! I'd really like to see it split up into at least
two parts.

1. The code drop from Soup, which should probably be separated somehow from
the rest of our source, so we can treat it specially (it will go away eventually, right?).
2. The bits that actually implement the cache in WebCore/WebKit.

That being said, I have a few comments on what I presume to be the second part.

> +        GRefPtr<WebKitSoupRequest> m_req;
> +	GRefPtr<WebKitSoupRequester> m_requester;

Please do not abbreviate member names. This could be m_request or, heck,
even m_soupRequest. Someone new will look at the code and know exactly what it is.
The indentation is a little funky here as well.

>  #include "SharedBuffer.h"
>  #include "TextEncoding.h"
>  
> +#include "webkitsouprequesthttp.h"
> +
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <gio/gio.h>

Just remove all spaces in the list of includes per the style guidelines. There is
no need to separate them by include style.

> +    if (m_req) {
> +        g_object_set_data(G_OBJECT(m_req.get()), "webkit-resource", 0);
> +        m_req.clear();

There's no chance that this may be the last reference to m_req here?

> +    GOwnPtr<char> uri(soup_uri_to_string(soup_message_get_uri(msg), false));
> +    String location = String(uri.get());

Even though URL are generally URL-encoded, I don't know if we can rely on that
here. You should use String::fromUTF8.

> +#define BUFFER_SIZE 8192

Why not move this to the top and make the name more descriptive?
INPUT_STREAM_BUFFER_SIZE or something like that.

> +    GInputStream* in = webkitSoupRequestSend(d->m_req.get(), 0, &error.outPtr());

This should be a full-blown variable name like inputStream.

> -    d->m_idleHandler = g_timeout_add(0, parseDataUrl, handle);
> +    d->m_idleHandler = g_idle_add(parseDataUrl, handle);

Why did you switch this to an idle? I think Gustavo is trying to move
away from that because they are often starved. Maybe ping him about this.

>      g_object_set(session,
>                   SOUP_SESSION_MAX_CONNS, MAX_CONNECTIONS,
>                   SOUP_SESSION_MAX_CONNS_PER_HOST, MAX_CONNECTIONS_PER_HOST,
> -                 NULL);
> +                 0);

The style bot should not complain about this, because the missing NULL causes
a compiler warning. Please just use NULL here.

> +
> +    if (d->m_cancellable) {
> +        g_object_unref(d->m_cancellable);
> +        d->m_cancellable = 0;
> +    }

m_cancellable can probably be a GRefPtr in another patch.

> +    if (d->m_inputStream) {
> +        g_object_set_data(G_OBJECT(d->m_inputStream), "webkit-resource", 0);
> +        g_object_unref(d->m_inputStream);
> +        d->m_inputStream = 0;
> +    }

Ditto for m_inputStream.

> +
> +    if (d->m_buffer) {
> +        g_free(d->m_buffer);
> +        d->m_buffer = 0;
> +    }

Ditto for m_buffer, except GOwnPtr.

> +    GInputStream* in = webkitSoupRequestSendFinish(d->m_req.get(), res, &error.outPtr());

inputStream again. :)

> +
> +    if (error) {
> +        if (d->m_msg && SOUP_STATUS_IS_TRANSPORT_ERROR(d->m_msg->status_code)) {
> +            SoupURI* uri = webkitSoupRequestGetUri(d->m_req.get());
> +            GOwnPtr<char> uriStr(soup_uri_to_string(uri, false));

No need to abbreviate the variable name here. uriString is fine.

>  
> -    // Used to set the authentication dialog toplevel; may be NULL
> +    // Used to set the authentication dialog toplevel; may be 0

I think "null" here is clearer and probably won't upset the style gods.

> +        // We have to convert it to UTF-16 due to limitations in KURL
> +        GOwnPtr<gunichar2> unicodeStr(g_utf8_to_utf16(d->m_buffer, bytesRead, 0, 0, 0));
> +        client->didReceiveData(handle.get(), reinterpret_cast<const char*>(unicodeStr.get()), g_utf8_strlen(d->m_buffer, bytesRead) * sizeof(UChar), 0);

Okay, again this is kind of a nit, but unicodeStr is not entirely precise. It should
probably be utf16String.  g_utf8_strlen(d->m_buffer, bytesRead) * sizeof(UChar) seems
unsafe against encoding errors and is a little clunky. I think it's better to do something
like this:

    long wordsWritten;
    GOwnPtr<gunichar2> utf16String(g_utf8_to_utf16(d->m_buffer, bytesRead, 0, &wordsWritten, 0));
    client->didReceiveData(handle.get(), reinterpret_cast<const char*>(utf16String()), wordsWritten * sizeof(UChar), 0);

> +    g_object_set(soupMessage, SOUP_MESSAGE_METHOD, httpMethod().utf8().data(), 0);

NULL here instead of 0.

[Sergio Villar Senin]

(In reply to comment #2)
> (From update of attachment 64849 [details])
> This patch is huge! I'd really like to see it split up into at least
> two parts.
> 
> 1. The code drop from Soup, which should probably be separated somehow from
> the rest of our source, so we can treat it specially (it will go away eventually, right?).
> 2. The bits that actually implement the cache in WebCore/WebKit.

Thx for reviewing Martin.

Well basically the patch consists of:
   1) changes in ResourceHandleSoup/ResourceHandleInternal: basically adaptations to work with the URILoader stuff
   2) WebCore/platform/network/soup/webkitsoup*: all the stuff moved from webkit
   3) WebKit/Wekit/gtk/webkit/webkitsoupcache.[c|h]: the webkitgtk+ API for clients

Now some comments:

> > +    if (m_req) {
> > +        g_object_set_data(G_OBJECT(m_req.get()), "webkit-resource", 0);
> > +        m_req.clear();
> 
> There's no chance that this may be the last reference to m_req here?

What's the problem then?
 
> > +    GOwnPtr<char> uri(soup_uri_to_string(soup_message_get_uri(msg), false));
> > +    String location = String(uri.get());
> 
> Even though URL are generally URL-encoded, I don't know if we can rely on that
> here. You should use String::fromUTF8.

soup_uri_to_string always returns an URL encoded URL, do you think we still need to use ::fromUTF8 ?

> > -    d->m_idleHandler = g_timeout_add(0, parseDataUrl, handle);
> > +    d->m_idleHandler = g_idle_add(parseDataUrl, handle);
> 
> Why did you switch this to an idle? I think Gustavo is trying to move
> away from that because they are often starved. Maybe ping him about this.

As stated here https://bugs.webkit.org/show_bug.cgi?id=40222#c11 there was 1 test that fails with timeout but not with idle. After a long time debugging it, it seems that it's just a matter of timing. All the signals and events are emitted properly but somehow in a different order depending on the function used.

[Martin Robinson]

> > > +    if (m_req) {
> > > +        g_object_set_data(G_OBJECT(m_req.get()), "webkit-resource", 0);
> > > +        m_req.clear();
> > 
> > There's no chance that this may be the last reference to m_req here?
> What's the problem then?

Sorry, misread this one! Looks fine actually.

> > here. You should use String::fromUTF8.
> soup_uri_to_string always returns an URL encoded URL, do you think we still need to use ::fromUTF8 ?

Yes. Since GLib strings are UTF-8, you should still use the proper String constructor. For instance, do you know what Soup's behavior is with IDNs?

> As stated here https://bugs.webkit.org/show_bug.cgi?id=40222#c11
> there was 1 test that fails with timeout but not with idle. After
> a long time debugging it, it seems that it's just a matter of timing.
> All the signals and events are emitted properly but somehow in a
> different order depending on the function used.

Interesting. I'm convinced, but I'd still like to get Gustavo's opinion
on it. He may have some further insights.

[Sergio Villar Senin]

(In reply to comment #4)

> > > here. You should use String::fromUTF8.
> > soup_uri_to_string always returns an URL encoded URL, do you think we still need to use ::fromUTF8 ?
> 
> Yes. Since GLib strings are UTF-8, you should still use the proper String constructor. For instance, do you know what Soup's behavior is with IDNs?

OK will fix that. Regarding IDNs there is no specific code about them in libsoup AFAIK
 
> > As stated here https://bugs.webkit.org/show_bug.cgi?id=40222#c11
> > there was 1 test that fails with timeout but not with idle. After
> > a long time debugging it, it seems that it's just a matter of timing.
> > All the signals and events are emitted properly but somehow in a
> > different order depending on the function used.
> 
> Interesting. I'm convinced, but I'd still like to get Gustavo's opinion
> on it. He may have some further insights.

I don't want to convince you :). Just sharing my conclussions after some investigation.

[Sergio Villar Senin]

Gently added kov and xan to the Cc

[Sergio Villar Senin]

Created attachment 65135 [details]
SoupURILoader stuff as part of webkitgtk+ core network

This patch adds the objects used by SoupURILoader branch of libsoup. This code is required for the HTTP cache implementation. WebKit code style has been used when moving code to WebKit.

[Sergio Villar Senin]

Created attachment 65136 [details]
Changes to WebKitGtk+ core network stuff to use the new API from the imported SoupURILoader code

See also https://bugs.webkit.org/show_bug.cgi?id=40222

[Sergio Villar Senin]

Created attachment 65137 [details]
New public API to enable clients to use the HTTP cache

Public API for the cache

[Sergio Villar Senin]

Patch splitted as suggested by Martin

[Gustavo Noronha (kov)]

Great, I'll take a look at these later today, is there any specific reason why you did not mark the patches for review, though?

[Sergio Villar Senin]

(In reply to comment #11)
> Great, I'll take a look at these later today, is there any specific reason why you did not mark the patches for review, though?

Cool! if you can review them

Regarding the r?, I always forget :/

[WebKit Review Bot]

Attachment 65135 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Last 3072 characters of output:
ty/naming] [4]
WebCore/platform/network/soup/webkitsouprequestdata.cpp:178:  request_data_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/network/soup/webkitsouprequestdata.cpp:180:  object_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/network/soup/webkitsouprequestdata.cpp:181:  request_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/network/soup/webkitsouprequestfile.cpp:47:  webkit_soup_request_file_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/network/soup/webkitsouprequestfile.cpp:54:  webkit_soup_request_file_finalize is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/network/soup/webkitsouprequestfile.cpp:268:  webkit_soup_request_file_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/network/soup/webkitsouprequest.h:57:  parent_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/network/soup/webkitsouprequest.h:65:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/network/soup/webkitsouprequest.h:66:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/network/soup/webkitsouprequest.h:67:  Tab found; better to use spaces  [whitespace/tab] [1]
WebCore/platform/network/soup/webkitsouprequest.h:76:  webkit_soup_request_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/network/soup/webkitsouprequester.cpp:39:  webkit_soup_requester_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/network/soup/webkitsouprequester.cpp:57:  webkit_soup_requester_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/network/soup/webkitsouprequester.cpp:57:  requester_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/network/soup/webkitsouprequester.cpp:59:  object_class is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/network/soup/webkitsouprequester.cpp:67:  init_request_types is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/network/soup/webkitsouprequester.cpp:93:  webkit_soup_requester_request is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/network/soup/webkitsouprequester.cpp:109:  webkit_soup_requester_request_uri is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 934 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 65136 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/network/ResourceHandleInternal.h:51:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/network/ResourceHandleInternal.h:210:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 65137 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Last 3072 characters of output:
kitsoupcache.cpp:107:  One line control clauses should not use braces.  [whitespace/braces] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:108:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/gtk/webkit/webkitsoupcache.cpp:112:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:114:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/gtk/webkit/webkitsoupcache.cpp:114:  Declaration has space between type name and * in GObjectClass *gobject_class  [whitespace/declaration] [3]
WebKit/gtk/webkit/webkitsoupcache.cpp:116:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/gtk/webkit/webkitsoupcache.cpp:118:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/gtk/webkit/webkitsoupcache.cpp:118:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:133:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:136:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:141:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:157:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:159:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/gtk/webkit/webkitsoupcache.cpp:159:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:170:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:172:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/gtk/webkit/webkitsoupcache.cpp:172:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:176:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:178:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/gtk/webkit/webkitsoupcache.cpp:178:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:182:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:184:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/gtk/webkit/webkitsoupcache.cpp:184:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:188:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:189:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/gtk/webkit/webkitsoupcache.cpp:191:  Tab found; better to use spaces  [whitespace/tab] [1]
WebKit/gtk/webkit/webkitsoupcache.cpp:191:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:195:  Extra space before ( in function call  [whitespace/parens] [4]
WebKit/gtk/webkit/webkitsoupcache.cpp:197:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 111 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 65135 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3757605

[WebKit Review Bot]

Attachment 65136 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3725601

[WebKit Review Bot]

Attachment 65137 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3751564

[Gustavo Noronha (kov)]

Comment on attachment 65135 [details]
SoupURILoader stuff as part of webkitgtk+ core network

I have three main concerns:

1. I believe we should import the code without style changes into a subdirectory; I know you have probably been told to change it into webkit's style, but I think it would make backporting fixes into soup's version harder than needed - and it's not something we plan to keep in the tree for long, anyway
2. do we really need to import the TPL one-file library? I guess the answer is yes (though we could use GVariant?), but then we probably need to figure out a way of isolating it from the rest of the build as the build failure suggests it'll be problematic =(
3. There's API mixed in there (like  webkit_soup_http_input_stream_new/webkitSoupHttpInputStreamNew) - I think I would prefer having this code go to WebKit/soup/; the API should be kept in soup/GTK+-style even if we decide to move the rest (which is one more argument in favor of keeping the style)

[Martin Robinson]

I'm also in favor of keeping this in the original style, in the interest of  pushing fixes back into your soup branch. I'd love to see this code isolated into Webkit/gtk/soup.

svillar, can you comment on the performance characteristics of the code which uses tpl? It may make more sense to use GVariant or sqlite, depending on what it does.

[Xan Lopez]

(In reply to comment #20)
> I'm also in favor of keeping this in the original style, in the interest of  pushing fixes back into your soup branch. I'd love to see this code isolated into Webkit/gtk/soup.

If we are going to do this then let's open a bug right away to make the style bots ignore that bunch of code, otherwise this is going to be annoying. That way we'll also have something to point people to when they come asking why is there a ball of code not following the style inside the source tree (hello Mark! :)).

> 
> svillar, can you comment on the performance characteristics of the code which uses tpl? It may make more sense to use GVariant or sqlite, depending on what it does.

I agree with the sentiment. Did you talk with Dan and decided that using TPL made sense here? As suggested GVariant seems to be made exactly for this kind of task, so it would make sense to reuse it here.

[Sergio Villar Senin]

Using Xan's comments for the answer the previous reviews

(In reply to comment #21)
> (In reply to comment #20)
> > I'm also in favor of keeping this in the original style, in the interest of  pushing fixes back into your soup branch. I'd love to see this code isolated into Webkit/gtk/soup.
> 
> If we are going to do this then let's open a bug right away to make the style bots ignore that bunch of code, otherwise this is going to be annoying. That way we'll also have something to point people to when they come asking why is there a ball of code not following the style inside the source tree (hello Mark! :)).

Not really sure how to make bots ignore that, could you help on that?

Provided you all are in favour of keeping the style do you think it still makes sense to add a "webkit_" prefix to all the symbols?

> > 
> > svillar, can you comment on the performance characteristics of the code which uses tpl? It may make more sense to use GVariant or sqlite, depending on what it does.
> 
> I agree with the sentiment. Did you talk with Dan and decided that using TPL made sense here? As suggested GVariant seems to be made exactly for this kind of task, so it would make sense to reuse it here.

Regarding why I chose tpl, there are some reasons:
   * I had previously used it for a personal project
   * The API is quite compact, easy and understandable
   * No external dependencies needed
   * It's supposed to be fast

Most of them are likely valid for the GVariant stuff, I guess I was mostly biased by the first one. I don't know about performance using GVariant, but if it's acceptable I agree it makes sense to use it.

[Gustavo Noronha (kov)]

To make the bots ignore the path you need to add the path, + information about which rules are allowed to be broken on that path to WebKit/WebKitTools/Scripts/webkitpy/style/checker.py. If we agree on the path I can make the patch. WebCore/platform/network/soup/cache for the internal files and WebKit/gtk/soup for the API? Or should we go with WebKit/gtk/soup/ for everything, making this gtk-only, essentially?

As for the webkit_ prefix, I think it makes sense to add the prefix to public API, so it won't clash with the proper soup symbols in the future.

TPL/GVariant, I'd like to go with GVariant. TPL does bring _1_ additional dependency, and that is the actual TPL files, which you then have to keep up-to-date yourself. GVariant on the other hand is a standard part of our platform, and will be maintained by the glib maintainers =).

[Lukasz Slachciak]

(In reply to comment #23)
> If we agree on the path I can make the patch. WebCore/platform/network/soup/cache for the internal files and WebKit/gtk/soup for the API? Or should we go with WebKit/gtk/soup/ for everything, making this gtk-only, essentially?

Please don't make it gtk-only  putting files into WebKit/gtk/soup/. Also EFL port is using soup as network backend, and this can be nice feature for EFL also.

[Gustavo Noronha (kov)]

> Please don't make it gtk-only  putting files into WebKit/gtk/soup/. Also EFL port is using soup as network backend, and this can be nice feature for EFL also.

That is why I initially suggested WebKit/soup/. I think for simplicity's sake we should use WebKit/soup for all files.

[Martin Robinson]

(In reply to comment #25)
> > Please don't make it gtk-only  putting files into WebKit/gtk/soup/. Also EFL port is using soup as network backend, and this can be nice feature for EFL also.
> That is why I initially suggested WebKit/soup/. I think for simplicity's sake we should use WebKit/soup for all files.

I can see why this is a good idea now. Here's another vote for WebKit/soup.

[Sergio Villar Senin]

(In reply to comment #23)
> To make the bots ignore the path you need to add the path, + information about which rules are allowed to be broken on that path to WebKit/WebKitTools/Scripts/webkitpy/style/checker.py. If we agree on the path I can make the patch. WebCore/platform/network/soup/cache for the internal files and WebKit/gtk/soup for the API? Or should we go with WebKit/gtk/soup/ for everything, making this gtk-only, essentially?

I like the former. Basically because most of stuff is only needed by the ResourceHandleSoup. That's why having something under WebCore and the public API under WebKit makes a lot of sense to me. Regarding everything under WebKit/soup, I thought that being WebCore objects they should be under WebCore/ but I could be obviously wrong.
 
> As for the webkit_ prefix, I think it makes sense to add the prefix to public API, so it won't clash with the proper soup symbols in the future.

Agree.
 
> TPL/GVariant, I'd like to go with GVariant. TPL does bring _1_ additional dependency, and that is the actual TPL files, which you then have to keep up-to-date yourself. GVariant on the other hand is a standard part of our platform, and will be maintained by the glib maintainers =).

Yep, I'm moving the serialization/persistence stuff to GVariant and removing the tpl dependency.

[Sergio Villar Senin]

Some extra information regarding the GVariant/tpl stuff. I coded a couple of serialization/deserialization routines both using tpl and GVariant in order to evaluate the performance. For the test the data that I wanted to serialize was an array of structures with a uint32, a boolean and 2 strings (the amount of data in the cache is actually higher but both systems claim to be O(n)).

Then I registered the time for serializing/deserializing arrays of different sizes. The first size I chose was 5000 elements. Basically because if we take a typical cache size of 50MB and an average file size of 10k then that means 5000 cache entries. The other test I did was with 10 times that size (50000 elements), for quite extreme cases.

For the case of tpl I implemented it both using fixed-length arrays and variable-length arrays.

These are the numbers I got (average values after 10 runs):

Serialization
*************

      | tpl (fixed) | tpl     | gvariant |
      ------------------------------------
5000  | 0.02456     | 0.02606 | 0.1098   |
50000 | 0.16678     | 0.23968 | 1.0829   |


Deserialization
***************

      | tpl (fixed) | tpl     | gvariant |
      ------------------------------------
5000  | 0.02456     | 0.02606 | 0.1098   |
50000 | 0.16678     | 0.23968 | 1.0829   |

The most important numbers are the deserialization ones because that happens when the applications start while the serialization is typically performed on application exit.

It's pretty clear that tpl (even using the not-so-fast variable length arrays is way faster (~85%) than gvariant. But if we consider that 5000 is an accurate figure then the difference in time is less than 1/10 sec.

Maybe a figure in between the two I chose fits better with actual numbers. In that case application start will wait during 1/2 second for cache to start. That can be partially avoided if we perform the deserialization in a different thread keeping the cache in a not_available state. Deserialization will most likely finish before the user could even start typing a URL (home page load could be affected by the concurrent deserialization BTW).

[Gustavo Noronha (kov)]

Created attachment 65567 [details]
patch for check-webkit-style to ignore our new stuff

[WebKit Review Bot]

Attachment 65567 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/Scripts/webkitpy/style/checker.py:218:  whitespace before ']'  [pep8/E202] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 65567 [details]
patch for check-webkit-style to ignore our new stuff

The style error seems like a false positive, so this looks good to me.

[Gustavo Noronha (kov)]

Comment on attachment 65567 [details]
patch for check-webkit-style to ignore our new stuff

Landed as r66120. Turns out that the python style rules require a , after the last item of a list (the original code had it), so I added it, and the style checker is happy with itself again =)

[Sergio Villar Senin]

Created attachment 65900 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

* Added files from libsoup with SoupURILoader stuff
* New API that adds WebKitSoupCache

[Sergio Villar Senin]

Created attachment 65901 [details]
Changes to WebKitGtk+ core network stuff to use the new API from the imported SoupURILoader code

WebKitGtk+ using SoupURILoader

[WebKit Review Bot]

Attachment 65900 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3850142

[WebKit Review Bot]

Attachment 65901 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3877142

[Gustavo Noronha (kov)]

Comment on attachment 65900 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

This will still need a changelog entry, and the build breakage implies we need to either depend on a newer glib or factor this code out if building with an oldish glib. There's a check in place to decide whether to use GSettings. I'd recommend adding a check specifically for GVariant, to decide whether to build this new code or not. I won't bother reviewing the actual code, except for the public APIs. The identation in soup-cache.c is completely broken (no spaces at all in various places, 4-space indentation in others), I think you may want to run indent in all soup cache files to have them match the libsoup style.

[Gustavo Noronha (kov)]

Actually, I think trying to make this buildable with older glibs is going to pull us into hell, so I would go with making us depend on the earliest GVariant-providing glib.

[Sergio Villar Senin]

(In reply to comment #38)
> Actually, I think trying to make this buildable with older glibs is going to pull us into hell, so I would go with making us depend on the earliest GVariant-providing glib.

Agree. I'll add changelog's and update the glib version

[Sergio Villar Senin]

Created attachment 66189 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

Now with ChangeLogs. Files imported from libsoup have now the proper libsoup coding style

[Sergio Villar Senin]

Created attachment 66190 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

g_timeout_add is now back in place after replacing g_idle_add. BTW the test that was working only with g_idle_add is now working with the timeout_add too.

[Sergio Villar Senin]

Created attachment 66191 [details]
Added fast/encoding/percent-escaping.html to the list of Skipped

That test does not work with the new SoupURILoader stuff.

See https://bugzilla.gnome.org/show_bug.cgi?id=523100#c28 for further details. A small patch to libsoup will fix it.

[WebKit Review Bot]

Attachment 66190 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3921019

[Sergio Villar Senin]

Comment on attachment 65901 [details]
Changes to WebKitGtk+ core network stuff to use the new API from the imported SoupURILoader code

Marked as obsolete

[Sergio Villar Senin]

Created attachment 66650 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

A few changes since the last version of the patch:

* glib dependency is now 2.24
* removed several data from cache that is not really needed (as it was saved anyway in the http headers)

[WebKit Review Bot]

Attachment 66650 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3914232

[Martin Robinson]

Comment on attachment 66190 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

This patch is looking good. I'm excited for this to land.

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

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:138
> +        m_soupRequest.clear();
m_soupRequest.clear(); is unecessary here, I believe. The destructor will take care of decrementing the smart pointer reference count. If this is needed otherwise there should really be a comment explaining why.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:196
> +    String location = String(uri.get());
Again, I believe this should be ::fromUTF8(...).

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:305
> +#define BUFFER_SIZE 8192
If this is a constant, why do we need to store m_bufferSize? Let's just get rid of that member if possible. Again, I think this should be called READ_BUFFER_SIZE and be at the top of the file too.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:689
> +                            sendRequestCallback, 0);
This newline seems unecessary. I usually split the line when it goes over 120 characters.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:846
> +        client->didReceiveData(handle.get(), reinterpret_cast<const char*>(unicodeStr.get()), g_utf8_strlen(d->m_buffer, bytesRead) * sizeof(UChar), 0);
Would it be possible to simply use WTFString here?

String data = String::fromUTF8(d->m_buffer, bytesRead);
client->didReceiveData(handle.get(), reinterpret_cast<const char*>(data.characters()), data.length() * sizeof(UChar), 0);

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:867
> +    GOwnPtr<char> urlStr;
Please use CString here, if possible. It was made just for this purpose.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:901
> +
Please remove the newline.

[Sergio Villar Senin]

Created attachment 66692 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

New version with fixes to the issues detected by Martin

[WebKit Review Bot]

Attachment 66692 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3938283

[Martin Robinson]

Comment on attachment 66650 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

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

My major concern with this patch is exposing a public API that will not be compatible with some further Soup version. Is it possible to keep this API private for now?

> GNUmakefile.am:325
>  
Please keep this list in alphabetical order.

> WebCore/GNUmakefile.am:2575
>  	WebCore/plugins/gtk/PluginDataGtk.cpp \
Please keep the source lists in alphabetical order. I've been ordered them by filtering them through sort.

[Gustavo Noronha (kov)]

(In reply to comment #50)
> (From update of attachment 66650 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66650&action=prettypatch
> 
> My major concern with this patch is exposing a public API that will not be compatible with some further Soup version. Is it possible to keep this API private for now?

The applications that uses WebKitGTK+ will need that API to setup the caching, so we can't make it private. That concern is why we are adding the WebKit prefix to the API bits, we should be fine.

[Sergio Villar Senin]

(In reply to comment #50)
> (From update of attachment 66650 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66650&action=prettypatch
> 
> My major concern with this patch is exposing a public API that will not be compatible with some further Soup version. Is it possible to keep this API private for now?

As Gustavo said it's a must for clients.

> > GNUmakefile.am:325
> >  
> Please keep this list in alphabetical order.

It's currently not properly sorted, do you think it makes sense to sort them in this patch?

[Sergio Villar Senin]

Comment on attachment 66692 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

Removing the review request as I found a bug that could lead to a crash

[Sergio Villar Senin]

Created attachment 67254 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

Fixed minor issues with sorting reported by Martin.

Apart from that several changes in cache *implementation* (no changes in pure WebKitGtk+ code or API)
   * Fixed crash caused by multiple free
   * Fixed memory leaks
   * LRU is now properly updated
   * Cache resources properly freed on finalize
   * Decreased cache's threads priority to priorize libsoup's and webkit's
   * GVariant does not like non-UTF8 strings. Validate them before serializing.

I won't be available next week so don't expect comments from my side till next Saturday at least :-)

[Sergio Villar Senin]

Comment on attachment 66692 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

Added missing r=?

[WebKit Review Bot]

Attachment 67254 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3897376

[Martin Robinson]

Comment on attachment 66191 [details]
Added fast/encoding/percent-escaping.html to the list of Skipped

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

> LayoutTests/platform/gtk/Skipped:1138
> +fast/encoding/percent-escaping.html

I think I'd rather see a work-around in our code than a regression. We should chat about this sometime.

[Michal Pakula vel Rutka]

As in WebKit-EFL we want to use your cache implementation could you tell me if it is already finished or are there any features that need to be implemented?

[Sergio Villar Senin]

(In reply to comment #57)
> (From update of attachment 66191 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=66191&action=prettypatch
> 
> > LayoutTests/platform/gtk/Skipped:1138
> > +fast/encoding/percent-escaping.html
> 
> I think I'd rather see a work-around in our code than a regression. We should chat about this sometime.

Unfortunatelly there is no work-around possible from our side without breaking all the new URI loader way of getting resources because the problem is how libsoup handles file: URLs. The fix should be added to libsoup, the patch is really small (see https://bugzilla.gnome.org/show_bug.cgi?id=523100#c28 for more details). I can talk with danw about that before pushing this upstream.

[Sergio Villar Senin]

(In reply to comment #58)
> As in WebKit-EFL we want to use your cache implementation could you tell me if it is already finished or are there any features that need to be implemented?

It's finished but under review process, hopefully will land soon.

[Sergio Villar Senin]

(In reply to comment #59)
> (In reply to comment #57)
> > (From update of attachment 66191 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=66191&action=prettypatch
> > 
> > > LayoutTests/platform/gtk/Skipped:1138
> > > +fast/encoding/percent-escaping.html
> > 
> > I think I'd rather see a work-around in our code than a regression. We should chat about this sometime.
> 
> Unfortunatelly there is no work-around possible from our side without breaking all the new URI loader way of getting resources because the problem is how libsoup handles file: URLs. The fix should be added to libsoup, the patch is really small (see https://bugzilla.gnome.org/show_bug.cgi?id=523100#c28 for more details). I can talk with danw about that before pushing this upstream.

Actually the problem is not with the percent signs but with the blank spaces in the filename. Soup will always try to convert them into "%20" because otherwise is won't be a valid URL. So even if you encode them before passing the url to libsoup you'll be generating a different filename and thus the test will fail. I think the best I can do is to have a little chat about this issue with danw and try to get the libsoup patch upstream.

[Martin Robinson]

(In reply to comment #61)
> Actually the problem is not with the percent signs but with the blank spaces in the filename. Soup will always try to convert them into "%20" because otherwise is won't be a valid URL. So even if you encode them before passing the url to libsoup you'll be generating a different filename and thus the test will fail. I think the best I can do is to have a little chat about this issue with danw and try to get the libsoup patch upstream.

So does this mean filenames with spaces will be inaccessible? That definitely seems like a blocker for merging this.

[Sergio Villar Senin]

Comment on attachment 66191 [details]
Added fast/encoding/percent-escaping.html to the list of Skipped

Removing as the test is working with the newest changes to be uploaded

[Sergio Villar Senin]

Created attachment 68349 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

New version of the patch. Added fix for the test that started to fail

[Sergio Villar Senin]

Created attachment 68350 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

New version of the patch. Rebased against latest changes to the master

[Sergio Villar Senin]

Comment on attachment 66692 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

Marking this as obsolete, forgot to do it before

[WebKit Review Bot]

Attachment 68349 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3996101

[WebKit Review Bot]

Attachment 68350 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4092036

[Sergio Villar Senin]

Comment on attachment 68349 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

Still one issue remaining. Removing the r? temporarily

[Sergio Villar Senin]

Created attachment 68505 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

This is the right version of the (hopefully) final patch.

I managed to fix the issue with the percent-escaping.html test that was failing and the good news is that the fix is part of the imported soup uri loader code. That way no special code is needed by current WebKitGtk+ code, and if it eventually lands on libsoup, every client will take advantage of that.

[WebKit Review Bot]

Attachment 68505 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4070098

[Martin Robinson]

Comment on attachment 68350 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

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

Looks good! I think we should merge this soon. I just have a few style nits.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:128
> +static void cleanupSoupRequestOperation(ResourceHandle* handle, bool isDestroying);
> +static void sendRequestCallback(GObject* source, GAsyncResult* res, gpointer);
> +static void readCallback(GObject* source, GAsyncResult* res, gpointer);
> +static void closeCallback(GObject* source, GAsyncResult* res, gpointer);

I think all of these parameter names are unnecessary except for isDestroying.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:381
> +    handle->ref();

This really needs a comment describing where the balancing deref() is.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:386
> +    g_input_stream_read_async(d->m_inputStream, d->m_buffer, READ_BUFFER_SIZE,
> +                              G_PRIORITY_DEFAULT, d->m_cancellable,
> +                              readCallback, (isBase64) ? 0 : GINT_TO_POINTER(1));

Perhaps smoosh this down to two lines in the interest of vertical space?

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:403
> +

This blank line seems accidental.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:501
> +            ResourceError resourceError(g_quark_to_string(SOUP_HTTP_ERROR),
> +                                        d->m_msg->status_code,
> +                                        uriStr.get(),
> +                                        String::fromUTF8(d->m_msg->reason_phrase));

Once again, smoosh this down to two lines.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:513
> +            ResourceError resourceError(g_quark_to_string(G_IO_ERROR),
> +                                        error->code,
> +                                        uriStr.get(),
> +                                        error ? String::fromUTF8(error->message) : String());

Ditto.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:574
> +    g_input_stream_read_async(d->m_inputStream, d->m_buffer, READ_BUFFER_SIZE,
> +                              G_PRIORITY_DEFAULT, d->m_cancellable,
> +                              readCallback, 0);

Ditto.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:824
>          ResourceError resourceError(g_quark_to_string(G_IO_ERROR),
>                                      error->code,
> -                                    uri,
> +                                    uriStr.get(),
>                                      error ? String::fromUTF8(error->message) : String());

Ditto.

> WebCore/platform/network/soup/ResourceRequest.h:70
> +        void updateSoupMessage(SoupMessage* soupMessage) const;

Please remove the parameter name.

[Sergio Villar Senin]

Created attachment 68664 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

New version with Martin's style fix requests

[WebKit Review Bot]

Attachment 68664 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4068122

[Dan Winship]

Created attachment 68717 [details]
patches to Sergio's patches

OK, the reason SoupRequester needed a special case to get file URIs working right was because there was already an incorrect special case for file URIs in ResourceHandleSoup::startGio() that was messing the URL up before it got to SoupRequester. So this patch removes both special cases (I didn't bother attaching it in the form of a git commit since it actually needs to be split between both of Sergio's existing patches.).

Additionally, to deal with the problem the startGio() hack was originally attempting to work around, we need to make soup_uri_decode() accept badly-%-encoded URIs rather than returning NULL (which is something we'd already discussed in gnomebug 523100 anyway). There is now a patch for that in https://bugzilla.gnome.org/show_bug.cgi?id=630540 but it's not yet committed because we're in hard code freeze for 2.32.

[Martin Robinson]

(In reply to comment #75)
> OK, the reason SoupRequester needed a special case to get file URIs working right was because there was already an incorrect special case for file URIs in ResourceHandleSoup::startGio() that was messing the URL up before it got to SoupRequester. So this patch removes both special cases (I didn't bother attaching it in the form of a git commit since it actually needs to be split between both of Sergio's existing patches.).

Thank you! Sergio will just need to merge these fixes into his patches before the final r+.

> Additionally, to deal with the problem the startGio() hack was originally attempting to work around, we need to make soup_uri_decode() accept badly-%-encoded URIs rather than returning NULL (which is something we'd already discussed in gnomebug 523100 anyway). There is now a patch for that in https://bugzilla.gnome.org/show_bug.cgi?id=630540 but it's not yet committed because we're in hard code freeze for 2.32.

So, if I understand correctly, to avoid a regression we'll either need to:
1. Re-implement this logic in WebKit GTK+ for older soup versions.
2. Wait to land this until you release a new version of soup with this fix and then depend on that.

[Dan Winship]

I haven't looked at the code in detail, but I think you probably want to rename all of the soup-prefixed stuff to be webkit-prefixed instead so that it doesn't start causing compile and run-time problems when the code lands in libsoup. (Even if it's not visible outside of libwebkit, you still have to worry about GType's type database; bad things will happen if libsoup and webkit both try to register a type with the name "SoupRequest".)

I have not looked at the tpl-using code or the GVariant-using alternative, but it does seem like using GVariant instead of bringing in tpl would be nice... if there are performance issues presumably GVariant can still be optimized a bunch?

[Dan Winship]

(In reply to comment #76)
> So, if I understand correctly, to avoid a regression we'll either need to:
> 1. Re-implement this logic in WebKit GTK+ for older soup versions.
> 2. Wait to land this until you release a new version of soup with this fix and then depend on that.

Yes. For #1, you could make soup-request-file.c use KURL.h's decodeURLEscapeSequences() instead of soup_uri_decode(), assuming it's not too obnoxious to call that from C...

For #2, it will be in libsoup 2.32.1. Are you planning to try to get this in for GNOME 2.32.0 still? I'm pretty sure it's too late to bump external dependency versions at this point...

[Sergio Villar Senin]

(In reply to comment #75)
> Created an attachment (id=68717) [details]
> patches to Sergio's patches
> 
> OK, the reason SoupRequester needed a special case to get file URIs working right was because there was already an incorrect special case for file URIs in ResourceHandleSoup::startGio() that was messing the URL up before it got to SoupRequester. So this patch removes both special cases (I didn't bother attaching it in the form of a git commit since it actually needs to be split between both of Sergio's existing patches.).
> 

Well that's not really the reason. The special case in souprequester needs the special case in startGio. One cannot work without the other. With both of them it works fine.

> Additionally, to deal with the problem the startGio() hack was originally attempting to work around, we need to make soup_uri_decode() accept badly-%-encoded URIs rather than returning NULL (which is something we'd already discussed in gnomebug 523100 anyway). There is now a patch for that in https://bugzilla.gnome.org/show_bug.cgi?id=630540 but it's not yet committed because we're in hard code freeze for 2.32.

That's the real issue IMO. That's why my first thought was to modify the decoding in soup-uri.c although your changes seem indeed better than adding a special case for file: URL's. I also considered removing that fixup parametter but I was not sure about breaking current URI parsing in soup. Note that glib APIs do return NULL when they cannot decode URIs

One last question Dan, did you run the tests with both patches?

[Xan Lopez]

Comment on attachment 68664 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

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

> WebCore/platform/network/ResourceHandleInternal.h:51
> +#include <GRefPtr.h>

Nitpick, but requester.h already includes request.h

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:196
> +    String location = String::fromUTF8(uri.get());

Can't we just commit this bit now?

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:305
>  // parseDataUrl() is taken from the CURL http backend.

I guess you want to remove this comment now, since the function is wildly different by now (or at least say 'originally' or something).

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:387
>  

GINT_TO_POINTER(!isBase64) ? Could fail?

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:453
> +

Any reason not to use platform ptrs here too?

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:459
> +

And here?

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:515
> +

These two ifs are almost identical, I think they can be trivially merged.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:544
> +    d->m_buffer = static_cast<char*>(g_malloc(READ_BUFFER_SIZE));

Am I the only one worried that we malloc 8k for each outgoing HTTP request? or am I missing something here. Is this just making something explicit that was implicit before or we adding an extra copy to the whole process?

[Sergio Villar Senin]

Comment on attachment 68664 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

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

>> WebCore/platform/network/ResourceHandleInternal.h:51
>> +#include <GRefPtr.h>
> 
> Nitpick, but requester.h already includes request.h

Indeed. Will remove it.

>> WebCore/platform/network/soup/ResourceHandleSoup.cpp:196
>> +    String location = String::fromUTF8(uri.get());
> 
> Can't we just commit this bit now?

Yes. Martin asked for the change, that's why it's included in this patch

>> WebCore/platform/network/soup/ResourceHandleSoup.cpp:305
>>  // parseDataUrl() is taken from the CURL http backend.
> 
> I guess you want to remove this comment now, since the function is wildly different by now (or at least say 'originally' or something).

True

>> WebCore/platform/network/soup/ResourceHandleSoup.cpp:387
>>  
> 
> GINT_TO_POINTER(!isBase64) ? Could fail?

I just wanted to be ultra-explicit but what you propose would be the same.

>> WebCore/platform/network/soup/ResourceHandleSoup.cpp:453
>> +
> 
> Any reason not to use platform ptrs here too?

They are not platform ptrs right now. Not sure if that change should be included in this patch. I thought it was a general rule not to include changes not directly related to the issue the patch tries to fix.

>> WebCore/platform/network/soup/ResourceHandleSoup.cpp:515
>> +
> 
> These two ifs are almost identical, I think they can be trivially merged.

Indeed they can

>> WebCore/platform/network/soup/ResourceHandleSoup.cpp:544
>> +    d->m_buffer = static_cast<char*>(g_malloc(READ_BUFFER_SIZE));
> 
> Am I the only one worried that we malloc 8k for each outgoing HTTP request? or am I missing something here. Is this just making something explicit that was implicit before or we adding an extra copy to the whole process?

:m really good point here. I think you're right and we're adding an extra copy for http requests when reading from the input stream. With the old code all the didReceiveData calls were using buffers provided by libsoup without any extra copy. I guess we need something better then...

[Dan Winship]

(In reply to comment #80)
> > WebCore/platform/network/soup/ResourceHandleSoup.cpp:544
> > +    d->m_buffer = static_cast<char*>(g_malloc(READ_BUFFER_SIZE));
> 
> Am I the only one worried that we malloc 8k for each outgoing HTTP request? or am I missing something here. Is this just making something explicit that was implicit before or we adding an extra copy to the whole process?

At the moment, given the kludginess of SoupHTTPInputStream, this is an additional malloc and an additional copy. When things are redone so that libsoup uses streams internally, then the malloc here will be replacing a malloc that is currently in soup-message-io.c, and libsoup will be reading directly from the network into your buffers.

At any rate, you can copy the buffer 10 times and it will still be unnoticeable compared to waiting for the network and rendering HTML, so, it's not that awful...

[Xan Lopez]

(In reply to comment #82)
> (In reply to comment #80)
> > > WebCore/platform/network/soup/ResourceHandleSoup.cpp:544
> > > +    d->m_buffer = static_cast<char*>(g_malloc(READ_BUFFER_SIZE));
> > 
> > Am I the only one worried that we malloc 8k for each outgoing HTTP request? or am I missing something here. Is this just making something explicit that was implicit before or we adding an extra copy to the whole process?
> 
> At the moment, given the kludginess of SoupHTTPInputStream, this is an additional malloc and an additional copy. When things are redone so that libsoup uses streams internally, then the malloc here will be replacing a malloc that is currently in soup-message-io.c, and libsoup will be reading directly from the network into your buffers.
> 
> At any rate, you can copy the buffer 10 times and it will still be unnoticeable compared to waiting for the network and rendering HTML, so, it's not that awful...

WebKit in general has a rule of never allowing performance regressions, even if new features are being added. I think it makes sense to enforce this rule in our port too.

If this is going to go away in the future and there's really no reasonably easy to avoid it I think we could make an exception, but it would still be unfortunate. FWIW talking with Holger some weeks ago he told me in QtWebKit the copying of data in the network stack is noticeable performance-wise in general, although in their case the number of copies was larger (something between 3 or 5 IIRC).

[Dan Winship]

(In reply to comment #83)
> WebKit in general has a rule of never allowing performance regressions, even if new features are being added. I think it makes sense to enforce this rule in our port too.

But presumably that means "measured performance regressions", not "theoretical performance regressions". And one would hope that caching is going to increase performance by much more than an extra copy is going to decrease it... :)

> If this is going to go away in the future and there's really no reasonably easy to avoid it

Duh, now that you mention it, there's an API designed exactly for avoiding this extra copy... soup_message_set_chunk_allocator() (added for the gstreamer guys when they were porting their http stream to use libsoup). SoupHTTPInputStream could be modified to use this to get libsoup to read directly into the buffers provided by webkit.

[Sergio Villar Senin]

(In reply to comment #84)
> (In reply to comment #83)
> > WebKit in general has a rule of never allowing performance regressions, even if new features are being added. I think it makes sense to enforce this rule in our port too.
> 
> But presumably that means "measured performance regressions", not "theoretical performance regressions". And one would hope that caching is going to increase performance by much more than an extra copy is going to decrease it... :)
> 
> > If this is going to go away in the future and there's really no reasonably easy to avoid it
> 
> Duh, now that you mention it, there's an API designed exactly for avoiding this extra copy... soup_message_set_chunk_allocator() (added for the gstreamer guys when they were porting their http stream to use libsoup). SoupHTTPInputStream could be modified to use this to get libsoup to read directly into the buffers provided by webkit.

Unfortunately I don't think that will fix anything. First of all because I think the chunk_allocator thing it's not properly implemented in libsoup, because the SoupBuffer (and its data pointer) that comes with the got_chunk signal is not the same than the one returned by the allocator. That's because the decoding routines create a different buffer for each decoder.

Secondly, even if we fix the problem stated above, we'd still have to copy the data returned by the decoders to the caller's buffer (soup_coding_apply_into wouldn't fix anything as input and output buffers must not overlap)

[Sergio Villar Senin]

Created attachment 69205 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

Includes Dan's patch that reverts the file: URIs hack

[Sergio Villar Senin]

Created attachment 69209 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

New version with Dan's patch applied.

It also includes Xan's requested changes:
   * Moved m_inputStream and m_cancellable to platform ptrs
   * Refactored the 2 errors
   * GINT_TO_POINTER(!isBase64)
   * Removed duplicated include file

Apart from that I removed some glib2.21 #ifdef code as the required version for this patch is 2.24

[WebKit Review Bot]

Attachment 69205 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4201011

[WebKit Review Bot]

Attachment 69209 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4225012

[Sergio Villar Senin]

(In reply to comment #77)
> I haven't looked at the code in detail, but I think you probably want to rename all of the soup-prefixed stuff to be webkit-prefixed instead so that it doesn't start causing compile and run-time problems when the code lands in libsoup. (Even if it's not visible outside of libwebkit, you still have to worry about GType's type database; bad things will happen if libsoup and webkit both try to register a type with the name "SoupRequest".)

I think it was kov the one that said that it'd be better not to change them all in order to easen the migration to libsoup.

Anyway I agree that it could be an important issue. I have a patch that changes all those names so if everybody think it's interesting I can upload new versions of the patches.

[Sergio Villar Senin]

Created attachment 69355 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

Renamed all the new Soup* symbols to WebKitSoup*

[Sergio Villar Senin]

Comment on attachment 69205 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

Forgot to mark this as obsolete

[Sergio Villar Senin]

Created attachment 69356 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

Renamed new Soup* symbols to WebKitSoup*

Also refactored some duplicated code in parseDataUrl (nice catch Martin)

[WebKit Review Bot]

Attachment 69355 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4167023

[Sergio Villar Senin]

As discussed on IRC, the fix for soup_uri_decode won't land on libsoup till 2.32.1 (mid November). As we want to release this first I'll copy&pastet the soup-uri.c code (uri_decoded_copy function) into the imported SoupURILoader stuff.

We'll remove that code once the 2.32.1 is released.

[WebKit Review Bot]

Attachment 69356 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4219041

[Sergio Villar Senin]

Created attachment 69384 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

New version of the patch:
   * A couple of indentation issues fixed
   * Do not need to decode base64 encoded data URLs
   * Imported uri_decoded_copy() with the fix that will land in libsoup 2.32.1. Use it instead of soup_uri_decode for file: and data: URLs

[Martin Robinson]

Comment on attachment 69356 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

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

This patch is so very close, but I've found some issues with memory leaks. When constructing a platformRefPtr, you should use adoptPlatformRef, unless you want the reference count to increase (the default constructor calls g_object_ref_sink).

> WebCore/platform/network/ResourceHandleInternal.h:118
> +            , m_soupRequest(0)
> +            , m_requester(0)
>              , m_inputStream(0)
>              , m_cancellable(0)

Since these are now PlatformRefPtrs you don't need to initialize them. They are automatically 0.

> WebCore/platform/network/ResourceHandleInternal.h:140
> +            m_requester = webkit_soup_requester_new();

This should use adoptPlatformRef, see below for an extended note about this.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:357
> +    d->m_soupRequest = webkit_soup_requester_request(d->m_requester.get(), handle->firstRequest().url().string().utf8().data(), session, &error.outPtr());

Ditto.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:365
> +    if (error)
> +        return false;
> +
> +    d->m_inputStream = in;

Careful here. From the soup code it looks like it is the callers responsibility to unref the value that webkit_soup_request_send returns. Constructing a PlatformRefPtr without using adoptPlatformRef has the effect of calling g_object_ref another time on the GInputStream. 

d->m_inputStream = adoptPlatformRef(webkit_soup_request_send(d->m_soupRequest.get(), 0, &error.outPtr()));
if (error) {
    d->m_inputStream = 0;
    return false;
}

and dispense with the intermediate 'in' variable entirely.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:373
> +    d->m_cancellable = g_cancellable_new();

This should be d->m_cancellable = adoptPlatformRef(g_cancellable_new()); as well.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:436
> +static void cleanupSoupRequestOperation(ResourceHandle* handle, bool isDestroying = false)
> +{
> +    ResourceHandleInternal* d = handle->getInternal();
> +
> +    if (d->m_soupRequest) {
> +        g_object_set_data(G_OBJECT(d->m_soupRequest.get()), "webkit-resource", 0);
> +        d->m_soupRequest.clear();
> +    }

We still need to clear the m_cancellable here to ensure that the unref happens, right?

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:519
> +    d->m_inputStream = in;

The same goes for this section. After this the reference count on d->m_inputStream will be 2.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:564
> +    d->m_soupRequest = webkit_soup_requester_request(d->m_requester.get(), url.string().utf8().data(), session, &error.outPtr());

Ditto.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:652
> +    d->m_cancellable = g_cancellable_new();

Same issue here.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:833
>      // GIO doesn't know how to handle refs and queries, so remove them

Shouldn't this comment be changed to reflect the fact that we're using soup now?

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:839
> +    urlStr = url.string().utf8().data();

This might as well be:
CString urlStr = url.string().utf8().data();

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:842
> +    d->m_soupRequest = webkit_soup_requester_request(d->m_requester.get(), urlStr.data(), session, &error.outPtr());

I think this should use adoptPlatformRef as well.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:851
>      d->m_cancellable = g_cancellable_new();

Same issue here.

[WebKit Review Bot]

Attachment 69384 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4141040

[Sergio Villar Senin]

Created attachment 69436 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

New version. Includes:
   * Added adoptPlatformRef where needed (thx Martin)
   * Renamed m_msg to m_soupMsg and converted it to PlatformRefPtr

[WebKit Review Bot]

Attachment 69436 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/network/soup/ResourceHandleSoup.cpp:481:  Declaration has space between type name and * in SoupMessage *soupMsg  [whitespace/declaration] [3]
WebCore/platform/network/soup/ResourceHandleSoup.cpp:578:  Declaration has space between type name and * in SoupMessage *soupMsg  [whitespace/declaration] [3]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sergio Villar Senin]

Created attachment 69438 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

Sorry for the previous one. Fixed the style issues.

[WebKit Review Bot]

Attachment 69436 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4179041

[WebKit Review Bot]

Attachment 69438 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4229039

[Martin Robinson]

Comment on attachment 69438 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

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

General comment: I'm not as picky about this as others, but some reviewers would have you end all comments with a period. That's something to consider for this patch.

> WebCore/platform/network/ResourceHandleInternal.h:189
> +        PlatformRefPtr<SoupMessage> m_soupMsg;

This should probably be m_soupMessage, as the guideline is to avoid abbreviations in new code.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:-132
> -    if (m_msg) {
> -        g_object_unref(m_msg);
> -        m_msg = 0;
> -    }

Nice change.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:145
> -    if (d->m_msg)
> -        g_signal_handlers_disconnect_matched(d->m_msg, G_SIGNAL_MATCH_DATA,
> +    if (d->m_soupMsg)
> +        g_signal_handlers_disconnect_matched(d->m_soupMsg.get(), G_SIGNAL_MATCH_DATA,

I could be wrong about this, but shouldn't this be in cleanupSoupRequestOperation? Otherwise, we might get responses to messages for a no-longer-valid SoupMessage.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:364
> +    d->m_buffer = static_cast<char*>(g_malloc(READ_BUFFER_SIZE));

My intuition is that this allocation should either be done with the GLib slice allocator or with fastAlloc/fastFree. Perhaps someone else can chime in here.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:445
> +    if (d->m_cancellable)
> +        d->m_cancellable.clear();
> +
> +    if (d->m_soupMsg)
> +        d->m_soupMsg.clear();

No need to check these before clearing them. Just clear them.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:506
> +            // WebCore might have cancelled the job in the while
> +            if (d->m_cancelled)
> +                return;
> +
> +            if (soupMsg->response_body->data)
> +                client->didReceiveData(handle.get(), soupMsg->response_body->data, soupMsg->response_body->length, true);

In the case that the request was cancelled this should be calling cleanupSoupRequestOperation(). So you might change 
    if (soupMsg->response_body->data)
to
    if (!d->m_cancelled && soupMsg->response_body->data)
and remove the early return.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:513
> +        if (d->m_cancelled || !client) {
> +          cleanupSoupRequestOperation(handle.get());
> +          return;
> +        }

The tab width here should be 4.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:525
> +    d->m_buffer = static_cast<char*>(g_malloc(READ_BUFFER_SIZE));

Same issue with the allocation.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:578
> +    SoupMessage* soupMsg = d->m_soupMsg.get();

Avoid abbreviations.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:582
> +    request.updateSoupMessage(soupMsg);
> +
> +    if (!d->m_soupMsg)
>          return false;

It doesn't look like you want to call updateSoupMessage with a null message, so you should probably move this check up to line 578.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:775
> +static void readCallback(GObject* source, GAsyncResult* res, gpointer data)

Please change res to asyncResult.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:826
> +    g_input_stream_read_async(d->m_inputStream.get(), d->m_buffer, READ_BUFFER_SIZE,
> +                              G_PRIORITY_DEFAULT, d->m_cancellable.get(),
> +                              readCallback, data);

This can be two lines.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:845
> +    CString urlStr = url.string().utf8().data();

This can just be: CString urlStr = url.string().utf8(); I think I suggested .data() originally, sorry.

[Sergio Villar Senin]

Created attachment 69611 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

Some more changes to the patch from Martin's comments:
   * renamed soupMsg to soupMessage
   * disconnect signals in cleanupSoupRequestOperation
   * replaced g_malloc by g_slice_alloc
   * some style fixes
   * moved early returns to better locations

[WebKit Review Bot]

Attachment 69611 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4200074

[Martin Robinson]

Comment on attachment 69611 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

Great. I'm going to wait for a nod from Kov or Xan to r+ the API changes.

[Gustavo Noronha (kov)]

Comment on attachment 69384 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

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

> WebCore/platform/network/soup/cache/webkit/soup-cache.c:1595
> +	filename = g_build_filename (priv->cache_dir, WEBKIT_SOUP_CACHE_FILE, NULL);
> +	if (!g_file_get_contents (filename, &contents, &length, NULL)) {
> +		g_free (filename);
> +		return;
> +	}

There are several indentation inconsistencies like this one (2 spaces indentation for the first level, but 4 spaces in second-level indentation) in the soup files. If you could fix those when landing, that'd be great. *nod* otherwise! (I'll leave the r+ for mrobinson)

[Xan Lopez]

Just a reminder, but EWS needs to be updated before this can land.

[Sergio Villar Senin]

(In reply to comment #109)
> (From update of attachment 69384 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69384&action=review
> 
> > WebCore/platform/network/soup/cache/webkit/soup-cache.c:1595
> > +	filename = g_build_filename (priv->cache_dir, WEBKIT_SOUP_CACHE_FILE, NULL);
> > +	if (!g_file_get_contents (filename, &contents, &length, NULL)) {
> > +		g_free (filename);
> > +		return;
> > +	}
> 
> There are several indentation inconsistencies like this one (2 spaces indentation for the first level, but 4 spaces in second-level indentation) in the soup files. If you could fix those when landing, that'd be great. *nod* otherwise! (I'll leave the r+ for mrobinson)

I think you're cheat by the way bugzilla shows patches with tabs, as libsoup code is using tabs for indentation. I have double checked that the patches are properly indented with 8-space tabs, it's just how they're rendered when reviewed.

Anyway I'll upload a new version with emacs headers for indentation as libsoup uses.

[Sergio Villar Senin]

Created attachment 69810 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

Added emacs indentation headers as the originals in libsoup. I also fixed a couple of typos in the names of some macros

[Martin Robinson]

Comment on attachment 69810 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

I double-checked the patch and it seems it's tabbed properly. We need to update the EWS version of libsoup before we can land this patch.

[Gajendra singh]

Just out of curiosity, Would this cache implementation suffice in multiprocess environment, Say Two processes A and B are linked with webkit and having same Cache location, then how the synchronization would be done for "soup.cache" file, if A and B load same pages.

[WebKit Commit Bot]

Comment on attachment 69611 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

Clearing flags on attachment: 69611

Committed r69589: <http://trac.webkit.org/changeset/69589>

[Martin Robinson]

Comment on attachment 69611 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

Just realized that these patches don't have complete ChangeLogs. Flipping the review flag until Sergio can fix them.

[Sergio Villar Senin]

(In reply to comment #114)
> Just out of curiosity, Would this cache implementation suffice in multiprocess environment, Say Two processes A and B are linked with webkit and having same Cache location, then how the synchronization would be done for "soup.cache" file, if A and B load same pages.

There wouldn't be any kind of synchronization. The implementation assumes that every process will use it own directory.

[Sergio Villar Senin]

Created attachment 70628 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

Corrected ChangeLogs

[Sergio Villar Senin]

Created attachment 70630 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

Updated ChangeLogs

[Martin Robinson]

Comment on attachment 70628 [details]
Imported SoupURILoader to Webkit. New public API for HTTP cache

Great. Thanks. I'll land these patches.

[Sergio Villar Senin]

Comment on attachment 69611 [details]
WebKitGtk+ to use the new API from the imported SoupURILoader code

Obsolete actually

[Martin Robinson]

Committed r69718: <http://trac.webkit.org/changeset/69718>

[Ryosuke Niwa]

http://trac.webkit.org/changeset/69718 seems to have broken Gtk/Qt release builds:

../../WebCore/platform/network/soup/cache/soup-directory-input-stream.c:114: warning: comparison between signed and unsigned integer expressions
../../WebCore/platform/network/soup/cache/soup-directory-input-stream.c:127: warning: comparison between signed and unsigned integer expressions
../../WebCore/platform/network/soup/cache/soup-request-data.c:82: warning: comparison between signed and unsigned integer expressions
../../WebCore/platform/network/soup/cache/soup-request-data.c:82: warning: signed and unsigned type in conditional expression
../../WebCore/platform/network/soup/cache/soup-request-file.c:171: warning: implicit declaration of function ‘soup_uri_get_path’
../../WebCore/platform/network/soup/cache/soup-request-file.c:171: warning: initialization makes pointer from integer without a cast
../../WebCore/platform/network/soup/cache/webkit/soup-cache.c:271: warning: signed and unsigned type in conditional expression
../../WebCore/platform/network/soup/cache/webkit/soup-cache.c:1347: warning: comparison between signed and unsigned integer expressions
../../WebCore/platform/network/soup/cache/webkit/soup-cache.c:1366: warning: comparison between signed and unsigned integer expressions

[Martin Robinson]

(In reply to comment #123)
> http://trac.webkit.org/changeset/69718 seems to have broken Gtk/Qt release builds:


The issue here seems be that the Release bots do not have a new enough version of libsoup to have soup_uri_get_path. Here's the failure line:

make[1]: Entering directory `/var/lib/buildbot/build/gtk-linux-32-release/build/WebKitBuild/Release'
  CC     WebKit/gtk/tests/Programs_unittests_testdomdocument-testdomdocument.o
  CCLD   Programs/unittests/testdomdocument
./.libs/libwebkitgtk-1.0.so: undefined reference to `soup_uri_get_path'


> ../../WebCore/platform/network/soup/cache/soup-directory-input-stream.c:114: warning: comparison between signed and unsigned integer expressions
> ../../WebCore/platform/network/soup/cache/soup-directory-input-stream.c:127: warning: comparison between signed and unsigned integer expressions
> ../../WebCore/platform/network/soup/cache/soup-request-data.c:82: warning: comparison between signed and unsigned integer expressions
> ../../WebCore/platform/network/soup/cache/soup-request-data.c:82: warning: signed and unsigned type in conditional expression
> ../../WebCore/platform/network/soup/cache/soup-request-file.c:171: warning: implicit declaration of function ‘soup_uri_get_path’
> ../../WebCore/platform/network/soup/cache/soup-request-file.c:171: warning: initialization makes pointer from integer without a cast
> ../../WebCore/platform/network/soup/cache/webkit/soup-cache.c:271: warning: signed and unsigned type in conditional expression
> ../../WebCore/platform/network/soup/cache/webkit/soup-cache.c:1347: warning: comparison between signed and unsigned integer expressions
> ../../WebCore/platform/network/soup/cache/webkit/soup-cache.c:1366: warning: comparison between signed and unsigned integer expressions

We should also fix these warnings before we try to land this patch again.

[Xan Lopez]

(In reply to comment #124)
> (In reply to comment #123)
> > http://trac.webkit.org/changeset/69718 seems to have broken Gtk/Qt release builds:
> 
> 
> The issue here seems be that the Release bots do not have a new enough version of libsoup to have soup_uri_get_path. Here's the failure line:
> 
> make[1]: Entering directory `/var/lib/buildbot/build/gtk-linux-32-release/build/WebKitBuild/Release'
>   CC     WebKit/gtk/tests/Programs_unittests_testdomdocument-testdomdocument.o
>   CCLD   Programs/unittests/testdomdocument
> ./.libs/libwebkitgtk-1.0.so: undefined reference to `soup_uri_get_path'

Fix it in tree by accessing path directly (uri->path), the struct is public. The getter was added in 2.32 only for the benefit of the bindings AFAIK.

[Martin Robinson]

(In reply to comment #125)
> Fix it in tree by accessing path directly (uri->path), the struct is public. The getter was added in 2.32 only for the benefit of the bindings AFAIK.

Okay. Fixed in the tree, but we really should fix those warnings.

[Martin Robinson]

Committed r69742: <http://trac.webkit.org/changeset/69742>

[Hayato Ito]

http://trac.webkit.org/changeset/69742 seems to have broken Gtk builds:

http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/6599/steps/compile-webkit/logs

../../WebCore/platform/network/soup/ResourceHandleSoup.cpp: In function ‘void WebCore::sendRequestCallback(GObject*, GAsyncResult*, void*)’:
../../WebCore/platform/network/soup/ResourceHandleSoup.cpp:495: error: ‘soupMessage’ was not declared in this scope
make[1]: *** [WebCore/platform/network/soup/libwebkitgtk_1_0_la-ResourceHandleSoup.lo] Error 1
make[1]: Leaving directory `/var/lib/buildbot/build/gtk-linux-32-release/build/WebKitBuild/Release'
make: *** [all] Error 2

(In reply to comment #127)
> Committed r69742: <http://trac.webkit.org/changeset/69742>

[Alejandro G. Castro]

(In reply to comment #128)
> http://trac.webkit.org/changeset/69742 seems to have broken Gtk builds:
> 
> http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/6599/steps/compile-webkit/logs
> 
> ../../WebCore/platform/network/soup/ResourceHandleSoup.cpp: In function ‘void WebCore::sendRequestCallback(GObject*, GAsyncResult*, void*)’:
> ../../WebCore/platform/network/soup/ResourceHandleSoup.cpp:495: error: ‘soupMessage’ was not declared in this scope
> make[1]: *** [WebCore/platform/network/soup/libwebkitgtk_1_0_la-ResourceHandleSoup.lo] Error 1
> make[1]: Leaving directory `/var/lib/buildbot/build/gtk-linux-32-release/build/WebKitBuild/Release'
> make: *** [all] Error 2
> 
> (In reply to comment #127)
> > Committed r69742: <http://trac.webkit.org/changeset/69742>

Fixed here, thanks for the report:

http://trac.webkit.org/changeset/69748