[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


Sergio Villar Senin <svillar at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |svillar at igalia.com




--- Comment #3 from Sergio Villar Senin <svillar at igalia.com>  2010-10-08 08:46:18 PST ---
(In reply to comment #2)
> (From update of attachment 69924 [details])
> 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?

Actually it's not related but I think it's missing anyway.

> > 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.

Not strong feelings about that, but take into account that we currently have CookieJarSoup or GOwnPtrSoup. That was the reason why I chose uri before Soup.

> > 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;

I like your approach.

> 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. :)

Ups, copy&paste badness I guess :)

> > WebCore/platform/network/soup/UriSoup.h:30
> > +#include <libsoup/soup.h>
> 
> I'd prefer a forward declaration here instead of a libsoup include.

Ok

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