[Webkit-unassigned] [Bug 38956] [Gtk] http/tests/xmlhttprequest/basic-auth-default.html fails

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38956


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #69924|review?                     |review-
               Flag|                            |




--- Comment #2 from Martin Robinson <mrobinson at webkit.org>  2010-10-08 08:35:58 PST ---
(From update of attachment 69924)
View in context: https://bugs.webkit.org/attachment.cgi?id=69924&action=review

Great patch. I have a few suggestions, but I like the way this fix is structured.

> WebCore/platform/network/soup/ResourceHandleSoup.cpp:755
> +    ResourceHandleInternal* d = handle->getInternal();
> +
> +    d->m_context = context;

I don't understand how this change is related. Was it included by accident?

> WebCore/platform/network/soup/ResourceRequestSoup.cpp:28
> +#include "UriSoup.h"

I think I'd prefer this include to be called SoupURI or SoupURIUtils to match the type name.

> WebCore/platform/network/soup/UriSoup.cpp:27
> +gchar*
> +uriSoupToStringWithPassword(SoupURI* soupUri)

No newline needed after the return type and the parameter should be soupURI. IMO, it should also be soupURITo<something> (see below).

> WebCore/platform/network/soup/UriSoup.cpp:42
> +    gchar* url = soup_uri_to_string(soupUri, FALSE);
> +
> +    if (!soupUri->password)
> +        return url;
> +
> +    GString* stringUrl = g_string_sized_new(strlen(url) + strlen(soupUri->password) + 1);
> +    const gchar* at = strchr(url, '@');
> +
> +    stringUrl = g_string_append(stringUrl, url);
> +    stringUrl = g_string_insert_c(stringUrl, at - url, ':');
> +    stringUrl = g_string_insert(stringUrl, at - url + 1, soupUri->password);
> +    g_free(url);
> +
> +    return g_string_free(stringUrl, FALSE);

Perhaps this method should just return a KURL. That might future-proof it in case soup ever starts including the password. It could just be something like this:

GOwnPtr<gchar> urlString(soup_uri_toString(soupURI));
KURL url(KURL(), String::fromUTF8(urlString.get()));
if (!soupURI->password)
    return url;
url->setPass(String::fromUTF8(soupURI->password);
return url;

You should also put a comment here with a link to the bug and a little text describing the issue.

> WebCore/platform/network/soup/UriSoup.h:3
> + * Copyright (C) 2008 Xan Lopez <xan at gnome.org>
> + * Copyright (C) 2008 Apple Inc. All rights reserved.

There should just be an Igalia line here and 2010. :)

> WebCore/platform/network/soup/UriSoup.h:30
> +#include <libsoup/soup.h>

I'd prefer a forward declaration here instead of a libsoup include.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.