Bug 50885 – [GTK] remove old data: URI handler, fix the SoupRequest-based one

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

[Dan Winship]

The SoupRequest stuff has always had a data: URI handler, but ResourceHandleSoup was never updated to use it. This fixes that, as well as removing some references to the old gio/gvfs-based loader that are no longer relevant with the SoupRequest code. It also makes it so that it attempts to use SoupRequest for all unrecognized URI schemes (thereby opening up the possibility of registering additional SoupRequest types from the application).

[Dan Winship]

Created attachment 76317 [details]
patch to use SoupRequestData

The comment about "limitations in KURL" in the old parseDataUrl() does not appear to be relevant to the current code; the tests pass fine without lying about the text encoding.

The changes to sendRequestCallback() are needed because the code was apparently broken before; it worked for file URIs because they always had !shouldContentSniff(), but for any other kind of non-HTTP URI it would result in didReceiveResponse never being called.

The fixes to soup-request-data.c have already been committed to libsoup, but this means that if we commit both this patch and bug 50675 then we need updated libsoup packages on the bots, or else we'll have to temporarily skip a few tests.

[Martin Robinson]

Sergio, do you mind doing an informal review here?

[Sergio Villar Senin]

(In reply to comment #2)
> Sergio, do you mind doing an informal review here?

Informally looking good to me :). Don't know if it makes sense to change the SoupRequestData stuff tough as it will be removed. Maybe it'd be better to:

1) update bots and EWS (are we still waiting for Google?)
2) migrate from webkit_soup_ -> soup_ first
3) remove SoupURILoader stuff from webkitgtk
3) Apply Dan's ResourceHandleSoup changes

[Dan Winship]

Created attachment 80575 [details]
updated patch

This is rebased on top of bug 50747. Not bothering to mark r? yet since it won't apply cleanly to HEAD

[Sergio Villar Senin]

Several tests are not working for me. Seems that data: URLs are not properly handled.

[Sergio Villar Senin]

(In reply to comment #5)
> Several tests are not working for me. Seems that data: URLs are not properly handled.

Fixed in libsoup 2.33.6 BTW

[Dan Winship]

Comment on attachment 80575 [details]
updated patch

50747 is in, so this can go now

[Martin Robinson]

Comment on attachment 80575 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=80575&action=review

This looks very good, but I have a few mostly stylistic suggestions.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:132
> +static bool startGeneric(ResourceHandle*, KURL);

I think the startHttp/startGeneric pair should probably be called startHTTPRequest and startNonHTTPRequest for the sake of explicitness and the WebKit style guidelines.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:470
> +        String mimeType = extractMIMETypeFromMediaType(contentType);
> +        String charset = extractCharsetFromMediaType(contentType);
> +        d->m_response.setMimeType(mimeType);
> +        d->m_response.setTextEncodingName(charset);

If possible, please just make this:

d->m_response.setMimeType(extractMIMETypeFromMediaType(contentType));
d->m_response.setTextEncodingName(extractCharsetFromMediaType(contentType));

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:-902
> -    // GIO doesn't know how to handle refs and queries, so remove them
> -    // TODO: use KURL.fileSystemPath after KURLGtk and FileSystemGtk are
> -    // using GIO internally, and providing URIs instead of file paths
> -    url.removeFragmentIdentifier();
> -    url.setQuery(String());
> -    url.removePort();

Do you mind explaining this removal in the startNonHTTP (nee startGeneric) section of the ChangeLog?

[Dan Winship]

Created attachment 80956 [details]
updated with Martin's suggestions

[Martin Robinson]

Committed r77408: <http://trac.webkit.org/changeset/77408>

[WebKit Review Bot]

http://trac.webkit.org/changeset/77408 might have broken GTK Linux 32-bit Release
The following tests are not passing:
editing/selection/find-yensign-and-backslash.html
http/tests/websocket/tests/handshake-fail-by-sub-protocol-mismatch.html