Bug 71611 – [GTK] simplify ResourceHandleSoup

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

[Dan Winship]

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.

[Dan Winship]

Created attachment 113758 [details]
Don't unnecessarily 0-initialize read buffers.

[Dan Winship]

Created attachment 113759 [details]
Remove a bit of dead code

[Dan Winship]

Created attachment 113760 [details]
Actually use the user_data arguments to gio async

[WebKit Review Bot]

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.

[Dan Winship]

Created attachment 113761 [details]
Remove lots of code that is now unnecessary after

[WebKit Review Bot]

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.

[WebKit Review Bot]

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.

[Collabora GTK+ EWS bot]

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

[Martin Robinson]

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.

[Martin Robinson]

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.

[Martin Robinson]

Comment on attachment 113760 [details]
Actually use the user_data arguments to gio async

Awesome!

[Martin Robinson]

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.

[Dan Winship]

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

[Martin Robinson]

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

[Dan Winship]

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?

[Collabora GTK+ EWS bot]

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

[Gustavo Noronha (kov)]

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?

[Dan Winship]

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

[Martin Robinson]

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

[Sergio Villar Senin]

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

[Sergio Villar Senin]

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

[Dan Winship]

Comment on attachment 113786 [details]
updated - Remove lots of code that is now unnecessary

needs rebasing now (and an updated libsoup version requirement)

[Dan Winship]

Created attachment 117607 [details]
rebased and with updated libsoup version requirement

[Gustavo Noronha (kov)]

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

[Martin Robinson]

Comment on attachment 117607 [details]
rebased and with updated libsoup version requirement

Yes!

[Martin Robinson]

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>

[Martin Robinson]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

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

[Dan Winship]

(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) ?

[Gustavo Noronha (kov)]

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