Bug 50747 - [GTK] drop soup cache stuff, use versions from libsoup
Summary: [GTK] drop soup cache stuff, use versions from libsoup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 50675
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-09 03:24 PST by Dan Winship
Modified: 2011-02-02 13:11 PST (History)
4 users (show)

See Also:


Attachments
patch to remove request/cache stuff and use it from libsoup instead (179.01 KB, patch)
2010-12-09 03:29 PST, Dan Winship
no flags Details | Formatted Diff | Diff
updated patch (for source tree moves and some other stuff) (180.15 KB, patch)
2011-01-29 11:04 PST, Dan Winship
no flags Details | Formatted Diff | Diff
updated again to (almost) pass check-webkit-style (180.15 KB, patch)
2011-01-29 14:45 PST, Dan Winship
no flags Details | Formatted Diff | Diff
updated with Martin's suggestion (180.27 KB, patch)
2011-02-02 11:07 PST, Dan Winship
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Winship 2010-12-09 03:24:18 PST
We're moving the request/cache code to libsoup
Comment 1 Dan Winship 2010-12-09 03:29:33 PST
Created attachment 76040 [details]
patch to remove request/cache stuff and use it from libsoup instead
Comment 2 Eric Seidel (no email) 2010-12-24 09:10:32 PST
You should consider using --suggest-reviewers when using webkit-patch upload so that folks who might review the patch get CC'd.
Comment 3 Xan Lopez 2010-12-25 11:55:23 PST
(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.
Comment 4 Eric Seidel (no email) 2010-12-28 12:09:16 PST
(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.
Comment 5 Dan Winship 2010-12-28 12:49:56 PST
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.
Comment 6 Gustavo Noronha (kov) 2011-01-28 11:47:45 PST
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.
Comment 7 Dan Winship 2011-01-29 11:04:25 PST
Created attachment 80568 [details]
updated patch (for source tree moves and some other stuff)
Comment 8 WebKit Review Bot 2011-01-29 11:08:28 PST
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.
Comment 9 Dan Winship 2011-01-29 14:45:27 PST
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.
Comment 10 WebKit Review Bot 2011-01-29 14:47:57 PST
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.
Comment 11 Martin Robinson 2011-01-29 18:16:44 PST
(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!
Comment 12 Martin Robinson 2011-01-29 18:26:36 PST
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.
Comment 13 Sergio Villar Senin 2011-01-31 00:27:06 PST
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.
Comment 14 Dan Winship 2011-02-02 11:07:56 PST
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.
Comment 15 WebKit Review Bot 2011-02-02 11:09:15 PST
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.
Comment 16 Martin Robinson 2011-02-02 11:13:15 PST
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.
Comment 17 Dan Winship 2011-02-02 12:19:04 PST
doh, sorry. i somehow lost the check-webkit-style fixes from before...
Comment 18 Martin Robinson 2011-02-02 12:25:01 PST
Committed r77399: <http://trac.webkit.org/changeset/77399>