Bug 50747

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

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

(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.

(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.

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 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.

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

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.

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.

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.

(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 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 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.

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.

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 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.

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

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