Bug 50747 – [GTK] drop soup cache stuff, use versions from libsoup

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Dan Winship]

We're moving the request/cache code to libsoup

[Dan Winship]

Created attachment 76040 [details]
patch to remove request/cache stuff and use it from libsoup instead

[Eric Seidel (no email)]

You should consider using --suggest-reviewers when using webkit-patch upload so that folks who might review the patch get CC'd.

[Xan Lopez]

(In reply to comment #2)
> You should consider using --suggest-reviewers when using webkit-patch upload so that folks who might review the patch get CC'd.

We are pretty much waiting to have a EWS with the properly up do date libraries to move forward with this; I believe the plan is to stop using the ones generously provided by you guys and move to ones hosted by Collabora, and that Gustavo is taking care of that.

[Eric Seidel (no email)]

(In reply to comment #3)
> (In reply to comment #2)
> > You should consider using --suggest-reviewers when using webkit-patch upload so that folks who might review the patch get CC'd.
> 
> We are pretty much waiting to have a EWS with the properly up do date libraries to move forward with this; I believe the plan is to stop using the ones generously provided by you guys and move to ones hosted by Collabora, and that Gustavo is taking care of that.

I'm not sure what your'e saying.

[Dan Winship]

He was clarifying (in a roundabout way) that the reason the patch hasn't been reviewed is because it's blocking on real-world issues, not because the potential reviewers were unaware of it.

[Gustavo Noronha (kov)]

Comment on attachment 76040 [details]
patch to remove request/cache stuff and use it from libsoup instead

Trying to trigger the GTK+ EWS on this patch.

[Dan Winship]

Created attachment 80568 [details]
updated patch (for source tree moves and some other stuff)

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:59:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:60:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:171:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:182:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/network/ResourceHandleInternal.h:52:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 5 in 5 files


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

[Dan Winship]

Created attachment 80574 [details]
updated again to (almost) pass check-webkit-style

The NULL-vs-0 warning from check-webkit-style is wrong. Apparently we could suppress it by squishing the entire g_object_set() call onto a single line, but I didn't do that because (a) it would be ugly, and (b) that code was already in ResourceHandleSoup.cpp anyway, I just moved it within the file.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:181:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 5 files


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

[Martin Robinson]

(In reply to comment #9)
> Created an attachment (id=80574) [details]
> updated again to (almost) pass check-webkit-style
> 
> The NULL-vs-0 warning from check-webkit-style is wrong. Apparently we could suppress it by squishing the entire g_object_set() call onto a single line, but I didn't do that because (a) it would be ugly, and (b) that code was already in ResourceHandleSoup.cpp anyway, I just moved it within the file.

It can also lead to crashes on 64-bit systems, so it's actively dangerous to use 0!

[Martin Robinson]

Comment on attachment 80574 [details]
updated again to (almost) pass check-webkit-style

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

Awesome! 

> Source/WebCore/ChangeLog:20
> +        * GNUmakefile.am:
> +        * platform/network/ResourceHandleInternal.h:
> +        (WebCore::ResourceHandleInternal::ResourceHandleInternal):
> +        * platform/network/soup/ResourceHandleSoup.cpp:
> +        (WebCore::ensureSessionIsInitialized):
> +        (WebCore::parseDataUrl):
> +        (WebCore::sendRequestCallback):
> +        (WebCore::startHttp):
> +        (WebCore::ResourceHandle::platformSetDefersLoading):
> +        (WebCore::readCallback):
> +        (WebCore::startGio):

I think these should be filled with some brief information about what's changing. One comment probably covers quite a few of them.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:155
> +#define MAX_CONNECTIONS_PER_HOST 6

These should probably be static const int instead of #defined. I know they weren't before, but since we're moving the entire block, we might as well fix the style issues now.

[Sergio Villar Senin]

Comment on attachment 80574 [details]
updated again to (almost) pass check-webkit-style

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

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:174
> +    SoupRequester* requester = soup_requester_new();

I think you should add a check for existence of a SoupRequester in the session instead of directly creating it.

[Dan Winship]

Created attachment 80933 [details]
updated with Martin's suggestion

(not tested yet... my machine is too slow)

I didn't make the change Sergio suggested, because unlike the situation with the cookie jar, there's no reason anyone else would want to override the default SoupRequester, since it doesn't have subclasses, so any other requester they set would just be identical to the one WebKit sets itself.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:59:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:60:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:171:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:182:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/network/ResourceHandleInternal.h:52:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 5 in 5 files


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

[Martin Robinson]

Comment on attachment 80933 [details]
updated with Martin's suggestion

Seems good, though we need to fix the style issues before landing this one. I will do that before landing as well as verifying that this builds.

[Dan Winship]

doh, sorry. i somehow lost the check-webkit-style fixes from before...

[Martin Robinson]

Committed r77399: <http://trac.webkit.org/changeset/77399>