Bug 50885 - [GTK] remove old data: URI handler, fix the SoupRequest-based one
Summary: [GTK] remove old data: URI handler, fix the SoupRequest-based one
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-12 02:51 PST by Dan Winship
Modified: 2011-02-02 15:46 PST (History)
5 users (show)

See Also:


Attachments
patch to use SoupRequestData (10.33 KB, patch)
2010-12-12 03:19 PST, Dan Winship
no flags Details | Formatted Diff | Diff
updated patch (8.46 KB, patch)
2011-01-29 14:47 PST, Dan Winship
mrobinson: review-
Details | Formatted Diff | Diff
updated with Martin's suggestions (9.55 KB, patch)
2011-02-02 13:08 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 2010-12-12 02:51:16 PST
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).
Comment 1 Dan Winship 2010-12-12 03:19:42 PST
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.
Comment 2 Martin Robinson 2011-01-03 13:15:14 PST
Sergio, do you mind doing an informal review here?
Comment 3 Sergio Villar Senin 2011-01-04 02:50:58 PST
(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
Comment 4 Dan Winship 2011-01-29 14:47:10 PST
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
Comment 5 Sergio Villar Senin 2011-01-31 01:32:49 PST
Several tests are not working for me. Seems that data: URLs are not properly handled.
Comment 6 Sergio Villar Senin 2011-02-01 05:11:16 PST
(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
Comment 7 Dan Winship 2011-02-02 12:33:23 PST
Comment on attachment 80575 [details]
updated patch

50747 is in, so this can go now
Comment 8 Martin Robinson 2011-02-02 12:45:01 PST
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?
Comment 9 Dan Winship 2011-02-02 13:08:37 PST
Created attachment 80956 [details]
updated with Martin's suggestions
Comment 10 Martin Robinson 2011-02-02 13:26:01 PST
Committed r77408: <http://trac.webkit.org/changeset/77408>
Comment 11 WebKit Review Bot 2011-02-02 15:46:48 PST
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