Bug 71611 - [GTK] simplify ResourceHandleSoup
: [GTK] simplify ResourceHandleSoup
Status: RESOLVED FIXED
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 72227
  Show dependency treegraph
 
Reported: 2011-11-05 10:31 PST by
Modified: 2011-12-08 08:27 PST (History)


Attachments
Don't unnecessarily 0-initialize read buffers. (1.84 KB, patch)
2011-11-05 10:32 PST, Dan Winship
no flags Review Patch | Details | Formatted Diff | Diff
Remove a bit of dead code (3.02 KB, patch)
2011-11-05 10:32 PST, Dan Winship
no flags Review Patch | Details | Formatted Diff | Diff
Actually use the user_data arguments to gio async (8.33 KB, patch)
2011-11-05 10:32 PST, Dan Winship
mrobinson: review+
Review Patch | Details | Formatted Diff | Diff
Remove lots of code that is now unnecessary after (17.93 KB, patch)
2011-11-05 10:33 PST, Dan Winship
gustavo.noronha: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
updated - Remove lots of code that is now unnecessary (19.29 KB, patch)
2011-11-06 10:01 PST, Dan Winship
gustavo.noronha: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
rebased and with updated libsoup version requirement (19.05 KB, patch)
2011-12-02 04:16 PST, Dan Winship
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-11-05 10:31:18 PST
One of the goals of the SoupRequest API was to make it possible to unify the HTTP and non-HTTP codepaths in ResourceHandleSoup, but this never completely happened. This should move us much further towards that goal (and gets rid of a lot of redundant code in the process).

The first three patches are miscellaneous cleanups, and can be committed at any time (and possibly squashed into a single commit?). But the last patch depends on the corresponding libsoup changes in https://bugzilla.gnome.org/show_bug.cgi?id=663451 and so will need to be coordinated with that.
------- Comment #1 From 2011-11-05 10:32:07 PST -------
Created an attachment (id=113758) [details]
Don't unnecessarily 0-initialize read buffers.
------- Comment #2 From 2011-11-05 10:32:26 PST -------
Created an attachment (id=113759) [details]
Remove a bit of dead code
------- Comment #3 From 2011-11-05 10:32:51 PST -------
Created an attachment (id=113760) [details]
Actually use the user_data arguments to gio async
------- Comment #4 From 2011-11-05 10:33:03 PST -------
Attachment 113758 [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/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #5 From 2011-11-05 10:33:17 PST -------
Created an attachment (id=113761) [details]
Remove lots of code that is now unnecessary after
------- Comment #6 From 2011-11-05 10:34:46 PST -------
Attachment 113759 [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/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #7 From 2011-11-05 10:35:55 PST -------
Attachment 113761 [details] did not pass style-queue:

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

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #8 From 2011-11-05 12:16:36 PST -------
(From update of attachment 113761 [details])
Attachment 113761 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10336291
------- Comment #9 From 2011-11-05 12:20:24 PST -------
(From update of attachment 113758 [details])
This change looks fine, but you should probably update the changelog to include the bug. You can use prepare-ChangeLog --bug <bugnumber> for that. It's also easier if you split each patch into it's own bug. There's no strict requirement for this, but it allows using the commit-queue to land the patches.
------- Comment #10 From 2011-11-05 12:22:20 PST -------
(From update of attachment 113759 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=113759&action=review

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:-835
> -    bool convertToUTF16 = static_cast<bool>(data);

It seems data is unused now? If so you should remove the argument name or surround it like this /* data */ to avoid a compiler warning.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:862
> -    if (G_LIKELY(!convertToUTF16))
> -        client->didReceiveData(handle.get(), d->m_buffer, bytesRead, bytesRead);
> -    else {
> -        // We have to convert it to UTF-16 due to limitations in KURL
> -        String data = String::fromUTF8(d->m_buffer, bytesRead);
> -        client->didReceiveData(handle.get(), reinterpret_cast<const char*>(data.characters()), data.length() * sizeof(UChar), bytesRead);
> -    }
> +    client->didReceiveData(handle.get(), d->m_buffer, bytesRead, bytesRead);

Oh, thank god. That code was so mysterious. I'm glad it's gone.
------- Comment #11 From 2011-11-05 12:23:54 PST -------
(From update of attachment 113760 [details])
Awesome!
------- Comment #12 From 2011-11-05 12:36:57 PST -------
(From update of attachment 113761 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=113761&action=review

This patch is awesome. I'm sad to see the soup dependency go up again, but we were using unstable API so it seem unavoidable.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:316
> +        if (handle->shouldContentSniff()
> +            && soupMessage->status_code != SOUP_STATUS_NOT_MODIFIED) {

This can be one line. In WebKit we generally go to about 120 character before splitting them.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:321
> +            const char* officialType = soup_message_headers_get_one(soupMessage->response_headers, "Content-Type");
> +            const char* sniffedType = soup_request_get_content_type(d->m_soupRequest.get());
> +            if (sniffedType && (!officialType || strcmp(officialType, sniffedType)))
> +                d->m_response.setSniffedContentType(sniffedType);
> +        }

I think it would make sense to always set the sniffed content type and move the logic about whether or not the sniffed type should override the advertised type to ResourceResponse::updateFromSoupMessage. I say this because otherwise there might be times when sniffedContentType returns an empty string even when Soup did sniff a content type.
------- Comment #13 From 2011-11-05 13:53:28 PST -------
(In reply to comment #9)
> It's also easier if you split each patch into it's own bug. There's no strict requirement for this, but it allows using the commit-queue to land the patches.

OK, should I do that then? (I need to update all the patches either way, to include bug numbers.)
------- Comment #14 From 2011-11-05 14:06:30 PST -------
(In reply to comment #13)
> (In reply to comment #9)
> > It's also easier if you split each patch into it's own bug. There's no strict requirement for this, but it allows using the commit-queue to land the patches.
> 
> OK, should I do that then? (I need to update all the patches either way, to include bug numbers.)

Sure. We could land some of these immediately then.
------- Comment #15 From 2011-11-06 10:01:07 PST -------
Created an attachment (id=113786) [details]
updated - Remove lots of code that is now unnecessary

ok, moved the sniffing override check to ResourceResponseSoup.cpp. I changed the existing references to "sniffedContentType()" to "m_sniffedContentType", which seemed more consistent with the surrounding code.

> I'm sad to see the soup dependency go up again

yeah... based on historical precedent, it seems likely it will get bumped again during/after the hackfest, so maybe there's an argument for not committing this until then?
------- Comment #16 From 2011-11-06 12:04:23 PST -------
(From update of attachment 113786 [details])
Attachment 113786 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10334538
------- Comment #17 From 2011-11-07 04:52:29 PST -------
I don't think we need to wait for the hackfest, we just need to coordinate updating the bots. It'll be better having this code in and getting tested before the hackfest =) I guess we need soup master for this patch to work, or is the new stuff in a release already?
------- Comment #18 From 2011-11-07 04:58:04 PST -------
it's not in a release yet, though i could make a 2.37.1.1 release for you if you'd like.

(it's also not committed to master yet; i didn't want to commit it until i knew for sure that this patch was going in too.)
------- Comment #19 From 2011-11-07 09:00:48 PST -------
(In reply to comment #18)

> (it's also not committed to master yet; i didn't want to commit it until i knew for sure that this patch was going in too.)

For what it's worth, I'd really like to see this happen. I don't think anyone will be opposed to this. CCing Sergio so he can take a look.
------- Comment #20 From 2011-11-07 10:41:33 PST -------
(In reply to comment #3)
> Created an attachment (id=113760) [details] [details]
> Actually use the user_data arguments to gio async

The g_object_set_data is legacy stuff that was kept when I first moved ResourceHandleSoup to the new API because the user_data argument was used for some other stuff I cannot really remember IIRC. Now it seems a pretty obvious change, thx!
------- Comment #21 From 2011-11-07 10:47:27 PST -------
(In reply to comment #19)
> (In reply to comment #18)
> 
> > (it's also not committed to master yet; i didn't want to commit it until i knew for sure that this patch was going in too.)
> 
> For what it's worth, I'd really like to see this happen. I don't think anyone will be opposed to this. CCing Sergio so he can take a look.

If we still pass the tests then this patch is not only awesome but super-awesome.
------- Comment #22 From 2011-12-02 01:39:23 PST -------
(From update of attachment 113786 [details])
needs rebasing now (and an updated libsoup version requirement)
------- Comment #23 From 2011-12-02 04:16:11 PST -------
Created an attachment (id=117607) [details]
rebased and with updated libsoup version requirement
------- Comment #24 From 2011-12-02 08:57:40 PST -------
(From update of attachment 117607 [details])
Attachment 117607 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10690380
------- Comment #25 From 2011-12-02 10:11:21 PST -------
(From update of attachment 117607 [details])
Yes!
------- Comment #26 From 2011-12-03 01:23:05 PST -------
(From update of attachment 117607 [details])
Clearing flags on attachment: 117607

Committed r101917: <http://trac.webkit.org/changeset/101917>
------- Comment #27 From 2011-12-03 01:23:15 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #28 From 2011-12-07 08:09:16 PST -------
Is there any way to make this change somehow backward-compatible? We (webkit-efl) also use the soup backend, but can't require users and developers to use a development libsoup version released a few weeks ago. Refreshing a page using EWebLauncher, for example, gives me the following backtrace:

0xb50ccd2b in WebCore::SubresourceLoader::didFail (this=0x8186fc0, error=...)
    at /home/profusion/dev/webkit-efl/Source/WebCore/loader/SubresourceLoader.cpp:270
270         ASSERT(!m_resource->resourceToRevalidate());
(gdb) bt
#0  0xb50ccd2b in WebCore::SubresourceLoader::didFail (this=0x8186fc0, error=...)
    at /home/profusion/dev/webkit-efl/Source/WebCore/loader/SubresourceLoader.cpp:270
#1  0xb50c944a in WebCore::ResourceLoader::didFail (this=0x8186fc0, error=...)
    at /home/profusion/dev/webkit-efl/Source/WebCore/loader/ResourceLoader.cpp:462
#2  0xb5b29d5e in WebCore::sendRequestCallback (source=0x80c0ea0, res=0x807aa48, data=0x8186ef8)
    at /home/profusion/dev/webkit-efl/Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:306
#3  0xb7c60ccf in g_simple_async_result_complete (simple=0x807aa48)
    at /build/buildd/glib2.0-2.28.6/./gio/gsimpleasyncresult.c:747
#4  0xb7d3c58e in sent_async (source=0x814f100, result=0x80c1a88, user_data=0x807aa48) at soup-request-http.c:109
#5  0xb7d2dc19 in wrapper_callback (source_object=0x814f100, res=0x80c1a88, user_data=0x807aa48)
    at soup-http-input-stream.c:539
#6  0xb7c60ccf in g_simple_async_result_complete (simple=0x80c1a88)
    at /build/buildd/glib2.0-2.28.6/./gio/gsimpleasyncresult.c:747
#7  0xb7d2db7e in send_async_finished (stream=0x814f100) at soup-http-input-stream.c:599
#8  0xb7d2dd7e in soup_http_input_stream_finished (msg=0x814e498, stream=0x814f100) at soup-http-input-stream.c:310
[...]
------- Comment #29 From 2011-12-08 05:16:21 PST -------
(In reply to comment #28)
> Is there any way to make this change somehow backward-compatible?

Short answer: No, we'd need to have both the old and the new code there, selected by ifdef or at runtime.

Longer answer: WebKitGTK follows the GNOME release cycle, and so it's easy for us to have unstable WebKit requiring unstable GNOME libraries. But the EFL port has no release cycle or stable branch yet, so any dependency change immediately impacts some downstream users.

This is going to continue to be a problem, because with jhbuild in the build system and on the bots, it makes it much easier for us to depend on newer glib/libsoup releases for bugfixes and new features.

Possible fixes: (1) EFL could drop libsoup in favor of curl, or (eventually) Ecore_Con_Url. (2) EFL could use the new jhbuild infrastructure to build libsoup, etc rather than depending on the system libraries being new enough. (3) EFL could fork the soup backend and not use new APIs in the fork. (4) ?
------- Comment #30 From 2011-12-08 08:27:39 PST -------
4 would be EFL maintaining a stable branch to which they cherry-pick commits as they see fit, like we do for our stable releases, or like we do for the clutter port too - we intentionally stayed behind trunk for a while because of a soup dependency bump that we didn't want to push our to downstreams just yet, and have since caught up with that one.

If the EFL maintainers would prefer to keep using trunk, then my sugestion would be that they fork ResourceHandleSoup for now, and start using the new code again when they feel like they can depend on a newer soup. Unfortunately it's hard to balance our need to push our infrastructure forward with making it possible to build with and use older versions