[webkit-reviews] review denied: [Bug 99914] [GTK] Move soup authentication from GtkAuthenticationDialog to WebCore : [Attachment 171528] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 99914: [GTK] Move soup authentication from GtkAuthenticationDialog to
WebCore
https://bugs.webkit.org/show_bug.cgi?id=99914

Attachment 171528: Patch
https://bugs.webkit.org/attachment.cgi?id=171528&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=171528&action=review


> Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:112
> -    SoupURI* uri = soup_message_get_uri(m_message.get());
> -    GOwnPtr<char>description(g_strdup_printf(_("A username and password are
being requested by the site %s"), uri->host));
> -    GtkWidget* descriptionLabel = gtk_label_new(description.get());
> +    String description = String::format(_("A username and password are being
requested by the site %s"),
> +	   m_challenge.protectionSpace().host().utf8().data());
> +    GtkWidget* descriptionLabel = gtk_label_new(description.utf8().data());

This is causing encoding issues in the dialog label, I guess it's because
String::format expects char* but not necessarily a utf8 encoded char*. I still
think previous code was simpler and more efficient.

> Source/WebCore/platform/gtk/GtkAuthenticationDialog.cpp:266
>      dialog->destroy();
> +   
dialog->m_challenge.authenticationClient()->receivedCancellation(dialog->m_chal
lenge);

When the dialog is cancelled, ::destroy() deletes the object immediately, so
you shouldn't use the object after calling destroy.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:875
> +    if (challenge != d->m_currentWebChallenge)
> +	   return;

I think we should implement AuthenticationChallenge::platformCompare() to make
sure we don't unpause a different soup message.

> Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp:903
> +    if (client())
> +	   client()->receivedCancellation(this, challenge);
> +    soup_session_unpause_message(challenge.soupSession(),
challenge.soupMessage());

This doesn't work, ResourceLoader::receivedCancellation() cancels the load
operation, so when soup_session_unpause_message() is called the message is not
in the queue anymore. I think we should not notify the client, and just unpause
the message to continue with the load instead of cancelling it as we have
always done.