Summary: | [GTK] remove old data: URI handler, fix the SoupRequest-based one | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dan Winship <danw> | ||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, eric, mrobinson, svillar, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Dan Winship
2010-12-12 02:51:16 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. Sergio, do you mind doing an informal review here? (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 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 Several tests are not working for me. Seems that data: URLs are not properly handled. (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 on attachment 80575 [details]
updated patch
50747 is in, so this can go now
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? Created attachment 80956 [details]
updated with Martin's suggestions
Committed r77408: <http://trac.webkit.org/changeset/77408> 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 |