Bug 71611 - [GTK] simplify ResourceHandleSoup
Summary: [GTK] simplify ResourceHandleSoup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 72227
  Show dependency treegraph
 
Reported: 2011-11-05 10:31 PDT by Dan Winship
Modified: 2011-12-08 08:27 PST (History)
8 users (show)

See Also:


Attachments
Don't unnecessarily 0-initialize read buffers. (1.84 KB, patch)
2011-11-05 10:32 PDT, Dan Winship
no flags Details | Formatted Diff | Diff
Remove a bit of dead code (3.02 KB, patch)
2011-11-05 10:32 PDT, Dan Winship
no flags Details | Formatted Diff | Diff
Actually use the user_data arguments to gio async (8.33 KB, patch)
2011-11-05 10:32 PDT, Dan Winship
mrobinson: review+
Details | Formatted Diff | Diff
Remove lots of code that is now unnecessary after (17.93 KB, patch)
2011-11-05 10:33 PDT, Dan Winship
gustavo.noronha: commit-queue-
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-
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 Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Winship 2011-11-05 10:31:18 PDT
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 Dan Winship 2011-11-05 10:32:07 PDT
Created attachment 113758 [details]
Don't unnecessarily 0-initialize read buffers.
Comment 2 Dan Winship 2011-11-05 10:32:26 PDT
Created attachment 113759 [details]
Remove a bit of dead code
Comment 3 Dan Winship 2011-11-05 10:32:51 PDT
Created attachment 113760 [details]
Actually use the user_data arguments to gio async
Comment 4 WebKit Review Bot 2011-11-05 10:33:03 PDT
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 Dan Winship 2011-11-05 10:33:17 PDT
Created attachment 113761 [details]
Remove lots of code that is now unnecessary after
Comment 6 WebKit Review Bot 2011-11-05 10:34:46 PDT
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 WebKit Review Bot 2011-11-05 10:35:55 PDT
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 Collabora GTK+ EWS bot 2011-11-05 12:16:36 PDT
Comment on attachment 113761 [details]
Remove lots of code that is now unnecessary after

Attachment 113761 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10336291
Comment 9 Martin Robinson 2011-11-05 12:20:24 PDT
Comment on attachment 113758 [details]
Don't unnecessarily 0-initialize read buffers.

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 Martin Robinson 2011-11-05 12:22:20 PDT
Comment on attachment 113759 [details]
Remove a bit of dead code

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 Martin Robinson 2011-11-05 12:23:54 PDT
Comment on attachment 113760 [details]
Actually use the user_data arguments to gio async

Awesome!
Comment 12 Martin Robinson 2011-11-05 12:36:57 PDT
Comment on attachment 113761 [details]
Remove lots of code that is now unnecessary after

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 Dan Winship 2011-11-05 13:53:28 PDT
(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 Martin Robinson 2011-11-05 14:06:30 PDT
(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 Dan Winship 2011-11-06 10:01:07 PST
Created attachment 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 Collabora GTK+ EWS bot 2011-11-06 12:04:23 PST
Comment on attachment 113786 [details]
updated - Remove lots of code that is now unnecessary

Attachment 113786 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10334538
Comment 17 Gustavo Noronha (kov) 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 Dan Winship 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 Martin Robinson 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 Sergio Villar Senin 2011-11-07 10:41:33 PST
(In reply to comment #3)
> Created an attachment (id=113760) [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 Sergio Villar Senin 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 Dan Winship 2011-12-02 01:39:23 PST
Comment on attachment 113786 [details]
updated - Remove lots of code that is now unnecessary

needs rebasing now (and an updated libsoup version requirement)
Comment 23 Dan Winship 2011-12-02 04:16:11 PST
Created attachment 117607 [details]
rebased and with updated libsoup version requirement
Comment 24 Gustavo Noronha (kov) 2011-12-02 08:57:40 PST
Comment on attachment 117607 [details]
rebased and with updated libsoup version requirement

Attachment 117607 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10690380
Comment 25 Martin Robinson 2011-12-02 10:11:21 PST
Comment on attachment 117607 [details]
rebased and with updated libsoup version requirement

Yes!
Comment 26 Martin Robinson 2011-12-03 01:23:05 PST
Comment on attachment 117607 [details]
rebased and with updated libsoup version requirement

Clearing flags on attachment: 117607

Committed r101917: <http://trac.webkit.org/changeset/101917>
Comment 27 Martin Robinson 2011-12-03 01:23:15 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Raphael Kubo da Costa (:rakuco) 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 Dan Winship 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 Gustavo Noronha (kov) 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