[webkit-reviews] review denied: [Bug 38956] [Gtk] http/tests/xmlhttprequest/basic-auth-default.html fails : [Attachment 69924] Fix basic authentication in WebKitGtk+

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 38956: [Gtk] http/tests/xmlhttprequest/basic-auth-default.html fails
https://bugs.webkit.org/show_bug.cgi?id=38956

Attachment 69924: Fix basic authentication in WebKitGtk+
https://bugs.webkit.org/attachment.cgi?id=69924&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.